[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect. ___ 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-1434875-net-client-disconnect into lp:widelands
Review: Approve I have tweaked the strings some more - all good now :) > Do we need a translator comment for both occurrences of "Replace with %s" or > is one comment enough when it is the same string? One comment is enough - Gettext uses the string as a key, so it will be presented for translation only once. We can disambiguate them with pgettext when needed. @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect. ___ 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-1434875-net-client-disconnect into lp:widelands
Continuous integration builds have changed state: Travis build 3965. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/428324625. Appveyor build 3763. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1434875_net_client_disconnect-3763. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect. ___ 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-1434875-net-client-disconnect into lp:widelands
Continuous integration builds have changed state: Travis build 3958. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/427902291. Appveyor build 3756. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1434875_net_client_disconnect-3756. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect. ___ 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-1434875-net-client-disconnect into lp:widelands
Thanks for the bug report, the local fix and the review. Your remarks are hopefully all fixed now. Do we need a translator comment for both occurrences of "Replace with %s" or is one comment enough when it is the same string? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect. ___ 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-1434875-net-client-disconnect into lp:widelands
Continuous integration builds have changed state: Travis build 3945. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/427533795. Appveyor build 3743. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1434875_net_client_disconnect-3743. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect. ___ 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-1434875-net-client-disconnect into lp:widelands
Review: Needs Fixing
I suggested some improvements for the i18n. There is also some code duplication
that we should get rid of.
Diff comments:
> === modified file 'src/network/gamehost.cc'
> --- src/network/gamehost.cc 2018-04-07 16:59:00 +
> +++ src/network/gamehost.cc 2018-09-12 05:38:27 +
> @@ -560,6 +561,25 @@
> ->instantiate(*d->game, p));
> }
>
> +void GameHost::start_ai_for(uint8_t playernumber, const std::string& ai) {
Call this function "replace_client_with_ai"?
> + assert(d->game->get_player(playernumber + 1)->get_ai().empty());
> + assert(d->game->get_player(playernumber + 1)->get_ai()
> + == d->settings.players.at(playernumber).ai);
> + // Inform all players about the change
> + // Has to be done at first in this method since the calls later on
> overwrite players[].name
> + send_system_message_code(
> + "CLIENT_X_REPLACED_WITH",
> + d->settings.players.at(playernumber).name,
> + ComputerPlayer::get_implementation(ai)->descname);
> + set_player_ai(playernumber, ai, false);
> + d->game->get_player(playernumber + 1)->set_ai(ai);
> + // Activate the ai
> + init_computer_player(playernumber + 1);
> + set_player_state(playernumber, PlayerSettings::State::kComputer);
> + assert(d->game->get_player(playernumber + 1)->get_ai()
> + == d->settings.players.at(playernumber).ai);
> +}
> +
> void GameHost::init_computer_players() {
> const Widelands::PlayerNumber nr_players =
> d->game->map().get_nrplayers();
> iterate_players_existing_novar(p, nr_players, *d->game) {
>
> === added file 'src/wui/game_client_disconnected.cc'
> --- src/wui/game_client_disconnected.cc 1970-01-01 00:00:00 +
> +++ src/wui/game_client_disconnected.cc 2018-09-12 05:38:27 +
> @@ -0,0 +1,197 @@
> +/*
> + * Copyright (C) 2002-2018 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301, USA.
> + *
> + */
> +
> +#include "wui/game_client_disconnected.h"
> +
> +#include
> +#include
> +
> +#include "ai/computer_player.h"
> +#include "ai/defaultai.h"
> +#include "base/i18n.h"
> +#include "network/gamehost.h"
> +#include "ui_basic/messagebox.h"
> +#include "wui/interactive_gamebase.h"
> +
> +namespace {
> +
> +const int32_t width = 256;
Make these constexpr
> +const int32_t margin = 10;
> +const int32_t vspacing = 5;
> +const uint32_t vgap = 3;
> +
> +/**
> + * Small helper class for a "Really exit game?" message box.
> + */
> +class GameClientDisconnectedExitConfirmBox : public UI::WLMessageBox {
> +public:
> + // TODO(GunChleoc): Arabic: Buttons need more height for Arabic
> + GameClientDisconnectedExitConfirmBox(GameClientDisconnected* gcd,
> InteractiveGameBase* gb)
> +: UI::WLMessageBox(gcd->get_parent(),
> + /** TRANSLATORS: Window label when "Exit game"
> has been pressed */
> + _("Exit Game Confirmation"),
> + _("Are you sure you wish to exit this game?"),
> + MBoxType::kOkCancel),
> + igb_(gb), gcd_(gcd) {
> + }
> +
> + void clicked_ok() override {
> +
> igb_->end_modal(UI::Panel::Returncodes::kBack);
> + }
> +
> + void clicked_back() override {
> + cancel();
> + die();
> + // Make parent window visible again so player can use another
> option
> + gcd_->set_visible(true);
> + }
> +
> +private:
> + InteractiveGameBase* igb_;
> + GameClientDisconnected* gcd_;
If we tie into the cancel() signal from GameClientDisconnected, we can get rid
of the gcd_ variable and of the code duplication with
GameOptionsMenuExitConfirmBox and have 1 common GameExitConfirmBox for both.
Also, the UniqueWindow class has a create and destroy hook that you could work
with.
> +};
> +
> +} // end anonymous namespace
> +
> +GameClientDisconnected::GameClientDisconnected(InteractiveGameBase* gb,
> + UI::UniqueWindow::Registry& registry,
> + GameHost* host)
> + : UI::UniqueWindow(gb, "client_disconnected", , 2 * margin +
> width,
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands
Bug is up: https://bugs.launchpad.net/widelands/+bug/1792079 I have 1 more idea for the memory leak. I'll try to fix it and then start the code review. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect 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/bug-1434875-net-client-disconnect into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands has been updated. Description changed to: When a client disconnects (either on purpose or due to network errors), the game is paused, saved, and a dialog is displayed to the host. There, the host can select whether to replace the client with a Normal/Weak/Very Weak/Empty AI or to exit the game. No dialog is shown, if the leaving player ... - is only an observer - has already lost. In that case the player is replaced by the Empty AI - has won the game. In that case the player is replaced by the Normal AI So far, this branch seems to work fine in my testcases and could be merged. [ignore next part, not an issue of this branch] However, ASAN is complaining about access to already freed memory when the host closes the game with Alt+F4 while the dialog is shown. Strangely, there is no complain if the "really exit game" confirmation dialog has been shown but aborted. I wasn't able to figure out what is happening there. So if someone has an idea what is causing the problem, please tell me or fix it yourself. Apart from this memory bug, the branch is ready for review and testing. [/ignore] For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect 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-1434875-net-client-disconnect into lp:widelands
Thanks for the explanation, it makes much more sense now. I haven't considered that the dropbox needs to access "higher" parents, so it was quite a strange effect. But I guess that means that this branch isn't to blame and can be reviewed and merged. Would you mind copying your explanation to a new bug report about the memory leak and targeting it for build-21? The leak doesn't seem to cause any immediate harm so I think we can postpone fixing it until your other branches are in. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect 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-1434875-net-client-disconnect into lp:widelands
I have not been able to fix the memory leak. The heart of the problem is that dropdowns break the expected tree hierarchy for panels by necessity - otherwise, we would not be able to show the list outside of the enclosing box. We have continually been struggling with this. It's also weir that the same problem doesn't happen for the editor player menu, where we also have dropdowns inside a box inside a unique window. I think we need to change the panel implementation to use shared_ptr instead of raw pointers. I have too many UI branches open at this time to do this now though - the changes will be huge and cause merge conflicts all over the place. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect 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/bug-1434875-net-client-disconnect into lp:widelands
Continuous integration builds have changed state: Travis build 3925. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/426559900. Appveyor build 3723. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1434875_net_client_disconnect-3723. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect 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/bug-1434875-net-client-disconnect into lp:widelands
Continuous integration builds have changed state: Travis build 3921. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/426390888. Appveyor build 3719. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1434875_net_client_disconnect-3719. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect 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/bug-1434875-net-client-disconnect into lp:widelands
Notabilis has proposed merging
lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands.
Commit message:
Adding a dialog for the host when the connection to a client is lost. Allows
the host to select whether to replace the client with an AI or to exit.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1434875 in widelands: "Multiplayer network problems go unnoticed"
https://bugs.launchpad.net/widelands/+bug/1434875
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531
This is not ready for merge yet!
When a client disconnects (either on purpose or due to network errors), the
game is paused, saved, and a dialog is displayed to the host. There, the host
can select whether to replace the client with a Normal/Weak/Very Weak/Empty AI
or to exit the game.
No dialog is shown, if the leaving player ...
- is only an observer
- has already lost. In that case the player is replaced by the Empty AI
- has won the game. In that case the player is replaced by the Normal AI
So far, this branch seems to work fine in my testcases and could be merged.
However, ASAN is complaining about access to already freed memory when the host
closes the game with Alt+F4 while the dialog is shown. Strangely, there is no
complain if the "really exit game" confirmation dialog has been shown but
aborted.
I wasn't able to figure out what is happening there. So if someone has an idea
what is causing the problem, please tell me or fix it yourself. Apart from this
memory bug, the branch is ready for review and testing.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands.
=== modified file 'src/network/gamehost.cc'
--- src/network/gamehost.cc 2018-04-07 16:59:00 +
+++ src/network/gamehost.cc 2018-09-09 17:19:05 +
@@ -31,6 +31,7 @@
#endif
#include "ai/computer_player.h"
+#include "ai/defaultai.h"
#include "base/i18n.h"
#include "base/md5.h"
#include "base/warning.h"
@@ -62,6 +63,7 @@
#include "ui_basic/progresswindow.h"
#include "ui_fsmenu/launch_mpg.h"
#include "wlapplication.h"
+#include "wui/game_client_disconnected.h"
#include "wui/game_tips.h"
#include "wui/interactive_player.h"
#include "wui/interactive_spectator.h"
@@ -560,6 +562,25 @@
->instantiate(*d->game, p));
}
+void GameHost::start_ai_for(uint8_t playernumber, const std::string& ai) {
+ assert(d->game->get_player(playernumber + 1)->get_ai().empty());
+ assert(d->game->get_player(playernumber + 1)->get_ai()
+ == d->settings.players.at(playernumber).ai);
+ // Inform all players about the change
+ // Has to be done at first in this method since the calls later on overwrite players[].name
+ send_system_message_code(
+ "CLIENT_X_REPLACED_WITH",
+ d->settings.players.at(playernumber).name,
+ ComputerPlayer::get_implementation(ai)->descname);
+ set_player_ai(playernumber, ai, false);
+ d->game->get_player(playernumber + 1)->set_ai(ai);
+ // Activate the ai
+ init_computer_player(playernumber + 1);
+ set_player_state(playernumber, PlayerSettings::State::kComputer);
+ assert(d->game->get_player(playernumber + 1)->get_ai()
+ == d->settings.players.at(playernumber).ai);
+}
+
void GameHost::init_computer_players() {
const Widelands::PlayerNumber nr_players = d->game->map().get_nrplayers();
iterate_players_existing_novar(p, nr_players, *d->game) {
@@ -2254,8 +2275,7 @@
}
set_player_state(number, PlayerSettings::State::kOpen);
- if (d->game)
- init_computer_player(number + 1);
+ // Don't replace player with AI, let host choose what to do
}
void GameHost::disconnect_client(uint32_t const number,
@@ -2266,6 +2286,37 @@
Client& client = d->clients.at(number);
+ // If the client is linked to a player and it is the client that closes the connection
+ // and the game has already started ...
+ if (client.playernum != UserSettings::none() && reason != "SERVER_LEFT" && d->game != nullptr) {
+ // And the client hasn't lost/won yet ...
+ if (d->settings.users.at(client.usernum).result == Widelands::PlayerEndResult::kUndefined) {
+ // And the window isn't visible yet ...
+ if (!client_disconnected_.window) {
+// Show a window and ask the host player what to do with the tribe of the leaving client
+
+if (!forced_pause()) {
+ force_pause();
+}
+
+WLApplication::emergency_save(*d->game);
+
+new GameClientDisconnected(d->game->get_ipl(), client_disconnected_, this);
+ }
+ // Client was active but is a winner of the game: Replace with normal AI
+ } else if (d->settings.users.at(
