fix(settings): make model downloads explicit and cancel the whole intent (#1006)
* fix(settings): make model downloads explicit and cancel the whole intent "Use it" on the recommendation card applied a different model than the one shown, a cancelled or failed apply left the staged model behind so the next Test Dictation downloaded it, and cancelling a download tore down and then reloaded the previous model while the dialog waited. - Recommendation is now passive text, compared by size tier, with a link to docs/MODEL_SELECTION.md. The "Use it" button is gone. - Every Settings download asks first and shows size and source host. Declining, cancelling or failing drops the staged model spec and puts the pickers back on the saved config. - Test Dictation refuses a model that is not on disk instead of applying it. - reconfigure() fetches a missing model before releasing the loaded one, so a cancel restores state without reloading or rehashing the old model. Cancellation state is reset only when a download claim is taken or ended. - Follow-layout mode shows the resolved language in the simple Main language control, read-only, and simple edits keep the "layout" setting. Fixes #979 Fixes #980 Fixes #981 Fixes #982 Fixes #985 Fixes #988 * fix(settings): restore follow-layout mode when a model change is declined * fix(settings): keep follow-layout and dictation consistent during model changes * style(tests): format follow-layout regression tests * fix(recognition): skip resume reinit while a model download is running * fix(recognition): keep a lazy reload pending when resume is skipped during a download --------- Co-authored-by: Jatin K Malik <jatinkrmalik@gmail.com>
This commit is contained in:
devin-ai-integration[bot]
co-authored by
Jatin K Malik
Jatin K Malik
parent
f013b7ff88
commit
3e4230ffc2
@@ -13,6 +13,7 @@ import pytest
|
||||
|
||||
import vocalinux.ui
|
||||
from vocalinux.common_types import RecognitionState
|
||||
from vocalinux.utils.model_choice import FASTEST, priority_for_size
|
||||
|
||||
|
||||
@pytest.fixture(scope="module")
|
||||
@@ -201,6 +202,112 @@ def test_failed_auto_apply_resyncs_the_pickers_with_the_config(settings_dialog,
|
||||
assert dialog._applying_settings is False
|
||||
|
||||
|
||||
def test_auto_apply_confirms_before_starting_a_download(
|
||||
settings_dialog: Any, dialog_class: type[Any]
|
||||
) -> None:
|
||||
dialog = _dialog_stub()
|
||||
dialog.get_selected_settings.return_value = {
|
||||
"engine": "whisper_cpp",
|
||||
"model_size": "small",
|
||||
"model_variant": "small",
|
||||
}
|
||||
dialog._confirm_model_download = Mock(return_value=False)
|
||||
dialog._drop_staged_model_spec_and_resync = Mock()
|
||||
|
||||
with patch.object(
|
||||
settings_dialog,
|
||||
"_undownloaded_model_info",
|
||||
return_value={"size_mb": 100, "url": "https://example.test/model.bin"},
|
||||
):
|
||||
dialog_class._auto_apply_settings(dialog, allow_download=True)
|
||||
|
||||
dialog._confirm_model_download.assert_called_once_with("whisper_cpp", "small", 100)
|
||||
dialog.speech_engine.try_begin_download.assert_not_called()
|
||||
dialog._drop_staged_model_spec_and_resync.assert_called_once_with()
|
||||
|
||||
|
||||
def test_apply_confirms_before_starting_a_download(
|
||||
settings_dialog: Any, dialog_class: type[Any]
|
||||
) -> None:
|
||||
dialog = _dialog_stub()
|
||||
dialog.get_selected_settings.return_value = {
|
||||
"engine": "whisper_cpp",
|
||||
"model_size": "small",
|
||||
"model_variant": "small",
|
||||
}
|
||||
dialog._confirm_model_download = Mock(return_value=False)
|
||||
dialog._drop_staged_model_spec_and_resync = Mock()
|
||||
|
||||
with patch.object(
|
||||
settings_dialog,
|
||||
"_undownloaded_model_info",
|
||||
return_value={"size_mb": 100, "url": "https://example.test/model.bin"},
|
||||
):
|
||||
assert dialog_class.apply_settings(dialog) is False
|
||||
|
||||
dialog._confirm_model_download.assert_called_once_with("whisper_cpp", "small", 100)
|
||||
dialog.speech_engine.try_begin_download.assert_not_called()
|
||||
dialog._drop_staged_model_spec_and_resync.assert_called_once_with()
|
||||
|
||||
|
||||
def test_download_confirmation_includes_catalog_host_and_cancel_default(
|
||||
settings_dialog: Any, dialog_class: type[Any]
|
||||
) -> None:
|
||||
gtk = settings_dialog.Gtk
|
||||
gtk.MessageDialog.reset_mock()
|
||||
message_dialog = gtk.MessageDialog.return_value
|
||||
message_dialog.run.return_value = gtk.ResponseType.CANCEL
|
||||
|
||||
with patch.object(
|
||||
settings_dialog,
|
||||
"_undownloaded_model_info",
|
||||
return_value={"url": "https://models.example.test/model.bin"},
|
||||
):
|
||||
assert dialog_class._confirm_model_download(Mock(), "whisper_cpp", "small", 125) is False
|
||||
|
||||
gtk.MessageDialog.assert_called_once()
|
||||
kwargs = gtk.MessageDialog.call_args.kwargs
|
||||
assert kwargs["message_type"] == gtk.MessageType.QUESTION
|
||||
assert kwargs["text"] == "Download Small?"
|
||||
message_dialog.format_secondary_text.assert_called_once_with(
|
||||
"About 125 MB from models.example.test. Nothing is downloaded until you confirm."
|
||||
)
|
||||
message_dialog.set_default_response.assert_called_once_with(gtk.ResponseType.CANCEL)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("engine", "model_name", "language", "host"),
|
||||
[
|
||||
("whisper", "small", "en-us", "openaipublic.azureedge.net"),
|
||||
("whisper_cpp", "small", "en-us", "huggingface.co"),
|
||||
("vosk", "small", "en-us", "alphacephei.com"),
|
||||
("parakeet", "v3-european", "en-us", "huggingface.co"),
|
||||
("faster_whisper", "tiny", "en-us", "huggingface.co"),
|
||||
],
|
||||
)
|
||||
def test_model_download_source_url_covers_each_downloadable_engine(
|
||||
settings_dialog: Any, engine: str, model_name: str, language: str, host: str
|
||||
) -> None:
|
||||
url = settings_dialog._model_download_source_url(engine, model_name, language)
|
||||
|
||||
assert settings_dialog.urlparse(url).netloc == host
|
||||
|
||||
|
||||
def test_download_confirmation_derives_host_when_catalog_has_no_url(
|
||||
settings_dialog: Any, dialog_class: type[Any]
|
||||
) -> None:
|
||||
gtk = settings_dialog.Gtk
|
||||
gtk.MessageDialog.reset_mock()
|
||||
gtk.MessageDialog.return_value.run.return_value = gtk.ResponseType.CANCEL
|
||||
|
||||
with patch.object(settings_dialog, "_undownloaded_model_info", return_value={"size_mb": 125}):
|
||||
assert dialog_class._confirm_model_download(Mock(), "whisper", "small", 125) is False
|
||||
|
||||
gtk.MessageDialog.return_value.format_secondary_text.assert_called_once_with(
|
||||
"About 125 MB from openaipublic.azureedge.net. Nothing is downloaded until you confirm."
|
||||
)
|
||||
|
||||
|
||||
def test_already_downloaded_auto_apply_runs_on_a_worker(settings_dialog, dialog_class):
|
||||
"""A model already on disk must not reconfigure on the GTK main loop."""
|
||||
dialog = _dialog_stub()
|
||||
@@ -430,7 +537,7 @@ def test_download_path_resyncs_when_the_apply_reports_failure(settings_dialog, d
|
||||
|
||||
modal = modal_class.return_value
|
||||
scheduled = [(func, args) for func, args in idle_calls]
|
||||
assert (dialog._idle_resync_model_ui_from_config, ()) in scheduled
|
||||
assert (dialog._idle_drop_staged_model_spec_and_resync, ()) 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()
|
||||
@@ -456,7 +563,7 @@ def test_download_path_resyncs_when_the_download_is_cancelled(settings_dialog, d
|
||||
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
|
||||
assert (dialog._idle_drop_staged_model_spec_and_resync, ()) in idle_calls
|
||||
assert (modal.set_complete, (False, "Download cancelled")) in idle_calls
|
||||
|
||||
|
||||
@@ -478,7 +585,7 @@ def test_modal_close_resyncs_an_engine_that_never_applied(settings_dialog, dialo
|
||||
):
|
||||
dialog_class._auto_apply_settings(dialog, allow_download=True)
|
||||
|
||||
dialog._resync_engine_ui_if_unapplied.assert_called_once()
|
||||
dialog._drop_staged_model_spec_and_resync.assert_called_once_with()
|
||||
|
||||
|
||||
def test_idle_resync_skips_a_destroyed_dialog(dialog_class):
|
||||
@@ -556,6 +663,107 @@ def test_resync_restores_language_from_saved_config(dialog_class):
|
||||
dialog.engine_combo.set_active_id.assert_not_called()
|
||||
|
||||
|
||||
def _resync_dialog_for_language(
|
||||
dialog_class: type[Any], saved_language: str
|
||||
) -> tuple[Mock, dict[str, Any]]:
|
||||
dialog = _dialog_stub()
|
||||
state: dict[str, Any] = {
|
||||
"language": "auto",
|
||||
"follow": False,
|
||||
"priority": FASTEST,
|
||||
}
|
||||
dialog.config_manager.get_settings.return_value = {
|
||||
"speech_recognition": {
|
||||
"engine": "whisper_cpp",
|
||||
"model_size": "medium",
|
||||
"language": saved_language,
|
||||
}
|
||||
}
|
||||
dialog.config_manager.get.return_value = ""
|
||||
dialog.engine_combo.get_active_text.return_value = "whisper.cpp"
|
||||
dialog.language_combo.get_active_id.side_effect = lambda: state["language"]
|
||||
dialog.language_combo.set_active_id.side_effect = lambda value: state.__setitem__(
|
||||
"language", value
|
||||
)
|
||||
dialog.follow_layout_switch.get_active.side_effect = lambda: state["follow"]
|
||||
dialog.follow_layout_switch.set_active.side_effect = lambda value: state.__setitem__(
|
||||
"follow", value
|
||||
)
|
||||
dialog.simple_priority_combo.get_active_id.side_effect = lambda: state["priority"]
|
||||
dialog.simple_priority_combo.set_active_id.side_effect = lambda value: state.__setitem__(
|
||||
"priority", value
|
||||
)
|
||||
dialog._get_selected_engine.return_value = "whisper_cpp"
|
||||
dialog._get_selected_whispercpp_model.return_value = "medium"
|
||||
dialog._get_recommended_whispercpp_model_for_language.return_value = ("small", "reason")
|
||||
dialog._follow_layout_supported.return_value = True
|
||||
dialog._is_selected_whispercpp_model_english_only.return_value = False
|
||||
|
||||
def set_combo_active_id(combo: Any, value: str) -> bool:
|
||||
combo.set_active_id(value)
|
||||
return True
|
||||
|
||||
dialog._set_combo_active_id_or_first.side_effect = set_combo_active_id
|
||||
dialog._populate_model_options = Mock()
|
||||
dialog._populate_language_options = Mock()
|
||||
dialog._update_model_info = Mock()
|
||||
dialog._update_model_picker_tooltips = Mock()
|
||||
dialog._update_simple_visibility = Mock()
|
||||
dialog._auto_apply_settings = Mock()
|
||||
dialog._confirm_model_download = Mock()
|
||||
dialog.config_manager.update_speech_recognition_settings = Mock()
|
||||
dialog.speech_engine.try_begin_download = Mock()
|
||||
dialog._simple_syncing = False
|
||||
dialog._staged_model_spec = {"engine": "whisper_cpp", "model_size": "large"}
|
||||
_bind_real(
|
||||
dialog,
|
||||
dialog_class,
|
||||
"_drop_staged_model_spec_and_resync",
|
||||
"_resync_model_ui_from_config",
|
||||
"_sync_language_options_for_selected_model",
|
||||
"_sync_follow_layout_controls",
|
||||
"_update_language_warning",
|
||||
"_sync_simple_from_advanced",
|
||||
)
|
||||
return dialog, state
|
||||
|
||||
|
||||
def test_declined_model_resync_restores_follow_layout_and_saved_simple_priority(
|
||||
settings_dialog: Any, dialog_class: type[Any]
|
||||
) -> None:
|
||||
dialog, state = _resync_dialog_for_language(dialog_class, "layout")
|
||||
|
||||
with patch.object(settings_dialog, "language_for_active_layout", return_value="en-us"):
|
||||
dialog._drop_staged_model_spec_and_resync()
|
||||
|
||||
expected_priority = priority_for_size("small", "medium")
|
||||
assert dialog._staged_model_spec is None
|
||||
assert dialog._follow_layout_saved is True
|
||||
assert dialog.follow_layout_switch.get_active() is True
|
||||
assert dialog._follow_layout_active is True
|
||||
assert dialog.language_combo.get_active_id() == "en-us"
|
||||
assert dialog.language == "en-us"
|
||||
assert state["priority"] == expected_priority
|
||||
dialog.config_manager.update_speech_recognition_settings.assert_not_called()
|
||||
dialog.speech_engine.try_begin_download.assert_not_called()
|
||||
dialog._confirm_model_download.assert_not_called()
|
||||
dialog._auto_apply_settings.assert_not_called()
|
||||
|
||||
|
||||
def test_resync_with_saved_language_disables_follow_layout(
|
||||
dialog_class: type[Any],
|
||||
) -> None:
|
||||
dialog, _state = _resync_dialog_for_language(dialog_class, "de")
|
||||
|
||||
dialog._drop_staged_model_spec_and_resync()
|
||||
|
||||
assert dialog._follow_layout_saved is False
|
||||
assert dialog.follow_layout_switch.get_active() is False
|
||||
assert dialog._follow_layout_active is False
|
||||
assert dialog.language_combo.get_active_id() == "de"
|
||||
assert dialog.language == "de"
|
||||
|
||||
|
||||
def test_picker_handlers_early_return_while_applying(dialog_class: type[Any]) -> None:
|
||||
"""Size, specialization, and language handlers must not rebuild or apply mid-apply."""
|
||||
dialog = _dialog_stub()
|
||||
@@ -744,6 +952,7 @@ def _download_setup(dialog: Mock) -> None:
|
||||
"language": "auto",
|
||||
}
|
||||
dialog._apply_settings_internal.return_value = True
|
||||
dialog._confirm_model_download.return_value = True
|
||||
|
||||
|
||||
@pytest.mark.parametrize("entry", ["_auto_apply_settings", "apply_settings"])
|
||||
@@ -772,7 +981,7 @@ def test_settings_refuses_a_download_while_the_tray_holds_the_engine(
|
||||
modal_class.assert_not_called()
|
||||
dialog._apply_settings_internal.assert_not_called()
|
||||
dialog._show_download_busy_dialog.assert_called_once_with()
|
||||
dialog._resync_model_ui_from_config.assert_called_once_with()
|
||||
dialog._drop_staged_model_spec_and_resync.assert_called_once_with()
|
||||
|
||||
|
||||
@pytest.mark.parametrize("entry", ["_auto_apply_settings", "apply_settings"])
|
||||
@@ -796,6 +1005,43 @@ def test_settings_releases_the_engine_once_the_download_is_over(
|
||||
dialog.speech_engine.end_download.assert_called_once_with()
|
||||
|
||||
|
||||
@pytest.mark.parametrize("entry", ["_auto_apply_settings", "apply_settings"])
|
||||
def test_cancel_poll_stays_registered_until_download_worker_removes_it(
|
||||
settings_dialog: Any, dialog_class: type[Any], entry: str
|
||||
) -> None:
|
||||
dialog = _dialog_stub()
|
||||
_download_setup(dialog)
|
||||
source_callbacks = []
|
||||
callback_results = []
|
||||
glib = MagicMock()
|
||||
|
||||
def timeout_add(_interval: int, callback: Callable[[], bool]) -> int:
|
||||
source_callbacks.append(callback)
|
||||
callback_results.append(callback())
|
||||
return 74
|
||||
|
||||
glib.timeout_add.side_effect = timeout_add
|
||||
|
||||
with (
|
||||
patch.object(settings_dialog, "_undownloaded_model_info", return_value={"size_mb": 100}),
|
||||
patch.object(settings_dialog, "ModelDownloadDialog") as modal_class,
|
||||
patch.object(settings_dialog, "GLib", glib),
|
||||
patch.object(settings_dialog.threading, "Thread", _InlineThread),
|
||||
):
|
||||
modal_class.return_value.cancelled = True
|
||||
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()
|
||||
assert len(source_callbacks) == 1
|
||||
assert callback_results == [True]
|
||||
glib.source_remove.assert_called_once_with(74)
|
||||
dialog.speech_engine.cancel_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:
|
||||
@@ -921,7 +1167,6 @@ def _update_model_info_stub(dialog_class: type[Any]) -> Mock:
|
||||
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
|
||||
|
||||
|
||||
@@ -1185,6 +1430,17 @@ def test_whisper_selected_settings_keep_combo_language(dialog_class):
|
||||
assert settings["language"] == "fr"
|
||||
|
||||
|
||||
def test_follow_layout_selected_settings_keep_the_layout_sentinel(
|
||||
settings_dialog: Any, dialog_class: type[Any]
|
||||
) -> None:
|
||||
dialog = _dialog_for_selected_settings("whisper.cpp", "de")
|
||||
|
||||
with patch.object(settings_dialog, "_is_following_layout", return_value=True):
|
||||
settings = dialog_class.get_selected_settings(dialog)
|
||||
|
||||
assert settings["language"] == settings_dialog.LANGUAGE_FOLLOWS_LAYOUT
|
||||
|
||||
|
||||
def test_parakeet_engine_change_forces_language_auto(dialog_class):
|
||||
"""A leftover catalog language from another engine must not stick in memory."""
|
||||
dialog = _dialog_stub()
|
||||
@@ -1458,7 +1714,12 @@ def test_speech_manager_reconfigure_to_parakeet_normalizes_language():
|
||||
defer_download=True,
|
||||
)
|
||||
assert manager.language == "en-us"
|
||||
manager.reconfigure(engine="parakeet", model_size="v3-european", language="fr")
|
||||
manager.reconfigure(
|
||||
engine="parakeet",
|
||||
model_size="v3-european",
|
||||
language="fr",
|
||||
force_download=False,
|
||||
)
|
||||
assert manager.engine == "parakeet"
|
||||
assert manager.language == "auto"
|
||||
assert manager.command_processor.language == "auto"
|
||||
|
||||
Reference in New Issue
Block a user