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

[Notabilis]

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

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.

[Benedikt Straub (nordfriese)]

Code LGTM, not tested.
Thanks!

review: Approve (code)

[bunnybot (widelandsofficial)]

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.

[GunChleoc (gunchleoc)]

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)]

I have added the info to the tooltips.

@bunnybot merge

review: Approve

[Notabilis (notabilis27)]

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

[bunnybot (widelandsofficial)]

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.

[GunChleoc (gunchleoc)]

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

[GunChleoc (gunchleoc)]

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.

[hessenfarmer (stephan-lutz)]

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

[hessenfarmer (stephan-lutz)]

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

[GunChleoc (gunchleoc)]

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)]

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)]

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

[Benedikt Straub (nordfriese)]

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)]

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)]

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)]

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)]

@bunnybot merge

[bunnybot (widelandsofficial)]

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.

[bunnybot (widelandsofficial)]

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.

[GunChleoc (gunchleoc)]

@bunnybot merge

[bunnybot (widelandsofficial)]

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.

[bunnybot (widelandsofficial)]

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.

[GunChleoc (gunchleoc)]

@bunnybot merge

=== modified file 'src/wui/inputqueuedisplay.cc'
--- src/wui/inputqueuedisplay.cc	2019-02-23 11:00:49 +0000
+++ src/wui/inputqueuedisplay.cc	2019-04-24 15:17:48 +0000
@@ -21,6 +21,8 @@
 
 #include <algorithm>
 
+#include <boost/format.hpp>
+
 #include "economy/input_queue.h"
 #include "economy/request.h"
 #include "graphic/graphic.h"
@@ -240,18 +242,35 @@
 	uint32_t x = Border;
 	uint32_t y = Border + (total_height_ - 2 * Border - WARE_MENU_PIC_WIDTH) / 2;
 
+	boost::format tooltip_format("%s<br><p><font size=%d bold=0>%s<br>%s</font></p>");
+
 	decrease_max_fill_ = new UI::Button(
 	   this, "decrease_max_fill", x, y, WARE_MENU_PIC_WIDTH, WARE_MENU_PIC_HEIGHT,
 	   UI::ButtonStyle::kWuiMenu, g_gr->images().get("images/ui_basic/scrollbar_left.png"),
-	   _("Decrease the number of wares you want to be stored here."));
+				(tooltip_format
+				 /** TRANSLATORS: Button tooltip in in a building's wares input queue */
+				 % _("Decrease the number of wares you want to be stored here")
+				 % UI_FONT_SIZE_MESSAGE
+				 /** TRANSLATORS: Button tooltip in in a building's wares input queue - option explanation */
+				 % _("Hold down Shift to decrease all ware types at the same time")
+				 /** TRANSLATORS: Button tooltip in in a building's wares input queue - option explanation */
+				 % _("Hold down Ctrl to allow none of this ware")).str());
 	decrease_max_fill_->sigclicked.connect(
 	   boost::bind(&InputQueueDisplay::decrease_max_fill_clicked, boost::ref(*this)));
 
 	x = Border + (cache_size_ + 1) * (CellWidth + CellSpacing);
+
 	increase_max_fill_ = new UI::Button(
 	   this, "increase_max_fill", x, y, WARE_MENU_PIC_WIDTH, WARE_MENU_PIC_HEIGHT,
 	   UI::ButtonStyle::kWuiMenu, g_gr->images().get("images/ui_basic/scrollbar_right.png"),
-	   _("Increase the number of wares you want to be stored here."));
+				(tooltip_format
+				 /** TRANSLATORS: Button tooltip in a building's wares input queue */
+				 % _("Increase the number of wares you want to be stored here")
+				 % UI_FONT_SIZE_MESSAGE
+				 /** TRANSLATORS: Button tooltip in in a building's wares input queue - option explanation */
+				 % _("Hold down Shift to increase all ware types at the same time")
+				 /** TRANSLATORS: Button tooltip in in a building's wares input queue - option explanation */
+				 % _("Hold down Ctrl to allow all of this ware")).str());
 	increase_max_fill_->sigclicked.connect(
 	   boost::bind(&InputQueueDisplay::increase_max_fill_clicked, boost::ref(*this)));
 
@@ -285,7 +304,7 @@
 		return;
 	}
 	if (SDL_GetModState() & KMOD_CTRL) {
-		update_siblings(state);
+		update_siblings_priority(state);
 	}
 	igb_.game().send_player_set_ware_priority(building_, type_, index_, priority);
 }
@@ -294,11 +313,11 @@
 	// Already set option has been clicked again
 	// Unimportant for this queue, but update other queues
 	if (SDL_GetModState() & KMOD_CTRL) {
-		update_siblings(priority_radiogroup_->get_state());
+		update_siblings_priority(priority_radiogroup_->get_state());
 	}
 }
 
-void InputQueueDisplay::update_siblings(int32_t state) {
+void InputQueueDisplay::update_siblings_priority(int32_t state) {
 	// "Release" the CTRL key to avoid recursion
 	const SDL_Keymod old_modifiers = SDL_GetModState();
 	SDL_SetModState(KMOD_NONE);
@@ -340,19 +359,63 @@
  * stored here has been clicked
  */
 void InputQueueDisplay::decrease_max_fill_clicked() {
-	assert(cache_max_fill_ > 0);
 	if (!igb_.can_act(building_.owner().player_number())) {
 		return;
 	}
-	igb_.game().send_player_set_input_max_fill(building_, index_, type_, cache_max_fill_ - 1);
+
+	// Update the value of this queue if required
+	if (cache_max_fill_ > 0) {
+		igb_.game().send_player_set_input_max_fill(building_, index_, type_,
+			((SDL_GetModState() & KMOD_CTRL) ? 0 : cache_max_fill_ - 1));
+	}
+
+	// Update other queues of this building
+	if (SDL_GetModState() & KMOD_SHIFT) {
+		// Using int16_t instead of int32_t on purpose to avoid over-/underflows
+		update_siblings_fill(
+			((SDL_GetModState() & KMOD_CTRL) ? std::numeric_limits<int16_t>::min() : -1));
+	}
 }
 
 void InputQueueDisplay::increase_max_fill_clicked() {
-	assert(cache_max_fill_ < queue_.get_max_size());
 	if (!igb_.can_act(building_.owner().player_number())) {
 		return;
 	}
-	igb_.game().send_player_set_input_max_fill(building_, index_, type_, cache_max_fill_ + 1);
+
+	if (cache_max_fill_ < cache_size_) {
+		igb_.game().send_player_set_input_max_fill(building_, index_, type_,
+			((SDL_GetModState() & KMOD_CTRL) ? cache_size_ : cache_max_fill_ + 1));
+	}
+
+	if (SDL_GetModState() & KMOD_SHIFT) {
+		update_siblings_fill(
+			((SDL_GetModState() & KMOD_CTRL) ? std::numeric_limits<int16_t>::max() : 1));
+	}
+}
+
+void InputQueueDisplay::update_siblings_fill(int32_t delta) {
+	Panel* sibling = get_parent()->get_first_child();
+	// Well, at least we should be a child of our parent
+	assert(sibling != nullptr);
+	do {
+		if (sibling == this) {
+			// We already have been set
+			continue;
+		}
+		InputQueueDisplay* display = dynamic_cast<InputQueueDisplay*>(sibling);
+		if (display == nullptr) {
+			// Cast failed. Sibling is no InputQueueDisplay
+			continue;
+		}
+		uint32_t new_fill = std::max(0,
+								std::min<int32_t>(
+									static_cast<int32_t>(display->cache_max_fill_) + delta,
+									display->cache_size_));
+		if (new_fill != display->cache_max_fill_) {
+			igb_.game().send_player_set_input_max_fill(
+											building_, display->index_, display->type_, new_fill);
+		}
+	} while ((sibling = sibling->get_next_sibling()));
 }
 
 void InputQueueDisplay::compute_max_fill_buttons_enabled_state() {
@@ -364,11 +427,5 @@
 			increase_max_fill_->set_enabled(false);
 		if (decrease_max_fill_)
 			decrease_max_fill_->set_enabled(false);
-	} else {
-
-		if (decrease_max_fill_)
-			decrease_max_fill_->set_enabled(cache_max_fill_ > 0);
-		if (increase_max_fill_)
-			increase_max_fill_->set_enabled(cache_max_fill_ < queue_.get_max_size());
 	}
 }

=== modified file 'src/wui/inputqueuedisplay.h'
--- src/wui/inputqueuedisplay.h	2019-02-23 11:00:49 +0000
+++ src/wui/inputqueuedisplay.h	2019-04-24 15:17:48 +0000
@@ -87,7 +87,8 @@
 	void increase_max_fill_clicked();
 	void radiogroup_changed(int32_t);
 	void radiogroup_clicked();
-	void update_siblings(int32_t);
+	void update_siblings_priority(int32_t);
+	void update_siblings_fill(int32_t);
 
 	void compute_max_fill_buttons_enabled_state();
 };