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

Proposed by Klaus Halfmann
Status: Superseded
Proposed branch: lp:~widelands-dev/widelands/bug_1794339_center_wo_parent
Merge into: lp:widelands
Diff against target: 26 lines (+9/-5)
1 file modified
src/ui_basic/window.cc (+9/-5)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug_1794339_center_wo_parent
Reviewer Review Type Date Requested Status
Widelands Developers Pending
Review via email: mp+355873@code.launchpad.net

This proposal has been superseded by a proposal from 2018-09-30.

Commit message

Avoid null access in Window::center_to_parent()

Description of the change

Simple check if Window has a parent instead of crashing.

This leaves the window in the top-lfet corner, but avoids the crash.

Gun: can we center to some global window instead.

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

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 :

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 :

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 :

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 :

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

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

Keep d->modal as parent window in GameClient::run()

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

Assert parent window

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

Fixed codecheck

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/ui_basic/window.cc'
2--- src/ui_basic/window.cc 2018-07-08 13:53:45 +0000
3+++ src/ui_basic/window.cc 2018-09-29 14:33:07 +0000
4@@ -221,13 +221,17 @@
5 }
6
7 /**
8- * Move the window so that it is centered wrt the parent.
9-*/
10+ * Move the window so that it is centered inside the parent.
11+ *
12+ * Do nothing if window has no parent.
13+ */
14 void Window::center_to_parent() {
15- Panel& parent = *get_parent();
16+ Panel* parent = get_parent();
17
18- set_pos(Vector2i((static_cast<int32_t>(parent.get_inner_w()) - get_w()) / 2,
19- (static_cast<int32_t>(parent.get_inner_h()) - get_h()) / 2));
20+ if (parent) {
21+ set_pos(Vector2i((static_cast<int32_t>(parent->get_inner_w()) - get_w()) / 2,
22+ (static_cast<int32_t>(parent->get_inner_h()) - get_h()) / 2));
23+ }
24 }
25
26 /**

Subscribers

People subscribed via source and target branches

to status/vote changes: