From e866cb85e43dd4ef8398dff0d65cc57ea90af5e6 Mon Sep 17 00:00:00 2001 From: "devin-ai-integration[bot]" <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Fri, 16 Oct 2026 09:48:28 -0700 Subject: [PATCH] fix(hotkeys): prefer evdev on GNOME X11 and skip redundant IBus engine restore (#963) Fixes #954. GNOME X11: mutter never delivers physical key events to the XRecord extension pynput relies on, so the listener started cleanly but shortcuts stayed dead. Prefer the evdev backend there (same path as Wayland), falling back to pynput when /dev/input is unusable. Shutdown: `ibus engine ` rewrites the whole X server XKB map, and the unconditional restore collapsed GNOME's multi-layout map and broke layout switching. The daemon check now uses _query_active_engine() (no GNOME gsettings fallback) so a cleared engine reads as None; the restore write is skipped when a real engine is live or the target is an xkb:* source GNOME owns, and still runs for real IM engines (#558). --- src/vocalinux/text_injection/ibus_engine.py | 57 ++++++-- .../ui/keyboard_backends/__init__.py | 20 +++ tests/test_ibus_engine.py | 123 +++++++++++++++++- tests/test_ibus_engine_core.py | 8 +- tests/test_ibus_engine_utils.py | 3 +- tests/test_keyboard_init_ext.py | 50 +++++++ 6 files changed, 244 insertions(+), 17 deletions(-) diff --git a/src/vocalinux/text_injection/ibus_engine.py b/src/vocalinux/text_injection/ibus_engine.py index 76e3d78..887ade3 100644 --- a/src/vocalinux/text_injection/ibus_engine.py +++ b/src/vocalinux/text_injection/ibus_engine.py @@ -553,6 +553,28 @@ def get_current_engine() -> Optional[str]: return None +def _query_active_engine() -> Optional[str]: + """ + Query the engine ibus-daemon actually reports, with no GNOME fallbacks. + + Unlike :func:`get_current_engine`, this never consults gsettings, so it + can distinguish a really-active engine from a cleared global engine — + ``None`` means either "No global engine" or that the query failed. + """ + try: + result = subprocess.run( + ["ibus", "engine"], capture_output=True, text=True, timeout=5, env=host_env() + ) + stderr_lower = result.stderr.lower() if result.stderr else "" + if "no global engine" in stderr_lower: + return None + if result.returncode == 0: + return result.stdout.strip() or None + except (subprocess.SubprocessError, FileNotFoundError): + pass + return None + + def _ibus_im_env_configured() -> bool: """True if the process env says IBus is the session input method.""" gtk_im = os.environ.get("GTK_IM_MODULE", "").lower() @@ -1588,15 +1610,32 @@ class IBusTextInjector: stop_engine_process() if restore_engine: - logger.info(f"Restoring previous engine after teardown: {restore_engine}") - restored = False - for attempt in range(3): - if switch_engine(restore_engine): - restored = True - break - time.sleep(0.15 * (attempt + 1)) - if not restored: - logger.error(f"Failed to restore IBus engine after teardown: {restore_engine}") + # `ibus engine ` applies that engine's XKB map server-wide. + # Re-check the daemon (not the GNOME fallback) after teardown and + # skip the write when a real engine is already active — rewriting + # it collapses GNOME's multi-layout map (#954). The same skip + # applies when teardown left the engine cleared but the restore + # target is an xkb:* source on GNOME: gnome-shell owns that map + # and the write would only collapse it. A cleared global engine + # for a real IM engine still gets restored (#558). + live = _query_active_engine() + if live and live != ENGINE_NAME: + logger.info(f"Skipping engine restore — live engine already active: {live}") + elif live is None and _is_gnome_session() and restore_engine.startswith("xkb:"): + logger.info( + "Skipping engine restore — engine cleared and GNOME owns " + f"the XKB map; not rewriting {restore_engine}" + ) + else: + logger.info(f"Restoring previous engine after teardown: {restore_engine}") + restored = False + for attempt in range(3): + if switch_engine(restore_engine): + restored = True + break + time.sleep(0.15 * (attempt + 1)) + if not restored: + logger.error(f"Failed to restore IBus engine after teardown: {restore_engine}") else: logger.info( "Skipping engine restoration — no restorable engine was available at shutdown" diff --git a/src/vocalinux/ui/keyboard_backends/__init__.py b/src/vocalinux/ui/keyboard_backends/__init__.py index 9e42c57..4585691 100644 --- a/src/vocalinux/ui/keyboard_backends/__init__.py +++ b/src/vocalinux/ui/keyboard_backends/__init__.py @@ -82,6 +82,11 @@ class DesktopEnvironment: return DesktopEnvironment.UNKNOWN +def _is_gnome_session() -> bool: + """Check if the current session is running under GNOME.""" + return "GNOME" in os.environ.get("XDG_CURRENT_DESKTOP", "").upper() + + def create_backend( preferred_backend: Optional[str] = None, shortcut: str = DEFAULT_SHORTCUT, @@ -152,6 +157,21 @@ def create_backend( ) return None + # On GNOME's X11 session, mutter does not deliver physical key events + # through the XRecord extension that pynput's Listener relies on, so the + # listener starts cleanly but shortcuts stay dead (#954). evdev reads the + # kernel input devices directly, so prefer it here — but fall back to + # pynput when /dev/input is unusable rather than leaving shortcuts dead. + if env == DesktopEnvironment.X11 and _is_gnome_session() and EVDEV_AVAILABLE: + backend = EvdevKeyboardBackend(shortcut=shortcut, mode=mode) # type: ignore + if backend.is_available(): + logger.info("Using evdev backend for global shortcuts (GNOME X11)") + return backend + hint = backend.get_permission_hint() + if hint: + logger.warning(f"Evdev backend not usable on GNOME X11: {hint}") + logger.warning("Falling back to pynput; global shortcuts may not respond on GNOME X11") + # Default to pynput for native X11 (or unknown) if PYNPUT_AVAILABLE: logger.info("Using pynput backend") diff --git a/tests/test_ibus_engine.py b/tests/test_ibus_engine.py index 87a3cbe..a9c3e2b 100644 --- a/tests/test_ibus_engine.py +++ b/tests/test_ibus_engine.py @@ -751,13 +751,14 @@ class TestIBusTextInjector(unittest.TestCase): self.assertEqual(sent["data"], b"\n") + @patch("vocalinux.text_injection.ibus_engine._query_active_engine", return_value=None) @patch("vocalinux.text_injection.ibus_engine.stop_engine_process") @patch("vocalinux.text_injection.ibus_engine.get_current_engine", return_value=None) @patch("vocalinux.text_injection.ibus_engine.IBUS_AVAILABLE", True) @patch("vocalinux.text_injection.ibus_engine.ensure_ibus_dir") @patch("vocalinux.text_injection.ibus_engine.switch_engine", return_value=True) def test_stop_restores_engine( - self, mock_switch, mock_ensure_dir, mock_get_current, mock_stop_proc + self, mock_switch, mock_ensure_dir, mock_get_current, mock_stop_proc, mock_live ): """Test stop() restores cached engine when live engine is unavailable (#558).""" from vocalinux.text_injection.ibus_engine import IBusTextInjector @@ -776,13 +777,14 @@ class TestIBusTextInjector(unittest.TestCase): self.assertEqual([c[0] for c in parent.mock_calls], ["stop_proc", "switch"]) self.assertIsNone(injector._previous_engine) + @patch("vocalinux.text_injection.ibus_engine._query_active_engine", return_value=None) @patch("vocalinux.text_injection.ibus_engine.stop_engine_process") @patch("vocalinux.text_injection.ibus_engine.get_current_engine", return_value="xkb:de::deu") @patch("vocalinux.text_injection.ibus_engine.switch_engine", return_value=True) @patch("vocalinux.text_injection.ibus_engine.IBUS_AVAILABLE", True) @patch("vocalinux.text_injection.ibus_engine.ensure_ibus_dir") def test_stop_prefers_live_engine_over_stale_warmup_cache( - self, mock_ensure_dir, mock_switch, mock_get_current, mock_stop_proc + self, mock_ensure_dir, mock_switch, mock_get_current, mock_stop_proc, mock_live ): """Quit must restore the live input source, not a stale warmup cache (#643 Bugbot).""" from vocalinux.text_injection.ibus_engine import IBusTextInjector @@ -797,13 +799,14 @@ class TestIBusTextInjector(unittest.TestCase): mock_switch.assert_called_once_with("xkb:de::deu") self.assertIsNone(injector._previous_engine) + @patch("vocalinux.text_injection.ibus_engine._query_active_engine", return_value=None) @patch("vocalinux.text_injection.ibus_engine.stop_engine_process") @patch("vocalinux.text_injection.ibus_engine.get_current_engine", return_value="xkb:es::spa") @patch("vocalinux.text_injection.ibus_engine.switch_engine", return_value=True) @patch("vocalinux.text_injection.ibus_engine.IBUS_AVAILABLE", True) @patch("vocalinux.text_injection.ibus_engine.ensure_ibus_dir") def test_stop_restores_current_engine_when_previous_unset( - self, mock_ensure_dir, mock_switch, mock_get_current, mock_stop_proc + self, mock_ensure_dir, mock_switch, mock_get_current, mock_stop_proc, mock_live ): """Warmup quit must restore the live engine after register_component teardown (#558).""" from vocalinux.text_injection.ibus_engine import IBusTextInjector @@ -822,6 +825,105 @@ class TestIBusTextInjector(unittest.TestCase): mock_switch.assert_called_once_with("xkb:es::spa") self.assertEqual([c[0] for c in parent.mock_calls], ["stop_proc", "switch"]) + @patch("vocalinux.text_injection.ibus_engine.stop_engine_process") + @patch( + "vocalinux.text_injection.ibus_engine._query_active_engine", + return_value="xkb:us::eng", + ) + @patch( + "vocalinux.text_injection.ibus_engine.get_current_engine", + return_value="xkb:us::eng", + ) + @patch("vocalinux.text_injection.ibus_engine.switch_engine") + @patch("vocalinux.text_injection.ibus_engine.IBUS_AVAILABLE", True) + @patch("vocalinux.text_injection.ibus_engine.ensure_ibus_dir") + def test_stop_skips_engine_write_when_live_engine_healthy( + self, mock_ensure_dir, mock_switch, mock_get_current, mock_live, mock_stop_proc + ): + """A healthy live engine must not be rewritten at quit (#954). + + `ibus engine ` applies the engine's XKB map server-wide; on + GNOME the redundant write collapses the user's multi-layout map and + breaks keyboard layout switching after exit. + """ + from vocalinux.text_injection.ibus_engine import IBusTextInjector + + injector = IBusTextInjector(auto_activate=False) + injector._previous_engine = "xkb:us::eng" + + injector.stop() + + mock_get_current.assert_called() + mock_stop_proc.assert_called_once() + mock_switch.assert_not_called() + self.assertIsNone(injector._previous_engine) + + @patch("vocalinux.text_injection.ibus_engine._is_gnome_session", return_value=True) + @patch("vocalinux.text_injection.ibus_engine._query_active_engine", return_value=None) + @patch("vocalinux.text_injection.ibus_engine.stop_engine_process") + @patch("vocalinux.text_injection.ibus_engine.get_current_engine", return_value=None) + @patch("vocalinux.text_injection.ibus_engine.switch_engine") + @patch("vocalinux.text_injection.ibus_engine.IBUS_AVAILABLE", True) + @patch("vocalinux.text_injection.ibus_engine.ensure_ibus_dir") + def test_stop_skips_xkb_restore_when_gnome_engine_cleared( + self, + mock_ensure_dir, + mock_switch, + mock_get_current, + mock_stop_proc, + mock_live, + mock_gnome, + ): + """On GNOME, a cleared engine with an xkb:* target must not be rewritten (#954). + + gnome-shell owns the XKB map; restoring an xkb engine would only + collapse the user's multi-layout map. + """ + from vocalinux.text_injection.ibus_engine import IBusTextInjector + + injector = IBusTextInjector(auto_activate=False) + injector._previous_engine = "xkb:us::eng" + + injector.stop() + + mock_stop_proc.assert_called_once() + mock_live.assert_called_once() + mock_switch.assert_not_called() + self.assertIsNone(injector._previous_engine) + + @patch("vocalinux.text_injection.ibus_engine._is_gnome_session", return_value=True) + @patch("vocalinux.text_injection.ibus_engine._query_active_engine", return_value=None) + @patch("vocalinux.text_injection.ibus_engine.stop_engine_process") + @patch("vocalinux.text_injection.ibus_engine.get_current_engine", return_value=None) + @patch("vocalinux.text_injection.ibus_engine.switch_engine", return_value=True) + @patch("vocalinux.text_injection.ibus_engine.IBUS_AVAILABLE", True) + @patch("vocalinux.text_injection.ibus_engine.ensure_ibus_dir") + def test_stop_restores_im_engine_when_gnome_teardown_cleared( + self, + mock_ensure_dir, + mock_switch, + mock_get_current, + mock_stop_proc, + mock_live, + mock_gnome, + ): + """On GNOME, a cleared engine with a real IM target still restores (#558). + + A libpinyin/mozc user left with no global engine would lose input + method support entirely, so the restore write must still run. + """ + from vocalinux.text_injection.ibus_engine import IBusTextInjector + + injector = IBusTextInjector(auto_activate=False) + injector._previous_engine = "libpinyin" + + injector.stop() + + mock_stop_proc.assert_called_once() + mock_live.assert_called_once() + mock_switch.assert_called_once_with("libpinyin") + self.assertIsNone(injector._previous_engine) + @patch("vocalinux.text_injection.ibus_engine.stop_engine_process") @patch("vocalinux.text_injection.ibus_engine.get_current_engine", return_value=None) @patch("vocalinux.text_injection.ibus_engine.switch_engine") @@ -840,6 +942,7 @@ class TestIBusTextInjector(unittest.TestCase): mock_stop_proc.assert_called_once() mock_switch.assert_not_called() + @patch("vocalinux.text_injection.ibus_engine._query_active_engine", return_value=None) @patch("vocalinux.text_injection.ibus_engine.restore_xkb_layout") @patch("vocalinux.text_injection.ibus_engine.stop_engine_process") @patch("vocalinux.text_injection.ibus_engine.get_current_engine", return_value="xkb:es::spa") @@ -847,7 +950,13 @@ class TestIBusTextInjector(unittest.TestCase): @patch("vocalinux.text_injection.ibus_engine.IBUS_AVAILABLE", True) @patch("vocalinux.text_injection.ibus_engine.ensure_ibus_dir") def test_stop_restores_xkb_layout( - self, mock_ensure_dir, mock_switch, mock_get_current, mock_stop_proc, mock_restore_xkb + self, + mock_ensure_dir, + mock_switch, + mock_get_current, + mock_stop_proc, + mock_restore_xkb, + mock_live, ): """Test stop() restores XKB after post-teardown engine restore (#292/#558).""" from vocalinux.text_injection.ibus_engine import IBusTextInjector @@ -892,6 +1001,10 @@ class TestIBusTextInjector(unittest.TestCase): mock_restore_xkb.assert_not_called() mock_switch.assert_not_called() + @patch( + "vocalinux.text_injection.ibus_engine._query_active_engine", + return_value="vocalinux", + ) @patch("vocalinux.text_injection.ibus_engine.stop_engine_process") @patch( "vocalinux.text_injection.ibus_engine.get_current_engine", @@ -901,7 +1014,7 @@ class TestIBusTextInjector(unittest.TestCase): @patch("vocalinux.text_injection.ibus_engine.IBUS_AVAILABLE", True) @patch("vocalinux.text_injection.ibus_engine.ensure_ibus_dir") def test_stop_uses_cached_engine_when_stuck_on_vocalinux( - self, mock_ensure_dir, mock_switch, mock_get_current, mock_stop_proc + self, mock_ensure_dir, mock_switch, mock_get_current, mock_stop_proc, mock_live ): """If quit finds vocalinux selected, fall back to the warmup-captured engine (#558).""" from vocalinux.text_injection.ibus_engine import IBusTextInjector diff --git a/tests/test_ibus_engine_core.py b/tests/test_ibus_engine_core.py index 342014d..b2df534 100644 --- a/tests/test_ibus_engine_core.py +++ b/tests/test_ibus_engine_core.py @@ -627,9 +627,13 @@ class TestIBusTextInjector(unittest.TestCase): @patch("vocalinux.text_injection.ibus_engine.ensure_ibus_dir") @patch("vocalinux.text_injection.ibus_engine.stop_engine_process") @patch("vocalinux.text_injection.ibus_engine.switch_engine") - @patch("vocalinux.text_injection.ibus_engine.get_current_engine", return_value="other-engine") + @patch( + "vocalinux.text_injection.ibus_engine.get_current_engine", + return_value="other-engine", + ) + @patch("vocalinux.text_injection.ibus_engine._query_active_engine", return_value=None) def test_ibus_text_injector_stop( - self, mock_current_engine, mock_switch, mock_stop_proc, mock_ensure_dir + self, mock_live, mock_current_engine, mock_switch, mock_stop_proc, mock_ensure_dir ): """Test IBusTextInjector stop restores previous engine after teardown.""" from vocalinux.text_injection.ibus_engine import IBusTextInjector diff --git a/tests/test_ibus_engine_utils.py b/tests/test_ibus_engine_utils.py index 30ef29a..4bc7b53 100644 --- a/tests/test_ibus_engine_utils.py +++ b/tests/test_ibus_engine_utils.py @@ -428,6 +428,7 @@ class TestStopEngineProcess(unittest.TestCase): class TestStopRestoreRetry(unittest.TestCase): """Coverage for stop() restore retry after teardown.""" + @patch("vocalinux.text_injection.ibus_engine._query_active_engine", return_value=None) @patch("vocalinux.text_injection.ibus_engine.stop_engine_process") @patch("vocalinux.text_injection.ibus_engine.get_current_engine", return_value="xkb:es::spa") @patch("vocalinux.text_injection.ibus_engine.switch_engine", return_value=False) @@ -435,7 +436,7 @@ class TestStopRestoreRetry(unittest.TestCase): @patch("vocalinux.text_injection.ibus_engine.IBUS_AVAILABLE", True) @patch("vocalinux.text_injection.ibus_engine.ensure_ibus_dir") def test_stop_retries_failed_engine_restore( - self, mock_ensure_dir, mock_sleep, mock_switch, mock_get_current, mock_stop_proc + self, mock_ensure_dir, mock_sleep, mock_switch, mock_get_current, mock_stop_proc, mock_live ): """stop() retries switch_engine after teardown when the first attempts fail.""" from vocalinux.text_injection.ibus_engine import IBusTextInjector diff --git a/tests/test_keyboard_init_ext.py b/tests/test_keyboard_init_ext.py index 292827c..fe78607 100644 --- a/tests/test_keyboard_init_ext.py +++ b/tests/test_keyboard_init_ext.py @@ -115,6 +115,56 @@ class TestDesktopEnvironmentDetect: mock_pynput.assert_called_once() mock_evdev.assert_not_called() + def test_create_backend_gnome_x11_prefers_evdev(self) -> None: + """GNOME X11 must prefer evdev: mutter's XRecord drops physical key events (#954).""" + from vocalinux.ui.keyboard_backends import create_backend + + fake = MagicMock() + fake.is_available.return_value = True + + env = { + "XDG_SESSION_TYPE": "x11", + "XDG_CURRENT_DESKTOP": "GNOME", + "DISPLAY": ":0", + } + with patch.dict(os.environ, env, clear=True): + with ( + patch.object(kb_module, "EVDEV_AVAILABLE", True), + patch.object(kb_module, "EvdevKeyboardBackend", return_value=fake) as mock_evdev, + patch.object(kb_module, "PYNPUT_AVAILABLE", True), + patch.object(kb_module, "PynputKeyboardBackend") as mock_pynput, + ): + backend = create_backend() + assert backend is fake + mock_evdev.assert_called_once() + mock_pynput.assert_not_called() + + def test_create_backend_gnome_x11_falls_back_to_pynput(self) -> None: + """GNOME X11 keeps pynput when evdev cannot read /dev/input (#954).""" + from vocalinux.ui.keyboard_backends import create_backend + + fake = MagicMock() + fake.is_available.return_value = False + fake.get_permission_hint.return_value = "not in input group" + + env = { + "XDG_SESSION_TYPE": "x11", + "XDG_CURRENT_DESKTOP": "ubuntu:GNOME", + "DISPLAY": ":0", + } + with patch.dict(os.environ, env, clear=True): + with ( + patch.object(kb_module, "EVDEV_AVAILABLE", True), + patch.object(kb_module, "EvdevKeyboardBackend", return_value=fake), + patch.object(kb_module, "PYNPUT_AVAILABLE", True), + patch.object( + kb_module, "PynputKeyboardBackend", return_value=MagicMock() + ) as mock_pynput, + ): + backend = create_backend() + assert backend is mock_pynput.return_value + mock_pynput.assert_called_once() + def test_detect_xdg_session_type_case_insensitive(self): """Test that XDG_SESSION_TYPE detection is case-insensitive.""" with patch.dict(os.environ, {"XDG_SESSION_TYPE": "WAYLAND"}, clear=False):