Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
You are right, it makes the function easier to read, so I've pulled it out. Thanks for the review :) @bunnybot merge -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
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 +
> +++ src/ui_fsmenu/launch_spg.cc 2016-10-28 18:04:48 +
> @@ -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 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 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 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' "
> +
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
you also need to merge trunk it seems.
Diff comments:
>
> === added file 'src/ui_basic/dropdown.cc'
> --- src/ui_basic/dropdown.cc 1970-01-01 00:00:00 +
> +++ src/ui_basic/dropdown.cc 2016-10-28 07:37:53 +
> @@ -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
> +
> +#include
> +
> +#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_(_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_(_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);
I am not UX savy, so I am fine with leaving it like it is. But to me, the
permanently pressed button does not signalize the affordance[1] of being
clickable anymore.
[1]
http://www.medien.ifi.lmu.de/fileadmin/mimuc/mmi_ws0506/essays/uebung2-holzer.html
> + button_box_.add(_button_, UI::Align::kLeft);
> + button_box_.add(_button_, UI::Align::kLeft);
> + button_box_.set_size(w, get_h());
> +
> +
> display_button_.sigclicked.connect(boost::bind(::toggle_list,
> this));
> + push_button_.sigclicked.connect(boost::bind(::toggle_list,
> this));
> + list_.clicked.connect(boost::bind(::set_value, this));
> + list_.clicked.connect(boost::bind(::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);
> +
Re: [Widelands-dev] [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 +
> +++ src/ui_basic/dropdown.cc 2016-10-28 07:37:53 +
> @@ -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
> +
> +#include
> +
> +#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_(_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_(_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(_button_, UI::Align::kLeft);
> + button_box_.add(_button_, UI::Align::kLeft);
> + button_box_.set_size(w, get_h());
> +
> +
> display_button_.sigclicked.connect(boost::bind(::toggle_list,
> this));
> + push_button_.sigclicked.connect(boost::bind(::toggle_list,
> this));
> + list_.clicked.connect(boost::bind(::set_value, this));
> + list_.clicked.connect(boost::bind(::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) {
> +
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Review: Needs Fixing A couple of change suggestions and questions in code -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
No, I think flattened is wrong for it - it is dropped down on top of the other elements, not below them. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Looks better now. Have you tested showing it flattened instead of raised? -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Beautification done :) I also want to give the scrollbar some love, but I'll have to do that in a separate branch, since that will affect all kinds of stuff. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
> One possible eye candy > nit/improvement could be a small line outside of the drop down at the bottom > and the sides - to connect the drop down better with the original button. +1 (i thought something like this also) -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Just tested under Mac OS. Really nice change. One possible eye candy nit/improvement could be a small line outside of the drop down at the bottom and the sides - to connect the drop down better with the original button. I will review the code eventually no hurry here, build 19 is still a few weeks off. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
That error can't possibly be related to this branch, since you already said that it happens in trunk as well. It's worth reporting as a bug though. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Review: Approve testing Great :-) -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Good point - deactivated the key when the mouse is not on the dropdown. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
One little thing: If the dropdown box is once opened and closed (with moving away the mouse pointer) and hitting the down arrow of the keyboard, the dropdownmenu sometimes pops up for little time. Best to see if you keep the down arrow down for a longer time. Not really an issue, so i leave it up to you to fix this. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Review: Approve compile, test OK, fine for me, I did not take a deep look at that code, sorry. As I can test macOS (Sierra) only, I would like to see Windows and Linux approvals, too -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Review: Resubmit Collapse with the scrollbar should be fixed now. Also instant highlighting for mouse movement. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Review: Needs Fixing Thanks for fixing the crash :-) The checkmark is IMHO good now. But clicking on the vertical slider in Options->Language let the drop down menu disappear. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Maybe I am, but your input is very helpful! I just gave it a spin, I think it would be best if the mouse position / current selection gets highlighted, and no tick. Back to the drawing board :) -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Review: Needs Fixing I get a crash as soon i click on one of the dropdown menus in options: src/ui_basic/panel.h:265: bool UI::Panel::has_focus() const: Assertion `get_can_focus()' failed. Backtrace: https://bugs.launchpad.net/widelands/+bug/536489/+attachment/4750279/+files/backtrace.txt -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Review: Needs Fixing compile, test Hello Gun, thanks for your work. * the Autoclose when leaving feels reasonbale. OK for me * Esc-Key is handled as expected. There is one confusion however: The menu has a Selection - via a background color - and a checkmark. When I use the cursor keys to scroll both, the selection and the checkmark change. So I assume I made the choice. When I then leave the list, I find that my selection was not applied (so the checkmark is wrong). Please improve as follows: do not move the checkmark until the selection in finally confirmed by clicking or by pressing return. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Good points. The escape key thing should be easy to do, for the collapse when clicked outside of it I will need to dig around a bit. Thanks for testing! -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-536489-dropdown into lp:widelands
Review: Needs Fixing compile, test This Drodown is missing a feature found in all normal GUIs: when you click outside the dropdown it will NOT collapse, allowing the user to click somewhere else. This implementation will do nothing in this case. So if I forgot that the Dropdown is open and click somwhere else It seems stuck as I get no visual feedback what to do. This may frustrate players upto a level where theey will kill the game. In additon it does not collapse when pressing Escape, instead the complete dialog is canceled. Sorry Gun. Good work otherwise. I will continue to review the usage and code. -- 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
