Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/tribe-economy-settings into lp:widelands
Bug filed: https://bugs.launchpad.net/widelands/+bug/1827696 Shall we discuss the rest on the forum? -- https://code.launchpad.net/~widelands-dev/widelands/tribe-economy-settings/+merge/366878 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/tribe-economy-settings. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/expedition_portspace_indicator into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/expedition_portspace_indicator into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/expedition_portspace_indicator/+merge/366640 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/expedition_portspace_indicator. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Continuous integration builds have changed state: Travis build 4871. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/527824464. Appveyor build 4652. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_refactor_gameclient-4652. -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/expedition_portspace_indicator into lp:widelands
Review: Approve -- https://code.launchpad.net/~widelands-dev/widelands/expedition_portspace_indicator/+merge/366640 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/expedition_portspace_indicator. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/expedition_portspace_indicator into lp:widelands
You have convinced me. I have done more testing - all working now :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/expedition_portspace_indicator/+merge/366640 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/expedition_portspace_indicator. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1827182-sort-client-list into lp:widelands
Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways. Travis build 4866. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/527644741. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1827182-sort-client-list/+merge/366843 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1827182-sort-client-list. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1827182-sort-client-list into lp:widelands
Continuous integration builds have changed state: Travis build 4866. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/527644741. Appveyor build 4647. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1827182_sort_client_list-4647. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1827182-sort-client-list/+merge/366843 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1827182-sort-client-list. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/font_size-lua into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/font_size-lua into lp:widelands has been updated. Commit message changed to: Except for scenario and win condition messages, define all font styles via Lua. Reduce the number of construtors in TextArea. For more details, see: https://code.launchpad.net/~widelands-dev/widelands/font_size-lua/+merge/366938 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/font_size-lua into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/ferry into lp:widelands
Continuous integration builds have changed state: Travis build 4865. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/527627946. Appveyor build 4646. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ferry-4646. -- https://code.launchpad.net/~widelands-dev/widelands/ferry/+merge/351880 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ferry. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/tribe-economy-settings into lp:widelands
I agree with Benedikt and Klaus as well as I think some of the defaults could be reduced (e.g. basic weapon from 30 to 10). I believe setting the values to the default starting values would be the lowest limit to reduce to as it would trigger production immediately if something is used. However having individual settings would be the best solution so +1 for this from my side. As a side effect this would lead to individual settings possibilities for the AI which might be good as well -- https://code.launchpad.net/~widelands-dev/widelands/tribe-economy-settings/+merge/366878 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/tribe-economy-settings. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/expedition_portspace_indicator into lp:widelands
> Port being built can be taken care of by buildcaps IMHO we should remove the portspaces either only in response to Notes or only autonomously. Mixing both feels messy. To use notes, we´d need subscriptions (in addition to what we already have) to – NoteExpeditionCanceled – FieldPossession (another possible reason why a portspace might become unavailable is that some other player conquers that land, which is not even necessarily reflected in the buildcaps) – FieldTerrainChanged (if the port space is removed by a terrain-changing script) – A tree grows over the port space (or is this forbidden somehow?) → is there a notification for that? – A script unsets a port space directly → is there a notification for that? That´s quite a lot for a pretty small feature IMHO; and I don´t think the autonomous cleanup of displayed portspaces is performance-critical – I can´t imagine a player will have so many expeditions at a time that it really matters. -- https://code.launchpad.net/~widelands-dev/widelands/expedition_portspace_indicator/+merge/366640 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/expedition_portspace_indicator. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands
Sadly it doesn't. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1825932-open-games/+merge/366860 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1827182-sort-client-list into lp:widelands
@bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-1827182-sort-client-list/+merge/366843 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1827182-sort-client-list. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands
I actually don't know whether changing text color is supported at this time. I don't want to touch the editbox code right now, because I have a big refactoring in the works. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1825932-open-games/+merge/366860 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands
Tooltip is implemented. But the changing text colour... that's beyond my understanding. Thought I could implement this with boost::format but that doesn't work. We have set_color() available for tables and textfields, but I have no idea how to implement this for editboxes. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1825932-open-games/+merge/366860 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
You could also do the game tips like this:
const std::string tribename = get_players_tribe();
assert(Widelands::tribe_exists(tribename));
std::unique_ptr tips(new GameTips(*loader, {"general_game",
"multiplayer", tribename}));
As to the ASan crash, I suspect that there is a problem with the lifetime of an
object caused by the pulling out of stuff into separate functions.
--
https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Still get a heap-use-after-free related to std::unique_ptr loader_ui(new UI::ProgressWindow()); But this is maybe just a problem as the game crashed with disconnect(CLIENT_CRASHED, ) A have a deja vue around GameClientImpl.modal which is released twice. -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Found a first bug, still need some debugging -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
* Used parent instead of This. * moved all Variables into Impl About that -2 magic: I got lost in finding the sematics of user/player etc. I added a TODO comment with our best guess by now. I am at the End of my vacation (sigh) so lets get this in before it starts lingering around for too long. -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/representative_image_in_font_renderer into lp:widelands
Review: Approve Code LGTM. Tested briefly, no issues seen. -- https://code.launchpad.net/~widelands-dev/widelands/representative_image_in_font_renderer/+merge/363781 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/representative_image_in_font_renderer. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/tribe-economy-settings into lp:widelands
Continuous integration builds have changed state: Travis build 4861. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/527614965. Appveyor build 4642. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_tribe_economy_settings-4642. -- https://code.launchpad.net/~widelands-dev/widelands/tribe-economy-settings/+merge/366878 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/tribe-economy-settings. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/tribe-economy-settings into lp:widelands
Review: Disapprove short review I go along with Benedikt. An average player will have a _lot_ of trouble to get the economy going and will have no clue what the problems are. We would have to rethink all the Tutorials and Scenarios. Some novice players may be trapped in on of the tribes special pitfalls even faster then now (e.g. trying to build a silk infrastructure without silk, winery without Marble etc...) If you want to create a challenge for experienced players, lets have a startcondition like "Limited Start Resources" or such -- https://code.launchpad.net/~widelands-dev/widelands/tribe-economy-settings/+merge/366878 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/tribe-economy-settings. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/expedition_portspace_indicator into lp:widelands
Review: Resubmit OK, now InteractiveBase receives a notification instead when a portspace is found. Since there are fairly many reasons why a port space can become unavailable again (ship sails on, port is constructed, ship is sunk…), the IBase takes care of cleaning up the set itself now when think()ing. This fixes the bug that the indicator won´t go away on portconstruction. Also added saving support for the indicators so they are also shown for ships that already are in PortSpaceFound state when the game is loaded. -- https://code.launchpad.net/~widelands-dev/widelands/expedition_portspace_indicator/+merge/366640 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/expedition_portspace_indicator. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands
I agree. We should change the colour in the editbox and have a tooltip. But currently I have no idea how to change the text's colour. Can you give me a hint, please? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1825932-open-games/+merge/366860 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:widelands/build20 into lp:widelands
Continuous integration builds have changed state: Travis build 4856. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/527411124. Appveyor build 4637. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_build20-4637. -- https://code.launchpad.net/~widelands-dev/widelands/build20/+merge/366850 Your team Widelands Developers is requested to review the proposed merge of lp:widelands/build20 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands
Having a message in the chat window isn't enough. I did not see it, and the same will happen to others. So, I still think that we should show the games as unconnectable rather than hiding them. Other things we could to is make the text in the editbox red and adding a tooltip to the editbox. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1825932-open-games/+merge/366860 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands
Thanks allot. Regarding your test see the diff comment, please,
Diff comments:
>
> === modified file 'src/ui_fsmenu/internet_lobby.cc'
> --- src/ui_fsmenu/internet_lobby.cc 2019-04-24 06:01:37 +
> +++ src/ui_fsmenu/internet_lobby.cc 2019-05-03 06:24:46 +
> @@ -218,26 +218,30 @@
> hostgame_.set_enabled(true);
> joingame_.set_enabled(false);
> std::string localservername = edit_servername_.text();
> + std::string localbuildid = build_id();
>
> if (games != nullptr) { // If no communication error occurred, fill
> the list.
> for (const InternetGame& game : *games) {
> const Image* pic;
> - if (game.connectable) {
> - if (game.build_id == build_id())
> - pic =
> g_gr->images().get("images/ui_basic/continue.png");
> - else {
> + if (game.connectable == INTERNET_GAME_SETUP &&
> game.build_id == localbuildid) {
> + // only clients with the same build number are
> displayed
> + pic =
> g_gr->images().get("images/ui_basic/continue.png");
> + opengames_list_.add(game.name, game, pic,
> false, game.build_id);
> + } else if (game.connectable == INTERNET_GAME_SETUP &&
> + game.build_id.compare(0,6,"build-") != 0 &&
> localbuildid.compare(0,6,"build-") != 0) {
> + // only development clients are allowed
> to see games openend by such
> pic =
> g_gr->images().get("images/ui_basic/different.png");
> - }
> - } else {
> - pic =
> g_gr->images().get("images/ui_basic/stop.png");
> + opengames_list_.add(game.name, game,
> pic, false, game.build_id);
> }
> // If one of the servers has the same name as the local
> name of the
> // clients server, we disable the 'hostgame' button to
> avoid having more
> // than one server with the same name.
> if (game.name == localservername) {
> hostgame_.set_enabled(false);
> + InternetGaming::ref().format_and_add_chat("",
> "", true,
> + (boost::format(_("A game named %s is already
> running. Please choose a different name."))
> + % game.name).str());
The user would then receive this message in the chat window.
> }
> - opengames_list_.add(game.name, game, pic, false,
> game.build_id);
> }
> }
> }
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1825932-open-games/+merge/366860
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1827182-sort-client-list into lp:widelands
Addressed your review, and one diff comment.
Diff comments:
> === modified file 'src/network/internet_gaming.cc'
> --- src/network/internet_gaming.cc2019-05-01 20:17:37 +
> +++ src/network/internet_gaming.cc2019-05-03 01:27:24 +
> @@ -558,7 +553,10 @@
> format_and_add_chat(
> "", "", true, (boost::format(_("%s
> joined the lobby")) % inc.name).str());
> }
> - clientlist_.insert(clientlist_.end(), irc.begin(),
> irc.end());
> +
> + std::sort(clientlist_.begin(), clientlist_.end(),
If I don't sort them here, the list won't be alphabetically. The internet-lobby
user list is sorted by column 0 (that's why I added
clientsonline_list_.sort();) which is the type. But If I hand over an unsorted
list, within the type sorting the names are not alphabetically.
> + []( const InternetClient ,
> const InternetClient )
> + { return ( left.name <
> right.name ); } );
>
> for (InternetClient& client : old) {
> if (client.name.size() && client.type !=
> INTERNET_CLIENT_IRC) {
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1827182-sort-client-list/+merge/366843
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1827182-sort-client-list into lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands
OK, Code LGTM now :) Some testing: - When a non-connectable game of the same name exists, the "Open a new game" button is disabled correctly, but the user has no idea why. It would be better to list the non-connectable games with the x icon. The rest is working as expected :) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1825932-open-games/+merge/366860 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/expedition_portspace_indicator into lp:widelands
Review: Needs Fixing We are trying to get rid of references to InteractiveBase from the logic library. Would you like to try your hand at adding new actions to NoteShip and subscribing to those in InteractivePlayer? After placing a port construction site, a big building plot icon is shown on top of the construction site. -- https://code.launchpad.net/~widelands-dev/widelands/expedition_portspace_indicator/+merge/366640 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/expedition_portspace_indicator. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/tribe-economy-settings into lp:widelands
Review: Disapprove I am really unhappy about this change. Setting the target to 0 for tools means that whenever a new building is completed, I have to wait several minutes until a worker is created. No thanks. Roads are not normally promoted one by one, but several in a short interval. Just one carrier animal in store again means a delay. Metals and ores area quite important, just so few in stock is too little. And when weapons and foodstuff have a target of 0, then it´ll take much longer until I get a newly built trainingsite ready to run. Inexperienced players will fairly often be in the situation where they need to produce new soldiers fast or get overrun, having only one basic weapon in store then is a joke that means losing for sure. These settings are optimized for very good players who can build an economy that is running completely smoothly and produces supersoldiers at the highest possible speed. But most of the players aren´t that good, and such settings will just cause unnecessary shortages and delays in a suboptimal economy. Also, less experienced players like to have some more wares in stock so a shortage is less likely to happen, and not everyone always remembers to (or even knows how to) change settings where they dislike the defaults. We should leave the default targets untouched, and ask the few players who really need such low targets to just set them for themselves instead of expecting all other players to increase their settings. -- https://code.launchpad.net/~widelands-dev/widelands/tribe-economy-settings/+merge/366878 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/tribe-economy-settings. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands
Addressed your comments. Thank you for reviewing.
Diff comments:
>
> === modified file 'src/ui_fsmenu/internet_lobby.cc'
> --- src/ui_fsmenu/internet_lobby.cc 2019-04-24 06:01:37 +
> +++ src/ui_fsmenu/internet_lobby.cc 2019-05-03 01:59:08 +
> @@ -218,26 +218,30 @@
> hostgame_.set_enabled(true);
> joingame_.set_enabled(false);
> std::string localservername = edit_servername_.text();
> + std::string localbuildid = build_id();
>
> if (games != nullptr) { // If no communication error occurred, fill
> the list.
> for (const InternetGame& game : *games) {
> const Image* pic;
> - if (game.connectable) {
> - if (game.build_id == build_id())
> - pic =
> g_gr->images().get("images/ui_basic/continue.png");
> - else {
> + if (game.connectable == INTERNET_GAME_SETUP &&
> game.build_id == localbuildid) {
> + // only clients with the same build number are
> displayed
> + pic =
> g_gr->images().get("images/ui_basic/continue.png");
> + opengames_list_.add(game.name, game, pic,
> false, game.build_id);
> + } else if (game.connectable == INTERNET_GAME_SETUP &&
> + game.build_id.compare(0,6,"build-") != 0 &&
> localbuildid.compare(0,6,"build-") != 0) {
The windows clients report with a lowercase B in the lobby (build-20-rc1). So I
think no need to worry.
> + // only development clients are allowed
> to see games openend by such
> pic =
> g_gr->images().get("images/ui_basic/different.png");
> - }
> - } else {
> - pic =
> g_gr->images().get("images/ui_basic/stop.png");
> + opengames_list_.add(game.name, game,
> pic, false, game.build_id);
> }
> // If one of the servers has the same name as the local
> name of the
> // clients server, we disable the 'hostgame' button to
> avoid having more
> // than one server with the same name.
> if (game.name == localservername) {
> hostgame_.set_enabled(false);
> + InternetGaming::ref().format_and_add_chat("",
> "", true,
> + (boost::format(_("A game named %s is already
> running. Please choose a different name."))
> + % game.name).str());
> }
> - opengames_list_.add(game.name, game, pic, false,
> game.build_id);
> }
> }
> }
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1825932-open-games/+merge/366860
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1825932-open-games into lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bridges into lp:widelands
Yep, let's wait for the ferry branch and do a final round of code review and testing in this branch when that's done. -- https://code.launchpad.net/~widelands-dev/widelands/bridges/+merge/364318 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ferry. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ferry into lp:widelands
Fieldaction code looks OK. How about passing wwWORKER/wwWARE to get_roadbase and checking for that there? Then you could just call get_roadbase as in your original attempt in r8874. -- https://code.launchpad.net/~widelands-dev/widelands/ferry/+merge/351880 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ferry. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
