Merge lp:~widelands-dev/widelands/bug_1794339_center_wo_parent into lp:widelands

[Klaus Halfmann]

Assert null access in Window::center_to_parent(), keep parent window in GameClient::run()

Assert that Window has a parent instead of crashing.

Keep d->modal as parent window in GameClient::run() and set to nullptr as late as possible. This should resolve the original bug.

[GunChleoc (gunchleoc)]

No, we can't center to a global parent, because the direct parent is already nullptr.

I am wondering whether this fix doesn't just mask the real bug - it would be interesting to track down why that particular window has no parent. There's something wrong with the control flow there.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4076. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/435018665.
Appveyor build 3872. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1794339_center_wo_parent-3872.

[Klaus Halfmann (klaus-halfmann)]

In my case the root cause was a savegame incompatibilty.

void GameClient::disconnect in network/gameclient.cc creates the messagebox as:
UI::WLMessageBox mmb(d->modal, ....

d is a GameClientImpl* and modal is explicily set as nullpointer in the CTor.
modal is always set to the currently show window and then back to nullptr.

there ist a catch all in GameClient:run() that will call disconnect("CLIENT_CRASHED");

I assume the execption is raised in game.init_savegame(loader_ui, d->settings);

I will try setting d->modal to InteractiveGameBase as early as possible.

[GunChleoc (gunchleoc)]

Another possible solution might be to do 2 things:

1. Set modal to nullptr only after calling disconnect
2. Showing the message only if model != nullptr and logging to console otherwise

So,

  d->modal = nullptr;
  WLApplication::emergency_save(game);
  d->game = nullptr;
  disconnect("CLIENT_CRASHED");
  // We will bounce back to the main menu, so we better log out
  if (internet_) {
   InternetGaming::ref().logout("CLIENT_CRASHED");
  }

Could become

  WLApplication::emergency_save(game);
  d->game = nullptr;
  disconnect("CLIENT_CRASHED");
  // We will bounce back to the main menu, so we better log out
  if (internet_) {
   InternetGaming::ref().logout("CLIENT_CRASHED");
  }
  d->modal = nullptr;

[Klaus Halfmann (klaus-halfmann)]

This works for me, I dont think it will break anything, well.

[GunChleoc (gunchleoc)]

I see a "previous version of this proposal" but no new code - did you push your changes? Did Launchpad break it?

[Klaus Halfmann (klaus-halfmann)]

Oops, I pushed to :parent, which was my local trunk.
Now I see that I used tabs instead of spaces, uhm.
Can earliest fix this tomorrow, perhpas.

[GunChleoc (gunchleoc)]

Don't worry about the tabs, bunnybot will take care of those.

I'd still like to see the changes to window.cc reverted, because it will mask bugs like the one we're fixing here. Maybe use an assert or an exception to ensure that the parent is not null?

[Klaus Halfmann (klaus-halfmann)]

Ok made this an assert

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4095. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/437873611.
Appveyor build 3891. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1794339_center_wo_parent-3891.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4097. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/437942149.
Appveyor build 3893. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1794339_center_wo_parent-3893.

[GunChleoc (gunchleoc)]

An assert works for me :)

@bunnybot merge

review: Approve

=== modified file 'src/network/gameclient.cc'
--- src/network/gameclient.cc	2018-04-21 10:57:12 +0000
+++ src/network/gameclient.cc	2018-10-06 07:26:32 +0000
@@ -172,6 +172,7 @@
 
 	try {
 		UI::ProgressWindow* loader_ui = new UI::ProgressWindow("images/loadscreens/progress.png");
+        d->modal = loader_ui;
 		std::vector<std::string> tipstext;
 		tipstext.push_back("general_game");
 		tipstext.push_back("multiplayer");
@@ -204,7 +205,7 @@
 		d->lasttimestamp = game.get_gametime();
 		d->lasttimestamp_realtime = SDL_GetTicks();
 
-		d->modal = game.get_ibase();
+        d->modal = igb;
 		game.run(loader_ui, d->settings.savegame ? Widelands::Game::Loaded : d->settings.scenario ?
 		                                           Widelands::Game::NewMPScenario :
 		                                           Widelands::Game::NewNonScenario,
@@ -217,7 +218,6 @@
 		d->modal = nullptr;
 		d->game = nullptr;
 	} catch (...) {
-		d->modal = nullptr;
 		WLApplication::emergency_save(game);
 		d->game = nullptr;
 		disconnect("CLIENT_CRASHED");
@@ -225,6 +225,7 @@
 		if (internet_) {
 			InternetGaming::ref().logout("CLIENT_CRASHED");
 		}
+        d->modal = nullptr;
 		throw;
 	}
 }

=== modified file 'src/ui_basic/window.cc'
--- src/ui_basic/window.cc	2018-07-08 13:53:45 +0000
+++ src/ui_basic/window.cc	2018-10-06 07:26:32 +0000
@@ -221,13 +221,16 @@
 }
 
 /**
- * Move the window so that it is centered wrt the parent.
-*/
+ * Move the window so that it is centered inside the parent.
+ *
+ * Do nothing if window has no parent.
+ */
 void Window::center_to_parent() {
-	Panel& parent = *get_parent();
+    Panel* parent = get_parent();
 
-	set_pos(Vector2i((static_cast<int32_t>(parent.get_inner_w()) - get_w()) / 2,
-	                 (static_cast<int32_t>(parent.get_inner_h()) - get_h()) / 2));
+    assert(parent);
+    set_pos(Vector2i((static_cast<int32_t>(parent->get_inner_w()) - get_w()) / 2,
+                     (static_cast<int32_t>(parent->get_inner_h()) - get_h()) / 2));
 }
 
 /**