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

Proposed by Klaus Halfmann
Status: Merged
Merged at revision: 8873
Proposed branch: lp:~widelands-dev/widelands/bug_1794339_center_wo_parent
Merge into: lp:widelands
Diff against target: 62 lines (+11/-7)
2 files modified
src/network/gameclient.cc (+3/-2)
src/ui_basic/window.cc (+8/-5)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug_1794339_center_wo_parent
Reviewer Review Type Date Requested Status
GunChleoc Approve
Review via email: mp+355885@code.launchpad.net

This proposal supersedes a proposal from 2018-09-29.

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
GunChleoc (gunchleoc) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
bunnybot (widelandsofficial) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
GunChleoc (gunchleoc) wrote : Posted in a previous version of this proposal

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;

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote : Posted in a previous version of this proposal

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

Revision history for this message
GunChleoc (gunchleoc) wrote :

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

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

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?

8864. By Klaus Halfman \<<email address hidden>\>

Assert parent window

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

Ok made this an assert

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

8865. By Klaus Halfman \<<email address hidden>\>

Fixed codecheck

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

An assert works for me :)

@bunnybot merge

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/network/gameclient.cc'
2--- src/network/gameclient.cc 2018-04-21 10:57:12 +0000
3+++ src/network/gameclient.cc 2018-10-06 07:26:32 +0000
4@@ -172,6 +172,7 @@
5
6 try {
7 UI::ProgressWindow* loader_ui = new UI::ProgressWindow("images/loadscreens/progress.png");
8+ d->modal = loader_ui;
9 std::vector<std::string> tipstext;
10 tipstext.push_back("general_game");
11 tipstext.push_back("multiplayer");
12@@ -204,7 +205,7 @@
13 d->lasttimestamp = game.get_gametime();
14 d->lasttimestamp_realtime = SDL_GetTicks();
15
16- d->modal = game.get_ibase();
17+ d->modal = igb;
18 game.run(loader_ui, d->settings.savegame ? Widelands::Game::Loaded : d->settings.scenario ?
19 Widelands::Game::NewMPScenario :
20 Widelands::Game::NewNonScenario,
21@@ -217,7 +218,6 @@
22 d->modal = nullptr;
23 d->game = nullptr;
24 } catch (...) {
25- d->modal = nullptr;
26 WLApplication::emergency_save(game);
27 d->game = nullptr;
28 disconnect("CLIENT_CRASHED");
29@@ -225,6 +225,7 @@
30 if (internet_) {
31 InternetGaming::ref().logout("CLIENT_CRASHED");
32 }
33+ d->modal = nullptr;
34 throw;
35 }
36 }
37
38=== modified file 'src/ui_basic/window.cc'
39--- src/ui_basic/window.cc 2018-07-08 13:53:45 +0000
40+++ src/ui_basic/window.cc 2018-10-06 07:26:32 +0000
41@@ -221,13 +221,16 @@
42 }
43
44 /**
45- * Move the window so that it is centered wrt the parent.
46-*/
47+ * Move the window so that it is centered inside the parent.
48+ *
49+ * Do nothing if window has no parent.
50+ */
51 void Window::center_to_parent() {
52- Panel& parent = *get_parent();
53+ Panel* parent = get_parent();
54
55- set_pos(Vector2i((static_cast<int32_t>(parent.get_inner_w()) - get_w()) / 2,
56- (static_cast<int32_t>(parent.get_inner_h()) - get_h()) / 2));
57+ assert(parent);
58+ set_pos(Vector2i((static_cast<int32_t>(parent->get_inner_w()) - get_w()) / 2,
59+ (static_cast<int32_t>(parent->get_inner_h()) - get_h()) / 2));
60 }
61
62 /**

Subscribers

People subscribed via source and target branches

to status/vote changes: