diff --git a/src/vocalinux/ui/settings_dialog.py b/src/vocalinux/ui/settings_dialog.py index 897bf31..d3a485f 100644 --- a/src/vocalinux/ui/settings_dialog.py +++ b/src/vocalinux/ui/settings_dialog.py @@ -2786,10 +2786,22 @@ 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) + # 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) + self.model_recommendation_box.set_no_show_all(True) + self.model_recommendation = Gtk.Label(xalign=0, wrap=True) self.model_recommendation.get_style_context().add_class("model-info-subtitle") - self.model_recommendation.set_no_show_all(True) - self.model_info_card.pack_start(self.model_recommendation, False, False, 0) + self.model_recommendation_box.pack_start(self.model_recommendation, True, True, 0) + + self.model_recommendation_button = Gtk.Button(label="Use it") + self.model_recommendation_button.set_valign(Gtk.Align.CENTER) + self.model_recommendation_button.set_no_show_all(True) + self.model_recommendation_button.connect("clicked", self._on_apply_recommendation) + self.model_recommendation_box.pack_end(self.model_recommendation_button, False, False, 0) + + self.model_info_card.pack_start(self.model_recommendation_box, False, False, 0) # Language warning (e.g. auto-detect, English-only models) lives in # the card so there is a single explanation surface below the group. @@ -4702,11 +4714,74 @@ class SettingsDialog(Gtk.Dialog): language_id, ) + def _downloaded_alternative_for(self, recommended: str) -> Optional[str]: + """Return a model already on disk that can stand in for ``recommended``. + + Every entry in these pickers is a download decision priced in gigabytes, so + suggesting a fresh download while something equivalent is already on disk + wastes the user's bandwidth (#778). Only candidates that are not smaller than + the recommendation qualify, so this never quietly downgrades accuracy, and + English-only weights are only offered when English is actually selected. + """ + if recommended in WHISPERCPP_MODEL_INFO and is_whispercpp_model_downloaded(recommended): + return None + + language_id = self.language_combo.get_active_id() or self.language + wants_english = _language_is_english(language_id) + recommended_mb = WHISPERCPP_MODEL_INFO.get(recommended, {}).get("size_mb", 0) + + # An English-only recommendation must not be answered with multilingual + # weights: they weigh the same and recognise English worse, which is the + # accuracy loss #776 is about. + needs_english_only = is_english_only_whispercpp_model(recommended) + + best = None + best_mb = None + for model_name in list_downloaded_whispercpp_models(): + info = WHISPERCPP_MODEL_INFO.get(model_name) + if not info: + continue + if is_english_only_whispercpp_model(model_name) and not wants_english: + continue + if needs_english_only and not is_english_only_whispercpp_model(model_name): + continue + if info["size_mb"] < recommended_mb: + continue + if best_mb is None or info["size_mb"] < best_mb: + best, best_mb = model_name, info["size_mb"] + + return best + def _get_default_whispercpp_variant_for_size(self, model_size: str) -> Optional[str]: """Return the default specialization for a user-selected size.""" language_id = self.language_combo.get_active_id() or self.language return _default_whispercpp_variant_for_size(model_size, language_id) + def _on_apply_recommendation(self, _button: Any) -> None: + """Set both pickers to the model the card is offering. + + Size and specialization must land under ``_populating_models`` so + ``_on_model_changed`` does not auto-apply the default size variant + (e.g. full ``large``) before the intended target (e.g. turbo q5_0). + """ + target = getattr(self, "_recommended_target_model", None) + if not target or target not in WHISPERCPP_MODEL_INFO: + return + + model_size = get_whispercpp_model_size(target) + self._populating_models = True + try: + self.model_combo.set_active_id(model_size) + self._populate_whispercpp_variant_options(model_size, target) + self.model_variant_combo.set_active_id(target) + self._sync_language_options_for_selected_model() + finally: + self._populating_models = False + + self._update_model_info() + self._refresh_unused_downloads() + self._auto_apply_settings() + def _is_selected_whispercpp_model_english_only(self) -> bool: """Return whether the selected model is a whisper.cpp English-only variant.""" if self._get_selected_engine() != "whisper_cpp": @@ -4895,7 +4970,14 @@ class SettingsDialog(Gtk.Dialog): saved_size = get_whispercpp_model_size(saved_model) for model_size in ENGINE_MODELS["whisper_cpp"]: + # Same shape as the specialization list: a size is a download decision + # too, so its price and whether it is already paid for belong here (#778). + size_variant = self._get_default_whispercpp_variant_for_size(model_size) display_text = _model_display_name(model_size) + if size_variant in WHISPERCPP_MODEL_INFO: + info = WHISPERCPP_MODEL_INFO[size_variant] + status = "✓" if is_whispercpp_model_downloaded(size_variant) else "↓" + display_text += f" ({_format_size(info['size_mb'])}) {status}" if model_size == recommended_size: display_text += " ★" self.model_combo.append(model_size, display_text) @@ -5291,8 +5373,13 @@ class SettingsDialog(Gtk.Dialog): self.language_warning.set_markup("") self.language_warning.hide() - def _on_language_changed(self, widget): - """Handle language selection change.""" + def _on_language_changed(self, widget: Any) -> None: + """Handle language selection change. + + Repaint the recommendation card after the size list rebuilds so + ``_recommended_target_model`` / ★ / "Use it" track EN→non-EN flips + instead of applying a stale English-only target. + """ if self._processing_language_change: return if self._initializing or self._applying_settings: @@ -5311,6 +5398,7 @@ class SettingsDialog(Gtk.Dialog): self.language = lang_code self._populate_model_options() self._update_language_warning() + self._update_model_info() self._auto_apply_settings() finally: self._processing_language_change = False @@ -5458,13 +5546,30 @@ class SettingsDialog(Gtk.Dialog): status = f"Download ~{_format_size(info['size_mb'])}" self.model_info_subtitle.set_markup(f"{extra_info} · {status}") - if model_name == recommended: - self.model_recommendation.hide() + target = recommended + message = None + if engine == "whisper_cpp": + already_have = self._downloaded_alternative_for(recommended) + if already_have and already_have != model_name: + target = already_have + message = ( + f"You already have {_model_display_name(already_have)} on disk — " + "using it needs no download" + ) + + if target == model_name: + self._recommended_target_model = None + self.model_recommendation_box.hide() + self.model_recommendation_button.hide() else: - self.model_recommendation.set_text( - f"Recommended: {recommended_display_name} ({reason})" - ) + self._recommended_target_model = target + if message is None: + message = f"Recommended: {_model_display_name(target)} ({reason})" + self.model_recommendation.set_text(message) self.model_recommendation.show() + can_apply = engine == "whisper_cpp" and target in WHISPERCPP_MODEL_INFO + self.model_recommendation_button.set_visible(can_apply) + self.model_recommendation_box.show() self._update_model_picker_tooltips() self.model_info_card.show() diff --git a/tests/test_recommendation_is_actionable.py b/tests/test_recommendation_is_actionable.py new file mode 100644 index 0000000..118c97e --- /dev/null +++ b/tests/test_recommendation_is_actionable.py @@ -0,0 +1,198 @@ +"""The recommendation must be applicable, and must not ignore the disk (#778).""" + +from __future__ import annotations + +import importlib +import sys +from collections.abc import Iterator +from typing import Any +from unittest.mock import MagicMock, Mock, call, patch + +import pytest + +import vocalinux.ui + + +@pytest.fixture(scope="module") +def settings_dialog() -> Iterator[Any]: + """Import settings_dialog with real base classes for its GTK subclasses. + + Same reasoning as tests/test_settings_model_state.py; both sys.modules and the + package attribute are restored so later tests patching the module by name do + not reach a second module object (#686). + """ + 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: Any) -> Any: + return settings_dialog.SettingsDialog + + +def _dialog_stub(language: str = "en-us") -> Mock: + dialog = Mock() + dialog.language = language + dialog.language_combo.get_active_id.return_value = language + dialog._populating_models = False + dialog._processing_language_change = False + return dialog + + +def _with_disk(settings_dialog: Any, downloaded: list[str]) -> tuple[Any, Any]: + """Patch the disk so only ``downloaded`` counts as present.""" + return ( + patch.object(settings_dialog, "list_downloaded_whispercpp_models", return_value=downloaded), + patch.object( + settings_dialog, + "is_whispercpp_model_downloaded", + side_effect=lambda name: name in downloaded, + ), + ) + + +def test_a_bigger_model_on_disk_is_offered_instead_of_a_download( + settings_dialog: Any, dialog_class: Any +) -> None: + """The reported case: recommended small.en, medium.en already paid for.""" + dialog = _dialog_stub() + listed, downloaded = _with_disk(settings_dialog, ["medium.en", "small", "tiny"]) + + with listed, downloaded: + alternative = dialog_class._downloaded_alternative_for(dialog, "small.en") + + assert alternative == "medium.en" + + +def test_nothing_is_offered_when_the_recommendation_is_already_on_disk( + settings_dialog: Any, dialog_class: Any +) -> None: + dialog = _dialog_stub() + listed, downloaded = _with_disk(settings_dialog, ["small.en", "medium.en"]) + + with listed, downloaded: + assert dialog_class._downloaded_alternative_for(dialog, "small.en") is None + + +def test_a_smaller_model_is_never_offered(settings_dialog: Any, dialog_class: Any) -> None: + """Reusing a download must not quietly cost accuracy.""" + dialog = _dialog_stub() + listed, downloaded = _with_disk(settings_dialog, ["tiny", "tiny.en"]) + + with listed, downloaded: + assert dialog_class._downloaded_alternative_for(dialog, "medium.en") is None + + +def test_english_only_weights_are_not_offered_for_another_language( + settings_dialog: Any, dialog_class: Any +) -> None: + dialog = _dialog_stub(language="pl") + listed, downloaded = _with_disk(settings_dialog, ["medium.en"]) + + with listed, downloaded: + assert dialog_class._downloaded_alternative_for(dialog, "small") is None + + +def test_the_smallest_qualifying_model_wins(settings_dialog: Any, dialog_class: Any) -> None: + """Between two usable downloads, take the cheaper one to run.""" + dialog = _dialog_stub() + listed, downloaded = _with_disk(settings_dialog, ["large", "medium.en"]) + + with listed, downloaded: + assert dialog_class._downloaded_alternative_for(dialog, "small.en") == "medium.en" + + +def test_applying_the_recommendation_sets_both_pickers( + settings_dialog: Any, dialog_class: Any +) -> None: + """Clicking must move size and specialization together, not just one.""" + dialog = _dialog_stub() + dialog._recommended_target_model = "small.en" + + dialog_class._on_apply_recommendation(dialog, None) + + dialog.model_combo.set_active_id.assert_called_once_with("small") + dialog._populate_whispercpp_variant_options.assert_called_once_with("small", "small.en") + dialog.model_variant_combo.set_active_id.assert_called_once_with("small.en") + dialog._sync_language_options_for_selected_model.assert_called_once_with() + dialog._update_model_info.assert_called_once_with() + dialog._refresh_unused_downloads.assert_called_once_with() + dialog._auto_apply_settings.assert_called_once_with() + assert dialog._populating_models is False + + +def test_applying_does_nothing_without_a_target(settings_dialog: Any, dialog_class: Any) -> None: + dialog = _dialog_stub() + dialog._recommended_target_model = None + + dialog_class._on_apply_recommendation(dialog, None) + + dialog.model_combo.set_active_id.assert_not_called() + dialog._auto_apply_settings.assert_not_called() + + +def test_applying_recommendation_suppresses_model_changed_auto_apply( + settings_dialog: Any, dialog_class: Any +) -> None: + """Size change must not briefly auto-apply the default size before the target. + + Without ``_populating_models``, ``set_active_id(large)`` would fire + ``_on_model_changed`` and download full large before turbo-q5_0 lands. + """ + dialog = _dialog_stub() + dialog._recommended_target_model = "large-v3-turbo-q5_0" + seen_while_setting_size: list[bool] = [] + + def capture_flag(_model_size: str) -> bool: + seen_while_setting_size.append(dialog._populating_models) + return True + + dialog.model_combo.set_active_id.side_effect = capture_flag + + dialog_class._on_apply_recommendation(dialog, None) + + assert seen_while_setting_size == [True] + dialog._populate_whispercpp_variant_options.assert_called_once_with( + "large", "large-v3-turbo-q5_0" + ) + dialog.model_variant_combo.set_active_id.assert_called_once_with("large-v3-turbo-q5_0") + # One apply after both pickers settle — not during the size change. + dialog._auto_apply_settings.assert_called_once_with() + assert dialog._populating_models is False + + +def test_language_change_refreshes_recommendation_ui( + settings_dialog: Any, dialog_class: Any +) -> None: + """EN→PL must refresh recommendation target / label so Use it is not stale.""" + dialog = _dialog_stub(language="en-us") + dialog.language_combo.get_active_id.return_value = "pl" + dialog.engine_combo.get_active_text.return_value = "whisper.cpp" + dialog._recommended_target_model = "small.en" + + dialog_class._on_language_changed(dialog, None) + + assert dialog.language == "pl" + dialog._populate_model_options.assert_called_once_with() + dialog._update_language_warning.assert_called_once_with() + dialog._update_model_info.assert_called_once_with() + dialog._auto_apply_settings.assert_called_once_with() + # Refresh happens before auto-apply so the card is current when settings land. + assert dialog.mock_calls.index(call._update_model_info()) < dialog.mock_calls.index( + call._auto_apply_settings() + ) + assert dialog._processing_language_change is False