diff --git a/pyobs_gui/base.py b/pyobs_gui/base.py index 5d36274..67c6a98 100644 --- a/pyobs_gui/base.py +++ b/pyobs_gui/base.py @@ -1,6 +1,7 @@ from __future__ import annotations import asyncio +import contextlib import logging from typing import TYPE_CHECKING, Any, Callable, Type, TypeVar, overload @@ -44,6 +45,13 @@ async def show_remote_error(parent: QtWidgets.QWidget, exception: Exception) -> await QAsyncMessageBox.warning(parent, "Error", str(exception)) +async def cancel_and_drain(task: asyncio.Task[Any]) -> None: + """Cancel a task and await its unwind, swallowing the resulting CancelledError.""" + task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await task + + class BaseWindow: def __init__(self) -> None: """Base class for MainWindow and all widgets.""" @@ -169,6 +177,14 @@ def __init__( # has it been initialized? self._initialized = False + # memoized one-shot init task -- two rapid show/hide/show cycles must not run _init() + # twice concurrently (see _showEvent) + self._init_task: asyncio.Task[Any] | None = None + + # keys of _init_once() sub-steps that already completed, so a retried _init() (see + # _showEvent) skips steps that already ran instead of re-subscribing to comm state + self._init_steps_done: set[str] = set() + # methods this GUI is permitted to invoke on self.module; None until fetched or if the # fetch failed, meaning "treat everything as permitted" self._permitted_methods: set[str] | None = None @@ -251,6 +267,15 @@ async def discard(self) -> None: in Comm._event_handlers forever, keeping this widget alive and still reacting to events for as long as the app runs. """ + # cancel a still-in-flight one-time _init() first: comm's own disconnect-triggered + # subscription cleanup (Comm._client_disconnected) runs before this method is called, + # so a subscribe_state() call inside _init_task that resolves afterwards would leak a + # subscription bound to this now-discarded widget -- the same stale-callback crash + # class this method otherwise guards against + if self._init_task is not None: + await cancel_and_drain(self._init_task) + self._init_task = None + for event_class, handler in self._registered_event_handlers: await self.comm.unregister_event(event_class, handler) self._registered_event_handlers.clear() @@ -263,14 +288,44 @@ def showEvent(self, event: QtGui.QShowEvent) -> None: asyncio.create_task(self._showEvent(event)) async def _showEvent(self, event: QtGui.QShowEvent) -> None: - if self._initialized is False and hasattr(self, "_init"): - await self._init() - self._initialized = True + if self._initialized is False: + if self._init_task is None: + self._init_task = asyncio.create_task(self._run_init()) + try: + await self._init_task + except Exception: + # transient init failure (e.g. a comm RPC hiccup): log, and allow a retry on + # the next show instead of leaving the widget permanently marked initialized + log.exception("Init of %s failed; will retry on next show.", type(self).__name__) + self._init_task = None if self._update_func: # start update task self._update_task = asyncio.create_task(self._update_loop()) + async def _run_init(self) -> None: + await self._init() + self._initialized = True + + async def _init(self) -> None: + """Default no-op init. Widgets with one-time subscription/state setup override this; + _showEvent memoizes the call so it runs exactly once per widget lifetime.""" + + async def _init_once(self, key: str, coro_func: Callable[..., Coroutine[Any, Any, None]], *args: Any) -> None: + """Run one sub-step of _init() at most once across retries. + + _showEvent retries the whole _init() after a partial failure (e.g. one of several + gathered comm.subscribe_state() calls raising while its siblings already subscribed). + comm.subscribe_state() unconditionally appends to comm's subscription list with no + deduplication, so a sub-step that already succeeded must not run again on retry -- + override _init() and wrap each independent, subscribing sub-step in this instead of + calling it directly. + """ + if key in self._init_steps_done: + return + await coro_func(*args) + self._init_steps_done.add(key) + def hideEvent(self, event: QtGui.QHideEvent) -> None: # run in loop asyncio.create_task(self._hide_event(event)) diff --git a/pyobs_gui/camerawidget.py b/pyobs_gui/camerawidget.py index 2b3d03b..af7c09c 100644 --- a/pyobs_gui/camerawidget.py +++ b/pyobs_gui/camerawidget.py @@ -1,3 +1,4 @@ +import asyncio import logging from typing import Any from PySide6 import QtWidgets, QtCore # type: ignore @@ -38,6 +39,13 @@ log = logging.getLogger(__name__) +# how long to wait for a module's first state value when initializing each control. The comm +# default is 10 s per interface, which would let a slow-publishing camera hold the page blank +# for ~70 s in the worst case; with this shorter timeout the control just keeps its default +# value and the subscription callback corrects it as soon as state arrives. +_WAIT_FOR_STATE_TIMEOUT = 2.0 + + class CameraWidget(BaseWidget, Ui_CameraWidget): signal_update_gui = QtCore.Signal() signal_new_image = QtCore.Signal(NewImageEvent, str) @@ -108,6 +116,26 @@ async def open( await self.add_to_sidebar(self.create_widget(TemperaturesWidget, module=self.module)) async def _init(self) -> None: + # every interface is initialized independently (caps -> state -> subscribe, in that + # order per interface), so run them concurrently instead of serially: a slow-publishing + # camera used to hold the page blank for up to ~70 s (7 interfaces x the 10 s default + # wait_for_state timeout); now each interface waits at most _WAIT_FOR_STATE_TIMEOUT and + # the others fill in regardless + await asyncio.gather( + self._init_once("window", self._init_window), + self._init_once("binning", self._init_binning), + self._init_once("gain", self._init_gain), + self._init_once("image_format", self._init_image_format), + self._init_once("image_type", self._init_image_type), + self._init_once("exposure", self._init_exposure), + self._init_once("exposure_time", self._init_exposure_time), + self._init_once("data_sequence", self._init_data_sequence), + ) + + # update GUI + self.signal_update_gui.emit() + + async def _init_window(self) -> None: # window window_caps = await self.comm.get_capabilities(self.module, IWindow) if window_caps is not None: @@ -117,7 +145,7 @@ async def _init(self) -> None: self.spinWindowHeight.setMaximum(int(window_caps.full_frame_height)) async with self.comm.safe_proxy(self.module, IWindow) as proxy: if proxy is not None: - state = await proxy.wait_for_state(IWindow) + state = await proxy.wait_for_state(IWindow, timeout=_WAIT_FOR_STATE_TIMEOUT) if state is not None: self.spinWindowLeft.setValue(state.x) self.spinWindowTop.setValue(state.y) @@ -129,6 +157,7 @@ async def _init(self) -> None: self.spinWindowWidth.setValue(window_caps.full_frame_width) self.spinWindowHeight.setValue(window_caps.full_frame_height) + async def _init_binning(self) -> None: # binning binning_caps = await self.comm.get_capabilities(self.module, IBinning) if binning_caps is not None: @@ -137,20 +166,22 @@ async def _init(self) -> None: self.comboBinning.addItems(binnings) async with self.comm.safe_proxy(self.module, IBinning) as proxy: if proxy is not None: - state = await proxy.wait_for_state(IBinning) + state = await proxy.wait_for_state(IBinning, timeout=_WAIT_FOR_STATE_TIMEOUT) if state is not None: self.comboBinning.setCurrentText(f"{state.x}x{state.y}") await self.comm.subscribe_state(self.module, IBinning, self._update_binning) + async def _init_gain(self) -> None: # gain async with self.comm.safe_proxy(self.module, IGain) as proxy: if proxy is not None: - state = await proxy.wait_for_state(IGain) + state = await proxy.wait_for_state(IGain, timeout=_WAIT_FOR_STATE_TIMEOUT) if state is not None: self.spinGain.setValue(state.gain) self.spinGainOffset.setValue(state.offset) await self.comm.subscribe_state(self.module, IGain, self._update_gain) + async def _init_image_format(self) -> None: # image format image_format_caps = await self.comm.get_capabilities(self.module, IImageFormat) if image_format_caps is not None: @@ -159,44 +190,45 @@ async def _init(self) -> None: self.comboImageFormat.addItems([f.name for f in image_formats]) async with self.comm.safe_proxy(self.module, IImageFormat) as proxy: if proxy is not None: - state = await proxy.wait_for_state(IImageFormat) + state = await proxy.wait_for_state(IImageFormat, timeout=_WAIT_FOR_STATE_TIMEOUT) if state is not None: self.comboImageFormat.setCurrentText(state.image_format.name) await self.comm.subscribe_state(self.module, IImageFormat, self._update_image_format) + async def _init_image_type(self) -> None: # image type async with self.comm.safe_proxy(self.module, IImageType) as proxy: if proxy is not None: - state = await proxy.wait_for_state(IImageType) + state = await proxy.wait_for_state(IImageType, timeout=_WAIT_FOR_STATE_TIMEOUT) if state is not None: self.comboImageType.setCurrentText(state.image_type.name) await self.comm.subscribe_state(self.module, IImageType, self._update_image_type) + async def _init_exposure(self) -> None: # exposure (status, progress, time left) async with self.comm.safe_proxy(self.module, IExposure) as proxy: if proxy is not None: - state = await proxy.wait_for_state(IExposure) + state = await proxy.wait_for_state(IExposure, timeout=_WAIT_FOR_STATE_TIMEOUT) if state is not None: self.exposure_status = state.status self.exposure_progress = state.progress self.exposure_time_left = state.exposure_time_left await self.comm.subscribe_state(self.module, IExposure, self._update_exposure) + async def _init_exposure_time(self) -> None: # exposure time async with self.comm.safe_proxy(self.module, IExposureTime) as proxy: if proxy is not None: - state = await proxy.wait_for_state(IExposureTime) + state = await proxy.wait_for_state(IExposureTime, timeout=_WAIT_FOR_STATE_TIMEOUT) if state is not None: self.spinExpTime.setValue(state.exposure_time) await self.comm.subscribe_state(self.module, IExposureTime, self._update_exposure_time) + async def _init_data_sequence(self) -> None: # data sequence if await self.comm.has_proxy(self.module, IDataSequence): await self.comm.subscribe_state(self.module, IDataSequence, self._update_sequence) - # update GUI - self.signal_update_gui.emit() - def _update_binning(self, state: BinningState): self.comboBinning.setCurrentText(f"{state.x}x{state.y}") diff --git a/pyobs_gui/mainwindow.py b/pyobs_gui/mainwindow.py index d63bdd2..e2859dd 100644 --- a/pyobs_gui/mainwindow.py +++ b/pyobs_gui/mainwindow.py @@ -1,4 +1,5 @@ import asyncio +import logging import os from typing import Optional, List, Any, Dict, Callable from PySide6 import QtWidgets, QtCore, QtGui # type: ignore @@ -29,7 +30,7 @@ IModule, ) -from .base import BaseWindow, BaseWidget +from .base import BaseWindow, BaseWidget, cancel_and_drain from .acquisitionwidget import AcquisitionWidget from .autofocuswidget import AutoFocusWidget from .autoguidingwidget import AutoGuidingWidget @@ -48,6 +49,8 @@ from .shellwidget import ShellWidget from .spectrographwidget import SpectrographWidget +log = logging.getLogger(__name__) + DEFAULT_WIDGETS = { ICamera: CameraWidget, ITelescope: TelescopeWidget, @@ -281,6 +284,11 @@ def __init__( # list of widgets self._widgets: Dict[str, BaseWidget] = {} + # client -> current page in the stacked widget (a "Loading…" placeholder until the + # widget's open() finished, then the widget itself) -- see _add_client/_open_client + self._pages: Dict[str, QtWidgets.QWidget] = {} + # client -> in-flight background open task (see _add_client) + self._pending_opens: Dict[str, asyncio.Task[None]] = {} self._current_widget = None self.shell: Optional[ShellWidget] = None self.events: Optional[EventsWidget] = None @@ -336,7 +344,7 @@ async def open(self, **kwargs: Any) -> None: # type: ignore self.listPages.currentRowChanged.connect(self._change_page) # get clients - await self._update_client_list() + self._update_clients_menu() await self._check_warnings() # subscribe to events @@ -351,9 +359,13 @@ async def open(self, **kwargs: Any) -> None: # type: ignore self.warning_task = asyncio.create_task(self._check_warning_task()) async def _init_clients(self) -> None: - # create other nav buttons and views - for client_name in self.comm.clients: - await self._client_connected(Event(), client_name) + # create other nav buttons and views -- in parallel, so a slow module (e.g. the + # telescope) no longer blocks every other module from appearing + await asyncio.gather(*(self._client_connected(Event(), c) for c in self.comm.clients)) + + # one fresh warning pass now that all modules are in (the periodic task keeps it + # current afterwards) + await self._check_warnings() def closeEvent(self, a0: QtGui.QCloseEvent) -> None: if self.warning_task is not None: @@ -377,6 +389,15 @@ async def discard_all_widgets(self) -> None: *before* this window is closed/deleted as part of a reconnect (GUI._logout()), otherwise a stray in-flight event/state callback can fire after Qt has already destroyed the widget it targets (e.g. "libshiboken: Internal C++ object already deleted").""" + # cancel and drain any in-flight background opens first, so no _open_client task is + # still mutating the stackedWidget / a widget after teardown begins -- cancel all + # before draining any, so they unwind concurrently instead of one after another + for task in self._pending_opens.values(): + task.cancel() + for task in list(self._pending_opens.values()): + await cancel_and_drain(task) + self._pending_opens.clear() + for widget in list(self._widgets.values()): await widget.discard() @@ -433,15 +454,91 @@ async def _add_client(self, client: str, icon: QtGui.QIcon, widget: BaseWidget) self.listPages.addItem(item) self.listPages.sortItems() - # open and add widget - await widget.open( - modules=[client] if client is not None else [], comm=self.comm, observer=self.observer, vfs=self.vfs - ) - self.stackedWidget.addWidget(widget) - - # store + # register immediately, so _change_page and the shortcuts always find the client -- + # even while the widget's open() is still running below self._widgets[client] = widget + # placeholder page: clickable now, explains the delay, needs no per-widget changes + placeholder = self._make_loading_page(client, icon) + self.stackedWidget.addWidget(placeholder) + self._pages[client] = placeholder + + # open in the background; swap in the real widget when done + self._pending_opens[client] = asyncio.create_task(self._open_client(client, widget)) + + async def _open_client(self, client: str, widget: BaseWidget) -> None: + try: + await widget.open( + modules=[client] if client is not None else [], comm=self.comm, observer=self.observer, vfs=self.vfs + ) + except Exception: + # open() failed (RPC errors etc.): tear the client down rather than leaving a + # permanent "Loading…" dead page behind. Note asyncio.CancelledError is NOT + # caught here (it subclasses BaseException), so a mid-open disconnect -- which + # cancels this task -- still unwinds through the finally below and never reaches + # this branch. + log.exception("Failed to open widget for %s", client) + await self._fail_open(client, widget) + return + finally: + self._pending_opens.pop(client, None) + + # swap placeholder -> real widget at the same stackedWidget index + placeholder = self._pages.get(client) + if placeholder is None: + return # client disconnected while opening + + # capture BEFORE removeWidget(placeholder): once the placeholder is removed from the + # stack it can never again be currentWidget(), so the check has to happen first or the + # "user is sitting on this page" branch below can never fire + was_current = self.stackedWidget.currentWidget() is placeholder + + idx = self.stackedWidget.indexOf(placeholder) + self.stackedWidget.removeWidget(placeholder) + placeholder.deleteLater() + self.stackedWidget.insertWidget(idx, widget) + self._pages[client] = widget + + # if the user is sitting on this page, show the real widget now -- showEvent -> + # _init() runs and the content fills in + if was_current: + self.stackedWidget.setCurrentWidget(widget) + + async def _fail_open(self, client: str, widget: BaseWidget) -> None: + """Undo a failed _add_client: nav item, placeholder page, and registry entries. + No-op if the client already disconnected mid-open (its own teardown handled it).""" + if self._pages.get(client) is None: + return + self._widgets.pop(client, None) + for row in range(self.listPages.count()): + if self.listPages.item(row).text() == client: + self.listPages.takeItem(row) + break + placeholder = self._pages.pop(client, None) + if placeholder is not None: + if self.stackedWidget.currentWidget() is placeholder: + self._current_widget = None + self.stackedWidget.removeWidget(placeholder) + placeholder.deleteLater() + await widget.discard() + + def _make_loading_page(self, client: str, icon: QtGui.QIcon) -> QtWidgets.QWidget: + """Returns a plain "Loading …" page (module icon + grey label) that stands in + for the real widget while its open() is still running. No new dependency.""" + page = QtWidgets.QWidget() + layout = QtWidgets.QVBoxLayout(page) + layout.addStretch() + icon_label = QtWidgets.QLabel() + icon_label.setPixmap(icon.pixmap(48, 48)) + icon_label.setAlignment(QtCore.Qt.AlignmentFlag.AlignCenter) + layout.addWidget(icon_label) + label = QtWidgets.QLabel(f"Loading {client}…") + label.setAlignment(QtCore.Qt.AlignmentFlag.AlignCenter) + label.setStyleSheet("color: grey;") + layout.addWidget(label) + layout.addStretch() + return page + @QtCore.Slot(int) # type: ignore def _change_page(self, idx: int) -> None: """Change page. @@ -454,12 +551,13 @@ def _change_page(self, idx: int) -> None: item = self.listPages.item(idx) client = item.text() if item is not None else None - # section headers (and an empty selection) aren't real pages - if client not in self._widgets: + # section headers (and an empty selection) aren't real pages -- they are in neither + # self._pages nor self._widgets + if client not in self._pages: return - # change to new page - self.stackedWidget.setCurrentWidget(self._widgets[client]) + # change to new page (may be a placeholder while the widget is still opening) + self.stackedWidget.setCurrentWidget(self._pages[client]) # get new widget self._current_widget = self.stackedWidget.currentWidget() @@ -523,8 +621,10 @@ def _recall_slot(self, slot: str) -> None: return self._select_page_by_name(name) - async def _update_client_list(self) -> None: - """Updates the select-clients menu for the log.""" + def _update_clients_menu(self) -> None: + """Rebuilds the select-clients menu for the log -- cheap and purely local, so it can run + per connect/disconnect. (The Shell command model is the Shell widget's own job -- it + rebuilds debounced and only while its page is visible, see shellwidget.py.)""" # rebuild the menu -- every connected client, checked (shown) by default. A checkbox # inside a QWidgetAction (rather than a plain checkable QAction) both lets the entry's @@ -544,10 +644,6 @@ async def _update_client_list(self) -> None: action.setDefaultWidget(checkbox) self._clients_menu.addAction(action) - # update shell - if self.shell is not None: - await self.shell.update_client_list() - def _clear_log(self) -> None: self.log_model.clear() @@ -648,8 +744,8 @@ async def _client_connected(self, event: Event, client: str) -> bool: if client in self._widgets: return False - # update client list - await self._update_client_list() + # update client list (cheap menu rebuild; the shell model is the shell's own job) + self._update_clients_menu() # what do we have? async with self.comm.proxy(client) as proxy: @@ -684,9 +780,6 @@ async def _client_connected(self, event: Event, client: str) -> bool: self._add_section_header("Modules") self._modules_header_added = True await self._add_client(client, icon, widget) - - # check mastermind - await self._check_warnings() return True async def _client_disconnected(self, event: Event, client: str) -> bool: @@ -696,20 +789,39 @@ async def _client_disconnected(self, event: Event, client: str) -> bool: client: Name of client. """ - # update client list - await self._update_client_list() + # update client list (cheap menu rebuild; the shell model is the shell's own job) + self._update_clients_menu() # not in list? if client not in self._widgets: return False + # cancel a pending open BEFORE discard(), and await the cancellation: cancel() is not + # synchronous (the coroutine only unwinds at its next await inside widget.open()), so + # without this await, widget.discard() below could run concurrently with the tail of a + # not-yet-finished open() -- e.g. sidebar widgets added after discard, or handlers + # re-registered after unregister. This race is newly exposed by registering the widget + # early in _add_client: before, the widget wasn't in self._widgets mid-open, so this + # method returned early and discard() could never run while open() was in flight. + task = self._pending_opens.pop(client, None) + if task is not None: + await cancel_and_drain(task) + # get widget widget = self._widgets[client] - # is current? - if self.stackedWidget.currentWidget() == widget: + # is current? (the placeholder may be the current page mid-open) + if self.stackedWidget.currentWidget() in (widget, self._pages.get(client)): self._current_widget = None + # remove placeholder page, if any -- a dict pop alone would leave a ghost QWidget in + # the stack, and if it was the current page it would stay visible after the module + # vanished + placeholder = self._pages.pop(client, None) + if placeholder is not None: + self.stackedWidget.removeWidget(placeholder) + placeholder.deleteLater() + # remove widget self.stackedWidget.removeWidget(widget) @@ -726,9 +838,6 @@ async def _client_disconnected(self, event: Event, client: str) -> bool: # remove from dict del self._widgets[client] - - # check mastermind - await self._check_warnings() return True def get_fits_headers(self, namespaces: Optional[List[str]] = None, **kwargs: Any) -> dict[str, FitsHeaderEntry]: diff --git a/pyobs_gui/shellwidget.py b/pyobs_gui/shellwidget.py index e6dd2a5..926788b 100644 --- a/pyobs_gui/shellwidget.py +++ b/pyobs_gui/shellwidget.py @@ -1,7 +1,7 @@ import asyncio import re from typing import Any -from PySide6 import QtWidgets, QtCore # type: ignore +from PySide6 import QtWidgets, QtCore, QtGui # type: ignore import inspect from enum import Enum import logging @@ -14,7 +14,6 @@ from .base import BaseWidget from .qt.shellwidget_ui import Ui_ShellWidget - log = logging.getLogger(__name__) @@ -113,6 +112,13 @@ def __init__(self, **kwargs: Any): # commands self.command_model: CommandModel | None = None self.completer: QtWidgets.QCompleter | None = None + # True when the command model may not reflect comm.clients (module events arrived while + # the Shell page was hidden); the next showEvent triggers a rebuild + self._model_stale = False + # debounced rebuild task -- one per burst of module events (see _module_changed) + self._model_debounce_task: asyncio.Task[Any] | None = None + # serializes CommandModel.init() runs, so two rebuilds can never interleave + self._model_lock = asyncio.Lock() self.command_regexp = re.compile(r"(\w+)\.(\w+[_\w+]*)\(([^\)]*)\)") self.args_regexp = re.compile(r'(?:[^\s,"]|"(?:\\.|[^"])*")+') @@ -158,12 +164,18 @@ async def open( table_view.setEditTriggers(QtWidgets.QAbstractItemView.EditTrigger.NoEditTriggers) # await self.command_model.init() - await self.update_client_list() + await self.update_client_list(force=True) if self.comm is not None: await self.comm.register_event(ModuleOpenedEvent, self._module_changed) await self.comm.register_event(ModuleClosedEvent, self._module_changed) + def showEvent(self, event: QtGui.QShowEvent) -> None: + super().showEvent(event) + # rebuild the command model lazily if module changes arrived while this page was hidden + if self._model_stale: + asyncio.create_task(self.update_client_list()) + def _add_command_log(self, msg: str, color: str | None = None) -> None: if color is not None: msg = '%s' % (color, msg) @@ -201,11 +213,35 @@ def _update_docs(self) -> None: doc = "" async def _module_changed(self, event: Event, sender: str) -> bool: - asyncio.create_task(self.update_client_list()) + # debounce: restart the short delay on every module event, so a burst of + # connects/disconnects triggers exactly one rebuild + if self._model_debounce_task is not None: + self._model_debounce_task.cancel() + self._model_debounce_task = asyncio.create_task(self._debounced_rebuild()) return True - async def update_client_list(self) -> None: + async def _debounced_rebuild(self) -> None: + try: + await asyncio.sleep(0.5) + except asyncio.CancelledError: + return + await self.update_client_list() + + async def update_client_list(self, force: bool = False) -> None: + """Rebuilds the command model from the current comm.clients. + + Skipped (model marked stale) when the Shell page isn't visible, unless force=True -- + open() forces the initial build, and the page's own showEvent triggers the rebuild + later. The lock serializes concurrent rebuilds so two CommandModel.init() runs can + never interleave. + """ # create model for commands - if self.command_model is not None and self.completer is not None: + if self.command_model is None or self.completer is None: + return + if not force and not self.isVisible(): + self._model_stale = True + return + async with self._model_lock: await self.command_model.init() self.completer.setModel(self.command_model) + self._model_stale = False diff --git a/pyobs_gui/telescopewidget.py b/pyobs_gui/telescopewidget.py index 771737b..ad01ec2 100644 --- a/pyobs_gui/telescopewidget.py +++ b/pyobs_gui/telescopewidget.py @@ -1,3 +1,4 @@ +import asyncio from enum import Enum from typing import Any @@ -199,15 +200,31 @@ async def open(self, **kwargs: Any) -> None: self.select_coord_type() async def _init(self) -> None: - await self.comm.subscribe_state(self.module, IMotion, self._on_motion_state) + # fire the (up to five) subscribe_state calls concurrently -- each is independent, and + # the comm's subscribe_state returns immediately anyway, so this mainly removes the + # sequential await hops + calls = [self._init_once("motion", self.comm.subscribe_state, self.module, IMotion, self._on_motion_state)] if IPointingRaDec in self._interfaces: - await self.comm.subscribe_state(self.module, IPointingRaDec, self._on_radec_state) + calls.append( + self._init_once("radec", self.comm.subscribe_state, self.module, IPointingRaDec, self._on_radec_state) + ) if IPointingAltAz in self._interfaces: - await self.comm.subscribe_state(self.module, IPointingAltAz, self._on_altaz_state) + calls.append( + self._init_once("altaz", self.comm.subscribe_state, self.module, IPointingAltAz, self._on_altaz_state) + ) if IOffsetsRaDec in self._interfaces: - await self.comm.subscribe_state(self.module, IOffsetsRaDec, self._on_offsets_radec_state) + calls.append( + self._init_once( + "offsets_radec", self.comm.subscribe_state, self.module, IOffsetsRaDec, self._on_offsets_radec_state + ) + ) if IOffsetsAltAz in self._interfaces: - await self.comm.subscribe_state(self.module, IOffsetsAltAz, self._on_offsets_altaz_state) + calls.append( + self._init_once( + "offsets_altaz", self.comm.subscribe_state, self.module, IOffsetsAltAz, self._on_offsets_altaz_state + ) + ) + await asyncio.gather(*calls) # ------------------------------------------------------------------------- # State callbacks diff --git a/tests/test_mainwindow_startup.py b/tests/test_mainwindow_startup.py new file mode 100644 index 0000000..a595278 --- /dev/null +++ b/tests/test_mainwindow_startup.py @@ -0,0 +1,322 @@ +import asyncio +from typing import Any + +import pytest +from PySide6 import QtGui + +from pyobs.events import Event +from pyobs.interfaces import IMotion, IPointingAltAz, IPointingRaDec, IOffsetsAltAz, IOffsetsRaDec +from pyobs_gui.base import BaseWidget +from pyobs_gui.mainwindow import MainWindow +from pyobs_gui.telescopewidget import TelescopeWidget + + +class _FakeComm: + """Minimal comm for MainWindow startup tests: a client list, no-op event registration, and + empty interface/autonomous/weather lookups.""" + + def __init__(self, clients: list[str] | None = None): + self.clients: list[str] = [] if clients is None else clients + + async def clients_with_interface(self, interface: Any) -> list[str]: + return [] + + async def register_event(self, event_class: Any, handler: Any) -> None: + pass + + async def unregister_event(self, event_class: Any, handler: Any) -> None: + pass + + +class _NoOpComm: + """Comm surface ShellWidget.open()/CommandModel.init() touch: empty client list, no-op + events.""" + + clients: list[str] = [] + + async def get_interfaces(self, client: str) -> list[Any]: + return [] + + async def register_event(self, event_class: Any, handler: Any) -> None: + pass + + +class _BlockingSubscribeComm: + """Records subscribe_state() calls and blocks until release -- proves asyncio.gather() + starts all subscriptions before any of them completes.""" + + def __init__(self): + self.calls: list[Any] = [] + self.release = asyncio.Event() + + async def subscribe_state(self, module: str, interface: Any, callback: Any) -> None: + self.calls.append(interface) + await self.release.wait() + + +class _SlowOpenWidget(BaseWidget): + """BaseWidget whose open() blocks until release is set -- for exercising the placeholder / + background-open / swap machinery without any real comm traffic.""" + + def __init__(self): + super().__init__() + self.release = asyncio.Event() + self.opened = False + self.discarded = False + self._init_calls = 0 + + # pyrefly: ignore [bad-override] + async def open(self, **kwargs: Any) -> None: + self.opened = True + await self.release.wait() + + async def _init(self) -> None: + self._init_calls += 1 + await asyncio.sleep(0.01) + + async def discard(self) -> None: + self.discarded = True + await super().discard() + + +class _FailingOpenWidget(_SlowOpenWidget): + # pyrefly: ignore [bad-override] + async def open(self, **kwargs: Any) -> None: + raise RuntimeError("boom") + + +class _SlowInitWidget(BaseWidget): + def __init__(self): + super().__init__() + self._init_calls = 0 + + async def _init(self) -> None: + self._init_calls += 1 + await asyncio.sleep(0.01) + + +class _FlakyInitWidget(BaseWidget): + """_init() raises on its first call, succeeds afterwards -- covers the retry-on-failure + semantics of BaseWidget._showEvent.""" + + def __init__(self): + super().__init__() + self._init_calls = 0 + self._fail_first = True + + async def _init(self) -> None: + self._init_calls += 1 + if self._fail_first: + self._fail_first = False + raise RuntimeError("boom") + + +def _make_window(clients: list[str] | None = None) -> MainWindow: + window = MainWindow(show_shell=False, show_events=False, show_status=False) + window._comm = _FakeComm(clients=clients) # pyrefly: ignore [bad-assignment] + return window + + +@pytest.mark.asyncio +async def test_add_client_placeholder_swaps_when_open_finishes(qapp) -> None: + """The nav item, registry entry, and a clickable "Loading…" page exist the moment + _add_client returns; once open() finishes the real widget replaces the placeholder and, + if the page was current, becomes current (and _init fires).""" + window = _make_window() + window.show() # so the real widget's showEvent -> _init fires once it becomes current + try: + widget = _SlowOpenWidget() + client = "telescope" + + await window._add_client(client, QtGui.QIcon(), widget) + + item = window.listPages.item(0) + assert item is not None and item.text() == client + assert window._widgets[client] is widget + assert client in window._pending_opens + placeholder = window._pages[client] + assert placeholder is not widget + + # clicking the nav row switches to the placeholder instantly -- the click is not dropped + window._change_page(0) + assert window.stackedWidget.currentWidget() is placeholder + + # finish the open: same stackedWidget index, real widget current, _init ran + task = window._pending_opens[client] + widget.release.set() + await task + + assert window._pages[client] is widget + assert window.stackedWidget.currentWidget() is widget + await asyncio.sleep(0.05) # let the showEvent -> _init task run + assert widget._init_calls == 1 + finally: + window.deleteLater() + + +@pytest.mark.asyncio +async def test_open_failure_removes_client(qapp) -> None: + """A failing open() must tear the client down (nav item, placeholder, registry) instead of + leaving a permanent "Loading…" page.""" + window = _make_window() + try: + widget = _FailingOpenWidget() + client = "camera" + + await window._add_client(client, QtGui.QIcon(), widget) + task = window._pending_opens[client] + await task # open() raises -> _fail_open tears the client down + + assert client not in window._widgets + assert client not in window._pages + assert widget.discarded + assert window.stackedWidget.count() == 0 # no "Loading…" page left behind + assert window.listPages.count() == 0 + finally: + window.deleteLater() + + +@pytest.mark.asyncio +async def test_disconnect_mid_open_tears_down_without_ghost(qapp) -> None: + """Disconnecting while open() is still in flight cancels the pending open (and awaits it) + before discarding, and removes the placeholder from the stack -- no ghost page.""" + window = _make_window() + try: + widget = _SlowOpenWidget() + client = "telescope" + + await window._add_client(client, QtGui.QIcon(), widget) + window._change_page(0) # placeholder is the current page + + assert await window._client_disconnected(Event(), client) + + assert client not in window._widgets + assert client not in window._pages + assert widget.discarded + assert not widget.release.is_set() # open() was cancelled, never completed + assert window.stackedWidget.count() == 0 # placeholder removed from the stack + assert window.listPages.count() == 0 + finally: + window.deleteLater() + + +@pytest.mark.asyncio +async def test_discard_all_widgets_drains_pending_opens(qapp) -> None: + """discard_all_widgets() (logout path) must cancel and drain in-flight open tasks before + discarding widgets, so no background task mutates the stack during teardown.""" + window = _make_window() + try: + widget = _SlowOpenWidget() + await window._add_client("telescope", QtGui.QIcon(), widget) + + await window.discard_all_widgets() + + assert window._pending_opens == {} + assert widget.discarded + assert not widget.release.is_set() # open was cancelled, never completed + finally: + window.deleteLater() + + +@pytest.mark.asyncio +async def test_init_clients_runs_in_parallel(qapp, monkeypatch) -> None: + """_init_clients must start every client's connect before any of them completes (event + ordering, not wall-clock).""" + window = _make_window(clients=["camera", "telescope"]) + try: + started: list[str] = [] + release = asyncio.Event() + + async def fake_connected(event: Event, client: str) -> bool: + started.append(client) + await release.wait() + return True + + monkeypatch.setattr(window, "_client_connected", fake_connected) + + task = asyncio.create_task(window._init_clients()) + for _ in range(100): + if len(started) == 2: + break + await asyncio.sleep(0.01) + assert started == ["camera", "telescope"] # both in flight before either completes + + release.set() + await task + finally: + window.deleteLater() + + +@pytest.mark.asyncio +async def test_telescope_init_subscribes_all_interfaces_in_parallel(qapp) -> None: + """TelescopeWidget._init must fire all five subscribe_state calls concurrently (all in + flight before any completes).""" + widget = TelescopeWidget(module="telescope") + widget.modules = ["telescope"] + widget._interfaces = [IMotion, IPointingRaDec, IPointingAltAz, IOffsetsRaDec, IOffsetsAltAz] + comm = _BlockingSubscribeComm() + widget._comm = comm # pyrefly: ignore [bad-assignment] + try: + task = asyncio.create_task(widget._init()) + for _ in range(100): + if len(comm.calls) == 5: + break + await asyncio.sleep(0.01) + assert len(comm.calls) == 5 + assert comm.calls[0] is IMotion + + comm.release.set() + await task + finally: + widget.deleteLater() + + +@pytest.mark.asyncio +async def test_show_event_runs_init_once_under_concurrent_shows(qapp) -> None: + """Two rapid show events must not run _init() twice (memoized init task).""" + widget = _SlowInitWidget() + try: + event = QtGui.QShowEvent() + await asyncio.gather(widget._showEvent(event), widget._showEvent(event)) + assert widget._init_calls == 1 + finally: + widget.deleteLater() + + +@pytest.mark.asyncio +async def test_show_event_retries_init_after_failure(qapp) -> None: + """A failing _init() leaves the widget un-initialized so the next show retries -- it must + not be permanently marked initialized.""" + widget = _FlakyInitWidget() + try: + event = QtGui.QShowEvent() + await widget._showEvent(event) # fails; logged, task cleared + assert widget._initialized is False + assert widget._init_task is None + + await widget._showEvent(event) # retries and succeeds + assert widget._initialized is True + assert widget._init_calls == 2 + finally: + widget.deleteLater() + + +@pytest.mark.asyncio +async def test_shell_rebuild_gated_on_visibility_and_lazy_on_show(qapp) -> None: + """The Shell command model must not rebuild while the page is hidden (only marked stale); + showing the page triggers the lazy rebuild.""" + from pyobs_gui.shellwidget import ShellWidget + + shell = ShellWidget() + try: + await shell.open(comm=_NoOpComm(), observer=None, vfs=None) # pyrefly: ignore [bad-argument-type] + + assert not shell.isVisible() + await shell.update_client_list() + assert shell._model_stale is True # skipped, not rebuilt + + shell.show() + await asyncio.sleep(0.05) # let the showEvent-triggered rebuild run + assert shell._model_stale is False + finally: + shell.deleteLater()