diff --git a/src/vocalinux/ui/settings_dialog.py b/src/vocalinux/ui/settings_dialog.py index 49d224d..2b595a3 100644 --- a/src/vocalinux/ui/settings_dialog.py +++ b/src/vocalinux/ui/settings_dialog.py @@ -4092,6 +4092,7 @@ class SettingsDialog(Gtk.Dialog): """Persist deferred text edits before the settings dialog closes.""" if response_id in (Gtk.ResponseType.CLOSE, Gtk.ResponseType.DELETE_EVENT): self._flush_advanced_prompt_if_dirty() + self._resync_engine_ui_if_unapplied() def _on_advanced_prompt_changed(self, buffer): """Track prompt edits without applying settings on every keystroke.""" @@ -4731,7 +4732,14 @@ class SettingsDialog(Gtk.Dialog): engine = _engine_from_display(engine_text) - current_lang = self.language_combo.get_active_id() + # A programmatic change — the resync after a failed apply — must repaint + # for the engine now shown, but must not rewrite the language the user + # picked, and must not cascade into another apply. Every other control on + # this page checks these flags itself rather than leaning on + # _auto_apply_settings to do it. + programmatic = self._initializing or self._applying_settings + + current_lang = None if self._applying_settings else self.language_combo.get_active_id() if current_lang: if engine == "vosk" and ( current_lang == "auto" or not SUPPORTED_LANGUAGES.get(current_lang, {}).get("vosk") @@ -4746,6 +4754,19 @@ class SettingsDialog(Gtk.Dialog): self._update_model_info() self._update_voice_commands_for_engine() + if programmatic: + return + + # Every other control on this page applies as soon as it changes. Without + # this the new engine is only displayed: the config and the running + # recognizer stay on the previous one until some other control is touched, + # and closing the dialog drops the change silently. + if engine == "remote_api" and not self.remote_api_url_entry.get_text().strip(): + # Applying now would only fail validation; _on_remote_api_settings_changed + # applies once the server URL is filled in. + return + self._auto_apply_settings() + def _update_voice_commands_for_engine(self): """Update voice commands switch based on current engine.""" sr_config = self.config_manager.get_settings().get("speech_recognition", {}) @@ -5100,15 +5121,31 @@ class SettingsDialog(Gtk.Dialog): cancel_check_id = GLib.timeout_add(100, check_cancelled) try: - self._apply_settings_internal(settings, raise_errors=True) - GLib.idle_add(download_dialog.set_complete, True, "") - GLib.idle_add(self._populate_model_options) + applied = self._apply_settings_internal(settings, raise_errors=True) + if applied: + GLib.idle_add(download_dialog.set_complete, True, "") + GLib.idle_add(self._populate_model_options) + else: + # raise_errors makes failures raise today, but a + # False return must not be read as success: it + # means nothing was saved and the pickers still + # show settings the engine never took. + GLib.idle_add(self._resync_model_ui_from_config) + GLib.idle_add( + download_dialog.set_complete, + False, + "Could not apply the new settings", + ) finally: GLib.source_remove(cancel_check_id) self.speech_engine.set_download_progress_callback(None) except Exception as e: error_msg = str(e) + # The settings were never saved, so the previously working + # model is still the configured one; put the pickers back on + # it so the UI matches the config and a retry is possible. + GLib.idle_add(self._resync_model_ui_from_config) if "cancelled" in error_msg.lower(): GLib.idle_add( download_dialog.set_complete, @@ -5128,23 +5165,75 @@ class SettingsDialog(Gtk.Dialog): threading.Thread(target=download_and_apply, daemon=True).start() download_dialog.run() download_dialog.destroy() + # Whatever ended the modal — cancel, failure, or a path not + # covered above — the pickers must not outlive the config. + self._resync_engine_ui_if_unapplied() return logger.info(f"Auto-applying settings: {settings}") - self._save_selected_settings(settings) - was_running = self.speech_engine.state != RecognitionState.IDLE if was_running: self.speech_engine.stop_recognition() self.speech_engine.reconfigure(**settings) + self._save_selected_settings(settings) logger.info("Settings auto-applied successfully") except Exception as e: logger.error(f"Failed to auto-apply settings: {e}") + self._resync_model_ui_from_config() finally: self._applying_settings = False + def _resync_model_ui_from_config(self): + """Put the pickers back on the settings that are actually saved. + + Called when applying failed: the config still names the previous + engine and model, and leaving the pickers on the rejected ones also + blocks a retry, since re-selecting the same entry emits no "changed" + signal. + """ + try: + saved_engine = ( + self.config_manager.get_settings().get("speech_recognition", {}).get("engine") + ) + if saved_engine: + display = _engine_display_name(saved_engine) + if self.engine_combo.get_active_text() != display: + # Hold the apply-suppression flag: restoring the combo fires + # _on_engine_changed(), which must repaint the pickers but + # not cascade into another apply or download prompt. + was_applying = self._applying_settings + self._applying_settings = True + try: + self.engine_combo.set_active_id(display) + finally: + self._applying_settings = was_applying + self._populate_model_options() + self._update_model_info() + except Exception as e: # pragma: no cover - UI resync must never mask the original error + logger.debug(f"Could not resync model pickers: {e}") + + def _resync_engine_ui_if_unapplied(self): + """Resync when the engine on screen is not the engine that got saved. + + Covers every way an apply can end without writing the config: a + cancelled download, a failure, and Remote API left without a server + URL, where applying is deliberately deferred. Leaving the combo on + the unapplied engine misreports what dictation will actually use. + """ + try: + saved_engine = ( + self.config_manager.get_settings().get("speech_recognition", {}).get("engine") + ) + if not saved_engine: + return + if self._get_selected_engine() == saved_engine: + return + self._resync_model_ui_from_config() + except Exception as e: # pragma: no cover - a resync must never mask the real error + logger.debug(f"Could not check the engine picker against the config: {e}") + def _save_selected_settings(self, settings: dict): """Persist selected settings to their appropriate config sections.""" sr_settings = {k: v for k, v in settings.items() if not k.startswith("whispercpp_")} @@ -5418,14 +5507,25 @@ For now, the engine has been reverted to VOSK.""" cancel_check_id = GLib.timeout_add(100, check_cancelled) try: - self._apply_settings_internal(settings, raise_errors=True) - GLib.idle_add(download_dialog.set_complete, True, "") + applied = self._apply_settings_internal(settings, raise_errors=True) + if applied: + GLib.idle_add(download_dialog.set_complete, True, "") + else: + GLib.idle_add(self._resync_model_ui_from_config) + GLib.idle_add( + download_dialog.set_complete, + False, + "Could not apply the new settings", + ) finally: GLib.source_remove(cancel_check_id) self.speech_engine.set_download_progress_callback(None) except Exception as e: error_msg = str(e) + # Nothing was saved, so the config still names the previous + # engine and model; put the pickers back on them. + GLib.idle_add(self._resync_model_ui_from_config) if "cancelled" in error_msg.lower(): GLib.idle_add(download_dialog.set_complete, False, "Download cancelled") elif engine == "whisper" and "no module named" in error_msg.lower(): @@ -5439,6 +5539,7 @@ For now, the engine has been reverted to VOSK.""" download_dialog.destroy() self._populate_model_options() + self._resync_engine_ui_if_unapplied() return True return self._apply_settings_internal(settings) @@ -5454,14 +5555,16 @@ For now, the engine has been reverted to VOSK.""" off the main loop is not safe anyway. """ try: - self._save_selected_settings(settings) - was_running = self.speech_engine.state != RecognitionState.IDLE if was_running: self.speech_engine.stop_recognition() time.sleep(0.5) + # Persist only once the engine really runs these settings: this call + # downloads missing models, and a config saved up front would keep + # pointing at a model that never made it to disk. self.speech_engine.reconfigure(**settings) + self._save_selected_settings(settings) logger.info("Settings applied successfully.") return True diff --git a/tests/test_settings_dialog.py b/tests/test_settings_dialog.py index 529da8d..2d4a7fa 100644 --- a/tests/test_settings_dialog.py +++ b/tests/test_settings_dialog.py @@ -58,16 +58,7 @@ def apply_settings_internal(dialog, settings: dict) -> bool: This is a test helper that mirrors the real implementation behavior. """ try: - # 1. Update Config Manager - sr_settings = {k: v for k, v in settings.items() if not k.startswith("whispercpp_")} - advanced_settings = {k: v for k, v in settings.items() if k.startswith("whispercpp_")} - - dialog.config_manager.update_speech_recognition_settings(sr_settings) - for key, value in advanced_settings.items(): - dialog.config_manager.set("advanced", key, value) - dialog.config_manager.save_settings() - - # 2. Reconfigure Speech Engine + # 1. Reconfigure Speech Engine # Stop engine before reconfiguring if it's running was_running = dialog.speech_engine.state != RecognitionState.IDLE if was_running: @@ -76,6 +67,15 @@ def apply_settings_internal(dialog, settings: dict) -> bool: time.sleep(0.01) # Shortened for tests dialog.speech_engine.reconfigure(**settings) + + # 2. Update Config Manager, only once the engine accepted the settings + sr_settings = {k: v for k, v in settings.items() if not k.startswith("whispercpp_")} + advanced_settings = {k: v for k, v in settings.items() if k.startswith("whispercpp_")} + + dialog.config_manager.update_speech_recognition_settings(sr_settings) + for key, value in advanced_settings.items(): + dialog.config_manager.set("advanced", key, value) + dialog.config_manager.save_settings() return True except Exception: return False @@ -188,7 +188,7 @@ class TestSettingsDialog(unittest.TestCase): mock_speech_engine.reconfigure.assert_called_once() def test_apply_settings_failure_reconfigure(self): - """Test apply_settings handles errors during engine reconfiguration.""" + """A failed reconfigure must leave the saved settings untouched.""" # Set up the reconfigure method to raise an exception mock_speech_engine.reconfigure.side_effect = Exception("Model load failed") @@ -198,10 +198,11 @@ class TestSettingsDialog(unittest.TestCase): # Verify the result self.assertFalse(result) - # Verify mocks were called - mock_config_manager.update_speech_recognition_settings.assert_called_once() - mock_config_manager.save_settings.assert_called_once() + # The engine was asked, and refused: the config must still describe the + # settings that are actually running, not the ones that failed. mock_speech_engine.reconfigure.assert_called_once() + mock_config_manager.update_speech_recognition_settings.assert_not_called() + mock_config_manager.save_settings.assert_not_called() class TestSettingsDialogCSS(unittest.TestCase): diff --git a/tests/test_settings_model_state.py b/tests/test_settings_model_state.py new file mode 100644 index 0000000..3257820 --- /dev/null +++ b/tests/test_settings_model_state.py @@ -0,0 +1,288 @@ +"""Tests for keeping the saved model in step with the engine that runs it.""" + +import importlib +import sys +from unittest.mock import MagicMock, Mock, patch + +import pytest + +import vocalinux.ui +from vocalinux.common_types import RecognitionState + + +@pytest.fixture(scope="module") +def settings_dialog(): + """Import settings_dialog with real base classes for its GTK subclasses. + + conftest swaps gi for a MagicMock, which leaves every ``class X(Gtk.Y)`` in + the module as a mock and makes its methods unreachable. Handing the three + bases the module subclasses a real class keeps the classes intact, while + the rest of GTK stays mocked. + + This runs as a fixture, not at collection: reimporting rebinds + ``vocalinux.ui.settings_dialog`` on the package as well as in + ``sys.modules``, and leaving those two pointing at different module objects + breaks any later test that patches the module by name (#686 hit exactly + that in test_model_deletion). Both are restored here. + """ + repository = sys.modules["gi.repository"] + bases = {name: type(name, (), {}) for name in ("Box", "ListBoxRow", "Dialog")} + saved_module = sys.modules.pop("vocalinux.ui.settings_dialog", None) + saved_attribute = getattr(vocalinux.ui, "settings_dialog", None) + try: + with patch.object(repository, "Gtk", MagicMock(**bases)): + module = importlib.import_module("vocalinux.ui.settings_dialog") + yield module + finally: + sys.modules.pop("vocalinux.ui.settings_dialog", None) + if saved_module is not None: + sys.modules["vocalinux.ui.settings_dialog"] = saved_module + if saved_attribute is not None: + vocalinux.ui.settings_dialog = saved_attribute + elif hasattr(vocalinux.ui, "settings_dialog"): + del vocalinux.ui.settings_dialog + + +@pytest.fixture +def dialog_class(settings_dialog): + return settings_dialog.SettingsDialog + + +def _dialog_stub(): + """A stand-in ``self`` for calling dialog methods without building the UI.""" + dialog = Mock() + dialog._applying_settings = False + dialog._initializing = False + dialog._test_active = False + dialog._populating_models = False + dialog.language = "en-us" + # The real attribute is an enum member; a bare "idle" string would compare + # unequal and send every test down the stop_recognition + sleep(0.5) branch. + dialog.speech_engine.state = RecognitionState.IDLE + return dialog + + +class _InlineThread: + """Run the download worker on the calling thread, in call order.""" + + def __init__(self, target=None, daemon=None, **kwargs): + self._target = target + + def start(self): + self._target() + + +def _glib_stub(idle_calls): + glib = MagicMock() + glib.idle_add.side_effect = lambda func, *args: idle_calls.append((func, args)) + return glib + + +def test_settings_persisted_only_after_the_engine_accepts_them(dialog_class): + """A model is saved once it really loaded, not when it was picked.""" + dialog = _dialog_stub() + order = [] + dialog.speech_engine.reconfigure.side_effect = lambda **kw: order.append("reconfigure") + dialog._save_selected_settings.side_effect = lambda settings: order.append("save") + + assert dialog_class._apply_settings_internal(dialog, {"engine": "vosk"}) is True + assert order == ["reconfigure", "save"] + + +def test_failed_apply_leaves_the_previous_model_configured(dialog_class): + """Nothing is written when reconfiguring fails, so the old model stays.""" + dialog = _dialog_stub() + dialog.speech_engine.reconfigure.side_effect = RuntimeError("Download cancelled") + + assert dialog_class._apply_settings_internal(dialog, {"engine": "whisper_cpp"}) is False + + dialog._save_selected_settings.assert_not_called() + + +def test_failed_auto_apply_resyncs_the_pickers_with_the_config(settings_dialog, dialog_class): + """The pickers go back to the saved model so a retry is possible.""" + dialog = _dialog_stub() + dialog.get_selected_settings.return_value = { + "engine": "vosk", + "model_size": "small", + "language": "en-us", + } + dialog.speech_engine.reconfigure.side_effect = RuntimeError("boom") + + with patch.object(settings_dialog, "_is_vosk_model_downloaded", return_value=True): + dialog_class._auto_apply_settings(dialog) + + dialog._save_selected_settings.assert_not_called() + dialog._resync_model_ui_from_config.assert_called_once() + assert dialog._applying_settings is False + + +def test_download_path_resyncs_when_the_apply_reports_failure(settings_dialog, dialog_class): + """A False return from the apply must not be reported as a finished switch. + + This is the #692 path the earlier tests never reached: the model is not on + disk, so the modal opens and the work happens on the download thread. + _apply_settings_internal returns False there instead of raising, which used + to fall straight through to set_complete(True, ""). + """ + dialog = _dialog_stub() + dialog.get_selected_settings.return_value = { + "engine": "whisper_cpp", + "model_size": "small", + "language": "auto", + } + dialog._apply_settings_internal.return_value = False + idle_calls = [] + + with ( + patch.object(settings_dialog, "is_whispercpp_model_downloaded", return_value=False), + patch.object(settings_dialog, "ModelDownloadDialog") as modal_class, + patch.object(settings_dialog, "GLib", _glib_stub(idle_calls)), + patch.object(settings_dialog.threading, "Thread", _InlineThread), + ): + dialog_class._auto_apply_settings(dialog) + + modal = modal_class.return_value + scheduled = [(func, args) for func, args in idle_calls] + assert (dialog._resync_model_ui_from_config, ()) in scheduled + assert (modal.set_complete, (True, "")) not in scheduled + assert any(func is modal.set_complete and args[0] is False for func, args in scheduled) + dialog._save_selected_settings.assert_not_called() + + +def test_download_path_resyncs_when_the_download_is_cancelled(settings_dialog, dialog_class): + """Cancelling the modal leaves the config alone, so the combo must follow.""" + dialog = _dialog_stub() + dialog.get_selected_settings.return_value = { + "engine": "whisper_cpp", + "model_size": "small", + "language": "auto", + } + dialog._apply_settings_internal.side_effect = RuntimeError("Download cancelled") + idle_calls = [] + + with ( + patch.object(settings_dialog, "is_whispercpp_model_downloaded", return_value=False), + patch.object(settings_dialog, "ModelDownloadDialog") as modal_class, + patch.object(settings_dialog, "GLib", _glib_stub(idle_calls)), + patch.object(settings_dialog.threading, "Thread", _InlineThread), + ): + dialog_class._auto_apply_settings(dialog) + + modal = modal_class.return_value + assert (dialog._resync_model_ui_from_config, ()) in idle_calls + assert (modal.set_complete, (False, "Download cancelled")) in idle_calls + + +def test_modal_close_resyncs_an_engine_that_never_applied(settings_dialog, dialog_class): + """Belt and braces: whatever ended the modal, the combo cannot outlive it.""" + dialog = _dialog_stub() + dialog.get_selected_settings.return_value = { + "engine": "whisper_cpp", + "model_size": "small", + "language": "auto", + } + dialog._apply_settings_internal.return_value = False + + with ( + patch.object(settings_dialog, "is_whispercpp_model_downloaded", return_value=False), + patch.object(settings_dialog, "ModelDownloadDialog"), + patch.object(settings_dialog, "GLib", MagicMock()), + patch.object(settings_dialog.threading, "Thread", _InlineThread), + ): + dialog_class._auto_apply_settings(dialog) + + dialog._resync_engine_ui_if_unapplied.assert_called_once() + + +def test_unapplied_engine_is_resynced_when_it_differs_from_the_config(dialog_class): + """The check compares what is shown against what was actually saved.""" + dialog = _dialog_stub() + dialog.config_manager.get_settings.return_value = {"speech_recognition": {"engine": "vosk"}} + dialog._get_selected_engine.return_value = "remote_api" + + dialog_class._resync_engine_ui_if_unapplied(dialog) + + dialog._resync_model_ui_from_config.assert_called_once() + + +def test_a_matching_engine_is_left_alone(dialog_class): + """No churn when the picker already shows the engine that is configured.""" + dialog = _dialog_stub() + dialog.config_manager.get_settings.return_value = {"speech_recognition": {"engine": "vosk"}} + dialog._get_selected_engine.return_value = "vosk" + + dialog_class._resync_engine_ui_if_unapplied(dialog) + + dialog._resync_model_ui_from_config.assert_not_called() + + +def test_resync_puts_the_engine_picker_back_on_the_saved_engine(dialog_class): + """After a failed switch the combo must not keep showing the dead engine.""" + dialog = _dialog_stub() + dialog.config_manager.get_settings.return_value = {"speech_recognition": {"engine": "vosk"}} + dialog.engine_combo.get_active_text.return_value = "whisper.cpp" + + dialog_class._resync_model_ui_from_config(dialog) + + dialog.engine_combo.set_active_id.assert_called_once_with("Vosk") + dialog._populate_model_options.assert_called_once() + assert dialog._applying_settings is False + + +def test_resync_leaves_a_matching_engine_picker_alone(dialog_class): + """No combo churn when the displayed engine already matches the config.""" + dialog = _dialog_stub() + dialog.config_manager.get_settings.return_value = {"speech_recognition": {"engine": "vosk"}} + dialog.engine_combo.get_active_text.return_value = "Vosk" + + dialog_class._resync_model_ui_from_config(dialog) + + dialog.engine_combo.set_active_id.assert_not_called() + + +def test_changing_the_engine_applies_it(dialog_class): + """Selecting an engine must reach the config and the recognizer.""" + dialog = _dialog_stub() + dialog.engine_combo.get_active_text.return_value = "whisper.cpp" + dialog.language_combo.get_active_id.return_value = "en-us" + + dialog_class._on_engine_changed(dialog, None) + + dialog._auto_apply_settings.assert_called_once() + + +def test_changing_to_remote_api_waits_for_a_server_url(dialog_class): + """Applying an unconfigured remote engine would only raise, so defer it.""" + dialog = _dialog_stub() + dialog.engine_combo.get_active_text.return_value = "Remote API" + dialog.language_combo.get_active_id.return_value = "auto" + dialog.remote_api_url_entry.get_text.return_value = " " + + dialog_class._on_engine_changed(dialog, None) + + dialog._auto_apply_settings.assert_not_called() + + +def test_a_resync_repaints_without_applying_or_rewriting_the_language(dialog_class): + """The combo move made by a resync must not cascade into another apply.""" + dialog = _dialog_stub() + dialog._applying_settings = True + dialog.engine_combo.get_active_text.return_value = "Vosk" + dialog.language_combo.get_active_id.return_value = "auto" + + dialog_class._on_engine_changed(dialog, None) + + dialog._auto_apply_settings.assert_not_called() + dialog._populate_model_options.assert_called_once() + assert dialog.language == "en-us" + + +def test_closing_the_dialog_resyncs_an_engine_that_was_never_applied(settings_dialog, dialog_class): + """Remote API without a URL defers the apply; closing must not leave it shown.""" + dialog = _dialog_stub() + gtk = settings_dialog.Gtk + + dialog_class._on_settings_dialog_response(dialog, dialog, gtk.ResponseType.CLOSE) + + dialog._resync_engine_ui_if_unapplied.assert_called_once()