From 329a7d7f6a6f2ec8c025f765d8afcd1879eb8488 Mon Sep 17 00:00:00 2001 From: "devin-ai-integration[bot]" <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Wed, 7 Oct 2026 17:17:08 -0700 Subject: [PATCH] fix(settings): stage Advanced model spec until Download is confirmed (#908) * fix(settings): stage Advanced model spec until Download is confirmed Picker changes in Speech Model -> Advanced staged a spec but immediately triggered a model download for every intermediate selection. Gate the auto-apply download path so a missing model only stages: a Download action on the model info card now runs apply_settings() as the single explicit confirmation. Simple-mode questions keep the instant apply-and-download flow. Fixes #894 * fix(settings): repaint model info card after a completed download The post-download paths repopulated pickers and resynced the engine but never repainted the info card, so a finished download left the amber status and the Download action visible for a model already on disk. The cancel path already repainted via the idle resync; give success its own. * fix(settings): keep staged spec through picker rebuilds and save non-model edits * fix(settings): persist non-model edits without a model-reloading reconfigure * fix(settings): live-apply non-model edits, keep whispercpp params saved-only while staged * fix(settings): live-set changed VAD/timeout attrs without reconfigure while staged --------- Co-authored-by: Jatin K Malik --- src/vocalinux/ui/settings_dialog.py | 131 +++++++++++++++++++- tests/test_settings_model_state.py | 180 +++++++++++++++++++++++++++- 2 files changed, 300 insertions(+), 11 deletions(-) diff --git a/src/vocalinux/ui/settings_dialog.py b/src/vocalinux/ui/settings_dialog.py index 7a603f7..45a1421 100644 --- a/src/vocalinux/ui/settings_dialog.py +++ b/src/vocalinux/ui/settings_dialog.py @@ -318,6 +318,20 @@ def _model_specialization_display_name(model_name: str) -> str: #: attribute, so tests driving the methods against a Mock still reach it. FOLLOW_LAYOUT_ENGINES = ("whisper", "whisper_cpp", "faster_whisper", "remote_api") +#: Selected-setting keys that make up the model spec. While a spec that needs a +#: download is staged (#894), these stay out of the saved config until the +#: explicit Download confirmation; every other key applies and saves as usual. +_MODEL_SPEC_KEYS = ("engine", "model_size", "model_variant", "language") + +#: Engine attributes a Settings edit can update live with the same clamping +#: ``reconfigure()`` applies — minus its recording cancel and any model +#: reload. Every other key (all ``whispercpp_*`` load-time params) stays +#: saved-only while a download is staged (#894). +_LIVE_ENGINE_SETTINGS = { + "vad_sensitivity": lambda value: max(1, min(5, int(value))), + "silence_timeout": lambda value: max(0.5, min(5.0, float(value))), +} + def _is_following_layout(dialog: Any) -> bool: """Whether the follow-keyboard-layout mode is on for this dialog (#821). @@ -2666,6 +2680,10 @@ class SettingsDialog(Gtk.Dialog): # Cached so Mock-driven tests and early calls read a real bool. self._follow_layout_active = False self._applying_settings = False # Flag to prevent recursive settings application + # The last Advanced picker spec that needs a download and is still + # unconfirmed: picker rebuilds keep offering it instead of falling back + # to the saved model, and a real apply or resync clears it (#894). + self._staged_model_spec: Optional[dict] = None self._advanced_prompt_dirty = False self._language_candidates_dirty = False # Deferred text edits stashed when the dialog closes mid-apply; persisted @@ -4757,6 +4775,15 @@ class SettingsDialog(Gtk.Dialog): self.model_info_subtitle.get_style_context().add_class("model-info-subtitle") self.model_info_card.pack_start(self.model_info_subtitle, False, False, 0) + # Picking a spec only stages it; this button is the explicit + # confirmation that turns the staged spec into a download (#894). + self.model_download_button = Gtk.Button(label="Download") + self.model_download_button.get_style_context().add_class("suggested-action") + self.model_download_button.set_halign(Gtk.Align.START) + self.model_download_button.set_no_show_all(True) + self.model_download_button.connect("clicked", self._on_download_model_clicked) + self.model_info_card.pack_start(self.model_download_button, False, False, 0) + # The recommendation used to be a plain label, which left the panel stating # the right answer while the pickers kept the wrong one (#778). self.model_recommendation_box = Gtk.Box(orientation=Gtk.Orientation.HORIZONTAL, spacing=8) @@ -8080,11 +8107,22 @@ class SettingsDialog(Gtk.Dialog): combo_id = size if engine == "faster_whisper" else size.capitalize() self.model_combo.append(combo_id, display_text) - # Determine which model to select + # Determine which model to select. A staged spec wins over the + # saved model so a rebuild (e.g. a language change) keeps the + # unconfirmed pick instead of snapping back to the saved one. saved_model = saved_model_for_engine.lower() valid_models = [m.lower() for m in ENGINE_MODELS.get(engine, [])] - if saved_model in valid_models: + staged = self._staged_model_spec + staged_model = None + if isinstance(staged, dict) and staged.get("engine") == engine: + candidate = str(staged.get("model_size") or "").lower() + if candidate in valid_models: + staged_model = candidate + + if staged_model: + selected = staged_model + elif saved_model in valid_models: selected = saved_model elif downloaded_models: selected = downloaded_models[0] @@ -8135,7 +8173,16 @@ class SettingsDialog(Gtk.Dialog): recommended_model, _ = self._get_recommended_whispercpp_model_for_language() recommended_size = get_whispercpp_model_size(recommended_model) - saved_model = self._resolve_saved_whispercpp_variant(saved_model_for_engine) + staged = self._staged_model_spec + staged_model = "" + if isinstance(staged, dict) and staged.get("engine") == "whisper_cpp": + staged_model = str(staged.get("model_size") or "") + if staged_model in WHISPERCPP_MODEL_INFO: + # The staged spec survives rebuilds until its Download confirmation + # or a real apply (#894); it wins over the saved seed. + saved_model = staged_model + else: + saved_model = self._resolve_saved_whispercpp_variant(saved_model_for_engine) if saved_model not in WHISPERCPP_MODEL_INFO: saved_model = ( recommended_model if recommended_model in WHISPERCPP_MODEL_INFO else "tiny" @@ -8948,7 +8995,9 @@ class SettingsDialog(Gtk.Dialog): ) self.config_manager.set("speech_recognition", "simple_second_language", second or "") self._update_simple_visibility() - self._auto_apply_settings() + # A simple answer resolves to a complete spec, so its pick keeps the + # instant apply-and-download flow the Advanced pickers no longer get. + self._auto_apply_settings(allow_download=True) def _commit_or_restore_simple_language_entry(self) -> bool: """Resolve text typed into the simple language box, or restore the last pick.""" @@ -9124,6 +9173,9 @@ class SettingsDialog(Gtk.Dialog): else: status = f"Download ~{_format_size(info['size_mb'])}" self.model_info_subtitle.set_markup(f"{extra_info} · {status}") + # The confirm action follows the displayed spec: offered only while it + # names a model that is not on disk (#894). + self.model_download_button.set_visible(not is_downloaded) target = recommended message = None @@ -9155,8 +9207,16 @@ class SettingsDialog(Gtk.Dialog): self.model_info_title.show() self.model_info_subtitle.show() - def _auto_apply_settings(self): - """Automatically apply settings when changed.""" + def _auto_apply_settings(self, allow_download: bool = False) -> None: + """Automatically apply settings when changed. + + A picker change stages a model spec; it never confirms a download. + ``allow_download`` is passed only by the simple questions, whose each + answer resolves to a complete spec, so their pick keeps the existing + apply-and-download flow. Every other caller leaves a missing model + staged: the pickers keep it, the info card offers Download, and only + ``apply_settings`` (that button, or Test) starts the transfer (#894). + """ if _handlers_suppressed(self): return @@ -9194,6 +9254,45 @@ class SettingsDialog(Gtk.Dialog): # Check if model needs to be downloaded missing_model = _undownloaded_model_info(self.language, engine, model_name) + if missing_model is not None and not allow_download: + # Staging, not confirming: leave the staged spec in the pickers + # and the saved one in the config, and repaint the info card so + # its Download action offers the explicit confirmation (#894). + self._staged_model_spec = { + "engine": engine, + "model_size": model_name, + "model_variant": settings.get("model_variant", ""), + "language": self.language, + } + # Edits outside the spec are not part of the staging: apply + # and persist them so VAD, timeouts and advanced params are + # not dropped while a download is staged. Not reconfigure() — + # it cancels a buffered recording and waits on its worker + # here on the GTK thread, and every whispercpp_* kwarg forces + # a model reload. Live-set only what actually changed; the + # load-time params stay saved-only until the Download apply + # or the next start. + non_model_settings = { + key: value for key, value in settings.items() if key not in _MODEL_SPEC_KEYS + } + applied = True + for key, clamp in _LIVE_ENGINE_SETTINGS.items(): + if key not in non_model_settings: + continue + try: + live_value = clamp(non_model_settings[key]) + except (TypeError, ValueError) as e: + logger.error(f"Invalid {key} while staging: {e}") + applied = False + continue + if getattr(self.speech_engine, key, None) == live_value: + continue + setattr(self.speech_engine, key, live_value) + if non_model_settings and applied: + self._save_selected_settings(non_model_settings) + self._update_model_info() + return + if missing_model is not None: if not self.speech_engine.try_begin_download(): # The tray is already downloading a model; a second download @@ -9272,6 +9371,10 @@ class SettingsDialog(Gtk.Dialog): # 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() + # Repaint the info card too: after a success it still showed the + # pre-download render (amber size line, Download action) until + # some other change repainted it (#894 follow-up). + self._update_model_info() return logger.info(f"Auto-applying settings: {settings}") @@ -9386,6 +9489,9 @@ class SettingsDialog(Gtk.Dialog): for key, value in advanced_settings.items(): self.config_manager.set("advanced", key, value) self.config_manager.save_settings() + if any(key in settings for key in _MODEL_SPEC_KEYS): + # A real spec just persisted, so nothing is staged anymore (#894). + self._staged_model_spec = None def get_selected_settings(self) -> dict[str, Any]: """Return the currently selected settings from the UI.""" @@ -9783,6 +9889,16 @@ For now, the engine has been reverted to VOSK.""" self._populate_model_options() self._update_engine_specific_ui() + def _on_download_model_clicked(self, _button: Any) -> None: + """Confirm the staged model spec and start its download (#894). + + The pickers only stage a specification; this is the one explicit + confirmation that turns it into a transfer. It goes through + ``apply_settings`` so the engine claim, the modal progress dialog, and + the post-download apply behave exactly like any other confirmed apply. + """ + self.apply_settings() + def apply_settings(self) -> bool: """Apply the selected settings.""" if self._applying_settings: @@ -9866,6 +9982,9 @@ For now, the engine has been reverted to VOSK.""" self._populate_model_options() self._resync_engine_ui_if_unapplied() + # Same repaint as the auto-apply path: a finished download must not + # leave the card offering a Download for the model just fetched. + self._update_model_info() return True return self._apply_settings_internal(settings) diff --git a/tests/test_settings_model_state.py b/tests/test_settings_model_state.py index 6268bf2..2e12aad 100644 --- a/tests/test_settings_model_state.py +++ b/tests/test_settings_model_state.py @@ -67,6 +67,9 @@ def _dialog_stub() -> Mock: dialog.language = "en-us" dialog._last_non_parakeet_language = None dialog._engine_for_language_memory = None + # A Mock attribute is not a dict, which keeps the staged-spec lookups on the + # saved-model fallback; tests that exercise staging set this themselves. + dialog._staged_model_spec = None # 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 @@ -423,7 +426,7 @@ def test_download_path_resyncs_when_the_apply_reports_failure(settings_dialog, d patch.object(settings_dialog, "GLib", _glib_stub(idle_calls)), patch.object(settings_dialog.threading, "Thread", _InlineThread), ): - dialog_class._auto_apply_settings(dialog) + dialog_class._auto_apply_settings(dialog, allow_download=True) modal = modal_class.return_value scheduled = [(func, args) for func, args in idle_calls] @@ -450,7 +453,7 @@ def test_download_path_resyncs_when_the_download_is_cancelled(settings_dialog, d patch.object(settings_dialog, "GLib", _glib_stub(idle_calls)), patch.object(settings_dialog.threading, "Thread", _InlineThread), ): - dialog_class._auto_apply_settings(dialog) + dialog_class._auto_apply_settings(dialog, allow_download=True) modal = modal_class.return_value assert (dialog._idle_resync_model_ui_from_config, ()) in idle_calls @@ -473,7 +476,7 @@ def test_modal_close_resyncs_an_engine_that_never_applied(settings_dialog, dialo patch.object(settings_dialog, "GLib", MagicMock()), patch.object(settings_dialog.threading, "Thread", _InlineThread), ): - dialog_class._auto_apply_settings(dialog) + dialog_class._auto_apply_settings(dialog, allow_download=True) dialog._resync_engine_ui_if_unapplied.assert_called_once() @@ -761,7 +764,10 @@ def test_settings_refuses_a_download_while_the_tray_holds_the_engine( patch.object(settings_dialog, "GLib", MagicMock()), patch.object(settings_dialog.threading, "Thread", _InlineThread), ): - getattr(dialog_class, entry)(dialog) + if entry == "_auto_apply_settings": + dialog_class._auto_apply_settings(dialog, allow_download=True) + else: + dialog_class.apply_settings(dialog) modal_class.assert_not_called() dialog._apply_settings_internal.assert_not_called() @@ -781,12 +787,176 @@ def test_settings_releases_the_engine_once_the_download_is_over( patch.object(settings_dialog, "GLib", MagicMock()), patch.object(settings_dialog.threading, "Thread", _InlineThread), ): - getattr(dialog_class, entry)(dialog) + if entry == "_auto_apply_settings": + dialog_class._auto_apply_settings(dialog, allow_download=True) + else: + dialog_class.apply_settings(dialog) dialog.speech_engine.try_begin_download.assert_called_once_with() dialog.speech_engine.end_download.assert_called_once_with() +def test_a_picker_change_only_stages_a_missing_model( + settings_dialog: Any, dialog_class: type[Any] +) -> None: + """#894: a spec that is not on disk must not start downloading on its own. + + Pickers stage the spec; nothing downloads and nothing is resynced away, so + the next pick keeps composing instead of fighting a modal. Edits outside + the spec are not part of it and still apply. + """ + dialog = _dialog_stub() + _download_setup(dialog) + dialog.get_selected_settings.return_value["vad_sensitivity"] = 2 + + 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", MagicMock()), + patch.object(settings_dialog.threading, "Thread", _InlineThread), + ): + dialog_class._auto_apply_settings(dialog) + + modal_class.assert_not_called() + dialog.speech_engine.try_begin_download.assert_not_called() + # The spec keys are staged, not applied; edits outside it reach the + # engine live and save — never through reconfigure, which would cancel + # a buffered recording and reload the model on whispercpp_* keys. + dialog._apply_settings_internal.assert_not_called() + dialog.speech_engine.reconfigure.assert_not_called() + assert dialog.speech_engine.vad_sensitivity == 2 + dialog._save_selected_settings.assert_called_once_with({"vad_sensitivity": 2}) + assert dialog._staged_model_spec == { + "engine": "whisper_cpp", + "model_size": "small", + "model_variant": "", + "language": "en-us", + } + dialog._resync_model_ui_from_config.assert_not_called() + dialog._resync_engine_ui_if_unapplied.assert_not_called() + dialog._update_model_info.assert_called_once_with() + + +def test_staging_several_specs_still_downloads_none( + settings_dialog: Any, dialog_class: type[Any] +) -> None: + """Each field of the spec can flip without a single download starting.""" + dialog = _dialog_stub() + _download_setup(dialog) + + 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", MagicMock()), + patch.object(settings_dialog.threading, "Thread", _InlineThread), + ): + for _flip in range(3): + dialog_class._auto_apply_settings(dialog) + + modal_class.assert_not_called() + dialog.speech_engine.try_begin_download.assert_not_called() + dialog._apply_settings_internal.assert_not_called() + + +def test_a_download_allowed_apply_still_downloads( + settings_dialog: Any, dialog_class: type[Any] +) -> None: + """The simple questions keep the instant apply-and-download flow.""" + dialog = _dialog_stub() + _download_setup(dialog) + + 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", MagicMock()), + patch.object(settings_dialog.threading, "Thread", _InlineThread), + ): + dialog_class._auto_apply_settings(dialog, allow_download=True) + + dialog.speech_engine.try_begin_download.assert_called_once_with() + modal_class.assert_called_once() + + +def test_the_download_button_confirms_the_staged_spec(dialog_class: type[Any]) -> None: + """The info card's Download action is the one explicit confirmation.""" + dialog = _dialog_stub() + + dialog_class._on_download_model_clicked(dialog, None) + + dialog.apply_settings.assert_called_once_with() + + +@pytest.mark.parametrize("entry", ["_auto_apply_settings", "apply_settings"]) +def test_a_finished_download_repaints_the_model_info_card( + settings_dialog: Any, dialog_class: type[Any], entry: str +) -> None: + """A completed download must not leave the card on the pre-download render. + + Without the repaint, the card keeps the amber size line and, since #894, + a Download action for the model that was just fetched. The cancel path + already repaints via ``_idle_resync_model_ui_from_config``; success needs + its own. + """ + dialog = _dialog_stub() + _download_setup(dialog) + + 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), + ): + if entry == "_auto_apply_settings": + dialog_class._auto_apply_settings(dialog, allow_download=True) + else: + dialog_class.apply_settings(dialog) + + dialog._update_model_info.assert_called_once() + + +def _update_model_info_stub(dialog_class: type[Any]) -> Mock: + """Enough widget state for the real ``_update_model_info`` on whisper.cpp.""" + dialog = _dialog_stub() + _bind_real(dialog, dialog_class, "_update_model_info") + dialog.engine_combo.get_active_text.return_value = "whisper.cpp" + dialog._get_selected_whispercpp_model.return_value = "small" + dialog._get_recommended_whispercpp_model_for_language.return_value = ("small", "fits") + dialog._downloaded_alternative_for.return_value = None + return dialog + + +def test_the_info_card_offers_download_for_a_missing_model( + settings_dialog: Any, dialog_class: type[Any] +) -> None: + """An undownloaded spec shows the confirm action on the info card.""" + dialog = _update_model_info_stub(dialog_class) + + with ( + patch.object(settings_dialog, "is_whispercpp_model_downloaded", return_value=False), + patch.object(settings_dialog, "detect_compute_backend", return_value=("cpu", {})), + patch.object(settings_dialog, "get_backend_display_name", return_value="CPU"), + ): + dialog._update_model_info() + + dialog.model_download_button.set_visible.assert_called_once_with(True) + + +def test_the_info_card_offers_no_download_for_a_downloaded_model( + settings_dialog: Any, dialog_class: type[Any] +) -> None: + """An already-downloaded spec never prompts (#894 acceptance).""" + dialog = _update_model_info_stub(dialog_class) + + with ( + patch.object(settings_dialog, "is_whispercpp_model_downloaded", return_value=True), + patch.object(settings_dialog, "detect_compute_backend", return_value=("cpu", {})), + patch.object(settings_dialog, "get_backend_display_name", return_value="CPU"), + ): + dialog._update_model_info() + + dialog.model_download_button.set_visible.assert_called_once_with(False) + + def _dialog_for_engine_ui(engine_text: str): """Stub widgets that `_update_engine_specific_ui` show/hides.""" dialog = _dialog_stub()