widelands

Merge lp://staging/~widelands-dev/widelands/bug-1734046-statistics_menu into lp://staging/widelands

bug-1734046-statistics_menu
Merge into trunk

Proposed by GunChleoc on 2017-11-28

Status:

Merged

Merged at revision:

8515

Proposed branch:

lp://staging/~widelands-dev/widelands/bug-1734046-statistics_menu

Merge into:

lp://staging/widelands

Diff against target:

199 lines (+20/-56)

7 files modified

src/ui_basic/unique_window.cc (+2/-21)
src/ui_basic/unique_window.h (+6/-14)
src/wui/game_options_menu.cc (+5/-2)
src/wui/game_statistics_menu.cc (+4/-6)
src/wui/game_statistics_menu.h (+0/-4)
src/wui/interactive_base.cc (+3/-5)
src/wui/interactive_base.h (+0/-4)

To merge this branch:

bzr merge lp://staging/~widelands-dev/widelands/bug-1734046-statistics_menu

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
SirVer		2017-11-28	Approve on 2017-11-29
Review via email: mp+334398@code.staging.launchpad.net

Commit message

Replaced UniqueWindow::Registry on_create and on_delete with boost signals. This fixes a memory problem.

Description of the change

The problem was that after the game statistics window is closed and a child windows still open, the on_delete function was still linked with boost::bind.

The windows that have graphs in them are still leaking memory, but that is a different problem. ASan report should be clear now for all other toolbar and game statistics windows.

Revision history for this message

SirVer (sirver) wrote on 2017-11-29:

two nits - always prefer lambdas over bind if you can.

review: Approve

Revision history for this message

GunChleoc (gunchleoc) wrote on 2017-11-29:

Well, in this case, it seems like I can't, except for the one in the interactive base - I'll get a heap-use-after-free because boost:signals2 will barf on it - it just doesn't like it when the window containing the button is gone, just like with the original bug.

Lambdas are nice though, they are so much easier to read! Me wants all over the code base, preciousss

@bunnybot merge

Revision history for this message

Klaus Halfmann (klaus-halfmann) wrote on 2017-11-29:

Thanks for all the cleanup you did!

Revision history for this message

bunnybot (widelandsofficial) wrote on 2017-11-30:

Continuous integration builds have changed state:

Travis build 2887. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/308874869.
Appveyor build 2696. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1734046_statistics_menu-2696.