Merge lp:~widelands-dev/widelands/bug-1616661_messagebox_hotkeys into lp:widelands

[kaputtnik]

Change the hotkeys used in message box to filter messages from [1..5] to Alt + [1...5]. Squash multiple triggering of Alt keys cause by interference with Linux window managers.

Fixes conflict with hotkeys of saved positions of map.

Change the hotkeys used in message box to filter messages from [1..5] to Alt + [1...5]. The Tooltips and the help texts are also changed accordingly.

There is a NOCOMM left. I didn't found any tooltip which correspond to this code. So maybe it could be removed.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1311. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/158847433.
Appveyor build 1153. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1616661_messagebox_hotkeys-1153.

[Klaus Halfmann (klaus-halfmann)]

Please wait until this could be tested on OSX.
No idea how the Mapping of the Alt key workds with SDL on OSX yet.

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

Running 'bzr pull --overwrite' failed. Output:

ssh_exchange_identification: Connection closed by remote host
ConnectionReset reading response for 'BzrDir.open_2.1', retrying
ssh_exchange_identification: Connection closed by remote host
bzr: ERROR: Connection closed: Unexpected end of message. Please check connectivity and permissions, and report a bug if problems persist.
Using saved parent location: bzr+ssh://bazaar.launchpad.net/~widelands-dev/widelands/bug-1616661_messagebox_hotkeys/

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

Running 'bzr pull --overwrite' failed. Output:

ssh_exchange_identification: Connection closed by remote host
ConnectionReset reading response for 'BzrDir.open_2.1', retrying
ssh_exchange_identification: Connection closed by remote host
bzr: ERROR: Connection closed: Unexpected end of message. Please check connectivity and permissions, and report a bug if problems persist.
Using saved parent location: bzr+ssh://bazaar.launchpad.net/~widelands-dev/widelands/bug-1616661_messagebox_hotkeys/

[GunChleoc (gunchleoc)]

The message button access doesn't work on Ubuntu.

review: Needs Fixing

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1311. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/158847433.
Appveyor build 1153. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1616661_messagebox_hotkeys-1153.

[kaputtnik (franku)]

> The message button access doesn't work on Ubuntu.

Pressing "n"? Clicking the "Message" button? Those work here on arch-linux.

Until tomorrow i am away for 14 days so if something goes wrong with this branch feel free to change it.

[kaputtnik (franku)]

Until -> As from tomorrow :-S

[SirVer (sirver)]

Works on Mac! there is a NOCOM in the code still.

[GunChleoc (gunchleoc)]

If I use

    if (code.mod & (KMOD_LCTRL | KMOD_RCTRL)) {

it works with the control keys, If I use

    if (code.mod & (KMOD_LALT | KMOD_RALT)) {

It does not work with the Alt keys. Same code, different key *headdesk*

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1315. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/159135849.
Appveyor build 1157. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1616661_messagebox_hotkeys-1157.

[SirVer (sirver)]

Maybe linux uses different key modifiers for the keys? Maybe there is a type issue somewhere? Does
if ((code.mod & KMOD_LALT) || (code.mod & KMOD_RALT)) work?

[GunChleoc (gunchleoc)]

I have found out what happens:

if (code.mod & KMOD_CTRL) { triggers once, toggling the button off or on.

if (code.mod & KMOD_ALT) { triggers twice, toggling the button off and on.

The next question to solve is why / how to circumvent the bug.

The intention is that the user can hit the same key twice in order to toggle the button on and off, rather than having to use ALT + 0 to toggle off, so I'd rather not reprogram the filter_messages function.

[SirVer (sirver)]

I would print out all the key events that end up in this function and try to understand why with alt 2 messages are sent.

[GunChleoc (gunchleoc)]

These are the events I get for key down:

Ctrl alone:
===========
sym = 1073742048, mod = 0

Alt alone:
==========
sym = 1073742050, mod = 0
sym = 1073742050, mod = 256 (LALT)

Ctrl-1:
=======
sym = 1073742048, mod = 0
sym = 49, mod = 64 (LCTRL)
pressed 1

Alt-1:
======
sym = 1073742050, mod = 0
sym = 1073742050, mod = 256 (LALT)
sym = 49, mod = 256 (LALT)
pressed 1

sym = 1073742050, mod = 256 (LALT)
sym = 49, mod = 256 (LALT)
pressed 1

My best guess is that the window manager is interfering and messing things up. The only other place where we use the Alt key is in the tool selection in the editor, where it doesn't matter if the key is triggered multiple times.

[SirVer (sirver)]

I remember that we had issues with Windowmanagers and alt before - some use this key also to make the window draggable as a whole. Maybe we should stick to ctrl shortcuts then. Or how about using the F1-F5 keys?

[GunChleoc (gunchleoc)]

Hmmm, people usually associate F1 with help, F5 with refresh etc... also, 0 has some nice semantics that we can't have with the function keys.

I have found a workaround that will work for everything except toggling a button on and off with the same key while the game is paused. What do you think?

We could also try to attack the problem at the poll event stage, but I'd rather do that when we're not in feature freeze.

review: Needs Resubmitting

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1318. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/159808318.
Appveyor build 1160. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1616661_messagebox_hotkeys-1160.

[SirVer (sirver)]

review: Needs Fixing

[GunChleoc (gunchleoc)]

Yep, fixing this in WLApplication is much cleaner.

review: Needs Resubmitting

[SirVer (sirver)]

one comment inline, but I think this can go in the way it is.

review: Approve

[GunChleoc (gunchleoc)]

Reordering the events is no problem there because of how often the function is called. So, I decided to go for efficiency and less code ;)

So, all bugs closed ow for Build 19! *happy dance*

@bunnybot merge

=== modified file 'data/tribes/scripting/help/controls.lua'
--- data/tribes/scripting/help/controls.lua	2016-05-17 16:40:22 +0000
+++ data/tribes/scripting/help/controls.lua	2016-09-16 16:56:31 +0000
@@ -73,17 +73,17 @@
          h3(_"In the message window, the following additional shortcuts are available:") ..
          p(
                -- TRANSLATORS: This is the helptext for an access key combination.
-               dl(help_format_hotkey("0"), _"Show all messages") ..
-               -- TRANSLATORS: This is the helptext for an access key combination.
-               dl(help_format_hotkey("1"), _"Show geologists’ messages only") ..
-               -- TRANSLATORS: This is the helptext for an access key combination.
-               dl(help_format_hotkey("2"), _"Show economy messages only") ..
-               -- TRANSLATORS: This is the helptext for an access key combination.
-               dl(help_format_hotkey("3"), _"Show seafaring messages only") ..
-               -- TRANSLATORS: This is the helptext for an access key combination.
-               dl(help_format_hotkey("4"), _"Show warfare messages only") ..
-               -- TRANSLATORS: This is the helptext for an access key combination.
-               dl(help_format_hotkey("5"), _"Show scenario messages only") ..
+               dl(help_format_hotkey(pgettext("hotkey", "Alt + 0")), _"Show all messages") ..
+               -- TRANSLATORS: This is the helptext for an access key combination.
+               dl(help_format_hotkey(pgettext("hotkey", "Alt + 1")), _"Show geologists’ messages only") ..
+               -- TRANSLATORS: This is the helptext for an access key combination.
+               dl(help_format_hotkey(pgettext("hotkey", "Alt + 2")), _"Show economy messages only") ..
+               -- TRANSLATORS: This is the helptext for an access key combination.
+               dl(help_format_hotkey(pgettext("hotkey", "Alt + 3")), _"Show seafaring messages only") ..
+               -- TRANSLATORS: This is the helptext for an access key combination.
+               dl(help_format_hotkey(pgettext("hotkey", "Alt + 4")), _"Show warfare messages only") ..
+               -- TRANSLATORS: This is the helptext for an access key combination.
+               dl(help_format_hotkey(pgettext("hotkey", "Alt + 5")), _"Show scenario messages only") ..
                -- TRANSLATORS: This is the helptext for an access key combination.
                dl(help_format_hotkey("G"), _"Jump to the location corresponding to the current message") ..
                -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2016-08-13 12:16:14 +0000
+++ src/wlapplication.cc	2016-09-16 16:56:31 +0000
@@ -534,13 +534,26 @@
 }
 
 void WLApplication::handle_input(InputCallback const* cb) {
+	// Container for keyboard events using the Alt key.
+	// <sym, mod>, type.
+	std::map<std::pair<int32_t, uint16_t>, uint32_t> alt_events;
+
 	SDL_Event ev;
 	while (poll_event(ev)) {
 		switch (ev.type) {
 		case SDL_KEYUP:
 		case SDL_KEYDOWN: {
 			bool handled = false;
-			if (cb && cb->key) {
+			// Workaround for duplicate triggering of the Alt key in Ubuntu:
+			// Don't accept the same key twice, so we use a map to squash them and handle them later.
+			if (ev.key.keysym.mod & KMOD_ALT) {
+				alt_events.insert(std::make_pair<std::pair<int32_t, uint16_t>, uint32_t>(
+				   std::make_pair<int32_t, uint16_t>(static_cast<int32_t>(ev.key.keysym.sym),
+				                                     static_cast<uint16_t>(ev.key.keysym.mod)),
+				   static_cast<uint32_t>(ev.type)));
+				handled = true;
+			}
+			if (!handled && cb && cb->key) {
 				handled = cb->key(ev.type == SDL_KEYDOWN, ev.key.keysym);
 			}
 			if (!handled) {
@@ -575,6 +588,20 @@
 		default:;
 		}
 	}
+
+	// Now constructing the events for the Alt key from the container and handling them.
+	for (const auto& event : alt_events) {
+		ev.type = event.second;
+		ev.key.keysym.sym = event.first.first;
+		ev.key.keysym.mod = event.first.second;
+		bool handled = false;
+		if (cb && cb->key) {
+			handled = cb->key(ev.type == SDL_KEYDOWN, ev.key.keysym);
+		}
+		if (!handled) {
+			handle_key(ev.type == SDL_KEYDOWN, ev.key.keysym.sym, ev.key.keysym.mod);
+		}
+	}
 }
 
 /*

=== modified file 'src/wui/game_message_menu.cc'
--- src/wui/game_message_menu.cc	2016-08-04 15:49:05 +0000
+++ src/wui/game_message_menu.cc	2016-09-16 16:56:31 +0000
@@ -357,23 +357,41 @@
 				center_view();
 			return true;
 		case SDLK_0:
-			filter_messages(Widelands::Message::Type::kAllMessages);
-			return true;
+			if (code.mod & KMOD_ALT) {
+				filter_messages(Widelands::Message::Type::kAllMessages);
+				return true;
+			}
+			return false;
 		case SDLK_1:
-			filter_messages(Widelands::Message::Type::kGeologists);
-			return true;
+			if (code.mod & KMOD_ALT) {
+				filter_messages(Widelands::Message::Type::kGeologists);
+				return true;
+			}
+			return false;
 		case SDLK_2:
-			filter_messages(Widelands::Message::Type::kEconomy);
-			return true;
+			if (code.mod & KMOD_ALT) {
+				filter_messages(Widelands::Message::Type::kEconomy);
+				return true;
+			}
+			return false;
 		case SDLK_3:
-			filter_messages(Widelands::Message::Type::kSeafaring);
-			return true;
+			if (code.mod & KMOD_ALT) {
+				filter_messages(Widelands::Message::Type::kSeafaring);
+				return true;
+			}
+			return false;
 		case SDLK_4:
-			filter_messages(Widelands::Message::Type::kWarfare);
-			return true;
+			if (code.mod & KMOD_ALT) {
+				filter_messages(Widelands::Message::Type::kWarfare);
+				return true;
+			}
+			return false;
 		case SDLK_5:
-			filter_messages(Widelands::Message::Type::kScenario);
-			return true;
+			if (code.mod & KMOD_ALT) {
+				filter_messages(Widelands::Message::Type::kScenario);
+				return true;
+			}
+			return false;
 		case SDLK_DELETE:
 			archive_or_restore();
 			return true;
@@ -494,10 +512,11 @@
 		scenariobtn_->set_perm_pressed(false);
 		button.set_perm_pressed(true);
 		message_filter_ = msgtype;
+
 		/** TRANSLATORS: %1% is a tooltip, %2% is the corresponding hotkey */
 		button.set_tooltip((boost::format(_("%1% (Hotkey: %2%)"))
 		                    /** TRANSLATORS: Tooltip in the messages window */
-		                    % _("Show all messages") % "0")
+		                    % _("Show all messages") % pgettext("hotkey", "Alt + 0"))
 		                      .str());
 	}
 }
@@ -508,23 +527,24 @@
 void GameMessageMenu::set_filter_messages_tooltips() {
 	geologistsbtn_->set_tooltip((boost::format(_("%1% (Hotkey: %2%)"))
 	                             /** TRANSLATORS: Tooltip in the messages window */
-	                             % _("Show geologists' messages only") % "1")
+	                             % _("Show geologists' messages only") %
+	                             pgettext("hotkey", "Alt + 1"))
 	                               .str());
 	economybtn_->set_tooltip((boost::format(_("%1% (Hotkey: %2%)"))
 	                          /** TRANSLATORS: Tooltip in the messages window */
-	                          % _("Show economy messages only") % "2")
+	                          % _("Show economy messages only") % pgettext("hotkey", "Alt + 2"))
 	                            .str());
 	seafaringbtn_->set_tooltip((boost::format(_("%1% (Hotkey: %2%)"))
 	                            /** TRANSLATORS: Tooltip in the messages window */
-	                            % _("Show seafaring messages only") % "3")
+	                            % _("Show seafaring messages only") % pgettext("hotkey", "Alt + 3"))
 	                              .str());
 	warfarebtn_->set_tooltip((boost::format(_("%1% (Hotkey: %2%)"))
 	                          /** TRANSLATORS: Tooltip in the messages window */
-	                          % _("Show warfare messages only") % "4")
+	                          % _("Show warfare messages only") % pgettext("hotkey", "Alt + 4"))
 	                            .str());
 	scenariobtn_->set_tooltip((boost::format(_("%1% (Hotkey: %2%)"))
 	                           /** TRANSLATORS: Tooltip in the messages window */
-	                           % _("Show scenario messages only") % "5")
+	                           % _("Show scenario messages only") % pgettext("hotkey", "Alt + 5"))
 	                             .str());
 }