← Back to team overview

widelands-dev team mailing list archive

Re: [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands

 

Added some replies/questions.

Diff comments:

> 
> === added file 'src/ui_basic/dropdown.cc'
> --- src/ui_basic/dropdown.cc	1970-01-01 00:00:00 +0000
> +++ src/ui_basic/dropdown.cc	2016-10-28 07:37:53 +0000
> @@ -0,0 +1,213 @@
> +/*
> + * Copyright (C) 2016 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + *
> + */
> +
> +#include "ui_basic/dropdown.h"
> +
> +#include <algorithm>
> +
> +#include <boost/format.hpp>
> +
> +#include "base/i18n.h"
> +#include "graphic/align.h"
> +#include "graphic/font_handler1.h"
> +#include "graphic/graphic.h"
> +#include "graphic/image.h"
> +#include "graphic/rendertarget.h"
> +#include "ui_basic/mouse_constants.h"
> +
> +namespace UI {
> +
> +BaseDropdown::BaseDropdown(
> +   UI::Panel* parent, int32_t x, int32_t y, uint32_t w, uint32_t h, const std::string& label)
> +   : UI::Panel(
> +        parent,
> +        x,
> +        y,
> +        w,
> +        std::max(24,
> +                 UI::g_fh1->render(as_uifont(UI::g_fh1->fontset()->representative_character()))
> +                       ->height() +
> +                    2)),  // Height only to fit the button, so we can use this in Box layout.
> +     max_list_height_(h - 2 * get_h()),
> +     mouse_tolerance_(50),
> +     button_box_(this, 0, 0, UI::Box::Horizontal, w, h),
> +     push_button_(&button_box_,
> +                  "dropdown_select",
> +                  0,
> +                  0,
> +                  24,
> +                  get_h(),
> +                  g_gr->images().get("images/ui_basic/but3.png"),
> +                  g_gr->images().get("images/ui_basic/scrollbar_down.png"),
> +                  pgettext("dropdown", "Select Item")),
> +     display_button_(&button_box_,
> +                     "dropdown_label",
> +                     0,
> +                     0,
> +                     w - 24,
> +                     get_h(),
> +                     g_gr->images().get("images/ui_basic/but1.png"),
> +                     label),
> +     // Hook into parent so we can drop down outside the panel
> +     list_(parent, x, y + get_h(), w, 0, ListselectLayout::kDropdown),
> +     label_(label) {
> +	list_.set_visible(false);
> +	list_.set_background(g_gr->images().get("images/ui_basic/but1.png"));
> +	// NOCOM(#codereview): I find this looks weird - having the button
> +	// permanently pressed. Why did you decide on that?
> +	display_button_.set_perm_pressed(true);

Because it looks better in the UI.

> +	button_box_.add(&display_button_, UI::Align::kLeft);
> +	button_box_.add(&push_button_, UI::Align::kLeft);
> +	button_box_.set_size(w, get_h());
> +
> +	display_button_.sigclicked.connect(boost::bind(&BaseDropdown::toggle_list, this));
> +	push_button_.sigclicked.connect(boost::bind(&BaseDropdown::toggle_list, this));
> +	list_.clicked.connect(boost::bind(&BaseDropdown::set_value, this));
> +	list_.clicked.connect(boost::bind(&BaseDropdown::toggle_list, this));
> +	set_can_focus(true);
> +}
> +
> +void BaseDropdown::add(const std::string& name,
> +                       const uint32_t value,
> +                       const Image* pic,
> +                       const bool select_this,
> +                       const std::string& tooltip_text) {
> +	list_.set_size(
> +	   list_.get_w(), std::min(list_.get_h() + list_.get_lineheight(), max_list_height_));
> +	list_.add(name, value, pic, select_this, tooltip_text);
> +	if (select_this) {
> +		set_value();
> +	}
> +}
> +
> +bool BaseDropdown::has_selection() const {
> +	return list_.has_selection();
> +}
> +
> +uint32_t BaseDropdown::get_selected() const {
> +	return list_.get_selected();
> +}
> +
> +void BaseDropdown::set_label(const std::string& text) {
> +	label_ = text;
> +	display_button_.set_title(label_);
> +}
> +
> +void BaseDropdown::set_tooltip(const std::string& text) {
> +	tooltip_ = text;
> +	display_button_.set_tooltip(tooltip_);
> +}
> +
> +void BaseDropdown::set_enabled(bool on) {
> +	set_can_focus(on);
> +	push_button_.set_enabled(on);
> +	push_button_.set_tooltip(on ? pgettext("dropdown", "Select Item") : "");
> +	display_button_.set_enabled(on);
> +	list_.set_visible(false);
> +}
> +
> +void BaseDropdown::set_pos(Vector2i point) {
> +	UI::Panel::set_pos(point);
> +	list_.set_pos(Vector2i(point.x, point.y + get_h()));
> +}
> +
> +void BaseDropdown::clear() {
> +	list_.clear();
> +	list_.set_size(list_.get_w(), 0);
> +	set_layout_toplevel(false);
> +}
> +
> +void BaseDropdown::think() {
> +	if (list_.is_visible()) {
> +		// Autocollapse with a bit of tolerance for the mouse movement to make it less fiddly.
> +		if (!(has_focus() || list_.has_focus()) || is_mouse_away()) {
> +			toggle_list();
> +		}
> +	}
> +}
> +
> +uint32_t BaseDropdown::size() const {
> +	return list_.size();
> +}
> +
> +void BaseDropdown::set_value() {
> +	const std::string name = list_.has_selection() ? list_.get_selected_name() :
> +	                                                 /** TRANSLATORS: Selection in Dropdown menus. */
> +	                            pgettext("dropdown", "Not Selected");
> +
> +	if (label_.empty()) {
> +		display_button_.set_title(name);
> +	} else {
> +		/** TRANSLATORS: Label: Value. */
> +		display_button_.set_title((boost::format(_("%1%: %2%")) % label_ % (name)).str());
> +	}
> +	display_button_.set_tooltip(list_.has_selection() ? list_.get_selected_tooltip() : tooltip_);
> +	selected();
> +	current_selection_ = list_.selection_index();
> +}
> +
> +void BaseDropdown::toggle_list() {
> +	list_.set_visible(!list_.is_visible());
> +	if (list_.is_visible()) {
> +		list_.move_to_top();
> +		focus();
> +	}
> +	// Make sure that the list covers and deactivates the elements below it
> +	set_layout_toplevel(list_.is_visible());
> +}
> +
> +bool BaseDropdown::is_mouse_away() const {
> +	return (get_mouse_position().x + mouse_tolerance_) < 0 ||
> +	       get_mouse_position().x > (get_w() + mouse_tolerance_) ||
> +	       (get_mouse_position().y + mouse_tolerance_ / 2) < 0 ||
> +	       get_mouse_position().y > (get_h() + list_.get_h() + mouse_tolerance_);
> +}
> +
> +bool BaseDropdown::handle_key(bool down, SDL_Keysym code) {
> +	if (down) {
> +		switch (code.sym) {
> +		case SDLK_KP_ENTER:
> +		case SDLK_RETURN:
> +			if (list_.is_visible()) {
> +				set_value();
> +			}
> +		case SDLK_ESCAPE:
> +			if (list_.is_visible()) {
> +				list_.select(current_selection_);
> +				toggle_list();
> +				return true;
> +			}
> +			break;
> +		case SDLK_DOWN:
> +			if (!list_.is_visible() && !is_mouse_away()) {
> +				toggle_list();
> +				return true;
> +			}
> +			break;
> +		default:
> +			break;  // not handled
> +		}
> +	}
> +	if (list_.is_visible()) {
> +		return list_.handle_key(down, code);
> +	}
> +	return false;
> +}
> +
> +}  // namespace UI
> 
> === added file 'src/ui_basic/dropdown.h'
> --- src/ui_basic/dropdown.h	1970-01-01 00:00:00 +0000
> +++ src/ui_basic/dropdown.h	2016-10-28 07:37:53 +0000
> @@ -0,0 +1,174 @@
> +/*
> + * Copyright (C) 2016 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + *
> + */
> +
> +#ifndef WL_UI_BASIC_DROPDOWN_H
> +#define WL_UI_BASIC_DROPDOWN_H
> +
> +#include <deque>
> +
> +#include <boost/signals2.hpp>
> +
> +#include "ui_basic/box.h"
> +#include "ui_basic/button.h"
> +#include "ui_basic/listselect.h"
> +#include "ui_basic/panel.h"
> +
> +namespace UI {
> +
> +/// Implementation for a dropdown menu that lets the user select a value.
> +// NOCOM(#codereview): Move all function implementation in the .cc file for clarity. 

I don't know how to do that with templates, so I just followed the same implementation pattern that Listselect uses.

> +class BaseDropdown : public Panel {
> +protected:
> +	/// \param parent     the parent panel
> +	/// \param x          the x-position within 'parent'
> +	/// \param y          the y-position within 'parent'
> +	/// \param w          the dropdown's width
> +	/// \param h          the maximum height for the dropdown list
> +	/// \param label      a label to prefix to the selected entry on the display button.
> +	BaseDropdown(
> +	   Panel* parent, int32_t x, int32_t y, uint32_t w, uint32_t h, const std::string& label);
> +	~BaseDropdown() {
> +		clear();
> +	}
> +
> +public:
> +	boost::signals2::signal<void()> selected;
> +
> +	/// \return true if an element has been selected from the list
> +	bool has_selection() const;
> +
> +	/// Sets a label that will be prefixed to the currently selected element's name
> +	/// and displayed on the display button.
> +	void set_label(const std::string& text);
> +
> +	/// Sets the tooltip for the display button.
> +	void set_tooltip(const std::string& text);
> +
> +	/// Enables/disables the dropdown selection.
> +	void set_enabled(bool on);
> +
> +	/// Move the dropdown. The dropdown's position is relative to the parent in
> +	/// pixels.
> +	void set_pos(Vector2i point) override;
> +
> +	/// The number of elements listed in the dropdown.
> +	uint32_t size() const;
> +
> +	/// Handle keypresses
> +	bool handle_key(bool down, SDL_Keysym code) override;
> +
> +protected:
> +	/// Add an element to the list
> +	/// \param name         the display name of the entry
> +	/// \param value        the index of the entry
> +	/// \param pic          an image to illustrate the entry
> +	/// \param select_this  whether this element should be selected
> +	/// \param tooltip_text a tooltip for this entry
> +	void add(const std::string& name,
> +	         uint32_t value,
> +	         const Image* pic = nullptr,
> +	         const bool select_this = false,
> +	         const std::string& tooltip_text = std::string());
> +
> +	/// \return the index of the selected element
> +	uint32_t get_selected() const;
> +
> +	/// Removes all elements from the list.
> +	void clear();
> +
> +	/// Automatically collapses the list if the mouse gets too far away from the dropdown, or if it
> +	/// loses focus.
> +	void think() override;
> +
> +private:
> +	/// Updates the title and tooltip of the display button and triggers a 'selected' signal.
> +	void set_value();
> +	/// Toggles the dropdown list on and off.
> +	void toggle_list();
> +
> +	/// Returns true if the mouse pointer left the vicinity of the dropdown.
> +	bool is_mouse_away() const;
> +
> +	uint32_t max_list_height_;
> +	const int mouse_tolerance_;  // Allow mouse outside the panel a bit before autocollapse
> +	UI::Box button_box_;
> +	UI::Button push_button_;
> +	UI::Button display_button_;
> +	UI::Listselect<uintptr_t> list_;
> +	std::string label_;
> +	std::string tooltip_;
> +	uint32_t current_selection_;
> +};
> +
> +/// A dropdown menu that lets the user select a value of the datatype 'Entry'.
> +template <typename Entry> class Dropdown : public BaseDropdown {
> +public:
> +	/// \param parent     the parent panel
> +	/// \param x          the x-position within 'parent'
> +	/// \param y          the y-position within 'parent'
> +	/// \param w          the dropdown's width
> +	/// \param h          the maximum height for the dropdown list
> +	/// \param label      a label to prefix to the selected entry on the display button.
> +	///                   entry.
> +	Dropdown(Panel* parent, int32_t x, int32_t y, uint32_t w, uint32_t h, const std::string& label)
> +	   : BaseDropdown(parent, x, y, w, h, label) {
> +	}
> +	~Dropdown() {
> +		clear();
> +	}
> +
> +	/// Add an element to the list
> +	/// \param name         the display name of the entry
> +	/// \param value        the value for the entry
> +	/// \param pic          an image to illustrate the entry
> +	/// \param select_this  whether this element should be selected
> +	/// \param tooltip_text a tooltip for this entry
> +	void add(const std::string& name,
> +	         Entry value,
> +	         const Image* pic = nullptr,
> +	         const bool select_this = false,
> +	         const std::string& tooltip_text = std::string()) {
> +		entry_cache_.push_back(new Entry(value));
> +		BaseDropdown::add(name, size(), pic, select_this, tooltip_text);
> +	}
> +
> +	/// \return the selected element
> +	const Entry& get_selected() const {
> +		return *entry_cache_[BaseDropdown::get_selected()];
> +	}
> +
> +	/// Removes all elements from the list.
> +	void clear() {
> +		BaseDropdown::clear();
> +		// NOCOM(#codereview): change the deque to contain unique_ptr<> and
> +		// remove the manually deletion here.

OK, will have a look :)

> +		for (Entry* entry : entry_cache_) {
> +			delete entry;
> +		}
> +		entry_cache_.clear();
> +	}
> +
> +private:
> +	// Contains the actual elements. The BaseDropdown registers the indices only.
> +	std::deque<Entry*> entry_cache_;
> +};
> +
> +}  // namespace UI
> +
> +#endif  // end of include guard: WL_UI_BASIC_DROPDOWN_H
> 
> === modified file 'src/ui_basic/listselect.cc'
> --- src/ui_basic/listselect.cc	2016-10-16 20:35:47 +0000
> +++ src/ui_basic/listselect.cc	2016-10-28 07:37:53 +0000
> @@ -277,6 +279,14 @@
>   * Throws an exception when no item is selected.
>   */
>  uint32_t BaseListselect::get_selected() const {
> +	// NOCOM(#codereview): Using exceptions here is wrong: this function should
> +	// return a sentinal value that nothing is selected and the others should
> +	// assert that something is seletected - i.e. the caller has to make sure
> +	// that something is selected before calling the functions.
> +	// Exceptions should only be used for something exceptional, not for side
> +	// loading a diferent return value kind.
> +	// Feel free to just add a TODO, but for your new functions, I suggest
> +	// requiring that something is selected.

OK, will have a look :)

>  	if (selection_ == no_selection_index())
>  		throw NoSelection();
>  
> @@ -288,12 +298,35 @@
>   * item is selected.
>   */
>  void BaseListselect::remove_selected() {
> +	// NOCOM(#codereview): I think these should assert that something is seletecd 

OK, will have a look :)

>  	if (selection_ == no_selection_index())
>  		throw NoSelection();
>  
>  	remove(selection_);
>  }
>  
> +/**
> + * \return The name of the currently selected entry.
> + * \throw  NoSelection() if no entry has been selected
> + */
> +const std::string& BaseListselect::get_selected_name() const {
> +	if (selection_ == no_selection_index())
> +		throw NoSelection();
> +
> +	return entry_records_[selection_]->name;
> +}
> +
> +/**
> + * \return The tooltip for the currently selected entry.
> + * \throw  NoSelection() if no entry has been selected
> + */
> +const std::string& BaseListselect::get_selected_tooltip() const {
> +	if (selection_ == no_selection_index())
> +		throw NoSelection();
> +
> +	return entry_records_[selection_]->tooltip;
> +}
> +
>  uint32_t BaseListselect::get_lineheight() const {
>  	return lineheight_ + kMargin;
>  }
> 
> === 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 07:37:53 +0000
> @@ -203,68 +198,113 @@
>  }
>  
>  /**
> - * 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. 

I'd rather refactor that when I do the actual implementation of MP, because it will be obvious then which properties are common.

> +			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();
> +}
> +
> +// NOCOM(#codereview): Can this not be a free standing function? It seems it is not using any state.

I'd rather refactor that when I do the actual implementation of MP, because it will be obvious then which properties are common.

> +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.


References