SCI: Fix control/Fx keys not working anymore

Was effectively caused by commit adding the keyboard driver bug
for SCI0/SCI01, although the bug is actually real and happens.

It seems Sierra did not check the key-modifier in kMenuSelect.
We do and that's why the code didn't recognize all sorts of
menu keys anymore.
We now isolate the lower byte before comparing.

I also noticed, that Sierra passed keyboard modifiers in mouse
events. This was probably done, so that owners of a 1-button
mouse were able to right-click. We do this now too.

Also added information about mouse modifiers in kGetEvent.

Moved the mouse modifier code into getScummVMEvent().

This should fix bug #7009.

3 files changed, 37 insertions, 33 deletions

diff --git a/engines/sci/engine/kevent.cpp b/engines/sci/engine/kevent.cpp
index beaad2628a..0e5cd7ae12 100644
--- a/engines/sci/engine/kevent.cpp
+++ b/engines/sci/engine/kevent.cpp
@@ -126,13 +126,13 @@ reg_t kGetEvent(EngineState *s, int argc, reg_t *argv) {
 
 		// It seems Sierra fixed this behaviour (effectively bug) in the SCI1 keyboard driver.
 		// SCI32 also resets the upper byte.
+
+		// This was verified in SSCI itself by creating a SCI game and checking behavior.
 		if (getSciVersion() <= SCI_VERSION_01) {
 			modifiers |= 0x0200;
 		}
 	}
 
-	//s->_gui->moveCursor(s->gfx_state->pointer_pos.x, s->gfx_state->pointer_pos.y);
-
 	switch (curEvent.type) {
 	case SCI_EVENT_QUIT:
 		s->abortScriptProcessing = kAbortQuitGame; // Terminate VM
@@ -151,7 +151,6 @@ reg_t kGetEvent(EngineState *s, int argc, reg_t *argv) {
 
 	case SCI_EVENT_MOUSE_RELEASE:
 	case SCI_EVENT_MOUSE_PRESS:
-
 		// track left buttton clicks, if requested
 		if (curEvent.type == SCI_EVENT_MOUSE_PRESS && curEvent.data == 1 && g_debug_track_mouse_clicks) {
 			g_sci->getSciDebugger()->debugPrintf("Mouse clicked at %d, %d\n",
@@ -159,19 +158,6 @@ reg_t kGetEvent(EngineState *s, int argc, reg_t *argv) {
 		}
 
 		if (mask & curEvent.type) {
-			int extra_bits = 0;
-
-			switch (curEvent.data) {
-			case 2:
-				extra_bits = SCI_KEYMOD_LSHIFT | SCI_KEYMOD_RSHIFT;
-				break;
-			case 3:
-				extra_bits = SCI_KEYMOD_CTRL;
-			default:
-				break;
-			}
-			modifiers |= extra_bits; // add these additional bits to the mix
-
 			writeSelectorValue(segMan, obj, SELECTOR(type), curEvent.type);
 			writeSelectorValue(segMan, obj, SELECTOR(message), 0);
 			writeSelectorValue(segMan, obj, SELECTOR(modifiers), modifiers);
diff --git a/engines/sci/event.cpp b/engines/sci/event.cpp
index 90ddaaf967..2d4fc0f50c 100644
--- a/engines/sci/event.cpp
+++ b/engines/sci/event.cpp
@@ -174,11 +174,36 @@ SciEvent EventManager::getScummVMEvent() {
 		return input;
 	}
 
+	int ourModifiers = em->getModifierState();
+
+	input.modifiers =
+		((ourModifiers & Common::KBD_ALT) ? SCI_KEYMOD_ALT : 0) |
+		((ourModifiers & Common::KBD_CTRL) ? SCI_KEYMOD_CTRL : 0) |
+		((ourModifiers & Common::KBD_SHIFT) ? SCI_KEYMOD_LSHIFT | SCI_KEYMOD_RSHIFT : 0);
+		// Caps lock and Scroll lock have been removed, cause we already handle upper
+		// case keys ad Scroll lock doesn't seem to be used anywhere
+		//((ourModifiers & Common::KBD_CAPS) ? SCI_KEYMOD_CAPSLOCK : 0) |
+		//((ourModifiers & Common::KBD_SCRL) ? SCI_KEYMOD_SCRLOCK : 0) |
+
 	// Handle mouse events
 	for (int i = 0; i < ARRAYSIZE(mouseEventMappings); i++) {
 		if (mouseEventMappings[i].commonType == ev.type) {
 			input.type = mouseEventMappings[i].sciType;
 			input.data = mouseEventMappings[i].data;
+			// Sierra passed keyboard modifiers for mouse events, too.
+
+			// Sierra also set certain modifiers within their mouse interrupt handler
+			// This whole thing was probably meant for people using a mouse, that only featured 1 button
+			// So the user was able to press Ctrl and click the mouse button to create a right click.
+			switch (input.data) {
+			case 2: // right button click
+				input.modifiers |= (SCI_KEYMOD_RSHIFT | SCI_KEYMOD_LSHIFT); // this value was hardcoded in the mouse interrupt handler
+				break;
+			case 3: //  middle button click
+				input.modifiers |= SCI_KEYMOD_CTRL; // this value was hardcoded in the mouse interrupt handler
+			default:
+				break;
+			}
 			return input;
 		}
 	}
@@ -197,23 +222,12 @@ SciEvent EventManager::getScummVMEvent() {
 
 	// Process keyboard events
 
-	int modifiers = em->getModifierState();
 	bool numlockOn = (ev.kbd.flags & Common::KBD_NUM);
 
 	input.data = ev.kbd.keycode;
 	input.character = ev.kbd.ascii;
 	input.type = SCI_EVENT_KEYBOARD;
 
-	input.modifiers =
-		((modifiers & Common::KBD_ALT) ? SCI_KEYMOD_ALT : 0) |
-		((modifiers & Common::KBD_CTRL) ? SCI_KEYMOD_CTRL : 0) |
-		((modifiers & Common::KBD_SHIFT) ? SCI_KEYMOD_LSHIFT | SCI_KEYMOD_RSHIFT : 0);
-
-	// Caps lock and Scroll lock have been removed, cause we already handle upper
-	// case keys ad Scroll lock doesn't seem to be used anywhere
-		//((ev.kbd.flags & Common::KBD_CAPS) ? SCI_KEYMOD_CAPSLOCK : 0) |
-		//((ev.kbd.flags & Common::KBD_SCRL) ? SCI_KEYMOD_SCRLOCK : 0) |
-
 	if (input.data >= Common::KEYCODE_KP0 && input.data <= Common::KEYCODE_KP9) {
 		if (!(ev.kbd.flags & Common::KBD_NUM)) {
 			// HACK: Num-Lock not enabled
@@ -241,7 +255,7 @@ SciEvent EventManager::getScummVMEvent() {
 		}
 		if (input.data == Common::KEYCODE_TAB) {
 			input.character = input.data = SCI_KEY_TAB;
-			if (modifiers & Common::KBD_SHIFT)
+			if (ourModifiers & Common::KBD_SHIFT)
 				input.character = SCI_KEY_SHIFT_TAB;
 		}
 		if (input.data == Common::KEYCODE_DELETE)
@@ -250,7 +264,7 @@ SciEvent EventManager::getScummVMEvent() {
 		// SCI_K_F1 == 59 << 8
 		// SCI_K_SHIFT_F1 == 84 << 8
 		input.character = input.data = SCI_KEY_F1 + ((input.data - Common::KEYCODE_F1)<<8);
-		if (modifiers & Common::KBD_SHIFT)
+		if (ourModifiers & Common::KBD_SHIFT)
 			input.character = input.data + 0x1900;
 	} else {
 		// Special keys that need conversion
@@ -265,13 +279,13 @@ SciEvent EventManager::getScummVMEvent() {
 	// When Ctrl AND Alt are pressed together with a regular key, Linux will give us control-key, Windows will give
 	//  us the actual key. My opinion is that windows is right, because under DOS the keys worked the same, anyway
 	//  we support the other case as well
-	if ((modifiers & Common::KBD_ALT) && input.character > 0 && input.character < 27)
+	if ((ourModifiers & Common::KBD_ALT) && input.character > 0 && input.character < 27)
 		input.character += 96; // 0x01 -> 'a'
 
 	// Scancodify if appropriate
-	if (modifiers & Common::KBD_ALT)
+	if (ourModifiers & Common::KBD_ALT)
 		input.character = altify(input.character);
-	if (getSciVersion() <= SCI_VERSION_1_MIDDLE && (modifiers & Common::KBD_CTRL) && input.character > 0 && input.character < 27)
+	if (getSciVersion() <= SCI_VERSION_1_MIDDLE && (ourModifiers & Common::KBD_CTRL) && input.character > 0 && input.character < 27)
 		input.character += 96; // 0x01 -> 'a'
 
 	// If no actual key was pressed (e.g. if only a modifier key was pressed),
diff --git a/engines/sci/graphics/menu.cpp b/engines/sci/graphics/menu.cpp
index 8e8c1d64c2..9d92039111 100644
--- a/engines/sci/graphics/menu.cpp
+++ b/engines/sci/graphics/menu.cpp
@@ -424,8 +424,12 @@ reg_t GfxMenu::kernelSelect(reg_t eventObject, bool pauseSound) {
 		default:
 			while (itemIterator != itemEnd) {
 				itemEntry = *itemIterator;
+				// Sierra actually did not check the modifier, they only checked the ascii code
+				// Which is why for example pressing Ctrl-I and Shift-Ctrl-I both brought up the inventory in QfG1
+				// We still check the modifier, but we need to isolate the lower byte, because of a keyboard
+				// driver bug (see engine/kevent.cpp / kGetEvent)
 				if (itemEntry->keyPress == keyPress &&
-					itemEntry->keyModifier == keyModifier &&
+					itemEntry->keyModifier == (keyModifier & 0xFF) &&
 					itemEntry->enabled)
 					break;
 				itemIterator++;