Merge lp:~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue into lp:widelands
- bug-1791891-key-modifier-inputqueue
- Merge into trunk
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 | ||||||||
Related bugs: |
|
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.
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 3943. State: failed. Details: https:/
Appveyor build 3741. State: success. Details: https:/
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.
GunChleoc (gunchleoc) wrote : | # |
I have added the info to the tooltips.
@bunnybot merge
Notabilis (notabilis27) wrote : | # |
Sorry, I wasn't able to work on this the last days. Your solution looks good, though, thanks!
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:/
GunChleoc (gunchleoc) wrote : | # |
Can you merge trunk please, so that we can get this in and call the feature freeze?
GunChleoc (gunchleoc) wrote : | # |
Since there is now a complication with this feature (see https:/
hessenfarmer (stephan-lutz) wrote : | # |
Perhaps it might be good to notice Bug #1792774 before merging
hessenfarmer (stephan-lutz) wrote : | # |
Using a different key might be a solution to have this quickly.
Don't know whether ALT is currently used.
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.
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.
GunChleoc (gunchleoc) wrote : | # |
Shift-Click will gain us nothing, because we'll get the same misclick conflict as we have with Ctrl-Click.
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.
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.
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.
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.
GunChleoc (gunchleoc) wrote : | # |
@bunnybot merge
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:/
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 4769. State: canceled. Details: https:/
Appveyor build 3779. State: success. Details: https:/
GunChleoc (gunchleoc) wrote : | # |
@bunnybot merge
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:/
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 4787. State: passed. Details: https:/
Appveyor build 4568. State: success. Details: https:/
GunChleoc (gunchleoc) wrote : | # |
@bunnybot merge
Preview Diff
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 | 21 | 21 | ||
6 | 22 | #include <algorithm> | 22 | #include <algorithm> |
7 | 23 | 23 | ||
8 | 24 | #include <boost/format.hpp> | ||
9 | 25 | |||
10 | 24 | #include "economy/input_queue.h" | 26 | #include "economy/input_queue.h" |
11 | 25 | #include "economy/request.h" | 27 | #include "economy/request.h" |
12 | 26 | #include "graphic/graphic.h" | 28 | #include "graphic/graphic.h" |
13 | @@ -240,18 +242,35 @@ | |||
14 | 240 | uint32_t x = Border; | 242 | uint32_t x = Border; |
15 | 241 | uint32_t y = Border + (total_height_ - 2 * Border - WARE_MENU_PIC_WIDTH) / 2; | 243 | uint32_t y = Border + (total_height_ - 2 * Border - WARE_MENU_PIC_WIDTH) / 2; |
16 | 242 | 244 | ||
17 | 245 | boost::format tooltip_format("%s<br><p><font size=%d bold=0>%s<br>%s</font></p>"); | ||
18 | 246 | |||
19 | 243 | decrease_max_fill_ = new UI::Button( | 247 | decrease_max_fill_ = new UI::Button( |
20 | 244 | this, "decrease_max_fill", x, y, WARE_MENU_PIC_WIDTH, WARE_MENU_PIC_HEIGHT, | 248 | this, "decrease_max_fill", x, y, WARE_MENU_PIC_WIDTH, WARE_MENU_PIC_HEIGHT, |
21 | 245 | UI::ButtonStyle::kWuiMenu, g_gr->images().get("images/ui_basic/scrollbar_left.png"), | 249 | UI::ButtonStyle::kWuiMenu, g_gr->images().get("images/ui_basic/scrollbar_left.png"), |
23 | 246 | _("Decrease the number of wares you want to be stored here.")); | 250 | (tooltip_format |
24 | 251 | /** TRANSLATORS: Button tooltip in in a building's wares input queue */ | ||
25 | 252 | % _("Decrease the number of wares you want to be stored here") | ||
26 | 253 | % UI_FONT_SIZE_MESSAGE | ||
27 | 254 | /** TRANSLATORS: Button tooltip in in a building's wares input queue - option explanation */ | ||
28 | 255 | % _("Hold down Shift to decrease all ware types at the same time") | ||
29 | 256 | /** TRANSLATORS: Button tooltip in in a building's wares input queue - option explanation */ | ||
30 | 257 | % _("Hold down Ctrl to allow none of this ware")).str()); | ||
31 | 247 | decrease_max_fill_->sigclicked.connect( | 258 | decrease_max_fill_->sigclicked.connect( |
32 | 248 | boost::bind(&InputQueueDisplay::decrease_max_fill_clicked, boost::ref(*this))); | 259 | boost::bind(&InputQueueDisplay::decrease_max_fill_clicked, boost::ref(*this))); |
33 | 249 | 260 | ||
34 | 250 | x = Border + (cache_size_ + 1) * (CellWidth + CellSpacing); | 261 | x = Border + (cache_size_ + 1) * (CellWidth + CellSpacing); |
35 | 262 | |||
36 | 251 | increase_max_fill_ = new UI::Button( | 263 | increase_max_fill_ = new UI::Button( |
37 | 252 | this, "increase_max_fill", x, y, WARE_MENU_PIC_WIDTH, WARE_MENU_PIC_HEIGHT, | 264 | this, "increase_max_fill", x, y, WARE_MENU_PIC_WIDTH, WARE_MENU_PIC_HEIGHT, |
38 | 253 | UI::ButtonStyle::kWuiMenu, g_gr->images().get("images/ui_basic/scrollbar_right.png"), | 265 | UI::ButtonStyle::kWuiMenu, g_gr->images().get("images/ui_basic/scrollbar_right.png"), |
40 | 254 | _("Increase the number of wares you want to be stored here.")); | 266 | (tooltip_format |
41 | 267 | /** TRANSLATORS: Button tooltip in a building's wares input queue */ | ||
42 | 268 | % _("Increase the number of wares you want to be stored here") | ||
43 | 269 | % UI_FONT_SIZE_MESSAGE | ||
44 | 270 | /** TRANSLATORS: Button tooltip in in a building's wares input queue - option explanation */ | ||
45 | 271 | % _("Hold down Shift to increase all ware types at the same time") | ||
46 | 272 | /** TRANSLATORS: Button tooltip in in a building's wares input queue - option explanation */ | ||
47 | 273 | % _("Hold down Ctrl to allow all of this ware")).str()); | ||
48 | 255 | increase_max_fill_->sigclicked.connect( | 274 | increase_max_fill_->sigclicked.connect( |
49 | 256 | boost::bind(&InputQueueDisplay::increase_max_fill_clicked, boost::ref(*this))); | 275 | boost::bind(&InputQueueDisplay::increase_max_fill_clicked, boost::ref(*this))); |
50 | 257 | 276 | ||
51 | @@ -285,7 +304,7 @@ | |||
52 | 285 | return; | 304 | return; |
53 | 286 | } | 305 | } |
54 | 287 | if (SDL_GetModState() & KMOD_CTRL) { | 306 | if (SDL_GetModState() & KMOD_CTRL) { |
56 | 288 | update_siblings(state); | 307 | update_siblings_priority(state); |
57 | 289 | } | 308 | } |
58 | 290 | igb_.game().send_player_set_ware_priority(building_, type_, index_, priority); | 309 | igb_.game().send_player_set_ware_priority(building_, type_, index_, priority); |
59 | 291 | } | 310 | } |
60 | @@ -294,11 +313,11 @@ | |||
61 | 294 | // Already set option has been clicked again | 313 | // Already set option has been clicked again |
62 | 295 | // Unimportant for this queue, but update other queues | 314 | // Unimportant for this queue, but update other queues |
63 | 296 | if (SDL_GetModState() & KMOD_CTRL) { | 315 | if (SDL_GetModState() & KMOD_CTRL) { |
65 | 297 | update_siblings(priority_radiogroup_->get_state()); | 316 | update_siblings_priority(priority_radiogroup_->get_state()); |
66 | 298 | } | 317 | } |
67 | 299 | } | 318 | } |
68 | 300 | 319 | ||
70 | 301 | void InputQueueDisplay::update_siblings(int32_t state) { | 320 | void InputQueueDisplay::update_siblings_priority(int32_t state) { |
71 | 302 | // "Release" the CTRL key to avoid recursion | 321 | // "Release" the CTRL key to avoid recursion |
72 | 303 | const SDL_Keymod old_modifiers = SDL_GetModState(); | 322 | const SDL_Keymod old_modifiers = SDL_GetModState(); |
73 | 304 | SDL_SetModState(KMOD_NONE); | 323 | SDL_SetModState(KMOD_NONE); |
74 | @@ -340,19 +359,63 @@ | |||
75 | 340 | * stored here has been clicked | 359 | * stored here has been clicked |
76 | 341 | */ | 360 | */ |
77 | 342 | void InputQueueDisplay::decrease_max_fill_clicked() { | 361 | void InputQueueDisplay::decrease_max_fill_clicked() { |
78 | 343 | assert(cache_max_fill_ > 0); | ||
79 | 344 | if (!igb_.can_act(building_.owner().player_number())) { | 362 | if (!igb_.can_act(building_.owner().player_number())) { |
80 | 345 | return; | 363 | return; |
81 | 346 | } | 364 | } |
83 | 347 | igb_.game().send_player_set_input_max_fill(building_, index_, type_, cache_max_fill_ - 1); | 365 | |
84 | 366 | // Update the value of this queue if required | ||
85 | 367 | if (cache_max_fill_ > 0) { | ||
86 | 368 | igb_.game().send_player_set_input_max_fill(building_, index_, type_, | ||
87 | 369 | ((SDL_GetModState() & KMOD_CTRL) ? 0 : cache_max_fill_ - 1)); | ||
88 | 370 | } | ||
89 | 371 | |||
90 | 372 | // Update other queues of this building | ||
91 | 373 | if (SDL_GetModState() & KMOD_SHIFT) { | ||
92 | 374 | // Using int16_t instead of int32_t on purpose to avoid over-/underflows | ||
93 | 375 | update_siblings_fill( | ||
94 | 376 | ((SDL_GetModState() & KMOD_CTRL) ? std::numeric_limits<int16_t>::min() : -1)); | ||
95 | 377 | } | ||
96 | 348 | } | 378 | } |
97 | 349 | 379 | ||
98 | 350 | void InputQueueDisplay::increase_max_fill_clicked() { | 380 | void InputQueueDisplay::increase_max_fill_clicked() { |
99 | 351 | assert(cache_max_fill_ < queue_.get_max_size()); | ||
100 | 352 | if (!igb_.can_act(building_.owner().player_number())) { | 381 | if (!igb_.can_act(building_.owner().player_number())) { |
101 | 353 | return; | 382 | return; |
102 | 354 | } | 383 | } |
104 | 355 | igb_.game().send_player_set_input_max_fill(building_, index_, type_, cache_max_fill_ + 1); | 384 | |
105 | 385 | if (cache_max_fill_ < cache_size_) { | ||
106 | 386 | igb_.game().send_player_set_input_max_fill(building_, index_, type_, | ||
107 | 387 | ((SDL_GetModState() & KMOD_CTRL) ? cache_size_ : cache_max_fill_ + 1)); | ||
108 | 388 | } | ||
109 | 389 | |||
110 | 390 | if (SDL_GetModState() & KMOD_SHIFT) { | ||
111 | 391 | update_siblings_fill( | ||
112 | 392 | ((SDL_GetModState() & KMOD_CTRL) ? std::numeric_limits<int16_t>::max() : 1)); | ||
113 | 393 | } | ||
114 | 394 | } | ||
115 | 395 | |||
116 | 396 | void InputQueueDisplay::update_siblings_fill(int32_t delta) { | ||
117 | 397 | Panel* sibling = get_parent()->get_first_child(); | ||
118 | 398 | // Well, at least we should be a child of our parent | ||
119 | 399 | assert(sibling != nullptr); | ||
120 | 400 | do { | ||
121 | 401 | if (sibling == this) { | ||
122 | 402 | // We already have been set | ||
123 | 403 | continue; | ||
124 | 404 | } | ||
125 | 405 | InputQueueDisplay* display = dynamic_cast<InputQueueDisplay*>(sibling); | ||
126 | 406 | if (display == nullptr) { | ||
127 | 407 | // Cast failed. Sibling is no InputQueueDisplay | ||
128 | 408 | continue; | ||
129 | 409 | } | ||
130 | 410 | uint32_t new_fill = std::max(0, | ||
131 | 411 | std::min<int32_t>( | ||
132 | 412 | static_cast<int32_t>(display->cache_max_fill_) + delta, | ||
133 | 413 | display->cache_size_)); | ||
134 | 414 | if (new_fill != display->cache_max_fill_) { | ||
135 | 415 | igb_.game().send_player_set_input_max_fill( | ||
136 | 416 | building_, display->index_, display->type_, new_fill); | ||
137 | 417 | } | ||
138 | 418 | } while ((sibling = sibling->get_next_sibling())); | ||
139 | 356 | } | 419 | } |
140 | 357 | 420 | ||
141 | 358 | void InputQueueDisplay::compute_max_fill_buttons_enabled_state() { | 421 | void InputQueueDisplay::compute_max_fill_buttons_enabled_state() { |
142 | @@ -364,11 +427,5 @@ | |||
143 | 364 | increase_max_fill_->set_enabled(false); | 427 | increase_max_fill_->set_enabled(false); |
144 | 365 | if (decrease_max_fill_) | 428 | if (decrease_max_fill_) |
145 | 366 | decrease_max_fill_->set_enabled(false); | 429 | decrease_max_fill_->set_enabled(false); |
146 | 367 | } else { | ||
147 | 368 | |||
148 | 369 | if (decrease_max_fill_) | ||
149 | 370 | decrease_max_fill_->set_enabled(cache_max_fill_ > 0); | ||
150 | 371 | if (increase_max_fill_) | ||
151 | 372 | increase_max_fill_->set_enabled(cache_max_fill_ < queue_.get_max_size()); | ||
152 | 373 | } | 430 | } |
153 | 374 | } | 431 | } |
154 | 375 | 432 | ||
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 | 87 | void increase_max_fill_clicked(); | 87 | void increase_max_fill_clicked(); |
160 | 88 | void radiogroup_changed(int32_t); | 88 | void radiogroup_changed(int32_t); |
161 | 89 | void radiogroup_clicked(); | 89 | void radiogroup_clicked(); |
163 | 90 | void update_siblings(int32_t); | 90 | void update_siblings_priority(int32_t); |
164 | 91 | void update_siblings_fill(int32_t); | ||
165 | 91 | 92 | ||
166 | 92 | void compute_max_fill_buttons_enabled_state(); | 93 | void compute_max_fill_buttons_enabled_state(); |
167 | 93 | }; | 94 | }; |
Code LGTM, not tested.
Thanks!