widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Review: Needs Fixing

I can confirm the reduced CPU usage with this changes. However, when disabling and re-enabling a player position for single player the tribes of all players are reset. The problem are too frequent calls of set_player_names_and_tribes() inside update(). From a quick look through the code it seems update() is called too often anyway. I think most of the method should only be called when the selected map is changed. So probably split it up into something like "on_map_changed()" and "on_player_setting_changed()"?

Diff comments:

> === modified file 'src/ui_fsmenu/launch_game.cc'
> --- src/ui_fsmenu/launch_game.cc	2018-04-27 06:11:05 +0000
> +++ src/ui_fsmenu/launch_game.cc	2018-05-26 16:29:55 +0000
> @@ -78,13 +78,6 @@
>  	delete lua_;
>  }
>  
> -void FullscreenMenuLaunchGame::think() {
> -	if (ctrl_)

Since this method has been removed, ctrl_ is useless in FullscreenMenuLaunchGame and in FullscreenMenuLaunchSPG. Remove it and add it to FullscreenMenuLaunchMPG?
Tried it locally and it compiles without problems. Not idea whether we want this field for some future changes.

> -		ctrl_->think();
> -
> -	refresh();
> -}
> -
>  bool FullscreenMenuLaunchGame::init_win_condition_label() {
>  	if (settings_->settings().scenario) {
>  		win_condition_dropdown_.set_enabled(false);
> 
> === modified file 'src/ui_fsmenu/launch_mpg.h'
> --- src/ui_fsmenu/launch_mpg.h	2018-04-27 06:11:05 +0000
> +++ src/ui_fsmenu/launch_mpg.h	2018-05-26 16:29:55 +0000
> @@ -49,7 +49,8 @@
>  	~FullscreenMenuLaunchMPG() override;
>  
>  	void set_chat_provider(ChatProvider&);
> -	void refresh() override;
> +	void think() override;
> +	void refresh();

Make refresh() private?

>  
>  protected:
>  	void clicked_ok() override;
> 
> === modified file 'src/ui_fsmenu/launch_spg.cc'
> --- src/ui_fsmenu/launch_spg.cc	2018-04-27 06:11:05 +0000
> +++ src/ui_fsmenu/launch_spg.cc	2018-05-26 16:29:55 +0000
> @@ -201,9 +209,7 @@
>  	select_map_.set_visible(settings_->can_change_map());
>  	select_map_.set_enabled(settings_->can_change_map());
>  
> -	if (settings.scenario) {
> -		set_scenario_values();
> -	}
> +	set_player_names_and_tribes();
>  
>  	// "Choose Position" Buttons in frond of PDG

frond -> front.
Frond sounds friendly, though.

>  	for (uint8_t i = 0; i < nr_players_; ++i) {
> @@ -252,7 +260,7 @@
>   * player names and player tribes and take care about visibility
>   * and usability of all the parts of the UI.
>   */
> -void FullscreenMenuLaunchSPG::set_scenario_values() {
> +void FullscreenMenuLaunchSPG::set_player_names_and_tribes() {
>  	if (settings_->settings().mapfilename.empty()) {
>  		throw wexception("settings()->scenario was set to true, but no map is available");

This exception text is no longer quite correct.

>  	}


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1769344-sp-thinks-too-much/+merge/346911
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1769344-sp-thinks-too-much.