widelands-dev team mailing list archive
-
widelands-dev team
-
Mailing list archive
-
Message #08584
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
