From 3c2450906bd39708f2cfa2d6588ce317dab6610d Mon Sep 17 00:00:00 2001 From: Asher Norland Date: Tue, 10 Mar 2026 14:41:51 -0700 Subject: [PATCH] Data Consolidation --- synodic_client/application/screen/schema.py | 61 ------ synodic_client/application/screen/settings.py | 62 +++--- synodic_client/application/screen/tray.py | 17 +- .../application/screen/update_banner.py | 66 ++++++- .../application/update_controller.py | 180 ++++++++++-------- synodic_client/application/update_model.py | 120 ++++++++++++ tests/unit/qt/test_settings.py | 43 +++-- tests/unit/qt/test_update_banner.py | 3 +- tests/unit/qt/test_update_controller.py | 142 ++++++++------ 9 files changed, 440 insertions(+), 254 deletions(-) create mode 100644 synodic_client/application/update_model.py diff --git a/synodic_client/application/screen/schema.py b/synodic_client/application/screen/schema.py index ceb96b7..28e3002 100644 --- a/synodic_client/application/screen/schema.py +++ b/synodic_client/application/screen/schema.py @@ -11,9 +11,7 @@ import enum from collections.abc import Callable from dataclasses import dataclass, field -from enum import Enum, auto from pathlib import Path -from typing import Protocol, runtime_checkable from porringer.schema import ( PluginInfo, @@ -343,62 +341,3 @@ class _DispatchState: action_index: dict[int, int] = field(default_factory=dict) got_parsed: bool = False - - -# --------------------------------------------------------------------------- -# Update view protocol & banner data models -# --------------------------------------------------------------------------- - - -@runtime_checkable -class UpdateView(Protocol): - """Minimal display contract for the self-update lifecycle. - - :class:`UpdateBanner` satisfies this protocol implicitly via - structural typing. The controller broadcasts state transitions - through a ``list[UpdateView]`` so that every window showing update - status stays in sync. - """ - - def show_downloading(self, version: str) -> None: - """Indicate that *version* is being downloaded.""" - ... - - def show_downloading_progress(self, percentage: int) -> None: - """Update the download progress indicator.""" - ... - - def show_ready(self, version: str) -> None: - """Indicate that *version* is downloaded and ready to install.""" - ... - - def show_error(self, message: str) -> None: - """Display an error *message* in the update area.""" - ... - - def hide_banner(self) -> None: - """Hide the update banner.""" - ... - - -class UpdateBannerState(Enum): - """Visual states for the update banner.""" - - HIDDEN = auto() - DOWNLOADING = auto() - READY = auto() - ERROR = auto() - - -@dataclass(frozen=True, slots=True) -class _BannerConfig: - """Bundled visual configuration for a banner state transition.""" - - state: UpdateBannerState - style: str - icon: str - text: str - text_style: str - version: str = '' - action_label: str = '' - show_progress: bool = False diff --git a/synodic_client/application/screen/settings.py b/synodic_client/application/screen/settings.py index fbc5af1..80b41f6 100644 --- a/synodic_client/application/screen/settings.py +++ b/synodic_client/application/screen/settings.py @@ -32,7 +32,8 @@ from synodic_client.application.icon import app_icon from synodic_client.application.screen import _format_relative_time from synodic_client.application.screen.card import CardFrame -from synodic_client.application.theme import SETTINGS_WINDOW_MIN_SIZE, UPDATE_STATUS_CHECKING_STYLE +from synodic_client.application.theme import SETTINGS_WINDOW_MIN_SIZE +from synodic_client.application.update_model import UpdateModel from synodic_client.logging import log_path, set_debug_level from synodic_client.schema import GITHUB_REPO_URL from synodic_client.startup import is_startup_registered, register_startup, remove_startup @@ -237,6 +238,32 @@ def _build_advanced_section(self) -> CardFrame: card.content_layout.addLayout(row) return card + # ------------------------------------------------------------------ + # Model binding + # ------------------------------------------------------------------ + + def connect_model(self, model: UpdateModel) -> None: + """Connect to an :class:`UpdateModel` for state observation. + + The model's settings-facing signals drive the update status + label, check button, restart button, and timestamp label. + """ + model.status_text_changed.connect(self._on_status_changed) + model.check_button_enabled_changed.connect(self._check_updates_btn.setEnabled) + model.restart_visible_changed.connect(self._restart_btn.setVisible) + model.last_checked_changed.connect(self._on_last_checked_changed) + + def _on_status_changed(self, text: str, style: str) -> None: + """Apply a status text and style from the model.""" + self._update_status_label.setText(text) + self._update_status_label.setStyleSheet(style) + + def _on_last_checked_changed(self, timestamp: str) -> None: + """Apply a *last updated* timestamp from the model.""" + relative = _format_relative_time(timestamp) + self._last_client_update_label.setText(f'Last updated: {relative}') + self._last_client_update_label.setToolTip(f'Last updated: {timestamp}') + # ------------------------------------------------------------------ # Public API # ------------------------------------------------------------------ @@ -275,37 +302,6 @@ def sync_from_config(self) -> None: else: self._last_client_update_label.setText('') - def set_update_status(self, text: str, style: str = '') -> None: - """Set the inline status text next to the *Check for Updates* button. - - Args: - text: The status message. - style: Optional stylesheet for the label (e.g. color). - """ - self._update_status_label.setText(text) - self._update_status_label.setStyleSheet(style) - - def set_checking(self) -> None: - """Enter the *checking* state — disable button and show status.""" - self._check_updates_btn.setEnabled(False) - self._restart_btn.hide() - self._update_status_label.setText('Checking\u2026') - self._update_status_label.setStyleSheet(UPDATE_STATUS_CHECKING_STYLE) - - def reset_check_updates_button(self) -> None: - """Re-enable the *Check for Updates* button after a check completes.""" - self._check_updates_btn.setEnabled(True) - - def set_last_checked(self, timestamp: str) -> None: - """Update the *last updated* label from an ISO 8601 timestamp.""" - relative = _format_relative_time(timestamp) - self._last_client_update_label.setText(f'Last updated: {relative}') - self._last_client_update_label.setToolTip(f'Last updated: {timestamp}') - - def show_restart_button(self) -> None: - """Show the *Restart & Update* button.""" - self._restart_btn.show() - def show(self) -> None: """Sync controls from config, size to content, then show the window.""" self.sync_from_config() @@ -360,8 +356,6 @@ def _block_signals(self) -> Iterator[None]: def _on_check_updates_clicked(self) -> None: """Handle the *Check for Updates* button click.""" - self._check_updates_btn.setEnabled(False) - self._update_status_label.setText('Checking\u2026') self.check_updates_requested.emit() def _on_channel_changed(self, index: int) -> None: diff --git a/synodic_client/application/screen/tray.py b/synodic_client/application/screen/tray.py index 3f1bda2..f544c1e 100644 --- a/synodic_client/application/screen/tray.py +++ b/synodic_client/application/screen/tray.py @@ -16,6 +16,7 @@ from synodic_client.application.screen.settings import SettingsWindow from synodic_client.application.screen.tool_update_controller import ToolUpdateOrchestrator from synodic_client.application.update_controller import UpdateController +from synodic_client.application.update_model import UpdateModel from synodic_client.client import Client if TYPE_CHECKING: @@ -66,17 +67,29 @@ def __init__( # MainWindow gear button -> open settings window.settings_requested.connect(self._show_settings) + # Update model — centralised observable state for the update lifecycle + self._update_model = UpdateModel() + # Update controller - owns the self-update lifecycle & timer self._banner = window.update_banner self._update_controller = UpdateController( app, client, - [self._banner], - settings_window=self._settings_window, + self._update_model, store=self._store, ) self._update_controller.set_user_active_predicate(self._is_user_active) + # Connect views to the model + self._banner.connect_model(self._update_model) + self._settings_window.connect_model(self._update_model) + + # Wire user-action signals back to the controller + self._banner.restart_requested.connect(self._update_controller.request_apply) + self._banner.retry_requested.connect(self._update_controller.request_retry) + self._settings_window.check_updates_requested.connect(self._update_controller.request_check) + self._settings_window.restart_requested.connect(self._update_controller.request_apply) + # Tool update orchestrator - owns tool/package update lifecycle self._tool_orchestrator = ToolUpdateOrchestrator( window, diff --git a/synodic_client/application/screen/update_banner.py b/synodic_client/application/screen/update_banner.py index 5a6e62e..abc67fa 100644 --- a/synodic_client/application/screen/update_banner.py +++ b/synodic_client/application/screen/update_banner.py @@ -15,6 +15,8 @@ from __future__ import annotations import logging +from dataclasses import dataclass +from enum import Enum, auto from PySide6.QtCore import ( QEasingCurve, @@ -34,7 +36,6 @@ QWidget, ) -from synodic_client.application.screen.schema import UpdateBannerState, _BannerConfig from synodic_client.application.theme import ( UPDATE_BANNER_ANIMATION_MS, UPDATE_BANNER_BTN_STYLE, @@ -47,10 +48,34 @@ UPDATE_BANNER_STYLE, UPDATE_BANNER_VERSION_STYLE, ) +from synodic_client.application.update_model import UpdateModel, UpdatePhase logger = logging.getLogger(__name__) +class UpdateBannerState(Enum): + """Visual states for the update banner.""" + + HIDDEN = auto() + DOWNLOADING = auto() + READY = auto() + ERROR = auto() + + +@dataclass(frozen=True, slots=True) +class _BannerConfig: + """Bundled visual configuration for a banner state transition.""" + + state: UpdateBannerState + style: str + icon: str + text: str + text_style: str + version: str = '' + action_label: str = '' + show_progress: bool = False + + # Height of the banner content (progress variant is slightly taller). _BANNER_HEIGHT = 38 _BANNER_HEIGHT_WITH_PROGRESS = 44 @@ -77,6 +102,7 @@ def __init__(self, parent: QWidget | None = None) -> None: self._state = UpdateBannerState.HIDDEN self._target_version: str = '' + self._error_dismiss_timer: QTimer | None = None # --- Layout --- self._outer = QVBoxLayout(self) @@ -129,6 +155,28 @@ def __init__(self, parent: QWidget | None = None) -> None: self._anim.setEasingCurve(QEasingCurve.Type.OutCubic) self._anim.setDuration(UPDATE_BANNER_ANIMATION_MS) + # --- Model binding --- + + def connect_model(self, model: UpdateModel) -> None: + """Connect to an :class:`UpdateModel` for state observation. + + The model's lifecycle signals drive the banner's visual state + transitions so the controller never needs to call banner + methods directly. + """ + self._model = model + model.phase_changed.connect(self._on_model_phase) + model.progress_changed.connect(self.show_downloading_progress) + + def _on_model_phase(self, phase: UpdatePhase) -> None: + """React to a lifecycle phase change from the model.""" + if phase == UpdatePhase.DOWNLOADING: + self.show_downloading(self._model.version) + elif phase == UpdatePhase.READY: + self.show_ready(self._model.version) + elif phase == UpdatePhase.ERROR: + self.show_error(self._model.error_message) + # --- Public API --- @property @@ -191,6 +239,12 @@ def show_error(self, message: str) -> None: Args: message: Human-readable error description. """ + # Cancel any pending auto-dismiss from a previous error to avoid + # stacking timers that would forcibly hide a freshly shown banner. + if self._error_dismiss_timer is not None: + self._error_dismiss_timer.stop() + self._error_dismiss_timer = None + self._configure( _BannerConfig( state=UpdateBannerState.ERROR, @@ -201,12 +255,20 @@ def show_error(self, message: str) -> None: action_label='Retry', ) ) - QTimer.singleShot(UPDATE_BANNER_ERROR_DISMISS_MS, self._auto_dismiss_error) + timer = QTimer(self) + timer.setSingleShot(True) + timer.setInterval(UPDATE_BANNER_ERROR_DISMISS_MS) + timer.timeout.connect(self._auto_dismiss_error) + timer.start() + self._error_dismiss_timer = timer def hide_banner(self) -> None: """Slide the banner out and reset to hidden.""" if self._state == UpdateBannerState.HIDDEN: return + if self._error_dismiss_timer is not None: + self._error_dismiss_timer.stop() + self._error_dismiss_timer = None self._state = UpdateBannerState.HIDDEN self._animate_height(0) diff --git a/synodic_client/application/update_controller.py b/synodic_client/application/update_controller.py index e92b63b..9b6aeb9 100644 --- a/synodic_client/application/update_controller.py +++ b/synodic_client/application/update_controller.py @@ -4,6 +4,10 @@ periodic auto-update timer. Extracted from :class:`TrayScreen` so that tray, settings, and banner concerns are cleanly separated from the update state-machine. + +The controller is the sole writer to an :class:`UpdateModel`; views +observe the model via Qt signals and never receive imperative calls +from the controller. """ from __future__ import annotations @@ -17,13 +21,13 @@ from PySide6.QtCore import QTimer from PySide6.QtWidgets import QApplication -from synodic_client.application.screen.schema import UpdateView -from synodic_client.application.screen.update_banner import UpdateBanner from synodic_client.application.theme import ( UPDATE_STATUS_AVAILABLE_STYLE, + UPDATE_STATUS_CHECKING_STYLE, UPDATE_STATUS_ERROR_STYLE, UPDATE_STATUS_UP_TO_DATE_STYLE, ) +from synodic_client.application.update_model import UpdateModel from synodic_client.application.workers import check_for_update, download_update from synodic_client.resolution import ( ResolvedConfig, @@ -33,7 +37,6 @@ if TYPE_CHECKING: from synodic_client.application.config_store import ConfigStore - from synodic_client.application.screen.settings import SettingsWindow from synodic_client.client import Client logger = logging.getLogger(__name__) @@ -42,17 +45,18 @@ class UpdateController: """Manages the self-update lifecycle: check → download → apply. + The controller is the sole writer to the :class:`UpdateModel`. + Views connect to the model's signals; the controller never calls + view methods directly. + Parameters ---------- app: The running ``QApplication`` (needed for ``quit()`` on auto-apply). client: The Synodic Client service facade. - views: - One or more :class:`UpdateView` implementations to broadcast - state transitions to (typically ``UpdateBanner`` instances). - settings_window: - The ``SettingsWindow`` (check button + last-updated label). + model: + The shared :class:`UpdateModel` that views observe. store: The centralised :class:`ConfigStore`. """ @@ -61,9 +65,8 @@ def __init__( self, app: QApplication, client: Client, - views: list[UpdateView], + model: UpdateModel, *, - settings_window: SettingsWindow, store: ConfigStore, ) -> None: """Initialise the controller and start the periodic timer. @@ -71,36 +74,29 @@ def __init__( Args: app: The running ``QApplication``. client: The Synodic Client service facade. - views: One or more :class:`UpdateView` implementations. - settings_window: The settings window (check button + timestamp). + model: The shared :class:`UpdateModel`. store: The centralised :class:`ConfigStore`. """ self._app = app self._client = client - self._views = views - self._settings_window = settings_window + self._model = model self._store = store self._is_user_active: Callable[[], bool] = lambda: False self._update_task: asyncio.Task[None] | None = None self._pending_version: str | None = None + self._failed_version: str | None = None # Derive auto-apply preference from config self._auto_apply: bool = store.config.auto_apply + # Track update-relevant config fields to avoid reinitialising + # on every config save (e.g. timestamp-only changes). + self._update_config_key = self._extract_update_key(store.config) + # Periodic auto-update timer self._auto_update_timer: QTimer | None = None self._restart_auto_update_timer() - # Wire banner signals (UpdateBanner-specific, outside the protocol) - for view in self._views: - if isinstance(view, UpdateBanner): - view.restart_requested.connect(self._apply_update) - view.retry_requested.connect(lambda: self.check_now(silent=True)) - - # Wire settings check-updates and restart buttons - self._settings_window.check_updates_requested.connect(self._on_manual_check) - self._settings_window.restart_requested.connect(self._apply_update) - # React to config changes from any source self._store.changed.connect(self._on_config_changed) @@ -138,20 +134,20 @@ def _persist_check_timestamp(self) -> None: """Persist the current time as *last_client_update* and refresh the label.""" ts = datetime.now(UTC).isoformat() self._store.update(last_client_update=ts) - self._settings_window.set_last_checked(ts) + self._model.set_last_checked(ts) def _report_error(self, message: str, *, silent: bool) -> None: """Show an error to the user or log it, depending on *silent*. Always updates the settings status line. When not *silent*, - also broadcasts the error to all update-banner views. + also transitions the model to the ERROR phase so the banner + displays the error. """ - self._settings_window.set_update_status('Check failed', UPDATE_STATUS_ERROR_STYLE) + self._model.set_status('Check failed', UPDATE_STATUS_ERROR_STYLE) if silent: logger.warning('%s', message) else: - for view in self._views: - view.show_error(message) + self._model.set_error(message) # ------------------------------------------------------------------ # Timer management @@ -190,15 +186,36 @@ def check_now(self, *, silent: bool = False) -> None: """ self._do_check(silent=silent) + def request_check(self) -> None: + """Handle a user-initiated update check (from settings button).""" + self._do_check(silent=False) + + def request_retry(self) -> None: + """Handle a banner retry — clear the failed version lock and re-check.""" + self._failed_version = None + self.check_now(silent=True) + + def request_apply(self) -> None: + """Handle a user-initiated apply/restart.""" + self._apply_update(silent=False) + def _on_config_changed(self, config: object) -> None: """React to a config change — reinitialise the updater and timers. - Also triggers an immediate (silent) check so the user gets - feedback after switching channels. + Only reinitialises when update-relevant fields change (channel, + source, interval, auto-apply). Timestamp-only changes (e.g. + ``last_client_update``) are ignored to prevent an infinite + check → persist → reinitialise → check loop. """ if not isinstance(config, ResolvedConfig): return self._auto_apply = config.auto_apply + + new_key = self._extract_update_key(config) + if new_key == self._update_config_key: + return + self._update_config_key = new_key + self._reinitialize_updater(config) self.check_now(silent=True) @@ -206,6 +223,16 @@ def _on_config_changed(self, config: object) -> None: # Updater re-initialisation # ------------------------------------------------------------------ + @staticmethod + def _extract_update_key(config: ResolvedConfig) -> tuple[object, ...]: + """Return a hashable tuple of the fields that affect the updater.""" + return ( + config.update_source, + config.update_channel, + config.auto_update_interval_minutes, + config.auto_apply, + ) + def _reinitialize_updater(self, config: ResolvedConfig) -> None: """Re-derive update settings and restart the updater and timer.""" update_cfg = resolve_update_config(config) @@ -221,10 +248,6 @@ def _reinitialize_updater(self, config: ResolvedConfig) -> None: # Check flow # ------------------------------------------------------------------ - def _on_manual_check(self) -> None: - """Handle manual check-for-updates (from settings button).""" - self._do_check(silent=False) - def _on_auto_check(self) -> None: """Handle automatic (periodic) check — silent. @@ -237,17 +260,17 @@ def _on_auto_check(self) -> None: def _do_check(self, *, silent: bool) -> None: """Run an update check.""" if self._client.updater is None: + self._model.set_check_button_enabled(True) if not silent: - for view in self._views: - view.show_error('Updater is not initialized.') + self._model.set_error('Updater is not initialized.') return - # Preserve the banner state when an update is already pending. - # Skip the visual "Checking…" transition when the settings - # window isn't visible to avoid widget operations on a hidden - # QMainWindow (which can cause transient flashes on Windows). - if self._pending_version is None and self._settings_window.isVisible(): - self._settings_window.set_checking() + # Always disable the button; only show "Checking…" when no + # download is already pending (to preserve the ready state). + self._model.set_check_button_enabled(False) + if self._pending_version is None: + self._model.set_restart_visible(False) + self._model.set_status('Checking\u2026', UPDATE_STATUS_CHECKING_STYLE) self._update_task = asyncio.create_task(self._async_check(silent=silent)) @@ -265,7 +288,7 @@ async def _async_check(self, *, silent: bool) -> None: def _on_check_finished(self, result: UpdateInfo | None, *, silent: bool = False) -> None: """Route the update-check result.""" - self._settings_window.reset_check_updates_button() + self._model.set_check_button_enabled(True) if result is None: self._report_error('Failed to check for updates.', silent=silent) @@ -279,7 +302,7 @@ def _on_check_finished(self, result: UpdateInfo | None, *, silent: bool = False) self._persist_check_timestamp() if not result.available: - self._settings_window.set_update_status('Up to date', UPDATE_STATUS_UP_TO_DATE_STYLE) + self._model.set_status('Up to date', UPDATE_STATUS_UP_TO_DATE_STYLE) if not silent: logger.info('No updates available (current: %s)', result.current_version) else: @@ -293,57 +316,64 @@ def _on_check_finished(self, result: UpdateInfo | None, *, silent: bool = False) self._show_ready(version) return + # Skip re-downloading a version that already failed (auto-check only). + # A manual retry via the banner clears ``_failed_version``. + if silent and version == self._failed_version: + logger.debug('Skipping download of previously failed version %s', version) + return + # New update available — download it - self._settings_window.set_update_status(f'v{version} available', UPDATE_STATUS_AVAILABLE_STYLE) - for view in self._views: - view.show_downloading(version) - self._start_download(version) + self._model.set_status(f'v{version} available', UPDATE_STATUS_AVAILABLE_STYLE) + self._model.begin_download(version) + self._start_download(version, silent=silent) def _on_check_error(self, error: str, *, silent: bool = False) -> None: """Handle unexpected exception during update check.""" - self._settings_window.reset_check_updates_button() + self._model.set_check_button_enabled(True) self._report_error(f'Update check error: {error}', silent=silent) # ------------------------------------------------------------------ # Download flow # ------------------------------------------------------------------ - def _start_download(self, version: str) -> None: + def _start_download(self, version: str, *, silent: bool = False) -> None: """Start downloading the update in the background.""" - self._update_task = asyncio.create_task(self._async_download(version)) + self._update_task = asyncio.create_task(self._async_download(version, silent=silent)) - async def _async_download(self, version: str) -> None: + async def _async_download(self, version: str, *, silent: bool = False) -> None: """Run the download coroutine and route results.""" try: success = await download_update( self._client, on_progress=self._on_download_progress, ) - self._on_download_finished(success, version) + self._on_download_finished(success, version, silent=silent) except asyncio.CancelledError: logger.debug('Update download cancelled (shutdown)') raise except Exception as exc: logger.exception('Update download failed') - self._on_download_error(str(exc)) + self._on_download_error(str(exc), silent=silent) def _on_download_progress(self, percentage: int) -> None: - """Broadcast download progress to all views.""" - for view in self._views: - view.show_downloading_progress(percentage) + """Broadcast download progress to the model.""" + self._model.set_progress(percentage) - def _on_download_finished(self, success: bool, version: str) -> None: + def _on_download_finished(self, success: bool, version: str, *, silent: bool = False) -> None: """Handle download completion.""" if not success: - self._settings_window.set_update_status('Download failed', UPDATE_STATUS_ERROR_STYLE) - for view in self._views: - view.show_error('Download failed. Please try again later.') + self._failed_version = version + self._model.set_status('Download failed', UPDATE_STATUS_ERROR_STYLE) + if not silent: + self._model.set_error('Download failed. Please try again later.') + else: + logger.warning('Download failed for %s (silent)', version) return # Persist the client-update timestamp (actual update downloaded) ts = datetime.now(UTC).isoformat() self._store.update(last_client_update=ts) - self._settings_window.set_last_checked(ts) + self._model.set_last_checked(ts) self._pending_version = version @@ -356,16 +386,17 @@ def _on_download_finished(self, success: bool, version: str) -> None: self._show_ready(version) def _show_ready(self, version: str) -> None: - """Present the *ready to restart* state across all views.""" - self._settings_window.set_update_status(f'v{version} ready', UPDATE_STATUS_UP_TO_DATE_STYLE) - self._settings_window.show_restart_button() - for view in self._views: - view.show_ready(version) - - def _on_download_error(self, error: str) -> None: - """Handle download error — show error across all views.""" - for view in self._views: - view.show_error(f'Download error: {error}') + """Present the *ready to restart* state via the model.""" + self._model.set_status(f'v{version} ready', UPDATE_STATUS_UP_TO_DATE_STYLE) + self._model.set_restart_visible(True) + self._model.set_ready(version) + + def _on_download_error(self, error: str, *, silent: bool = False) -> None: + """Handle download error.""" + if not silent: + self._model.set_error(f'Download error: {error}') + else: + logger.warning('Download error (silent): %s', error) # ------------------------------------------------------------------ # Apply @@ -388,5 +419,4 @@ def _apply_update(self, *, silent: bool = False) -> None: self._app.quit() except Exception as e: logger.error('Failed to apply update: %s', e) - for view in self._views: - view.show_error(f'Failed to apply update: {e}') + self._model.set_error(f'Failed to apply update: {e}') diff --git a/synodic_client/application/update_model.py b/synodic_client/application/update_model.py new file mode 100644 index 0000000..368f461 --- /dev/null +++ b/synodic_client/application/update_model.py @@ -0,0 +1,120 @@ +"""Centralised state model for the self-update lifecycle. + +All update-related display state lives here. The +:class:`UpdateController` is the sole writer; views +(:class:`UpdateBanner`, :class:`SettingsWindow`) observe changes via +Qt signals and never mutate the model directly. +""" + +from __future__ import annotations + +from enum import Enum, auto + +from PySide6.QtCore import QObject, Signal + + +class UpdatePhase(Enum): + """High-level phases of the self-update lifecycle.""" + + IDLE = auto() + CHECKING = auto() + DOWNLOADING = auto() + READY = auto() + ERROR = auto() + + +class UpdateModel(QObject): + """Observable state for the self-update lifecycle. + + Signals + ------- + phase_changed(UpdatePhase) + Emitted when the lifecycle phase transitions. + progress_changed(int) + Emitted when download progress (0--100) updates. + status_text_changed(str, str) + Emitted when the settings status label text or style changes. + check_button_enabled_changed(bool) + Emitted when the *Check for Updates* button state changes. + restart_visible_changed(bool) + Emitted when the *Restart & Update* button visibility changes. + last_checked_changed(str) + Emitted when the *last updated* timestamp changes. + """ + + phase_changed = Signal(object) + progress_changed = Signal(int) + status_text_changed = Signal(str, str) + check_button_enabled_changed = Signal(bool) + restart_visible_changed = Signal(bool) + last_checked_changed = Signal(str) + + def __init__(self, parent: QObject | None = None) -> None: + super().__init__(parent) + self._phase = UpdatePhase.IDLE + self._version: str = '' + self._error_message: str = '' + + # --- Read-only properties --- + + @property + def phase(self) -> UpdatePhase: + """Current lifecycle phase.""" + return self._phase + + @property + def version(self) -> str: + """Version string associated with the current phase.""" + return self._version + + @property + def error_message(self) -> str: + """Error description when :attr:`phase` is ``ERROR``.""" + return self._error_message + + # --- Lifecycle transitions (controller writes) --- + + def begin_download(self, version: str) -> None: + """Enter the *DOWNLOADING* phase for *version*.""" + self._version = version + self._phase = UpdatePhase.DOWNLOADING + self.phase_changed.emit(self._phase) + + def set_ready(self, version: str) -> None: + """Enter the *READY* phase for *version*.""" + self._version = version + self._phase = UpdatePhase.READY + self.phase_changed.emit(self._phase) + + def set_error(self, message: str) -> None: + """Enter the *ERROR* phase with *message*.""" + self._error_message = message + self._phase = UpdatePhase.ERROR + self.phase_changed.emit(self._phase) + + def set_idle(self) -> None: + """Return to the *IDLE* phase.""" + self._phase = UpdatePhase.IDLE + self.phase_changed.emit(self._phase) + + def set_progress(self, percentage: int) -> None: + """Update download progress (0--100).""" + self.progress_changed.emit(percentage) + + # --- Settings-panel properties (controller writes) --- + + def set_status(self, text: str, style: str) -> None: + """Update the inline status text and style.""" + self.status_text_changed.emit(text, style) + + def set_check_button_enabled(self, enabled: bool) -> None: + """Enable or disable the *Check for Updates* button.""" + self.check_button_enabled_changed.emit(enabled) + + def set_restart_visible(self, visible: bool) -> None: + """Show or hide the *Restart & Update* button.""" + self.restart_visible_changed.emit(visible) + + def set_last_checked(self, timestamp: str) -> None: + """Update the *last updated* ISO 8601 timestamp.""" + self.last_checked_changed.emit(timestamp) diff --git a/tests/unit/qt/test_settings.py b/tests/unit/qt/test_settings.py index 4e5ff1a..24e6f5b 100644 --- a/tests/unit/qt/test_settings.py +++ b/tests/unit/qt/test_settings.py @@ -8,6 +8,7 @@ from synodic_client.application.config_store import ConfigStore from synodic_client.application.screen.settings import SettingsWindow from synodic_client.application.theme import SETTINGS_WINDOW_MIN_SIZE +from synodic_client.application.update_model import UpdateModel from synodic_client.resolution import ResolvedConfig from synodic_client.schema import DEFAULT_AUTO_UPDATE_INTERVAL_MINUTES, DEFAULT_TOOL_UPDATE_INTERVAL_MINUTES @@ -331,8 +332,8 @@ def test_button_and_label_exist() -> None: assert not window._update_status_label.text() @staticmethod - def test_click_emits_signal_and_disables() -> None: - """Clicking the button emits check_updates_requested and disables it.""" + def test_click_emits_signal() -> None: + """Clicking the button emits check_updates_requested.""" window = _make_window() signal_spy = MagicMock() window.check_updates_requested.connect(signal_spy) @@ -340,34 +341,44 @@ def test_click_emits_signal_and_disables() -> None: window._check_updates_btn.click() signal_spy.assert_called_once() - assert window._check_updates_btn.isEnabled() is False - assert window._update_status_label.text() == 'Checking\u2026' @staticmethod - def test_set_update_status() -> None: - """set_update_status sets the label text and style.""" + def test_model_status_updates_label() -> None: + """Model status_text_changed updates the label text and style.""" window = _make_window() - window.set_update_status('Up to date', 'color: green;') + model = UpdateModel() + window.connect_model(model) + + model.set_status('Up to date', 'color: green;') + assert window._update_status_label.text() == 'Up to date' assert 'green' in window._update_status_label.styleSheet() @staticmethod - def test_reset_check_updates_button() -> None: - """reset_check_updates_button re-enables the button.""" + def test_model_check_button_enabled() -> None: + """Model check_button_enabled_changed toggles the button.""" window = _make_window() - window._check_updates_btn.setEnabled(False) + model = UpdateModel() + window.connect_model(model) - window.reset_check_updates_button() + model.set_check_button_enabled(False) + assert window._check_updates_btn.isEnabled() is False + model.set_check_button_enabled(True) assert window._check_updates_btn.isEnabled() is True @staticmethod - def test_set_checking() -> None: - """set_checking disables the button and shows 'Checking\u2026' status.""" + def test_model_restart_visible() -> None: + """Model restart_visible_changed toggles the restart button.""" window = _make_window() - window.set_checking() - assert window._check_updates_btn.isEnabled() is False - assert window._update_status_label.text() == 'Checking\u2026' + model = UpdateModel() + window.connect_model(model) + + model.set_restart_visible(True) + assert window._restart_btn.isHidden() is False + + model.set_restart_visible(False) + assert window._restart_btn.isHidden() is True # --------------------------------------------------------------------------- diff --git a/tests/unit/qt/test_update_banner.py b/tests/unit/qt/test_update_banner.py index 0bac321..952dc61 100644 --- a/tests/unit/qt/test_update_banner.py +++ b/tests/unit/qt/test_update_banner.py @@ -2,8 +2,7 @@ from __future__ import annotations -from synodic_client.application.screen.schema import UpdateBannerState -from synodic_client.application.screen.update_banner import UpdateBanner +from synodic_client.application.screen.update_banner import UpdateBanner, UpdateBannerState _PROGRESS_MAX = 100 _TEST_PROGRESS = 42 diff --git a/tests/unit/qt/test_update_controller.py b/tests/unit/qt/test_update_controller.py index f4bf50a..09218d6 100644 --- a/tests/unit/qt/test_update_controller.py +++ b/tests/unit/qt/test_update_controller.py @@ -15,6 +15,7 @@ UPDATE_STATUS_UP_TO_DATE_STYLE, ) from synodic_client.application.update_controller import UpdateController +from synodic_client.application.update_model import UpdateModel from synodic_client.resolution import ResolvedConfig from synodic_client.schema import ( DEFAULT_AUTO_UPDATE_INTERVAL_MINUTES, @@ -22,6 +23,26 @@ UpdateInfo, ) + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +class ModelSpy: + """Records signal emissions from an :class:`UpdateModel`.""" + + def __init__(self, model: UpdateModel) -> None: + self.status: list[tuple[str, str]] = [] + self.check_button_enabled: list[bool] = [] + self.restart_visible: list[bool] = [] + self.last_checked: list[str] = [] + + model.status_text_changed.connect(lambda t, s: self.status.append((t, s))) + model.check_button_enabled_changed.connect(self.check_button_enabled.append) + model.restart_visible_changed.connect(self.restart_visible.append) + model.last_checked_changed.connect(self.last_checked.append) + # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- @@ -51,10 +72,10 @@ def _make_controller( auto_apply: bool = True, auto_update_interval_minutes: int = 0, is_user_active: bool = False, -) -> tuple[UpdateController, MagicMock, MagicMock, UpdateBanner, MagicMock]: +) -> tuple[UpdateController, MagicMock, MagicMock, UpdateBanner, UpdateModel]: """Build an ``UpdateController`` with mocked collaborators. - Returns (controller, app_mock, client_mock, banner, settings_mock). + Returns (controller, app_mock, client_mock, banner, model). """ config = _make_config( auto_apply=auto_apply, @@ -64,8 +85,9 @@ def _make_controller( app = MagicMock() client = MagicMock() client.updater = MagicMock() + model = UpdateModel() banner = UpdateBanner() - settings = MagicMock() + banner.connect_model(model) store = ConfigStore(config) with patch('synodic_client.application.update_controller.resolve_update_config') as mock_ucfg: @@ -75,13 +97,12 @@ def _make_controller( controller = UpdateController( app, client, - [banner], - settings_window=settings, + model, store=store, ) controller.set_user_active_predicate(lambda: is_user_active) - return controller, app, client, banner, settings + return controller, app, client, banner, model # --------------------------------------------------------------------------- @@ -95,16 +116,17 @@ class TestCheckFinished: @staticmethod def test_none_result_sets_error_status() -> None: """A None result should set 'Check failed' in red.""" - ctrl, _app, _client, banner, settings = _make_controller() + ctrl, _app, _client, banner, model = _make_controller() + spy = ModelSpy(model) ctrl._on_check_finished(None, silent=False) - settings.reset_check_updates_button.assert_called_once() - settings.set_update_status.assert_called_once_with('Check failed', UPDATE_STATUS_ERROR_STYLE) + assert True in spy.check_button_enabled + assert ('Check failed', UPDATE_STATUS_ERROR_STYLE) in spy.status @staticmethod def test_none_result_shows_banner_when_not_silent() -> None: """A None result with silent=False should show the error banner.""" - ctrl, _app, _client, banner, settings = _make_controller() + ctrl, _app, _client, banner, model = _make_controller() ctrl._on_check_finished(None, silent=False) assert banner.state.name == 'ERROR' @@ -112,7 +134,7 @@ def test_none_result_shows_banner_when_not_silent() -> None: @staticmethod def test_none_result_no_banner_when_silent() -> None: """A None result with silent=True should NOT show the error banner.""" - ctrl, _app, _client, banner, settings = _make_controller() + ctrl, _app, _client, banner, model = _make_controller() ctrl._on_check_finished(None, silent=True) assert banner.state.name == 'HIDDEN' @@ -120,35 +142,35 @@ def test_none_result_no_banner_when_silent() -> None: @staticmethod def test_error_result_sets_error_status() -> None: """An error result should set 'Check failed' status.""" - ctrl, _app, _client, banner, settings = _make_controller() + ctrl, _app, _client, banner, model = _make_controller() + spy = ModelSpy(model) result = UpdateInfo(available=False, current_version=Version('1.0.0'), error='No releases found') ctrl._on_check_finished(result, silent=False) - settings.set_update_status.assert_called_once_with('Check failed', UPDATE_STATUS_ERROR_STYLE) + assert ('Check failed', UPDATE_STATUS_ERROR_STYLE) in spy.status @staticmethod def test_no_update_sets_up_to_date() -> None: """No update available should set 'Up to date' in green.""" - ctrl, _app, _client, banner, settings = _make_controller() + ctrl, _app, _client, banner, model = _make_controller() + spy = ModelSpy(model) result = UpdateInfo(available=False, current_version=Version('1.0.0')) ctrl._on_check_finished(result, silent=False) - settings.set_update_status.assert_called_once_with('Up to date', UPDATE_STATUS_UP_TO_DATE_STYLE) + assert ('Up to date', UPDATE_STATUS_UP_TO_DATE_STYLE) in spy.status @staticmethod def test_update_available_sets_status_and_starts_download() -> None: """Available update should set orange status and start download.""" - ctrl, _app, _client, banner, settings = _make_controller() + ctrl, _app, _client, banner, model = _make_controller() + spy = ModelSpy(model) result = UpdateInfo(available=True, current_version=Version('1.0.0'), latest_version=Version('2.0.0')) with patch.object(ctrl, '_start_download') as mock_dl: ctrl._on_check_finished(result, silent=False) - settings.set_update_status.assert_called_once_with( - 'v2.0.0 available', - UPDATE_STATUS_AVAILABLE_STYLE, - ) - mock_dl.assert_called_once_with('2.0.0') + assert ('v2.0.0 available', UPDATE_STATUS_AVAILABLE_STYLE) in spy.status + mock_dl.assert_called_once_with('2.0.0', silent=False) # --------------------------------------------------------------------------- @@ -162,7 +184,7 @@ class TestDownloadFinished: @staticmethod def test_auto_apply_calls_apply_update() -> None: """When auto_apply=True, a successful download should call _apply_update(silent=True).""" - ctrl, app, client, banner, settings = _make_controller(auto_apply=True) + ctrl, app, client, banner, model = _make_controller(auto_apply=True) with patch.object(ctrl, '_apply_update') as mock_apply: ctrl._on_download_finished(True, '2.0.0') @@ -172,7 +194,7 @@ def test_auto_apply_calls_apply_update() -> None: @staticmethod def test_auto_apply_does_not_show_ready_banner() -> None: """When auto_apply=True, the ready banner should NOT be shown.""" - ctrl, app, client, banner, settings = _make_controller(auto_apply=True) + ctrl, app, client, banner, model = _make_controller(auto_apply=True) with patch.object(ctrl, '_apply_update'): ctrl._on_download_finished(True, '2.0.0') @@ -183,7 +205,7 @@ def test_auto_apply_does_not_show_ready_banner() -> None: @staticmethod def test_no_auto_apply_shows_ready_banner() -> None: """When auto_apply=False, a successful download should show the ready banner.""" - ctrl, app, client, banner, settings = _make_controller(auto_apply=False) + ctrl, app, client, banner, model = _make_controller(auto_apply=False) with patch.object(ctrl._store, 'update'): ctrl._on_download_finished(True, '2.0.0') @@ -192,45 +214,46 @@ def test_no_auto_apply_shows_ready_banner() -> None: @staticmethod def test_no_auto_apply_sets_ready_status() -> None: """When auto_apply=False, status should show 'v2.0.0 ready' in green.""" - ctrl, app, client, banner, settings = _make_controller(auto_apply=False) + ctrl, app, client, banner, model = _make_controller(auto_apply=False) + spy = ModelSpy(model) with patch.object(ctrl._store, 'update'): ctrl._on_download_finished(True, '2.0.0') - settings.set_update_status.assert_called_with( - 'v2.0.0 ready', - UPDATE_STATUS_UP_TO_DATE_STYLE, - ) + assert ('v2.0.0 ready', UPDATE_STATUS_UP_TO_DATE_STYLE) in spy.status @staticmethod def test_no_auto_apply_shows_restart_button() -> None: """When auto_apply=False, the restart button should be shown in settings.""" - ctrl, app, client, banner, settings = _make_controller(auto_apply=False) + ctrl, app, client, banner, model = _make_controller(auto_apply=False) + spy = ModelSpy(model) with patch.object(ctrl._store, 'update'): ctrl._on_download_finished(True, '2.0.0') - settings.show_restart_button.assert_called_once() + assert True in spy.restart_visible @staticmethod def test_user_active_shows_restart_button() -> None: """When user is active, the restart button should be shown in settings.""" - ctrl, app, client, banner, settings = _make_controller( + ctrl, app, client, banner, model = _make_controller( auto_apply=True, is_user_active=True, ) + spy = ModelSpy(model) with patch.object(ctrl, '_apply_update'): ctrl._on_download_finished(True, '2.0.0') - settings.show_restart_button.assert_called_once() + assert True in spy.restart_visible @staticmethod def test_download_failure_shows_error() -> None: """A failed download should show an error banner.""" - ctrl, app, client, banner, settings = _make_controller() + ctrl, app, client, banner, model = _make_controller() + spy = ModelSpy(model) ctrl._on_download_finished(False, '2.0.0') assert banner.state.name == 'ERROR' - settings.set_update_status.assert_called_with('Download failed', UPDATE_STATUS_ERROR_STYLE) + assert ('Download failed', UPDATE_STATUS_ERROR_STYLE) in spy.status # --------------------------------------------------------------------------- @@ -248,7 +271,7 @@ class TestUserActiveGating: @staticmethod def test_auto_check_always_runs() -> None: """_on_auto_check should call _do_check even when user is active.""" - ctrl, _app, _client, banner, settings = _make_controller(is_user_active=True) + ctrl, _app, _client, banner, model = _make_controller(is_user_active=True) with patch.object(ctrl, '_do_check') as mock_check: ctrl._on_auto_check() @@ -257,18 +280,18 @@ def test_auto_check_always_runs() -> None: @staticmethod def test_manual_check_unaffected_by_active_user() -> None: - """_on_manual_check should always call _do_check regardless of user activity.""" - ctrl, _app, _client, banner, settings = _make_controller(is_user_active=True) + """request_check should always call _do_check regardless of user activity.""" + ctrl, _app, _client, banner, model = _make_controller(is_user_active=True) with patch.object(ctrl, '_do_check') as mock_check: - ctrl._on_manual_check() + ctrl.request_check() mock_check.assert_called_once_with(silent=False) @staticmethod def test_auto_apply_deferred_when_user_active() -> None: """When auto_apply=True but user is active, show READY banner instead of applying.""" - ctrl, app, client, banner, settings = _make_controller( + ctrl, app, client, banner, model = _make_controller( auto_apply=True, is_user_active=True, ) @@ -282,7 +305,7 @@ def test_auto_apply_deferred_when_user_active() -> None: @staticmethod def test_auto_apply_proceeds_when_user_inactive() -> None: """When auto_apply=True and user is inactive, _apply_update is called.""" - ctrl, app, client, banner, settings = _make_controller( + ctrl, app, client, banner, model = _make_controller( auto_apply=True, is_user_active=False, ) @@ -322,7 +345,7 @@ class TestApplyUpdate: @staticmethod def test_apply_update_calls_client_and_quits() -> None: """_apply_update should call client.apply_update_on_exit and app.quit.""" - ctrl, app, client, banner, settings = _make_controller() + ctrl, app, client, banner, model = _make_controller() ctrl._apply_update() client.apply_update_on_exit.assert_called_once_with(restart=True, silent=False) @@ -331,21 +354,13 @@ def test_apply_update_calls_client_and_quits() -> None: @staticmethod def test_apply_update_noop_without_updater() -> None: """_apply_update should be a no-op when client.updater is None.""" - ctrl, app, client, banner, settings = _make_controller() + ctrl, app, client, banner, model = _make_controller() client.updater = None ctrl._apply_update() client.apply_update_on_exit.assert_not_called() app.quit.assert_not_called() - @staticmethod - def test_restart_requested_signal_triggers_apply() -> None: - """The settings restart_requested signal should be connected to _apply_update.""" - ctrl, app, client, banner, settings = _make_controller() - - # Verify the signal was connected - settings.restart_requested.connect.assert_called_once_with(ctrl._apply_update) - # --------------------------------------------------------------------------- # Settings changed → immediate check @@ -358,7 +373,7 @@ class TestConfigChanged: @staticmethod def test_config_changed_triggers_reinit_and_check() -> None: """Changing config should reinitialise the updater and check.""" - ctrl, app, client, banner, settings = _make_controller() + ctrl, app, client, banner, model = _make_controller() new_config = _make_config(update_channel='dev') @@ -374,7 +389,7 @@ def test_config_changed_triggers_reinit_and_check() -> None: @staticmethod def test_config_changed_updates_auto_apply() -> None: """Changing config should update the auto_apply flag.""" - ctrl, app, client, banner, settings = _make_controller(auto_apply=True) + ctrl, app, client, banner, model = _make_controller(auto_apply=True) new_config = _make_config(auto_apply=False) @@ -398,15 +413,16 @@ class TestCheckError: @staticmethod def test_check_error_sets_failed_status() -> None: """An exception during check should set 'Check failed' status.""" - ctrl, app, client, banner, settings = _make_controller() + ctrl, app, client, banner, model = _make_controller() + spy = ModelSpy(model) ctrl._on_check_error('connection refused', silent=False) - settings.set_update_status.assert_called_with('Check failed', UPDATE_STATUS_ERROR_STYLE) + assert ('Check failed', UPDATE_STATUS_ERROR_STYLE) in spy.status @staticmethod def test_check_error_shows_banner_when_not_silent() -> None: """An exception during check should show banner when not silent.""" - ctrl, app, client, banner, settings = _make_controller() + ctrl, app, client, banner, model = _make_controller() ctrl._on_check_error('timeout', silent=False) assert banner.state.name == 'ERROR' @@ -414,7 +430,7 @@ def test_check_error_shows_banner_when_not_silent() -> None: @staticmethod def test_check_error_no_banner_when_silent() -> None: """An exception during check should NOT show banner when silent.""" - ctrl, app, client, banner, settings = _make_controller() + ctrl, app, client, banner, model = _make_controller() ctrl._on_check_error('timeout', silent=True) assert banner.state.name == 'HIDDEN' @@ -431,19 +447,20 @@ class TestPersistCheckTimestamp: @staticmethod def test_persist_updates_store() -> None: """_persist_check_timestamp should call store.update with a timestamp.""" - ctrl, _app, _client, _banner, settings = _make_controller() + ctrl, _app, _client, _banner, model = _make_controller() + spy = ModelSpy(model) fake_resolved = _make_config(last_client_update='2026-03-09T00:00:00+00:00') with patch.object(ctrl._store, 'update', return_value=fake_resolved) as mock_update: ctrl._persist_check_timestamp() mock_update.assert_called_once() - settings.set_last_checked.assert_called_once() + assert len(spy.last_checked) == 1 @staticmethod def test_on_check_finished_success_syncs_via_store() -> None: """A successful check should persist timestamp via the store.""" - ctrl, _app, _client, _banner, settings = _make_controller() + ctrl, _app, _client, _banner, model = _make_controller() result = UpdateInfo(available=False, current_version=Version('1.0.0')) fake_resolved = _make_config(last_client_update='2026-03-09T00:00:00+00:00') @@ -454,11 +471,12 @@ def test_on_check_finished_success_syncs_via_store() -> None: @staticmethod def test_download_finished_syncs_via_store() -> None: """_on_download_finished should sync via the store and update the label.""" - ctrl, _app, _client, _banner, settings = _make_controller(auto_apply=False) + ctrl, _app, _client, _banner, model = _make_controller(auto_apply=False) + spy = ModelSpy(model) fake_resolved = _make_config(last_client_update='2026-03-09T00:00:00+00:00') with patch.object(ctrl._store, 'update', return_value=fake_resolved) as mock_update: ctrl._on_download_finished(True, '2.0.0') mock_update.assert_called_once() - settings.set_last_checked.assert_called_once() + assert len(spy.last_checked) == 1