widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Approve

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

Diff comments:

> 
> === 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 05:07:40 +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));

Please use a lambda function instead of bind. std::bind has unexpected behaviour around references and should be avoided. In c++14 there is no reason to ever use it, in c++11 there are very, very few.

> +	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 05:07:40 +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));

also lambdas.

> +		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)));
>  	}


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1734046-statistics_menu/+merge/334398
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1734046-statistics_menu.