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

[GunChleoc]

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

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.

[SirVer (sirver)]

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

review: Approve

[GunChleoc (gunchleoc)]

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

[Klaus Halfmann (klaus-halfmann)]

Thanks for all the cleanup you did!

[bunnybot (widelandsofficial)]

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.

[bunnybot (widelandsofficial)]

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.

[GunChleoc (gunchleoc)]

MacOX can't install sphinx

@bunnybot merge force

=== modified file 'src/ui_basic/unique_window.cc'
--- src/ui_basic/unique_window.cc	2017-02-22 07:14:15 +0000
+++ src/ui_basic/unique_window.cc	2017-11-29 08:57:42 +0000
@@ -71,21 +71,6 @@
 	delete window;
 }
 
-void UniqueWindow::Registry::assign_toggle_button(UI::Button* button) {
-	assert(!on_create);
-	assert(!on_delete);
-	on_create = boost::bind(&UI::Button::set_style, button, UI::Button::Style::kPermpressed);
-	on_delete = boost::bind(&UI::Button::set_style, button, UI::Button::Style::kRaised);
-	if (window) {
-		button->set_style(UI::Button::Style::kPermpressed);
-	}
-}
-
-void UniqueWindow::Registry::unassign_toggle_button() {
-	on_create = 0;
-	on_delete = 0;
-}
-
 /**
  * Register, position according to the registry information.
 */
@@ -104,9 +89,7 @@
 			set_pos(Vector2i(registry_->x, registry_->y));
 			usedefaultpos_ = false;
 		}
-		if (registry_->on_create) {
-			registry_->on_create();
-		}
+		registry_->opened();
 	}
 }
 
@@ -122,9 +105,7 @@
 		registry_->y = get_y();
 		registry_->valid_pos = true;
 
-		if (registry_->on_delete) {
-			registry_->on_delete();
-		}
+		registry_->closed();
 	}
 }
 }
 
=== modified file 'src/ui_basic/unique_window.h'
--- src/ui_basic/unique_window.h	2017-01-25 18:55:59 +0000
+++ src/ui_basic/unique_window.h	2017-11-29 08:57:42 +0000
@@ -22,6 +22,8 @@
 
 #include <functional>
 
+#include <boost/signals2.hpp>
+
 #include "ui_basic/button.h"
 #include "ui_basic/window.h"
 
@@ -41,11 +43,10 @@
 		// closed.
 		std::function<void()> open_window;
 
-		// Called when the window opens.
-		std::function<void()> on_create;
-
-		// Called when the window is deleted (i.e. closed).
-		std::function<void()> on_delete;
+		// Triggered when the window opens
+		boost::signals2::signal<void()> opened;
+		// Triggered when the window closes
+		boost::signals2::signal<void()> closed;
 
 		void create();
 		void destroy();
@@ -60,15 +61,6 @@
 		Registry() : window(nullptr), x(0), y(0), valid_pos(false) {
 		}
 		~Registry();
-
-		/// The 'button' will be permpressed or not depending on whether this window is
-		/// open (on_create/on_delete callback function hooks).
-		/// This can be assigned only once.
-		void assign_toggle_button(UI::Button* button);
-
-		/// Remove the callback as a safeguard in case somewhere else in the
-		/// code someone would overwrite our hooks.
-		void unassign_toggle_button();
 	};
 
 	UniqueWindow(Panel* parent,
 
=== modified file 'src/wui/game_options_menu.cc'
--- src/wui/game_options_menu.cc	2017-02-21 07:56:18 +0000
+++ src/wui/game_options_menu.cc	2017-11-29 08:57:42 +0000
@@ -114,14 +114,17 @@
 	exit_game_.sigclicked.connect(
 	   boost::bind(&GameOptionsMenu::clicked_exit_game, boost::ref(*this)));
 
-	windows_.sound_options.assign_toggle_button(&sound_);
+	if (windows_.sound_options.window) {
+		sound_.set_style(UI::Button::Style::kPermpressed);
+	}
+	windows_.sound_options.opened.connect(boost::bind(&UI::Button::set_style, &sound_, UI::Button::Style::kPermpressed));
+	windows_.sound_options.closed.connect(boost::bind(&UI::Button::set_style, &sound_, UI::Button::Style::kRaised));
 
 	if (get_usedefaultpos())
 		center_to_parent();
 }
 
 GameOptionsMenu::~GameOptionsMenu() {
-	windows_.sound_options.unassign_toggle_button();
 }
 
 void GameOptionsMenu::clicked_save_game() {
 
=== modified file 'src/wui/game_statistics_menu.cc'
--- src/wui/game_statistics_menu.cc	2017-02-21 07:56:18 +0000
+++ src/wui/game_statistics_menu.cc	2017-11-29 08:57:42 +0000
@@ -72,10 +72,11 @@
 	                  g_gr->images().get("images/" + image_basename + ".png"), tooltip_text);
 	box_.add(button);
 	if (window) {
-		if (!window->on_create) {
-			window->assign_toggle_button(button);
-			registries_.push_back(*window);
+		if (window->window) {
+			button->set_style(UI::Button::Style::kPermpressed);
 		}
+		window->opened.connect(boost::bind(&UI::Button::set_style, button, UI::Button::Style::kPermpressed));
+		window->closed.connect(boost::bind(&UI::Button::set_style, button, UI::Button::Style::kRaised));
 		button->sigclicked.connect(
 		   boost::bind(&UI::UniqueWindow::Registry::toggle, boost::ref(*window)));
 	}
@@ -83,7 +84,4 @@
 }
 
 GameStatisticsMenu::~GameStatisticsMenu() {
-	for (auto& registry : registries_) {
-		registry.unassign_toggle_button();
-	}
 }
 
=== modified file 'src/wui/game_statistics_menu.h'
--- src/wui/game_statistics_menu.h	2017-02-12 09:10:57 +0000
+++ src/wui/game_statistics_menu.h	2017-11-29 08:57:42 +0000
@@ -47,10 +47,6 @@
 	InteractivePlayer& player_;
 	InteractivePlayer::GameMainMenuWindows& windows_;
 	UI::Box box_;
-
-	// These get collected by add_button
-	// so we can call unassign_toggle_button on them in the destructor.
-	std::vector<UI::UniqueWindow::Registry> registries_;
 };
 
 #endif  // end of include guard: WL_WUI_GAME_STATISTICS_MENU_H
 
=== modified file 'src/wui/interactive_base.cc'
--- src/wui/interactive_base.cc	2017-09-17 18:17:50 +0000
+++ src/wui/interactive_base.cc	2017-11-29 08:57:42 +0000
@@ -192,9 +192,6 @@
 	if (buildroad_) {
 		abort_build_road();
 	}
-	for (auto& registry : registries_) {
-		registry.unassign_toggle_button();
-	}
 }
 
 const InteractiveBase::BuildhelpOverlay*
@@ -286,8 +283,9 @@
 	   g_gr->images().get("images/" + image_basename + ".png"), tooltip_text);
 	toolbar_.add(button);
 	if (window) {
-		window->assign_toggle_button(button);
-		registries_.push_back(*window);
+		window->opened.connect([this, button] { button->set_style(UI::Button::Style::kPermpressed); });
+		window->closed.connect([this, button] { button->set_style(UI::Button::Style::kRaised); });
+
 		if (bind_default_toggle) {
 			button->sigclicked.connect(
 			   boost::bind(&UI::UniqueWindow::Registry::toggle, boost::ref(*window)));
 
=== modified file 'src/wui/interactive_base.h'
--- src/wui/interactive_base.h	2017-09-13 07:27:00 +0000
+++ src/wui/interactive_base.h	2017-11-29 08:57:42 +0000
@@ -260,10 +260,6 @@
 	MapView map_view_;
 	ChatOverlay* chat_overlay_;
 
-	// These get collected by add_toolbar_button
-	// so we can call unassign_toggle_button on them in the destructor.
-	std::vector<UI::UniqueWindow::Registry> registries_;
-
 	UI::Box toolbar_;
 	// No unique_ptr on purpose: 'minimap_' is a UniqueWindow, its parent will
 	// delete it.