fix(settings): report failed model downloads instead of claiming success (#690)
Both modal download paths ran _apply_settings_internal() on a worker thread and then unconditionally marked the progress dialog complete. Since _apply_settings_internal() swallows every exception internally, a download that failed or was cancelled still ended with "Complete!", the dedicated except blocks below (the "Download cancelled" and Whisper-not-installed branches) were unreachable, and the swallowed path built and ran a Gtk.MessageDialog from the download thread, off the main loop. Give _apply_settings_internal() a raise_errors flag and pass it from the two thread call sites. Their handlers become reachable and already report through the progress dialog via GLib.idle_add, so failures now surface with the real reason and no Gtk work happens off the main loop. Main-loop callers keep the old contract: swallow, show the error dialog, return False.
This commit is contained in:
@@ -4780,7 +4780,7 @@ class SettingsDialog(Gtk.Dialog):
|
||||
cancel_check_id = GLib.timeout_add(100, check_cancelled)
|
||||
|
||||
try:
|
||||
self._apply_settings_internal(settings)
|
||||
self._apply_settings_internal(settings, raise_errors=True)
|
||||
GLib.idle_add(download_dialog.set_complete, True, "")
|
||||
GLib.idle_add(self._populate_model_options)
|
||||
finally:
|
||||
@@ -5070,7 +5070,7 @@ For now, the engine has been reverted to VOSK."""
|
||||
cancel_check_id = GLib.timeout_add(100, check_cancelled)
|
||||
|
||||
try:
|
||||
self._apply_settings_internal(settings)
|
||||
self._apply_settings_internal(settings, raise_errors=True)
|
||||
GLib.idle_add(download_dialog.set_complete, True, "")
|
||||
finally:
|
||||
GLib.source_remove(cancel_check_id)
|
||||
@@ -5095,8 +5095,16 @@ For now, the engine has been reverted to VOSK."""
|
||||
|
||||
return self._apply_settings_internal(settings)
|
||||
|
||||
def _apply_settings_internal(self, settings: dict) -> bool:
|
||||
"""Internal method to apply settings."""
|
||||
def _apply_settings_internal(self, settings: dict, raise_errors: bool = False) -> bool:
|
||||
"""Internal method to apply settings.
|
||||
|
||||
Args:
|
||||
settings: The settings to persist and hand to the engine.
|
||||
raise_errors: Re-raise failures instead of showing an error dialog.
|
||||
The download threads pass True: their own handlers report the
|
||||
failure through the progress dialog, and building a Gtk dialog
|
||||
off the main loop is not safe anyway.
|
||||
"""
|
||||
try:
|
||||
self._save_selected_settings(settings)
|
||||
|
||||
@@ -5111,6 +5119,8 @@ For now, the engine has been reverted to VOSK."""
|
||||
return True
|
||||
except Exception as e:
|
||||
logger.error(f"Failed to apply settings: {e}", exc_info=True)
|
||||
if raise_errors:
|
||||
raise
|
||||
|
||||
if "whisper" in str(e).lower() and "no module named" in str(e).lower():
|
||||
self._show_whisper_install_dialog()
|
||||
|
||||
@@ -0,0 +1,101 @@
|
||||
"""Tests for reporting failed model downloads instead of swallowing them."""
|
||||
|
||||
import importlib
|
||||
import sys
|
||||
from unittest.mock import MagicMock, Mock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
def _load_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.
|
||||
"""
|
||||
repository = sys.modules["gi.repository"]
|
||||
bases = {name: type(name, (), {}) for name in ("Box", "ListBoxRow", "Dialog")}
|
||||
saved = sys.modules.pop("vocalinux.ui.settings_dialog", None)
|
||||
try:
|
||||
with patch.object(repository, "Gtk", MagicMock(**bases)):
|
||||
module = importlib.import_module("vocalinux.ui.settings_dialog")
|
||||
finally:
|
||||
sys.modules.pop("vocalinux.ui.settings_dialog", None)
|
||||
if saved is not None:
|
||||
sys.modules["vocalinux.ui.settings_dialog"] = saved
|
||||
return module
|
||||
|
||||
|
||||
settings_dialog = _load_settings_dialog()
|
||||
SettingsDialog = settings_dialog.SettingsDialog
|
||||
|
||||
|
||||
def _dialog_stub():
|
||||
dialog = Mock()
|
||||
dialog.speech_engine.state = "idle"
|
||||
return dialog
|
||||
|
||||
|
||||
def test_download_threads_get_the_failure():
|
||||
"""With raise_errors the caller's handler sees the exception.
|
||||
|
||||
The download threads rely on this: their except blocks turn the failure
|
||||
into set_complete(False, reason) on the progress dialog. Without it they
|
||||
reported "Complete!" for downloads that were cancelled or never finished.
|
||||
"""
|
||||
dialog = _dialog_stub()
|
||||
dialog.speech_engine.reconfigure.side_effect = RuntimeError("Download cancelled")
|
||||
|
||||
with pytest.raises(RuntimeError, match="Download cancelled"):
|
||||
SettingsDialog._apply_settings_internal(dialog, {"engine": "vosk"}, raise_errors=True)
|
||||
|
||||
|
||||
def test_no_gtk_dialog_is_built_when_errors_are_raised():
|
||||
"""The failing thread path must not touch Gtk — that is the caller's job.
|
||||
|
||||
Building and running a Gtk.MessageDialog off the main loop is what the
|
||||
swallowed-error path used to do from the download thread.
|
||||
"""
|
||||
dialog = _dialog_stub()
|
||||
dialog.speech_engine.reconfigure.side_effect = RuntimeError("boom")
|
||||
|
||||
with patch.object(settings_dialog, "Gtk") as gtk:
|
||||
with pytest.raises(RuntimeError):
|
||||
SettingsDialog._apply_settings_internal(dialog, {"engine": "vosk"}, raise_errors=True)
|
||||
|
||||
gtk.MessageDialog.assert_not_called()
|
||||
|
||||
|
||||
def test_main_loop_callers_keep_the_dialog_and_the_false_return():
|
||||
"""Without the flag the old contract stays: swallow, show, return False."""
|
||||
dialog = _dialog_stub()
|
||||
dialog.speech_engine.reconfigure.side_effect = RuntimeError("boom")
|
||||
|
||||
with patch.object(settings_dialog, "Gtk") as gtk:
|
||||
result = SettingsDialog._apply_settings_internal(dialog, {"engine": "vosk"})
|
||||
|
||||
assert result is False
|
||||
gtk.MessageDialog.assert_called_once()
|
||||
|
||||
|
||||
def test_success_still_returns_true():
|
||||
dialog = _dialog_stub()
|
||||
|
||||
assert SettingsDialog._apply_settings_internal(dialog, {"engine": "vosk"}) is True
|
||||
assert (
|
||||
SettingsDialog._apply_settings_internal(dialog, {"engine": "vosk"}, raise_errors=True)
|
||||
is True
|
||||
)
|
||||
|
||||
|
||||
def test_both_download_threads_ask_for_the_failure():
|
||||
"""Source guard: every _apply_settings_internal call made off the main loop
|
||||
passes raise_errors=True, so a regression cannot silently reintroduce the
|
||||
swallowed-error path in a download thread."""
|
||||
import inspect
|
||||
|
||||
source = inspect.getsource(settings_dialog)
|
||||
thread_calls = source.count("_apply_settings_internal(settings, raise_errors=True)")
|
||||
assert thread_calls == 2
|
||||
Reference in New Issue
Block a user