Merge lp:~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue into lp:widelands

Proposed by Notabilis
Status: Merged
Merged at revision: 9079
Proposed branch: lp:~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue
Merge into: lp:widelands
Diff against target: 167 lines (+74/-16)
2 files modified
src/wui/inputqueuedisplay.cc (+72/-15)
src/wui/inputqueuedisplay.h (+2/-1)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue
Reviewer Review Type Date Requested Status
GunChleoc Approve
Benedikt Straub code Approve
Review via email: mp+354732@code.launchpad.net

Commit message

Adding support for Ctrl and Shift keys when using the increase/decrease buttons of input queues.

Description of the change

Adding keyboard modifiers for faster adaption of input queue settings (see bug report).

Slight disadvantage of this branch: To allow a "decreasing" shift-click on an input queue that is already at minimum (and vice versa), the code to disable the buttons had to be removed. That means that the increase/decrease buttons will always look the same, even when a "normal" click on them won't change anything.

To post a comment you must log in.
Revision history for this message
Benedikt Straub (nordfriese) wrote :

Code LGTM, not tested.
Thanks!

review: Approve (code)
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 3943. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/427411087.
Appveyor build 3741. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1791891_key_modifier_inputqueue-3741.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Tested.

Now, how will the user find out about this feature? There should be additional info in the tooltip and/or in the helptexts explaining the keyboard shortcuts.

8824. By GunChleoc

Explain new keys in the inputqueues' tooltips.

Revision history for this message
GunChleoc (gunchleoc) wrote :

I have added the info to the tooltips.

@bunnybot merge

review: Approve
Revision history for this message
Notabilis (notabilis27) wrote :

Sorry, I wasn't able to work on this the last days. Your solution looks good, though, thanks!

Revision history for this message
bunnybot (widelandsofficial) wrote :

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 3981. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/428918967.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Can you merge trunk please, so that we can get this in and call the feature freeze?

Revision history for this message
GunChleoc (gunchleoc) wrote :

Since there is now a complication with this feature (see https://bugs.launchpad.net/widelands/+bug/1792774), let's postpone this - I'm calling the feature freeze now.

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

Perhaps it might be good to notice Bug #1792774 before merging

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

Using a different key might be a solution to have this quickly.
Don't know whether ALT is currently used.

Revision history for this message
GunChleoc (gunchleoc) wrote :

What is the usual Mac keyboard combination for Middle-click? If that's not CTRL-click, it might be better to change the window minimize shortcut.

Revision history for this message
Benedikt Straub (nordfriese) wrote :

Alt-Click often doesn´t work on my linux, as it´s a system key combination. Shift-Click for minimizing would be good imho. Super-Click would be better, but that wouldn´t work on Windows.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Shift-Click will gain us nothing, because we'll get the same misclick conflict as we have with Ctrl-Click.

Revision history for this message
Benedikt Straub (nordfriese) wrote :

Well the capacity buttons are larger and somewhat harder to miss than the priority buttons…
But Alt-click should be fine since it´s just an alternative to a middleclick (without modifiers) anyway.

Revision history for this message
Notabilis (notabilis27) wrote :

Personally, I would prefer to keep the modifiers in this branch as they are and modify the minimize feature, e.g., by only minimizing on clicking the window title.
The bug report only mentions the priority buttons, but we have the same problem with the dismantle and burn buttons. Admittedly the latter ones already are in build19 and nobody complained about them yet. But that would also mean that the big buttons of this branch will probably be fine independent of the bug report.

Unfortunately I am unable to merge trunk in this branch, since I am currently in a "secure" WiFi where only web and mail is allowed. So no bzr for me at the moment.

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

Hm, perhaps the problem really only arises for the priority buttons.
What about having them displayed bigger and horizontally instaead of vertically behind the inputs that would allow for better precision in clicking them.

Revision history for this message
GunChleoc (gunchleoc) wrote :

I think the priority buttons make a really tiny click target anyway, so making them bigger is a good idea.

I'm also OK with minimizing by clicking on the window title only.

Revision history for this message
GunChleoc (gunchleoc) wrote :

@bunnybot merge

Revision history for this message
bunnybot (widelandsofficial) wrote :

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 3981. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/428918967.

8825. By GunChleoc

Merged trunk.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4769. State: canceled. Details: https://travis-ci.org/widelands/widelands/builds/523574258.
Appveyor build 3779. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1791891_key_modifier_inputqueue-3779.

8826. By GunChleoc

Merged trunk.

8827. By GunChleoc

Fixed codecheck.

Revision history for this message
GunChleoc (gunchleoc) wrote :

@bunnybot merge

Revision history for this message
bunnybot (widelandsofficial) wrote :

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 4769. State: canceled. Details: https://travis-ci.org/widelands/widelands/builds/523574258.

8828. By GunChleoc

Merged trunk.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4787. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/524095891.
Appveyor build 4568. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1791891_key_modifier_inputqueue-4568.

Revision history for this message
GunChleoc (gunchleoc) wrote :

@bunnybot merge

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/wui/inputqueuedisplay.cc'
2--- src/wui/inputqueuedisplay.cc 2019-02-23 11:00:49 +0000
3+++ src/wui/inputqueuedisplay.cc 2019-04-24 15:17:48 +0000
4@@ -21,6 +21,8 @@
5
6 #include <algorithm>
7
8+#include <boost/format.hpp>
9+
10 #include "economy/input_queue.h"
11 #include "economy/request.h"
12 #include "graphic/graphic.h"
13@@ -240,18 +242,35 @@
14 uint32_t x = Border;
15 uint32_t y = Border + (total_height_ - 2 * Border - WARE_MENU_PIC_WIDTH) / 2;
16
17+ boost::format tooltip_format("%s<br><p><font size=%d bold=0>%s<br>%s</font></p>");
18+
19 decrease_max_fill_ = new UI::Button(
20 this, "decrease_max_fill", x, y, WARE_MENU_PIC_WIDTH, WARE_MENU_PIC_HEIGHT,
21 UI::ButtonStyle::kWuiMenu, g_gr->images().get("images/ui_basic/scrollbar_left.png"),
22- _("Decrease the number of wares you want to be stored here."));
23+ (tooltip_format
24+ /** TRANSLATORS: Button tooltip in in a building's wares input queue */
25+ % _("Decrease the number of wares you want to be stored here")
26+ % UI_FONT_SIZE_MESSAGE
27+ /** TRANSLATORS: Button tooltip in in a building's wares input queue - option explanation */
28+ % _("Hold down Shift to decrease all ware types at the same time")
29+ /** TRANSLATORS: Button tooltip in in a building's wares input queue - option explanation */
30+ % _("Hold down Ctrl to allow none of this ware")).str());
31 decrease_max_fill_->sigclicked.connect(
32 boost::bind(&InputQueueDisplay::decrease_max_fill_clicked, boost::ref(*this)));
33
34 x = Border + (cache_size_ + 1) * (CellWidth + CellSpacing);
35+
36 increase_max_fill_ = new UI::Button(
37 this, "increase_max_fill", x, y, WARE_MENU_PIC_WIDTH, WARE_MENU_PIC_HEIGHT,
38 UI::ButtonStyle::kWuiMenu, g_gr->images().get("images/ui_basic/scrollbar_right.png"),
39- _("Increase the number of wares you want to be stored here."));
40+ (tooltip_format
41+ /** TRANSLATORS: Button tooltip in a building's wares input queue */
42+ % _("Increase the number of wares you want to be stored here")
43+ % UI_FONT_SIZE_MESSAGE
44+ /** TRANSLATORS: Button tooltip in in a building's wares input queue - option explanation */
45+ % _("Hold down Shift to increase all ware types at the same time")
46+ /** TRANSLATORS: Button tooltip in in a building's wares input queue - option explanation */
47+ % _("Hold down Ctrl to allow all of this ware")).str());
48 increase_max_fill_->sigclicked.connect(
49 boost::bind(&InputQueueDisplay::increase_max_fill_clicked, boost::ref(*this)));
50
51@@ -285,7 +304,7 @@
52 return;
53 }
54 if (SDL_GetModState() & KMOD_CTRL) {
55- update_siblings(state);
56+ update_siblings_priority(state);
57 }
58 igb_.game().send_player_set_ware_priority(building_, type_, index_, priority);
59 }
60@@ -294,11 +313,11 @@
61 // Already set option has been clicked again
62 // Unimportant for this queue, but update other queues
63 if (SDL_GetModState() & KMOD_CTRL) {
64- update_siblings(priority_radiogroup_->get_state());
65+ update_siblings_priority(priority_radiogroup_->get_state());
66 }
67 }
68
69-void InputQueueDisplay::update_siblings(int32_t state) {
70+void InputQueueDisplay::update_siblings_priority(int32_t state) {
71 // "Release" the CTRL key to avoid recursion
72 const SDL_Keymod old_modifiers = SDL_GetModState();
73 SDL_SetModState(KMOD_NONE);
74@@ -340,19 +359,63 @@
75 * stored here has been clicked
76 */
77 void InputQueueDisplay::decrease_max_fill_clicked() {
78- assert(cache_max_fill_ > 0);
79 if (!igb_.can_act(building_.owner().player_number())) {
80 return;
81 }
82- igb_.game().send_player_set_input_max_fill(building_, index_, type_, cache_max_fill_ - 1);
83+
84+ // Update the value of this queue if required
85+ if (cache_max_fill_ > 0) {
86+ igb_.game().send_player_set_input_max_fill(building_, index_, type_,
87+ ((SDL_GetModState() & KMOD_CTRL) ? 0 : cache_max_fill_ - 1));
88+ }
89+
90+ // Update other queues of this building
91+ if (SDL_GetModState() & KMOD_SHIFT) {
92+ // Using int16_t instead of int32_t on purpose to avoid over-/underflows
93+ update_siblings_fill(
94+ ((SDL_GetModState() & KMOD_CTRL) ? std::numeric_limits<int16_t>::min() : -1));
95+ }
96 }
97
98 void InputQueueDisplay::increase_max_fill_clicked() {
99- assert(cache_max_fill_ < queue_.get_max_size());
100 if (!igb_.can_act(building_.owner().player_number())) {
101 return;
102 }
103- igb_.game().send_player_set_input_max_fill(building_, index_, type_, cache_max_fill_ + 1);
104+
105+ if (cache_max_fill_ < cache_size_) {
106+ igb_.game().send_player_set_input_max_fill(building_, index_, type_,
107+ ((SDL_GetModState() & KMOD_CTRL) ? cache_size_ : cache_max_fill_ + 1));
108+ }
109+
110+ if (SDL_GetModState() & KMOD_SHIFT) {
111+ update_siblings_fill(
112+ ((SDL_GetModState() & KMOD_CTRL) ? std::numeric_limits<int16_t>::max() : 1));
113+ }
114+}
115+
116+void InputQueueDisplay::update_siblings_fill(int32_t delta) {
117+ Panel* sibling = get_parent()->get_first_child();
118+ // Well, at least we should be a child of our parent
119+ assert(sibling != nullptr);
120+ do {
121+ if (sibling == this) {
122+ // We already have been set
123+ continue;
124+ }
125+ InputQueueDisplay* display = dynamic_cast<InputQueueDisplay*>(sibling);
126+ if (display == nullptr) {
127+ // Cast failed. Sibling is no InputQueueDisplay
128+ continue;
129+ }
130+ uint32_t new_fill = std::max(0,
131+ std::min<int32_t>(
132+ static_cast<int32_t>(display->cache_max_fill_) + delta,
133+ display->cache_size_));
134+ if (new_fill != display->cache_max_fill_) {
135+ igb_.game().send_player_set_input_max_fill(
136+ building_, display->index_, display->type_, new_fill);
137+ }
138+ } while ((sibling = sibling->get_next_sibling()));
139 }
140
141 void InputQueueDisplay::compute_max_fill_buttons_enabled_state() {
142@@ -364,11 +427,5 @@
143 increase_max_fill_->set_enabled(false);
144 if (decrease_max_fill_)
145 decrease_max_fill_->set_enabled(false);
146- } else {
147-
148- if (decrease_max_fill_)
149- decrease_max_fill_->set_enabled(cache_max_fill_ > 0);
150- if (increase_max_fill_)
151- increase_max_fill_->set_enabled(cache_max_fill_ < queue_.get_max_size());
152 }
153 }
154
155=== modified file 'src/wui/inputqueuedisplay.h'
156--- src/wui/inputqueuedisplay.h 2019-02-23 11:00:49 +0000
157+++ src/wui/inputqueuedisplay.h 2019-04-24 15:17:48 +0000
158@@ -87,7 +87,8 @@
159 void increase_max_fill_clicked();
160 void radiogroup_changed(int32_t);
161 void radiogroup_clicked();
162- void update_siblings(int32_t);
163+ void update_siblings_priority(int32_t);
164+ void update_siblings_fill(int32_t);
165
166 void compute_max_fill_buttons_enabled_state();
167 };

Subscribers

People subscribed via source and target branches

to status/vote changes: