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.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
