diff --git a/src/vocalinux/text_injection/ibus_engine.py b/src/vocalinux/text_injection/ibus_engine.py index 00d17ca..399a73a 100644 --- a/src/vocalinux/text_injection/ibus_engine.py +++ b/src/vocalinux/text_injection/ibus_engine.py @@ -599,6 +599,39 @@ def _handle_engine_destroy( return next_active_instance +def _ibus_ping_status(active_instance: object, focus_event: threading.Event) -> bytes: + """Readiness bytes for the inject socket PING probe (#523). + + FOCUSED: bound to a client context (safe to commit). + OK: engine instance exists but FocusIn has not arrived yet. + NO_ENGINE: no active instance. + """ + # Use identity checks: VocalinuxEngine may subclass MagicMock in tests, + # so truthiness on the instance is unreliable. + if active_instance is None: + return b"NO_ENGINE" + if focus_event.is_set(): + return b"FOCUSED" + return b"OK" + + +def _ibus_ensure_client_focus( + active_instance: object, + focus_event: threading.Event, + timeout: float, +) -> bool: + """Wait for FocusIn before commit_text when needed (#523). + + Returns True when a client context is focused (or already was). + Returns False when there is no instance or FocusIn times out. + """ + if active_instance is None: + return False + if focus_event.is_set(): + return True + return focus_event.wait(timeout=timeout) + + def switch_engine(engine_name: str) -> bool: """Switch to the specified IBus engine.""" import time @@ -787,9 +820,14 @@ class VocalinuxEngine(IBus.Engine if IBUS_AVAILABLE else object): # Class-level reference to active engine instance _active_instance: Optional["VocalinuxEngine"] = None + # Set only after do_focus_in binds the engine to a client input context. + # do_enable alone is not enough for commit_text delivery (#523). + _focus_event: threading.Event = threading.Event() _socket_server: Optional[threading.Thread] = None _server_socket: Optional[socket.socket] = None _server_running: bool = False + # Max wait for FocusIn before committing text (cold first activation). + _FOCUS_WAIT_TIMEOUT_S: float = 1.0 def __init__(self): """Initialize the Vocalinux IBus engine.""" @@ -801,6 +839,8 @@ class VocalinuxEngine(IBus.Engine if IBUS_AVAILABLE else object): """Called when the engine is enabled/selected.""" logger.info("VocalinuxEngine enabled - ready for text injection") VocalinuxEngine._active_instance = self + # do_enable does not imply a focused client context; wait for do_focus_in + # before commit_text (#523). # Start socket server if not already running if VocalinuxEngine._socket_server is None: @@ -820,21 +860,26 @@ class VocalinuxEngine(IBus.Engine if IBUS_AVAILABLE else object): super_destroy: Optional[Callable[[], None]] = None if IBUS_AVAILABLE: super_destroy = super().do_destroy + was_active = VocalinuxEngine._active_instance is self VocalinuxEngine._active_instance = _handle_engine_destroy( VocalinuxEngine._active_instance, self, IBUS_AVAILABLE, super_destroy, ) + if was_active: + VocalinuxEngine._focus_event.clear() def do_focus_in(self) -> None: - """Called when the engine gains focus.""" + """Called when the engine gains focus on a client input context.""" logger.debug("VocalinuxEngine focus in") VocalinuxEngine._active_instance = self + VocalinuxEngine._focus_event.set() def do_focus_out(self) -> None: - """Called when the engine loses focus.""" + """Called when the engine loses focus on a client input context.""" logger.debug("VocalinuxEngine focus out") + VocalinuxEngine._focus_event.clear() def do_process_key_event(self, keyval: int, keycode: int, state: int) -> bool: """ @@ -899,14 +944,31 @@ class VocalinuxEngine(IBus.Engine if IBUS_AVAILABLE else object): data = conn.recv(65536) # Max text size if data: if data == b"\x00PING": - if cls._active_instance: - conn.sendall(b"OK") - else: - conn.sendall(b"NO_ENGINE") + conn.sendall( + _ibus_ping_status(cls._active_instance, cls._focus_event) + ) continue text = data.decode("utf-8") - # Snapshot active instance to avoid TOCTOU race with do_destroy + # Wait for FocusIn before commit. On cold first + # activation, do_enable can set _active_instance + # before mutter binds the client context; committing + # then reports success while text is dropped (#523). + if ( + cls._active_instance is not None + and not _ibus_ensure_client_focus( + cls._active_instance, + cls._focus_event, + cls._FOCUS_WAIT_TIMEOUT_S, + ) + ): + logger.warning( + "Timed out waiting for IBus FocusIn " "before text commit" + ) + conn.sendall(b"NO_FOCUS") + continue + + # Snapshot after focus wait (FocusIn may set a new instance) instance = cls._active_instance if instance: done = threading.Event() @@ -1154,7 +1216,8 @@ class IBusTextInjector: sock.sendall(b"\x00PING") response = sock.recv(64).decode("utf-8") - if response == "OK": + # FOCUSED = client context ready; OK = instance exists (not yet focused). + if response in ("OK", "FOCUSED"): logger.debug(f"IBus engine readiness probe succeeded on attempt {attempt + 1}") return @@ -1162,7 +1225,7 @@ class IBusTextInjector: logger.debug("IBus engine socket is ready; active engine instance not required") return - if response != "NO_ENGINE": + if response not in ("NO_ENGINE", "OK", "FOCUSED"): logger.warning(f"Unexpected readiness probe response: {response}") except (socket.timeout, FileNotFoundError, ConnectionRefusedError, OSError) as e: logger.debug(f"IBus readiness probe attempt {attempt + 1} failed: {e}") @@ -1285,10 +1348,14 @@ class IBusTextInjector: if response == "OK": logger.debug("Text injection successful") return True - elif response == "NO_ENGINE" and attempt < max_attempts - 1: - # Engine instance was destroyed (layout switch). - # Re-activate to create a new instance and retry. - logger.info("Engine instance not active, re-activating and retrying...") + elif response in ("NO_ENGINE", "NO_FOCUS") and attempt < max_attempts - 1: + # NO_ENGINE: instance destroyed (layout switch). + # NO_FOCUS: selected but FocusIn never arrived (#523). + # Re-activate and retry. + logger.info( + f"Engine not ready for commit ({response}); " + "re-activating and retrying..." + ) switch_engine(ENGINE_NAME) time.sleep(0.3) continue diff --git a/tests/test_ibus_engine_core.py b/tests/test_ibus_engine_core.py index f997358..c75a9df 100644 --- a/tests/test_ibus_engine_core.py +++ b/tests/test_ibus_engine_core.py @@ -12,6 +12,7 @@ import subprocess import sys import tempfile import threading +import time import unittest from pathlib import Path from unittest.mock import MagicMock, Mock, call, patch @@ -323,6 +324,7 @@ class TestVocalinuxEngine(unittest.TestCase): from vocalinux.text_injection.ibus_engine import VocalinuxEngine VocalinuxEngine._active_instance = None + VocalinuxEngine._focus_event.clear() VocalinuxEngine._socket_server = None VocalinuxEngine._server_socket = None VocalinuxEngine._server_running = False @@ -368,6 +370,7 @@ class TestVocalinuxEngine(unittest.TestCase): engine = VocalinuxEngine() engine.do_focus_in() self.assertEqual(VocalinuxEngine._active_instance, engine) + self.assertTrue(VocalinuxEngine._focus_event.is_set()) def test_vocalinux_engine_do_focus_out(self): """Test engine focus out signal handler.""" @@ -375,9 +378,84 @@ class TestVocalinuxEngine(unittest.TestCase): engine = VocalinuxEngine() VocalinuxEngine._active_instance = engine + engine.do_focus_in() engine.do_focus_out() - # Focus out should not clear the active instance + # Focus out should not clear the active instance, only client focus self.assertEqual(VocalinuxEngine._active_instance, engine) + self.assertFalse(VocalinuxEngine._focus_event.is_set()) + + def test_do_enable_does_not_mark_client_focused(self): + """Enable alone must not claim FocusIn readiness (#523).""" + from vocalinux.text_injection.ibus_engine import VocalinuxEngine + + engine = VocalinuxEngine() + with patch.object(engine, "_start_socket_server"): + engine.do_enable() + self.assertEqual(VocalinuxEngine._active_instance, engine) + self.assertFalse(VocalinuxEngine._focus_event.is_set()) + + def test_ping_status_matrix(self): + """PING status distinguishes no-engine / enabled / focused (#523).""" + from vocalinux.text_injection.ibus_engine import VocalinuxEngine, _ibus_ping_status + + focus = threading.Event() + self.assertEqual(_ibus_ping_status(None, focus), b"NO_ENGINE") + + engine = VocalinuxEngine() + self.assertEqual(_ibus_ping_status(engine, focus), b"OK") + + focus.set() + self.assertEqual(_ibus_ping_status(engine, focus), b"FOCUSED") + + def test_ensure_client_focus_true_when_already_focused(self): + """Already-focused engine does not wait before commit.""" + from vocalinux.text_injection.ibus_engine import _ibus_ensure_client_focus + + focus = threading.Event() + focus.set() + self.assertTrue(_ibus_ensure_client_focus(object(), focus, timeout=0.05)) + + def test_ensure_client_focus_false_without_instance(self): + """No active instance is not ready to commit.""" + from vocalinux.text_injection.ibus_engine import _ibus_ensure_client_focus + + focus = threading.Event() + self.assertFalse(_ibus_ensure_client_focus(None, focus, timeout=0.05)) + + def test_ensure_client_focus_waits_then_succeeds(self): + """Cold path: FocusIn arriving after enable unblocks commit (#523).""" + from vocalinux.text_injection.ibus_engine import _ibus_ensure_client_focus + + focus = threading.Event() + + def set_focus_soon(): + time.sleep(0.05) + focus.set() + + t = threading.Thread(target=set_focus_soon, daemon=True) + t.start() + self.assertTrue(_ibus_ensure_client_focus(object(), focus, timeout=0.5)) + t.join(timeout=1.0) + self.assertTrue(focus.is_set()) + + def test_ensure_client_focus_times_out(self): + """Timeout without FocusIn returns False so caller can send NO_FOCUS.""" + from vocalinux.text_injection.ibus_engine import _ibus_ensure_client_focus + + focus = threading.Event() + self.assertFalse(_ibus_ensure_client_focus(object(), focus, timeout=0.05)) + + def test_do_destroy_clears_focus_when_active(self): + """Destroy of the active instance clears FocusIn readiness.""" + from vocalinux.text_injection.ibus_engine import VocalinuxEngine + + engine = VocalinuxEngine() + engine.do_focus_in() + self.assertTrue(VocalinuxEngine._focus_event.is_set()) + with patch("vocalinux.text_injection.ibus_engine.IBUS_AVAILABLE", False): + engine.do_destroy() + self.assertIsNone(VocalinuxEngine._active_instance) + self.assertFalse(VocalinuxEngine._focus_event.is_set()) def test_vocalinux_engine_do_process_key_event(self): """Test engine key event processing.""" @@ -1148,6 +1226,32 @@ class TestIBusTextInjectorRetry(unittest.TestCase): # 3 max_attempts: first 2 get NO_ENGINE and retry, 3rd gets NO_ENGINE and fails self.assertEqual(mock_sock.recv.call_count, 3) + @patch("vocalinux.text_injection.ibus_engine.ensure_ibus_dir") + @patch("vocalinux.text_injection.ibus_engine.switch_engine", return_value=True) + @patch("vocalinux.text_injection.ibus_engine.is_engine_active", return_value=True) + @patch("socket.socket") + @patch("vocalinux.text_injection.ibus_engine.SOCKET_PATH") + @patch("vocalinux.text_injection.ibus_engine.time") + def test_retry_on_no_focus_then_succeeds( + self, mock_time, mock_socket_path, mock_socket_cls, mock_active, mock_switch, mock_ensure + ): + """NO_FOCUS (FocusIn timeout) should re-activate and succeed on retry (#523).""" + from vocalinux.text_injection.ibus_engine import IBusTextInjector + + mock_socket_path.exists.return_value = True + mock_sock = MagicMock() + mock_sock.__enter__.return_value = mock_sock + mock_sock.__exit__.return_value = None + mock_sock.recv.side_effect = [b"NO_FOCUS", b"OK"] + mock_socket_cls.return_value = mock_sock + + injector = IBusTextInjector(auto_activate=False) + result = injector.inject_text("hello") + + self.assertTrue(result) + self.assertEqual(mock_sock.recv.call_count, 2) + self.assertGreaterEqual(mock_switch.call_count, 1) + class TestIBusEngineStartupReadiness(unittest.TestCase): """Test startup readiness probe behavior in setup flow.""" @@ -1224,6 +1328,27 @@ class TestIBusEngineStartupReadiness(unittest.TestCase): mock_sock.sendall.assert_called_once_with(b"\x00PING") + @patch("vocalinux.text_injection.ibus_engine.ensure_ibus_dir") + @patch("socket.socket") + @patch("vocalinux.text_injection.ibus_engine.SOCKET_PATH") + def test_wait_for_engine_ready_accepts_focused( + self, mock_socket_path, mock_socket_cls, mock_ensure_dir + ): + """Readiness probe treats FOCUSED as success (#523).""" + from vocalinux.text_injection.ibus_engine import IBusTextInjector + + mock_socket_path.exists.return_value = True + mock_sock = MagicMock() + mock_sock.__enter__.return_value = mock_sock + mock_sock.__exit__.return_value = None + mock_sock.recv.return_value = b"FOCUSED" + mock_socket_cls.return_value = mock_sock + + injector = IBusTextInjector(auto_activate=False) + injector._wait_for_engine_ready(max_attempts=1) + + mock_sock.sendall.assert_called_once_with(b"\x00PING") + @patch("vocalinux.text_injection.ibus_engine.ensure_ibus_dir") @patch("vocalinux.text_injection.ibus_engine.time") @patch("socket.socket")