widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Approve

I added a inline comment there. Essentially, I still think it is worthwhile for clarity to pull this function out. 

otherwise lgtm. 

Diff comments:

> 
> === modified file 'src/ui_fsmenu/launch_spg.cc'
> --- src/ui_fsmenu/launch_spg.cc	2016-10-24 14:05:58 +0000
> +++ src/ui_fsmenu/launch_spg.cc	2016-10-28 18:04:48 +0000
> @@ -203,68 +198,114 @@
>  }
>  
>  /**
> - * WinCondition button has been pressed
> - */
> -void FullscreenMenuLaunchSPG::win_condition_clicked() {
> -	if (settings_->can_change_map()) {
> -		cur_wincondition_++;
> -		cur_wincondition_ %= win_condition_scripts_.size();
> -		settings_->set_win_condition_script(win_condition_scripts_[cur_wincondition_]);
> -	}
> -
> -	win_condition_update();
> -}
> -
> -/**
> - * update win conditions information
> - */
> -void FullscreenMenuLaunchSPG::win_condition_update() {
> + * Fill the dropdown with the available win conditions.
> + */
> +void FullscreenMenuLaunchSPG::load_win_conditions() {
> +	win_condition_dropdown_.clear();
>  	if (settings_->settings().scenario) {
> -		wincondition_.set_title(_("Scenario"));
> -		wincondition_.set_tooltip(_("Win condition is set through the scenario"));
> +		win_condition_dropdown_.set_label(_("Scenario"));
> +		win_condition_dropdown_.set_tooltip(_("Win condition is set through the scenario"));
> +		win_condition_dropdown_.set_enabled(false);
>  	} else {
> -		win_condition_load();
> +		win_condition_dropdown_.set_label("");
> +		win_condition_dropdown_.set_tooltip("");
> +		Widelands::Map map;
> +		std::unique_ptr<Widelands::MapLoader> ml =
> +		   map.get_correct_loader(settings_->settings().mapfilename);
> +		if (ml != nullptr) {
> +			// NOCOM(#codereview): pull out into a function? You want to reuse this in the MP anyways.
> +			// NOCOM(GunChleoc): Why not simply reuse all of load_win_conditions() in the MP?

that is even better, but this function is still too long and pulling this part out is improving the code.

> +			try {
> +				ml->preload_map(true);
> +				const std::set<std::string> tags = map.get_tags();
> +				// Make sure that the last win condition is still valid. If not, pick the first one
> +				// available.
> +				if (last_win_condition_.empty()) {
> +					last_win_condition_ = settings_->settings().win_condition_scripts.front();
> +				}
> +				std::unique_ptr<LuaTable> t = win_condition_if_valid(last_win_condition_, tags);
> +				for (const std::string& win_condition_script :
> +				     settings_->settings().win_condition_scripts) {
> +					if (t) {
> +						break;
> +					} else {
> +						last_win_condition_ = win_condition_script;
> +						t = win_condition_if_valid(last_win_condition_, tags);
> +					}
> +				}
> +
> +				// Now fill the dropdown.
> +				for (const std::string& win_condition_script :
> +				     settings_->settings().win_condition_scripts) {
> +					try {
> +						t = win_condition_if_valid(win_condition_script, tags);
> +						if (t) {
> +							i18n::Textdomain td("win_conditions");
> +							win_condition_dropdown_.add(
> +							   _(t->get_string("name")), win_condition_script, nullptr,
> +							   win_condition_script == last_win_condition_, t->get_string("description"));
> +						}
> +					} catch (LuaTableKeyError& e) {
> +						log("LaunchSPG: Error loading win condition: %s %s\n",
> +						    win_condition_script.c_str(), e.what());
> +					}
> +				}
> +			} catch (const std::exception& e) {
> +				const std::string error_message =
> +				   (boost::format(_("Unable to determine valid win conditions because the map '%s' "
> +				                    "could not be loaded.")) %
> +				    settings_->settings().mapfilename)
> +				      .str();
> +				win_condition_dropdown_.set_label(_("Error"));
> +				win_condition_dropdown_.set_tooltip(error_message);
> +				log("LaunchSPG: Exception: %s %s\n", error_message.c_str(), e.what());
> +			}
> +
> +		} else {
> +			const std::string error_message =
> +			   (boost::format(_("Unable to determine valid win conditions because the map '%s' could "
> +			                    "not be loaded.")) %
> +			    settings_->settings().mapfilename)
> +			      .str();
> +			win_condition_dropdown_.set_label(_("Error"));
> +			win_condition_dropdown_.set_tooltip(error_message);
> +			log("LaunchSPG: No map loader: %s\n", error_message.c_str());
> +		}
> +		win_condition_dropdown_.set_enabled(true);
>  	}
>  }
>  
> -/**
> - * Loads the current win condition script from the settings provider.
> - * Calls win_condition_clicked() if the current map can't handle the win condition.
> - */
> -void FullscreenMenuLaunchSPG::win_condition_load() {
> +void FullscreenMenuLaunchSPG::win_condition_selected() {
> +	last_win_condition_ = win_condition_dropdown_.get_selected();
> +}
> +
> +// TODO(GunChleoc): Turn this into a free standing function. It seems it is not using any state.
> +std::unique_ptr<LuaTable>
> +FullscreenMenuLaunchSPG::win_condition_if_valid(const std::string& win_condition_script,
> +                                                std::set<std::string> tags) const {
>  	bool is_usable = true;
> +	std::unique_ptr<LuaTable> t;
>  	try {
> -		std::unique_ptr<LuaTable> t = lua_->run_script(settings_->get_win_condition_script());
> +		t = lua_->run_script(win_condition_script);
>  		t->do_not_warn_about_unaccessed_keys();
>  
>  		// Skip this win condition if the map doesn't have all the required tags
> -		if (t->has_key("map_tags") && !settings_->settings().mapfilename.empty()) {
> -			Widelands::Map map;
> -			std::unique_ptr<Widelands::MapLoader> ml =
> -			   map.get_correct_loader(settings_->settings().mapfilename);
> -			ml->preload_map(true);
> +		if (t->has_key("map_tags")) {
>  			for (const std::string& map_tag : t->get_table("map_tags")->array_entries<std::string>()) {
> -				if (!map.has_tag(map_tag)) {
> +				if (!tags.count(map_tag)) {
>  					is_usable = false;
>  					break;
>  				}
>  			}
>  		}
> -
> -		const std::string name = t->get_string("name");
> -		const std::string descr = t->get_string("description");
> -		{
> -			i18n::Textdomain td("win_conditions");
> -			wincondition_.set_title(_(name));
> -		}
> -		wincondition_.set_tooltip(descr.c_str());
> -	} catch (LuaTableKeyError&) {
> -		// might be that this is not a win condition after all.
> -		is_usable = false;
> +	} catch (LuaTableKeyError& e) {
> +		log(
> +		   "LaunchSPG: Error loading win condition: %s %s\n", win_condition_script.c_str(), e.what());
>  	}
>  	if (!is_usable) {
> -		win_condition_clicked();
> +		t.reset(nullptr);
>  	}
> +	return t;
>  }
>  
>  /**


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-536489-dropdown/+merge/306303
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-536489-dropdown.