widelands

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

  1. bug-1734046-statistics_menu
  2. Merge into trunk
Proposed by GunChleoc
Status: Merged
Merged at revision: 8515
Proposed branch: lp:~widelands-dev/widelands/bug-1734046-statistics_menu
Merge into: lp: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:~widelands-dev/widelands/bug-1734046-statistics_menu
Reviewer Review Type Date Requested Status
SirVer Approve
Review via email: mp+334398@code.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.

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

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

review: Approve
Revision history for this message
GunChleoc (gunchleoc) wrote :

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 :

Thanks for all the cleanup you did!

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 2887. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/308874869.

Revision history for this message
GunChleoc (gunchleoc) wrote :

MacOX can't install sphinx

@bunnybot merge force

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/ui_basic/unique_window.cc'
2--- src/ui_basic/unique_window.cc 2017-02-22 07:14:15 +0000
3+++ src/ui_basic/unique_window.cc 2017-11-29 08:57:42 +0000
4@@ -71,21 +71,6 @@
5 delete window;
6 }
7
8-void UniqueWindow::Registry::assign_toggle_button(UI::Button* button) {
9- assert(!on_create);
10- assert(!on_delete);
11- on_create = boost::bind(&UI::Button::set_style, button, UI::Button::Style::kPermpressed);
12- on_delete = boost::bind(&UI::Button::set_style, button, UI::Button::Style::kRaised);
13- if (window) {
14- button->set_style(UI::Button::Style::kPermpressed);
15- }
16-}
17-
18-void UniqueWindow::Registry::unassign_toggle_button() {
19- on_create = 0;
20- on_delete = 0;
21-}
22-
23 /**
24 * Register, position according to the registry information.
25 */
26@@ -104,9 +89,7 @@
27 set_pos(Vector2i(registry_->x, registry_->y));
28 usedefaultpos_ = false;
29 }
30- if (registry_->on_create) {
31- registry_->on_create();
32- }
33+ registry_->opened();
34 }
35 }
36
37@@ -122,9 +105,7 @@
38 registry_->y = get_y();
39 registry_->valid_pos = true;
40
41- if (registry_->on_delete) {
42- registry_->on_delete();
43- }
44+ registry_->closed();
45 }
46 }
47 }
48
49=== modified file 'src/ui_basic/unique_window.h'
50--- src/ui_basic/unique_window.h 2017-01-25 18:55:59 +0000
51+++ src/ui_basic/unique_window.h 2017-11-29 08:57:42 +0000
52@@ -22,6 +22,8 @@
53
54 #include <functional>
55
56+#include <boost/signals2.hpp>
57+
58 #include "ui_basic/button.h"
59 #include "ui_basic/window.h"
60
61@@ -41,11 +43,10 @@
62 // closed.
63 std::function<void()> open_window;
64
65- // Called when the window opens.
66- std::function<void()> on_create;
67-
68- // Called when the window is deleted (i.e. closed).
69- std::function<void()> on_delete;
70+ // Triggered when the window opens
71+ boost::signals2::signal<void()> opened;
72+ // Triggered when the window closes
73+ boost::signals2::signal<void()> closed;
74
75 void create();
76 void destroy();
77@@ -60,15 +61,6 @@
78 Registry() : window(nullptr), x(0), y(0), valid_pos(false) {
79 }
80 ~Registry();
81-
82- /// The 'button' will be permpressed or not depending on whether this window is
83- /// open (on_create/on_delete callback function hooks).
84- /// This can be assigned only once.
85- void assign_toggle_button(UI::Button* button);
86-
87- /// Remove the callback as a safeguard in case somewhere else in the
88- /// code someone would overwrite our hooks.
89- void unassign_toggle_button();
90 };
91
92 UniqueWindow(Panel* parent,
93
94=== modified file 'src/wui/game_options_menu.cc'
95--- src/wui/game_options_menu.cc 2017-02-21 07:56:18 +0000
96+++ src/wui/game_options_menu.cc 2017-11-29 08:57:42 +0000
97@@ -114,14 +114,17 @@
98 exit_game_.sigclicked.connect(
99 boost::bind(&GameOptionsMenu::clicked_exit_game, boost::ref(*this)));
100
101- windows_.sound_options.assign_toggle_button(&sound_);
102+ if (windows_.sound_options.window) {
103+ sound_.set_style(UI::Button::Style::kPermpressed);
104+ }
105+ windows_.sound_options.opened.connect(boost::bind(&UI::Button::set_style, &sound_, UI::Button::Style::kPermpressed));
106+ windows_.sound_options.closed.connect(boost::bind(&UI::Button::set_style, &sound_, UI::Button::Style::kRaised));
107
108 if (get_usedefaultpos())
109 center_to_parent();
110 }
111
112 GameOptionsMenu::~GameOptionsMenu() {
113- windows_.sound_options.unassign_toggle_button();
114 }
115
116 void GameOptionsMenu::clicked_save_game() {
117
118=== modified file 'src/wui/game_statistics_menu.cc'
119--- src/wui/game_statistics_menu.cc 2017-02-21 07:56:18 +0000
120+++ src/wui/game_statistics_menu.cc 2017-11-29 08:57:42 +0000
121@@ -72,10 +72,11 @@
122 g_gr->images().get("images/" + image_basename + ".png"), tooltip_text);
123 box_.add(button);
124 if (window) {
125- if (!window->on_create) {
126- window->assign_toggle_button(button);
127- registries_.push_back(*window);
128+ if (window->window) {
129+ button->set_style(UI::Button::Style::kPermpressed);
130 }
131+ window->opened.connect(boost::bind(&UI::Button::set_style, button, UI::Button::Style::kPermpressed));
132+ window->closed.connect(boost::bind(&UI::Button::set_style, button, UI::Button::Style::kRaised));
133 button->sigclicked.connect(
134 boost::bind(&UI::UniqueWindow::Registry::toggle, boost::ref(*window)));
135 }
136@@ -83,7 +84,4 @@
137 }
138
139 GameStatisticsMenu::~GameStatisticsMenu() {
140- for (auto& registry : registries_) {
141- registry.unassign_toggle_button();
142- }
143 }
144
145=== modified file 'src/wui/game_statistics_menu.h'
146--- src/wui/game_statistics_menu.h 2017-02-12 09:10:57 +0000
147+++ src/wui/game_statistics_menu.h 2017-11-29 08:57:42 +0000
148@@ -47,10 +47,6 @@
149 InteractivePlayer& player_;
150 InteractivePlayer::GameMainMenuWindows& windows_;
151 UI::Box box_;
152-
153- // These get collected by add_button
154- // so we can call unassign_toggle_button on them in the destructor.
155- std::vector<UI::UniqueWindow::Registry> registries_;
156 };
157
158 #endif // end of include guard: WL_WUI_GAME_STATISTICS_MENU_H
159
160=== modified file 'src/wui/interactive_base.cc'
161--- src/wui/interactive_base.cc 2017-09-17 18:17:50 +0000
162+++ src/wui/interactive_base.cc 2017-11-29 08:57:42 +0000
163@@ -192,9 +192,6 @@
164 if (buildroad_) {
165 abort_build_road();
166 }
167- for (auto& registry : registries_) {
168- registry.unassign_toggle_button();
169- }
170 }
171
172 const InteractiveBase::BuildhelpOverlay*
173@@ -286,8 +283,9 @@
174 g_gr->images().get("images/" + image_basename + ".png"), tooltip_text);
175 toolbar_.add(button);
176 if (window) {
177- window->assign_toggle_button(button);
178- registries_.push_back(*window);
179+ window->opened.connect([this, button] { button->set_style(UI::Button::Style::kPermpressed); });
180+ window->closed.connect([this, button] { button->set_style(UI::Button::Style::kRaised); });
181+
182 if (bind_default_toggle) {
183 button->sigclicked.connect(
184 boost::bind(&UI::UniqueWindow::Registry::toggle, boost::ref(*window)));
185
186=== modified file 'src/wui/interactive_base.h'
187--- src/wui/interactive_base.h 2017-09-13 07:27:00 +0000
188+++ src/wui/interactive_base.h 2017-11-29 08:57:42 +0000
189@@ -260,10 +260,6 @@
190 MapView map_view_;
191 ChatOverlay* chat_overlay_;
192
193- // These get collected by add_toolbar_button
194- // so we can call unassign_toggle_button on them in the destructor.
195- std::vector<UI::UniqueWindow::Registry> registries_;
196-
197 UI::Box toolbar_;
198 // No unique_ptr on purpose: 'minimap_' is a UniqueWindow, its parent will
199 // delete it.

Subscribers

People subscribed via source and target branches

to status/vote changes: