From 3c2450906bd39708f2cfa2d6588ce317dab6610d Mon Sep 17 00:00:00 2001 From: Asher Norland Date: Tue, 10 Mar 2026 14:41:51 -0700 Subject: [PATCH 1/3] 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 From 9fdec6fe402c242cfdb8b2edbc1814a6180c9049 Mon Sep 17 00:00:00 2001 From: Asher Norland Date: Tue, 10 Mar 2026 14:44:45 -0700 Subject: [PATCH 2/3] Update test_update_controller.py --- tests/unit/qt/test_update_controller.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/qt/test_update_controller.py b/tests/unit/qt/test_update_controller.py index 09218d6..deb8f22 100644 --- a/tests/unit/qt/test_update_controller.py +++ b/tests/unit/qt/test_update_controller.py @@ -23,7 +23,6 @@ UpdateInfo, ) - # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- @@ -43,6 +42,7 @@ def __init__(self, model: UpdateModel) -> None: model.restart_visible_changed.connect(self.restart_visible.append) model.last_checked_changed.connect(self.last_checked.append) + # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- From ca6042dffb5882fd9e7d846c52cabe37fd1564f8 Mon Sep 17 00:00:00 2001 From: Asher Norland Date: Tue, 10 Mar 2026 22:01:02 -0700 Subject: [PATCH 3/3] Remove Project Action Stages --- pdm.lock | 8 +- pyproject.toml | 2 +- synodic_client/application/screen/__init__.py | 20 ++- .../application/screen/action_card.py | 61 +++---- synodic_client/application/screen/install.py | 16 +- .../application/screen/install_workers.py | 16 +- .../application/screen/log_panel.py | 7 +- synodic_client/application/screen/schema.py | 17 +- synodic_client/application/theme.py | 4 +- synodic_client/application/update_model.py | 1 + tests/unit/qt/test_action_card.py | 160 ++++++------------ tests/unit/qt/test_install_preview.py | 42 +++-- tests/unit/qt/test_log_panel.py | 1 - tests/unit/qt/test_preview_model.py | 6 +- tests/unit/qt/test_update_controller.py | 1 + 15 files changed, 148 insertions(+), 214 deletions(-) diff --git a/pdm.lock b/pdm.lock index f2d9875..df4dd57 100644 --- a/pdm.lock +++ b/pdm.lock @@ -5,7 +5,7 @@ groups = ["default", "build", "lint", "test"] strategy = ["inherit_metadata"] lock_version = "4.5.0" -content_hash = "sha256:18995a70ffa148eee18c62fe7dca60cfe1028305073ededcde318b8bf056d404" +content_hash = "sha256:52b09137c7599dc609e087bb58f7ced9bd9940ac53f77e0b6401eeb30d3f6bb3" [[metadata.targets]] requires_python = ">=3.14,<3.15" @@ -336,7 +336,7 @@ files = [ [[package]] name = "porringer" -version = "0.2.1.dev78" +version = "0.2.1.dev79" requires_python = ">=3.14" summary = "" groups = ["default"] @@ -349,8 +349,8 @@ dependencies = [ "userpath>=1.9.2", ] files = [ - {file = "porringer-0.2.1.dev78-py3-none-any.whl", hash = "sha256:557edbcda23f37b7f6b3226ba7d0ec76c31b9aa8f6c1af64a12537b3f0d35433"}, - {file = "porringer-0.2.1.dev78.tar.gz", hash = "sha256:7118736c60b371c5ca8a56fd583acdc5ca263a679eb6ea85fa1fc477525f570d"}, + {file = "porringer-0.2.1.dev79-py3-none-any.whl", hash = "sha256:fa010a1887f376827d154794dfa1ec8552263fb596e987bea8fa2e0e3920e478"}, + {file = "porringer-0.2.1.dev79.tar.gz", hash = "sha256:328de37f930e0042341378037c1b3bb9e36479485748f01569d8fac0936883df"}, ] [[package]] diff --git a/pyproject.toml b/pyproject.toml index 52d5731..1eb06d0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -14,7 +14,7 @@ requires-python = ">=3.14, <3.15" dependencies = [ "pyside6>=6.10.2", "packaging>=26.0", - "porringer>=0.2.1.dev78", + "porringer>=0.2.1.dev79", "qasync>=0.28.0", "velopack>=0.0.1444.dev49733", "typer>=0.24.1", diff --git a/synodic_client/application/screen/__init__.py b/synodic_client/application/screen/__init__.py index 109ae5e..bbac2ea 100644 --- a/synodic_client/application/screen/__init__.py +++ b/synodic_client/application/screen/__init__.py @@ -8,7 +8,7 @@ from datetime import UTC, datetime -from porringer.schema import SetupAction, SkipReason +from porringer.schema import SetupAction, SetupActionResult, SkipReason from porringer.schema.plugin import PluginKind _SECONDS_PER_MINUTE = 60 @@ -63,18 +63,26 @@ def skip_reason_label(reason: SkipReason | None) -> str: return SKIP_REASON_LABELS.get(reason, reason.name.replace('_', ' ').capitalize()) -def format_cli_command(action: SetupAction, *, suppress_description: bool = False) -> str: +def format_cli_command( + action: SetupAction, + *, + result: SetupActionResult | None = None, + suppress_description: bool = False, +) -> str: """Return a human-readable CLI command string for *action*. - Prefers ``cli_command``, falls back to ``command``, then synthesises - an ``installer install `` string for package actions, and + Prefers ``result.cli_command`` (populated after dry-run), falls + back to ``action.command``, then synthesises an + ``installer install `` string for package actions, and finally returns the action description as a last resort. When *suppress_description* is ``True`` the final description fallback returns an empty string instead. """ - if parts := (action.cli_command or action.command): - return ' '.join(parts) + if result is not None and result.cli_command: + return ' '.join(result.cli_command) + if action.command: + return ' '.join(action.command) if action.kind == PluginKind.PACKAGE and action.package: return f'{action.installer or "pip"} install {action.package}' return '' if suppress_description else action.description diff --git a/synodic_client/application/screen/action_card.py b/synodic_client/application/screen/action_card.py index ca0c7bf..d5911d6 100644 --- a/synodic_client/application/screen/action_card.py +++ b/synodic_client/application/screen/action_card.py @@ -73,19 +73,6 @@ _KIND_ORDER: dict[PluginKind | None, int] = {kind: i for i, kind in enumerate(PHASE_ORDER)} -def action_key(action: SetupAction) -> tuple[object, ...]: - """Return a stable identity key for *action*. - - Uses content-based fields (kind, installer, package name, command) - so the same logical action from different ``execute_stream`` runs - resolves to the same key. - """ - pkg_name = str(action.package.name) if action.package else None - pt_name = str(action.plugin_target.name) if action.plugin_target else None - cmd = tuple(action.command) if action.command else None - return (action.kind, action.installer, pkg_name, pt_name, cmd) - - def action_sort_key(action: SetupAction) -> int: """Return a sort key that groups cards by execution phase. @@ -396,24 +383,6 @@ def populate( else: self._prerelease_cb.hide() - def update_command(self, action: SetupAction) -> None: - """Update the CLI command label after the resolved preview arrives. - - Called from the two-phase display flow once ``MANIFEST_LOADED`` - provides actions with their ``cli_command`` populated. - - Args: - action: The setup action with resolved CLI command. - """ - if self._is_skeleton: - return - cmd_text = format_cli_command(action, suppress_description=True) - if cmd_text: - self._command_label.setText(cmd_text) - self._command_row.show() - else: - self._command_row.hide() - def _populate_status( self, action: SetupAction, @@ -494,7 +463,7 @@ def set_check_result(self, result: SetupActionResult) -> None: self._status_label.setText(label) self._status_label.setStyleSheet(ACTION_CARD_STATUS_UPDATE) elif result.skipped: - label = skip_reason_label(result.skip_reason) + label = '\u2713 ' + skip_reason_label(result.skip_reason) self._status_label.setText(label) self._status_label.setStyleSheet(ACTION_CARD_STATUS_SATISFIED) elif not result.success: @@ -517,6 +486,13 @@ def set_check_result(self, result: SetupActionResult) -> None: else: self._status_label.setToolTip('') + # CLI command — update with resolved cli_command from result + assert self._action is not None + cmd_text = format_cli_command(self._action, result=result, suppress_description=True) + if cmd_text: + self._command_label.setText(cmd_text) + self._command_row.show() + # Version column self._check_available_version = result.available_version if result.installed_version and result.available_version: @@ -524,6 +500,9 @@ def set_check_result(self, result: SetupActionResult) -> None: self._version_label.setStyleSheet(ACTION_CARD_VERSION_STYLE + ' color: #d7ba7d;') elif result.installed_version: self._version_label.setText(result.installed_version) + elif result.available_version: + self._version_label.setText(f'\u2192 {result.available_version}') + self._version_label.setStyleSheet(ACTION_CARD_VERSION_STYLE + ' color: grey;') def finalize_checking(self) -> None: """Resolve a still-pending 'Checking\u2026' status to 'Needed'. @@ -611,9 +590,8 @@ def is_update_available(self) -> bool: class ActionCardList(QWidget): """Container of :class:`ActionCard` widgets. - Cards are keyed by :func:`action_key` (content-based) so that - look-ups work across different ``execute_stream`` runs where the - ``SetupAction`` objects are different Python instances. + Cards are keyed by ``SetupAction`` directly (frozen dataclass) so + that look-ups work across different ``execute_stream`` runs. """ prerelease_toggled = Signal(str, bool) @@ -629,7 +607,7 @@ def __init__(self, parent: QWidget | None = None) -> None: self._layout.addStretch() self._cards: list[ActionCard] = [] - self._action_map: dict[tuple[object, ...], ActionCard] = {} + self._action_map: dict[SetupAction, ActionCard] = {} # ------------------------------------------------------------------ # Skeleton loading @@ -679,7 +657,7 @@ def populate( card.prerelease_toggled.connect(self.prerelease_toggled.emit) self._layout.insertWidget(self._layout.count() - 1, card) self._cards.append(card) - self._action_map[action_key(act)] = card + self._action_map[act] = card # ------------------------------------------------------------------ # Card lookup @@ -696,11 +674,10 @@ def card_count(self) -> int: return len(self._cards) def get_card(self, action: SetupAction) -> ActionCard | None: - """Look up the card for a given action by stable content key. + """Look up the card for a given action. - Works across different ``execute_stream`` runs — the preview - and install phases produce different ``SetupAction`` instances - but the same logical action maps to the same card. + ``SetupAction`` is a frozen dataclass, so the same logical + action from different ``execute_stream`` runs hashes equally. Args: action: The setup action to find. @@ -708,7 +685,7 @@ def get_card(self, action: SetupAction) -> ActionCard | None: Returns: The card widget, or ``None`` if not found. """ - return self._action_map.get(action_key(action)) + return self._action_map.get(action) # ------------------------------------------------------------------ # Bulk operations diff --git a/synodic_client/application/screen/install.py b/synodic_client/application/screen/install.py index 0b4335b..0c1f432 100644 --- a/synodic_client/application/screen/install.py +++ b/synodic_client/application/screen/install.py @@ -45,7 +45,7 @@ from synodic_client.application.package_state import PackageStateStore from synodic_client.application.screen import skip_reason_label -from synodic_client.application.screen.action_card import ActionCardList, action_key +from synodic_client.application.screen.action_card import ActionCardList from synodic_client.application.screen.card import CardFrame from synodic_client.application.screen.install_workers import run_install, run_preview from synodic_client.application.screen.log_panel import ExecutionLogPanel @@ -595,11 +595,11 @@ def _on_preview_ready(self, preview: SetupResults, manifest_path: str, temp_dir_ self._install_btn.setEnabled(True) def _on_preview_resolved(self, preview: SetupResults, manifest_path: str, temp_dir_path: str) -> None: - """Handle the fully-resolved preview (CLI commands populated). + """Handle the fully-resolved preview. Called after ``MANIFEST_LOADED`` — cards are already visible - from the earlier ``_on_manifest_parsed`` handler. This only - updates CLI command text and the temp-dir reference. + from the earlier ``_on_manifest_parsed`` handler. This updates + the temp-dir reference and emits metadata. """ if self._model.preview is None: return @@ -609,19 +609,13 @@ def _on_preview_resolved(self, preview: SetupResults, manifest_path: str, temp_d if preview.metadata: self.metadata_ready.emit(preview) - for action in preview.actions: - if action.cli_command: - card = self._card_list.get_card(action) - if card is not None: - card.update_command(action) - def _on_action_checked(self, row: int, result: SetupActionResult) -> None: """Update the model and action card with a dry-run result.""" m = self._model if result.skipped and result.skip_reason == SkipReason.UPDATE_AVAILABLE: label = skip_reason_label(result.skip_reason) if 0 <= row < len(m.action_states): - m.upgradable_keys.add(action_key(m.action_states[row].action)) + m.upgradable_keys.add(m.action_states[row].action) elif result.skipped: label = skip_reason_label(result.skip_reason) elif not result.success: diff --git a/synodic_client/application/screen/install_workers.py b/synodic_client/application/screen/install_workers.py index 86bb87c..b52986b 100644 --- a/synodic_client/application/screen/install_workers.py +++ b/synodic_client/application/screen/install_workers.py @@ -159,10 +159,9 @@ def _dispatch_preview_event( ) -> None: """Route a single preview stream event to the appropriate callback. - Mutates *state* in-place with updated ``action_index`` / ``got_parsed``. + Mutates *state* in-place (``got_parsed`` flag). """ if event.kind == ProgressEventKind.MANIFEST_PARSED and event.manifest: - state.action_index = {id(a): i for i, a in enumerate(event.manifest.actions)} if cb.on_manifest_parsed is not None: cb.on_manifest_parsed(event.manifest, manifest_path, temp_dir_str) state.got_parsed = True @@ -176,16 +175,17 @@ def _dispatch_preview_event( return if event.kind == ProgressEventKind.MANIFEST_LOADED and event.manifest: - if not state.got_parsed: - state.action_index = {id(a): i for i, a in enumerate(event.manifest.actions)} if cb.on_preview_ready is not None: cb.on_preview_ready(event.manifest, manifest_path, temp_dir_str) return - if event.kind == ProgressEventKind.ACTION_COMPLETED and event.result and event.action: - row = state.action_index.get(id(event.action)) - if row is not None and cb.on_action_checked is not None: - cb.on_action_checked(row, event.result) + if ( + event.kind == ProgressEventKind.ACTION_COMPLETED + and event.result + and event.action_index is not None + and cb.on_action_checked is not None + ): + cb.on_action_checked(event.action_index, event.result) # --------------------------------------------------------------------------- diff --git a/synodic_client/application/screen/log_panel.py b/synodic_client/application/screen/log_panel.py index 83e2422..4375fa0 100644 --- a/synodic_client/application/screen/log_panel.py +++ b/synodic_client/application/screen/log_panel.py @@ -26,7 +26,6 @@ ) from synodic_client.application.screen import ACTION_KIND_LABELS, skip_reason_label -from synodic_client.application.screen.action_card import action_key from synodic_client.application.screen.card import CHEVRON_DOWN, CHEVRON_RIGHT, ClickableHeader from synodic_client.application.theme import ( LOG_CHEVRON_STYLE, @@ -186,7 +185,7 @@ def __init__(self, parent: QWidget | None = None) -> None: self._layout.addStretch() # Map action content-key → section widget for quick lookup - self._sections: dict[tuple[object, ...], ActionLogSection] = {} + self._sections: dict[SetupAction, ActionLogSection] = {} self._section_count = 0 # --- Public API --- @@ -204,7 +203,7 @@ def add_section(self, action: SetupAction) -> ActionLogSection: section = ActionLogSection(action, self._section_count, self) # Insert before the stretch self._layout.insertWidget(self._layout.count() - 1, section) - self._sections[action_key(action)] = section + self._sections[action] = section return section @@ -217,7 +216,7 @@ def get_section(self, action: SetupAction) -> ActionLogSection | None: Returns: The section widget, or ``None`` if not found. """ - return self._sections.get(action_key(action)) + return self._sections.get(action) def on_sub_progress(self, action: SetupAction, progress: SubActionProgress) -> None: """Handle a sub-action progress event. diff --git a/synodic_client/application/screen/schema.py b/synodic_client/application/screen/schema.py index 28e3002..bc88cea 100644 --- a/synodic_client/application/screen/schema.py +++ b/synodic_client/application/screen/schema.py @@ -23,7 +23,6 @@ ) from porringer.schema.plugin import RuntimePackageResult -from synodic_client.application.screen.action_card import action_key from synodic_client.application.uri import normalize_manifest_key # --------------------------------------------------------------------------- @@ -237,7 +236,6 @@ class PreviewModel: def __init__(self) -> None: """Initialise a blank preview model.""" - self._action_key = action_key self._normalize = normalize_manifest_key self.phase: PreviewPhase = PreviewPhase.IDLE @@ -248,19 +246,19 @@ def __init__(self) -> None: self.plugin_installed: dict[str, bool] = {} self.prerelease_overrides: set[str] = set() self.action_states: list[ActionState] = [] - self._action_state_map: dict[tuple[object, ...], ActionState] = {} + self._action_state_map: dict[SetupAction, ActionState] = {} self._action_state_map_len: int = 0 - self.upgradable_keys: set[tuple[object, ...]] = set() + self.upgradable_keys: set[SetupAction] = set() self.checked_count: int = 0 self.completed_count: int = 0 self.temp_dir: str | None = None # -- Computed helpers -------------------------------------------------- - def _ensure_action_state_map(self) -> dict[tuple[object, ...], ActionState]: - """Return the action-key → state lookup, rebuilding if stale.""" + def _ensure_action_state_map(self) -> dict[SetupAction, ActionState]: + """Return the action → state lookup, rebuilding if stale.""" if len(self.action_states) != self._action_state_map_len: - self._action_state_map = {self._action_key(s.action): s for s in self.action_states} + self._action_state_map = {s.action: s for s in self.action_states} self._action_state_map_len = len(self.action_states) return self._action_state_map @@ -279,8 +277,8 @@ def install_enabled(self) -> bool: return self.actionable_count > 0 or any(s.action.kind is None for s in self.action_states) def action_state_for(self, act: SetupAction) -> ActionState | None: - """Look up :class:`ActionState` by content key (O(1) amortized).""" - return self._ensure_action_state_map().get(self._action_key(act)) + """Look up :class:`ActionState` for *act* (O(1) amortized).""" + return self._ensure_action_state_map().get(act) def has_same_manifest(self, key: str) -> bool: """Return ``True`` if *key* matches the current manifest key.""" @@ -339,5 +337,4 @@ class PreviewConfig: class _DispatchState: """Mutable accumulator for :func:`_dispatch_preview_event`.""" - action_index: dict[int, int] = field(default_factory=dict) got_parsed: bool = False diff --git a/synodic_client/application/theme.py b/synodic_client/application/theme.py index e6e4e73..e96ac34 100644 --- a/synodic_client/application/theme.py +++ b/synodic_client/application/theme.py @@ -427,8 +427,8 @@ ACTION_CARD_STATUS_NEEDED = 'color: palette(text); font-size: 11px; font-weight: bold;' """Status label: Needed.""" -ACTION_CARD_STATUS_SATISFIED = 'color: grey; font-size: 11px;' -"""Status label: Already installed.""" +ACTION_CARD_STATUS_SATISFIED = 'color: #6a9955; font-size: 11px;' +"""Status label: Already installed (muted green with checkmark).""" ACTION_CARD_STATUS_UPDATE = 'color: #d7ba7d; font-size: 11px; font-weight: bold;' """Status label: Update available (amber).""" diff --git a/synodic_client/application/update_model.py b/synodic_client/application/update_model.py index 368f461..48cc33f 100644 --- a/synodic_client/application/update_model.py +++ b/synodic_client/application/update_model.py @@ -50,6 +50,7 @@ class UpdateModel(QObject): last_checked_changed = Signal(str) def __init__(self, parent: QObject | None = None) -> None: + """Initialize the update model.""" super().__init__(parent) self._phase = UpdatePhase.IDLE self._version: str = '' diff --git a/tests/unit/qt/test_action_card.py b/tests/unit/qt/test_action_card.py index b21e4ed..a62c598 100644 --- a/tests/unit/qt/test_action_card.py +++ b/tests/unit/qt/test_action_card.py @@ -17,7 +17,6 @@ from synodic_client.application.screen.action_card import ( ActionCard, ActionCardList, - action_key, action_sort_key, ) from synodic_client.application.theme import ( @@ -43,7 +42,7 @@ def _make_action( *, kind: PluginKind | None = PluginKind.PACKAGE, description: str = 'Install requests', - installer: str = 'pip', + installer: str | None = 'pip', package: str = 'requests', **overrides: Any, ) -> SetupAction: @@ -63,7 +62,6 @@ def _make_action( action.package = pkg_mock action.package_description = overrides.get('package_description', description) action.command = overrides.get('command') - action.cli_command = overrides.get('cli_command') action.include_prereleases = overrides.get('include_prereleases', False) action.plugin_target = overrides.get('plugin_target') return action @@ -80,7 +78,7 @@ def _make_result( """Create a SetupActionResult. Extra keyword arguments (``action``, ``installed_version``, - ``available_version``) are forwarded to the constructor. + ``available_version``, ``cli_command``) are forwarded to the constructor. """ return SetupActionResult( action=overrides.get('action') or _make_action(), @@ -90,6 +88,7 @@ def _make_result( message=message, installed_version=overrides.get('installed_version'), available_version=overrides.get('available_version'), + cli_command=overrides.get('cli_command'), ) @@ -244,7 +243,7 @@ def test_needed_status() -> None: @staticmethod def test_already_installed_status() -> None: - """Skipped ALREADY_INSTALLED shows 'Already installed'.""" + """Skipped ALREADY_INSTALLED shows '\u2713 Already installed'.""" card = ActionCard() card.populate(_make_action()) result = _make_result( @@ -253,7 +252,7 @@ def test_already_installed_status() -> None: installed_version='3.5.2', ) card.set_check_result(result) - assert card.status_text() == 'Already installed' + assert card.status_text() == '\u2713 Already installed' assert ACTION_CARD_STATUS_SATISFIED in card._status_label.styleSheet() @staticmethod @@ -301,6 +300,20 @@ def test_installed_version_shown() -> None: card.set_check_result(result) assert card._version_label.text() == '3.5.2' + @staticmethod + def test_available_version_only_shown() -> None: + """Version label shows '→ target' when only available_version is set.""" + card = ActionCard() + card.populate(_make_action()) + result = _make_result( + success=True, + skipped=False, + available_version='1.2.0', + ) + card.set_check_result(result) + assert '\u2192 1.2.0' in card._version_label.text() + assert 'grey' in card._version_label.styleSheet() + @staticmethod def test_finalize_checking_resolves_to_needed() -> None: """finalize_checking stops spinner and changes to 'Needed'.""" @@ -349,8 +362,7 @@ def test_failed_check_shows_failed_status() -> None: def test_failed_check_shows_error_tooltip() -> None: """A failed check result surfaces the error message as a tooltip.""" card = ActionCard() - action = _make_action(kind=PluginKind.SCM, package='repo') - action.installer = None # Simulate an unresolved deferred action + action = _make_action(kind=PluginKind.SCM, package='repo', installer=None) card.populate(action) msg = "SCM environment 'None' is not available" result = _make_result(success=False, skipped=False, message=msg) @@ -506,8 +518,7 @@ def test_populate_includes_command_actions() -> None: """Populate includes actions with kind=None.""" card_list = ActionCardList() a1 = _make_action(package='pkg1') - a2 = _make_action(package='pkg2') - a2.kind = None # bare command + a2 = _make_action(package='pkg2', kind=None) actions = [a1, a2] card_list.populate(actions) assert card_list.card_count() == len(actions) @@ -528,21 +539,6 @@ def test_get_card_by_stable_key() -> None: assert c1._package_label.text() == 'first' assert c2._package_label.text() == 'second' - @staticmethod - def test_get_card_cross_instance() -> None: - """get_card works with a different object that has the same content.""" - card_list = ActionCardList() - original = _make_action(package='numpy', installer='pip') - card_list.populate([original]) - - # Create a separate mock with the same content fields - duplicate = _make_action(package='numpy', installer='pip') - assert original is not duplicate - - card = card_list.get_card(duplicate) - assert card is not None - assert card._package_label.text() == 'numpy' - @staticmethod def test_get_card_returns_none_for_unknown() -> None: """get_card returns None for an unknown action.""" @@ -582,7 +578,7 @@ def test_finalize_all_checking() -> None: card_list.finalize_all_checking() - assert c1.status_text() == 'Already installed' # unchanged + assert c1.status_text() == '\u2713 Already installed' # unchanged c2 = card_list.get_card(a2) assert c2 is not None assert c2.status_text() == 'Needed' # resolved @@ -612,53 +608,6 @@ def test_card_at_out_of_range() -> None: assert card_list.card_at(-1) is None -# --------------------------------------------------------------------------- -# action_key — stable identity -# --------------------------------------------------------------------------- - - -class TestActionKey: - """Tests for the action_key function.""" - - @staticmethod - def test_same_content_same_key() -> None: - """Two actions with identical content produce the same key.""" - a = _make_action(package='numpy', installer='pip') - b = _make_action(package='numpy', installer='pip') - assert a is not b - assert action_key(a) == action_key(b) - - @staticmethod - def test_different_package_different_key() -> None: - """Actions with different packages produce different keys.""" - a = _make_action(package='numpy') - b = _make_action(package='scipy') - assert action_key(a) != action_key(b) - - @staticmethod - def test_different_installer_different_key() -> None: - """Actions with different installers produce different keys.""" - a = _make_action(package='numpy', installer='pip') - b = _make_action(package='numpy', installer='uv') - assert action_key(a) != action_key(b) - - @staticmethod - def test_different_kind_different_key() -> None: - """Actions with different kinds produce different keys.""" - a = _make_action(package='ruff', kind=PluginKind.PACKAGE) - b = _make_action(package='ruff', kind=PluginKind.TOOL) - assert action_key(a) != action_key(b) - - @staticmethod - def test_command_action_key() -> None: - """Command actions include the command in the key.""" - a = _make_action(command=['echo', 'hello']) - b = _make_action(command=['echo', 'hello']) - c = _make_action(command=['echo', 'world']) - assert action_key(a) == action_key(b) - assert action_key(a) != action_key(c) - - # --------------------------------------------------------------------------- # ActionCard — CLI command label # --------------------------------------------------------------------------- @@ -677,11 +626,18 @@ def test_default_package_command() -> None: assert not card._command_row.isHidden() @staticmethod - def test_explicit_cli_command() -> None: - """Actions with cli_command show that instead of the default.""" + def test_explicit_cli_command_from_result() -> None: + """set_check_result with cli_command updates the command label.""" card = ActionCard() - action = _make_action(cli_command=['uv', 'tool', 'install', 'ruff']) + action = _make_action(package='ruff', installer='pip') card.populate(action) + assert card._command_label.text() == 'pip install ruff' + + result = _make_result( + action=action, + cli_command=('uv', 'tool', 'install', 'ruff'), + ) + card.set_check_result(result) assert card._command_label.text() == 'uv tool install ruff' @staticmethod @@ -694,46 +650,30 @@ def test_command_label_selectable() -> None: assert flags & Qt.TextInteractionFlag.TextSelectableByMouse @staticmethod - def test_update_command_updates_text() -> None: - """update_command replaces the command label text.""" + def test_set_check_result_updates_command_label() -> None: + """set_check_result with result cli_command updates the command label.""" card = ActionCard() action = _make_action(package='ruff', installer='pip') card.populate(action) assert card._command_label.text() == 'pip install ruff' - resolved = _make_action(cli_command=['uv', 'tool', 'install', 'ruff']) - card.update_command(resolved) + result = _make_result( + action=action, + cli_command=('uv', 'tool', 'install', 'ruff'), + ) + card.set_check_result(result) assert card._command_label.text() == 'uv tool install ruff' assert not card._command_row.isHidden() - @staticmethod - def test_update_command_hides_label_when_empty() -> None: - """update_command hides the row when the resolved action has no command.""" - card = ActionCard() - action = _make_action(package='ruff', installer='pip') - card.populate(action) - assert not card._command_row.isHidden() - - empty_action = _make_action(kind=PluginKind.RUNTIME) - empty_action.cli_command = None - empty_action.command = None - empty_action.package = None - card.update_command(empty_action) - assert card._command_row.isHidden() - - @staticmethod - def test_update_command_noop_on_skeleton() -> None: - """update_command does nothing when card is a skeleton.""" - card = ActionCard(skeleton=True) - action = _make_action(cli_command=['uv', 'tool', 'install', 'ruff']) - # Should not raise — skeleton simply returns early - card.update_command(action) - @staticmethod def test_copy_button_copies_command(monkeypatch: object) -> None: """Clicking the copy button copies the command text to the clipboard.""" card = ActionCard() - action = _make_action(cli_command=['uv', 'tool', 'install', 'ruff']) + action = _make_action( + package='ruff', + installer='uv', + command=('uv', 'tool', 'install', 'ruff'), + ) card.populate(action) clipboard = QApplication.clipboard() @@ -746,7 +686,11 @@ def test_copy_button_copies_command(monkeypatch: object) -> None: def test_copy_button_shows_feedback() -> None: """Clicking copy shows a check-mark on the button.""" card = ActionCard() - action = _make_action(cli_command=['uv', 'tool', 'install', 'ruff']) + action = _make_action( + package='ruff', + installer='uv', + command=('uv', 'tool', 'install', 'ruff'), + ) card.populate(action) card._copy_btn.click() @@ -936,7 +880,7 @@ def test_already_latest_shows_satisfied_style() -> None: skip_reason=SkipReason.ALREADY_LATEST, ) ) - assert card.status_text() == 'Already latest' + assert card.status_text() == '\u2713 Already latest' assert ACTION_CARD_STATUS_SATISFIED in card._status_label.styleSheet() @staticmethod @@ -974,8 +918,8 @@ def test_already_installed_vs_already_latest() -> None: ) ) - assert card_installed.status_text() == 'Already installed' - assert card_latest.status_text() == 'Already latest' + assert card_installed.status_text() == '\u2713 Already installed' + assert card_latest.status_text() == '\u2713 Already latest' # Both use the satisfied stylesheet assert ACTION_CARD_STATUS_SATISFIED in card_installed._status_label.styleSheet() assert ACTION_CARD_STATUS_SATISFIED in card_latest._status_label.styleSheet() diff --git a/tests/unit/qt/test_install_preview.py b/tests/unit/qt/test_install_preview.py index bc144ec..6413899 100644 --- a/tests/unit/qt/test_install_preview.py +++ b/tests/unit/qt/test_install_preview.py @@ -49,7 +49,6 @@ def _make_action( action.installer = installer action.package = package action.command = None - action.cli_command = None return action @staticmethod @@ -85,7 +84,6 @@ def _make_action(**overrides: Any) -> MagicMock: 'description': 'Install test', 'installer': 'pip', 'package': 'requests', - 'cli_command': None, 'command': None, } defaults.update(overrides) @@ -96,19 +94,20 @@ def _make_action(**overrides: Any) -> MagicMock: action.installer = defaults['installer'] action.package = defaults['package'] action.command = defaults['command'] - action.cli_command = defaults['cli_command'] return action - def test_prefers_cli_command(self) -> None: - """Verify cli_command takes precedence over command and fallback.""" - action = self._make_action(cli_command=['uv', 'pip', 'install', 'requests']) - assert format_cli_command(action) == 'uv pip install requests' + def test_prefers_cli_command_from_result(self) -> None: + """Verify result cli_command takes precedence over command and fallback.""" + action = self._make_action() + result = MagicMock() + result.cli_command = ('uv', 'pip', 'install', 'requests') + assert format_cli_command(action, result=result) == 'uv pip install requests' def test_falls_back_to_command(self) -> None: """Verify command is used when cli_command is absent.""" action = self._make_action( kind='TOOL', - command=['echo', 'hello'], + command=('echo', 'hello'), ) assert format_cli_command(action) == 'echo hello' @@ -150,6 +149,7 @@ def test_worker_emits_finished_on_success() -> None: kind=ProgressEventKind.ACTION_COMPLETED, action=action, result=result, + action_index=0, ) async def mock_stream(*args, **kwargs): @@ -389,7 +389,12 @@ def test_emits_preview_ready_and_finished(tmp_path: Path) -> None: # Dry-run stream yields manifest loaded then one completed event manifest_event = ProgressEvent(kind=ProgressEventKind.MANIFEST_LOADED, manifest=preview) result = SetupActionResult(action=action, success=True, skipped=False, skip_reason=None) - completed_event = ProgressEvent(kind=ProgressEventKind.ACTION_COMPLETED, action=action, result=result) + completed_event = ProgressEvent( + kind=ProgressEventKind.ACTION_COMPLETED, + action=action, + result=result, + action_index=0, + ) async def mock_stream(*args: Any, **kwargs: Any) -> Any: yield manifest_event @@ -448,7 +453,7 @@ async def _run() -> None: @staticmethod def test_action_checked_maps_correct_rows(tmp_path: Path) -> None: - """Verify on_action_checked receives correct row indices via identity matching.""" + """Verify on_action_checked receives correct row indices via content-based matching.""" manifest = tmp_path / 'porringer.json' manifest.write_text('{}') @@ -466,8 +471,18 @@ def test_action_checked_maps_correct_rows(tmp_path: Path) -> None: result_a = SetupActionResult(action=action_a, success=True, skipped=False, skip_reason=None) # Stream returns in execution order (b before a), not preview order - event_b = ProgressEvent(kind=ProgressEventKind.ACTION_COMPLETED, action=action_b, result=result_b) - event_a = ProgressEvent(kind=ProgressEventKind.ACTION_COMPLETED, action=action_a, result=result_a) + event_b = ProgressEvent( + kind=ProgressEventKind.ACTION_COMPLETED, + action=action_b, + result=result_b, + action_index=1, + ) + event_a = ProgressEvent( + kind=ProgressEventKind.ACTION_COMPLETED, + action=action_a, + result=result_a, + action_index=0, + ) async def mock_stream(*args: Any, **kwargs: Any) -> Any: yield manifest_event @@ -815,7 +830,6 @@ def _make_scm_action(description: str = 'Clone repo') -> MagicMock: action.installer = 'git' action.package = None action.command = None - action.cli_command = None return action def test_scm_already_installed_emits_correct_result(self, tmp_path: Path) -> None: @@ -838,6 +852,7 @@ def test_scm_already_installed_emits_correct_result(self, tmp_path: Path) -> Non kind=ProgressEventKind.ACTION_COMPLETED, action=action, result=result, + action_index=0, ) async def mock_stream(*args: Any, **kwargs: Any) -> Any: @@ -884,6 +899,7 @@ def test_scm_needed_emits_correct_result(self, tmp_path: Path) -> None: kind=ProgressEventKind.ACTION_COMPLETED, action=action, result=result, + action_index=0, ) async def mock_stream(*args: Any, **kwargs: Any) -> Any: diff --git a/tests/unit/qt/test_log_panel.py b/tests/unit/qt/test_log_panel.py index 812f661..813bb20 100644 --- a/tests/unit/qt/test_log_panel.py +++ b/tests/unit/qt/test_log_panel.py @@ -61,7 +61,6 @@ def _make_action( action.package = pkg_mock action.package_description = package_description or description action.command = None - action.cli_command = None action.plugin_target = None return action diff --git a/tests/unit/qt/test_preview_model.py b/tests/unit/qt/test_preview_model.py index 19ef933..957cef6 100644 --- a/tests/unit/qt/test_preview_model.py +++ b/tests/unit/qt/test_preview_model.py @@ -8,7 +8,6 @@ from porringer.schema import SetupAction from porringer.schema.plugin import PluginKind -from synodic_client.application.screen.action_card import action_key from synodic_client.application.screen.schema import ActionState, PreviewModel, PreviewPhase from synodic_client.application.uri import normalize_manifest_key @@ -36,7 +35,6 @@ def _make_action( action.package = pkg_mock action.package_description = overrides.get('package_description', description) action.command = overrides.get('command') - action.cli_command = overrides.get('cli_command') action.include_prereleases = overrides.get('include_prereleases', False) action.plugin_target = overrides.get('plugin_target') return action @@ -105,7 +103,7 @@ def test_install_enabled_true_when_ready_with_upgradable() -> None: state = ActionState(action=_make_action()) state.status = 'Already installed' model.action_states.append(state) - model.upgradable_keys.add(action_key(state.action)) + model.upgradable_keys.add(state.action) assert model.install_enabled is True @staticmethod @@ -149,7 +147,7 @@ def test_actionable_count() -> None: upgradable = ActionState(action=_make_action(package='c')) upgradable.status = 'Update available' model.action_states = [needed, satisfied, upgradable] - model.upgradable_keys.add(action_key(upgradable.action)) + model.upgradable_keys.add(upgradable.action) expected_actionable = 2 # 1 needed + 1 upgradable assert model.actionable_count == expected_actionable diff --git a/tests/unit/qt/test_update_controller.py b/tests/unit/qt/test_update_controller.py index deb8f22..e1f4362 100644 --- a/tests/unit/qt/test_update_controller.py +++ b/tests/unit/qt/test_update_controller.py @@ -32,6 +32,7 @@ class ModelSpy: """Records signal emissions from an :class:`UpdateModel`.""" def __init__(self, model: UpdateModel) -> None: + """Connect to *model* signals and record emissions.""" self.status: list[tuple[str, str]] = [] self.check_button_enabled: list[bool] = [] self.restart_visible: list[bool] = []