Wayland: Unstuck keys with same keycode

This fixes once and for all the core issue of different Godot `keycode`s
released from the same raw XKB keycode.

The `InputEventKey` `keycode` value _should_ map to the "unmodified"
key, but unfortunately there's an ambiguity with their encoding for
"special" keys ("delete", "insert", etc.), in witch they ignore their
unicode representation. This means that a key that is special when plain
but a character when modified would never be properly picked up, so we
do indeed change its keycode. As a consequence of this exception, some
Godot keys never receive release events and get "stuck".

This patch adds an extra check through an `HashMap` to "unstuck" keys
that changed while having the same keycode.

I also could not resist simplifying a bit the regular key event
generation method but this makes things more consistent and predictable
IMO.

Author: Riteo

platform/linuxbsd/wayland/key_mapping_xkb.cpp

@@ -369,33 +369,6 @@ void KeyMappingXKB::initialize() {
 	location_map[0x86] = KeyLocation::RIGHT;
 }
 
-bool KeyMappingXKB::is_sym_numpad(xkb_keysym_t p_keysym) {
-	switch (p_keysym) {
-		case XKB_KEY_KP_Equal:
-		case XKB_KEY_KP_Add:
-		case XKB_KEY_KP_Subtract:
-		case XKB_KEY_KP_Multiply:
-		case XKB_KEY_KP_Divide:
-		case XKB_KEY_KP_Separator:
-		case XKB_KEY_KP_Decimal:
-		case XKB_KEY_KP_Delete:
-		case XKB_KEY_KP_0:
-		case XKB_KEY_KP_1:
-		case XKB_KEY_KP_2:
-		case XKB_KEY_KP_3:
-		case XKB_KEY_KP_4:
-		case XKB_KEY_KP_5:
-		case XKB_KEY_KP_6:
-		case XKB_KEY_KP_7:
-		case XKB_KEY_KP_8:
-		case XKB_KEY_KP_9: {
-			return true;
-		} break;
-	}
-
-	return false;
-}
-
 Key KeyMappingXKB::get_keycode(xkb_keycode_t p_keysym) {
 	if (p_keysym >= 0x20 && p_keysym < 0x7E) { // ASCII, maps 1-1
 		if (p_keysym > 0x60 && p_keysym < 0x7B) { // Lowercase ASCII.

platform/linuxbsd/wayland/key_mapping_xkb.h

@@ -56,7 +56,6 @@ class KeyMappingXKB {
 public:
 	static void initialize();
 
-	static bool is_sym_numpad(xkb_keysym_t p_keysym);
 	static Key get_keycode(xkb_keysym_t p_keysym);
 	static xkb_keycode_t get_xkb_keycode(Key p_keycode);
 	static Key get_scancode(unsigned int p_code);

platform/linuxbsd/wayland/wayland_thread.cpp

@@ -191,31 +191,34 @@ Vector<uint8_t> WaylandThread::_wp_primary_selection_offer_read(struct wl_displa
 	return Vector<uint8_t>();
 }
 
-// Sets up an `InputEventKey` and returns whether it has any meaningful value.
-bool WaylandThread::_seat_state_configure_key_event(SeatState &p_ss, Ref<InputEventKey> p_event, xkb_keycode_t p_keycode, bool p_pressed) {
-	xkb_keysym_t shifted_sym = xkb_state_key_get_one_sym(p_ss.xkb_state, p_keycode);
+Ref<InputEventKey> WaylandThread::_seat_state_get_key_event(SeatState *p_ss, xkb_keycode_t p_keycode, bool p_pressed) {
+	Ref<InputEventKey> event;
 
-	xkb_keysym_t plain_sym = XKB_KEY_NoSymbol;
+	ERR_FAIL_NULL_V(p_ss, event);
+
+	Key shifted_key = KeyMappingXKB::get_keycode(xkb_state_key_get_one_sym(p_ss->xkb_state, p_keycode));
+
+	Key plain_key = Key::NONE;
 	// NOTE: xkbcommon's API really encourages to apply the modifier state but we
 	// only want a "plain" symbol so that we can convert it into a godot keycode.
 	const xkb_keysym_t *syms = nullptr;
-	int num_sys = xkb_keymap_key_get_syms_by_level(p_ss.xkb_keymap, p_keycode, p_ss.current_layout_index, 0, &syms);
+	int num_sys = xkb_keymap_key_get_syms_by_level(p_ss->xkb_keymap, p_keycode, p_ss->current_layout_index, 0, &syms);
 	if (num_sys > 0 && syms) {
-		plain_sym = syms[0];
+		plain_key = KeyMappingXKB::get_keycode(syms[0]);
 	}
 
 	Key physical_keycode = KeyMappingXKB::get_scancode(p_keycode);
 	KeyLocation key_location = KeyMappingXKB::get_location(p_keycode);
-	uint32_t unicode = xkb_state_key_get_utf32(p_ss.xkb_state, p_keycode);
+	uint32_t unicode = xkb_state_key_get_utf32(p_ss->xkb_state, p_keycode);
 
 	Key keycode = Key::NONE;
 
-	if (KeyMappingXKB::is_sym_numpad(shifted_sym) || KeyMappingXKB::is_sym_numpad(plain_sym)) {
-		keycode = KeyMappingXKB::get_keycode(shifted_sym);
+	if ((shifted_key & Key::SPECIAL) != Key::NONE || (plain_key & Key::SPECIAL) != Key::NONE) {
+		keycode = shifted_key;
 	}
 
 	if (keycode == Key::NONE) {
-		keycode = KeyMappingXKB::get_keycode(plain_sym);
+		keycode = plain_key;
 	}
 
 	if (keycode == Key::NONE) {
@@ -227,41 +230,70 @@ bool WaylandThread::_seat_state_configure_key_event(SeatState &p_ss, Ref<InputEv
 	}
 
 	if (physical_keycode == Key::NONE && keycode == Key::NONE && unicode == 0) {
-		return false;
+		return event;
 	}
 
-	p_event->set_window_id(DisplayServer::MAIN_WINDOW_ID);
+	event.instantiate();
+
+	event->set_window_id(DisplayServer::MAIN_WINDOW_ID);
 
 	// Set all pressed modifiers.
-	p_event->set_shift_pressed(p_ss.shift_pressed);
-	p_event->set_ctrl_pressed(p_ss.ctrl_pressed);
-	p_event->set_alt_pressed(p_ss.alt_pressed);
-	p_event->set_meta_pressed(p_ss.meta_pressed);
+	event->set_shift_pressed(p_ss->shift_pressed);
+	event->set_ctrl_pressed(p_ss->ctrl_pressed);
+	event->set_alt_pressed(p_ss->alt_pressed);
+	event->set_meta_pressed(p_ss->meta_pressed);
 
-	p_event->set_pressed(p_pressed);
-	p_event->set_keycode(keycode);
-	p_event->set_physical_keycode(physical_keycode);
-	p_event->set_location(key_location);
+	event->set_pressed(p_pressed);
+	event->set_keycode(keycode);
+	event->set_physical_keycode(physical_keycode);
+	event->set_location(key_location);
 
 	if (unicode != 0) {
-		p_event->set_key_label(fix_key_label(unicode, keycode));
+		event->set_key_label(fix_key_label(unicode, keycode));
 	} else {
-		p_event->set_key_label(keycode);
+		event->set_key_label(keycode);
 	}
 
 	if (p_pressed) {
-		p_event->set_unicode(fix_unicode(unicode));
+		event->set_unicode(fix_unicode(unicode));
 	}
 
 	// Taken from DisplayServerX11.
-	if (p_event->get_keycode() == Key::BACKTAB) {
+	if (event->get_keycode() == Key::BACKTAB) {
 		// Make it consistent across platforms.
-		p_event->set_keycode(Key::TAB);
-		p_event->set_physical_keycode(Key::TAB);
-		p_event->set_shift_pressed(true);
+		event->set_keycode(Key::TAB);
+		event->set_physical_keycode(Key::TAB);
+		event->set_shift_pressed(true);
 	}
 
-	return true;
+	return event;
+}
+
+// NOTE: Due to the nature of the way keys are encoded, there's an ambiguity
+// regarding "special" keys. In other words: there's no reliable way of
+// switching between a special key and a character key if not marking a
+// different Godot keycode, even if we're actually using the same XKB raw
+// keycode. This means that, during this switch, the old key will get "stuck",
+// as it will never receive a release event. This method returns the necessary
+// event to fix this if needed.
+Ref<InputEventKey> WaylandThread::_seat_state_get_unstuck_key_event(SeatState *p_ss, xkb_keycode_t p_keycode, bool p_pressed, Key p_key) {
+	Ref<InputEventKey> event;
+
+	if (p_pressed) {
+		Key *old_key = p_ss->pressed_keycodes.getptr(p_keycode);
+		if (old_key != nullptr && *old_key != p_key) {
+			print_verbose(vformat("%s and %s have same keycode. Generating release event for %s", keycode_get_string(*old_key), keycode_get_string(p_key), keycode_get_string(*old_key)));
+			event = _seat_state_get_key_event(p_ss, p_keycode, false);
+			if (event.is_valid()) {
+				event->set_keycode(*old_key);
+			}
+		}
+		p_ss->pressed_keycodes[p_keycode] = p_key;
+	} else {
+		p_ss->pressed_keycodes.erase(p_keycode);
+	}
+
+	return event;
 }
 
 void WaylandThread::_set_current_seat(struct wl_seat *p_seat) {
@@ -1920,13 +1952,19 @@ void WaylandThread::_wl_keyboard_on_key(void *data, struct wl_keyboard *wl_keybo
 		ss->repeating_keycode = XKB_KEYCODE_INVALID;
 	}
 
-	Ref<InputEventKey> k;
-	k.instantiate();
-
-	if (!_seat_state_configure_key_event(*ss, k, xkb_keycode, pressed)) {
+	Ref<InputEventKey> k = _seat_state_get_key_event(ss, xkb_keycode, pressed);
+	if (!k.is_valid()) {
 		return;
 	}
 
+	Ref<InputEventKey> uk = _seat_state_get_unstuck_key_event(ss, xkb_keycode, pressed, k->get_keycode());
+	if (uk.is_valid()) {
+		Ref<InputEventMessage> u_msg;
+		u_msg.instantiate();
+		u_msg->event = uk;
+		wayland_thread->push_message(u_msg);
+	}
+
 	Ref<InputEventMessage> msg;
 	msg.instantiate();
 	msg->event = k;
@@ -3236,15 +3274,18 @@ void WaylandThread::seat_state_echo_keys(SeatState *p_ss) {
 			int keys_amount = (ticks_delta / p_ss->repeat_key_delay_msec);
 
 			for (int i = 0; i < keys_amount; i++) {
-				Ref<InputEventKey> k;
-				k.instantiate();
-
-				if (!_seat_state_configure_key_event(*p_ss, k, p_ss->repeating_keycode, true)) {
+				Ref<InputEventKey> k = _seat_state_get_key_event(p_ss, p_ss->repeating_keycode, true);
+				if (!k.is_valid()) {
 					continue;
 				}
 
 				k->set_echo(true);
 
+				Ref<InputEventKey> uk = _seat_state_get_unstuck_key_event(p_ss, p_ss->repeating_keycode, true, k->get_keycode());
+				if (uk.is_valid()) {
+					Input::get_singleton()->parse_input_event(uk);
+				}
+
 				Input::get_singleton()->parse_input_event(k);
 			}
 

platform/linuxbsd/wayland/wayland_thread.h

@@ -428,6 +428,8 @@ class WaylandThread {
 		const char *keymap_buffer = nullptr;
 		uint32_t keymap_buffer_size = 0;
 
+		HashMap<xkb_keycode_t, Key> pressed_keycodes;
+
 		xkb_layout_index_t current_layout_index = 0;
 
 		int32_t repeat_key_delay_msec = 0;
@@ -903,7 +905,8 @@ class WaylandThread {
 	static Vector<uint8_t> _wp_primary_selection_offer_read(struct wl_display *wl_display, const char *p_mime, struct zwp_primary_selection_offer_v1 *wp_primary_selection_offer);
 
 	static void _seat_state_set_current(WaylandThread::SeatState &p_ss);
-	static bool _seat_state_configure_key_event(WaylandThread::SeatState &p_seat, Ref<InputEventKey> p_event, xkb_keycode_t p_keycode, bool p_pressed);
+	static Ref<InputEventKey> _seat_state_get_key_event(SeatState *p_ss, xkb_keycode_t p_keycode, bool p_pressed);
+	static Ref<InputEventKey> _seat_state_get_unstuck_key_event(SeatState *p_ss, xkb_keycode_t p_keycode, bool p_pressed, Key p_key);
 
 	static void _wayland_state_update_cursor();