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/pyobs_gui/videowidget.py b/pyobs_gui/videowidget.py index cd73c3a..56e13eb 100644 --- a/pyobs_gui/videowidget.py +++ b/pyobs_gui/videowidget.py @@ -81,6 +81,10 @@ def __init__(self, **kwargs: Any): self.socket: QtNetwork.QAbstractSocket | None = None self.scheme: str | None = None + # Authorization header for the raw-socket stream request, taken from the + # HttpFile the widget opens in _init (None when the VFS root configures no token) + self._auth_header: str | None = None + # whether the HTTP response headers of the current stream connection have # been stripped from self.buffer yet (see _received_data) self._headers_received = False @@ -141,6 +145,10 @@ async def _init(self) -> None: log.error("VFS path to video of module %s must be an HttpFile.", self.module) return + # keep the Authorization header (Bearer token) for the raw-socket stream request -- + # HttpFile sends it itself for VFS reads, but the stream bypasses HttpFile entirely + self._auth_header = video_file.headers.get("Authorization") + # parse URL o = urlparse(video_file.url) if o.scheme not in ["http", "https"]: @@ -204,7 +212,9 @@ async def _showEvent(self, event: QtGui.QShowEvent) -> None: # as the plain MJPEG byte stream (with Connection: close, which just means the # stream runs until the client disconnects). self.socket.write( - b"GET %s HTTP/1.0\r\nHost: %s\r\n\r\n" % (bytes(self.path, "UTF-8"), bytes(host_header, "UTF-8")) + b"GET %s HTTP/1.0\r\nHost: %s\r\n" % (bytes(self.path, "UTF-8"), bytes(host_header, "UTF-8")) + + (b"Authorization: %s\r\n" % bytes(self._auth_header, "UTF-8") if self._auth_header else b"") + + b"\r\n" ) # new connection: the next bytes start with the HTTP response headers self._headers_received = False diff --git a/pyproject.toml b/pyproject.toml index 155cd94..e945588 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "pyobs-gui" -version = "2.0.0.dev19" +version = "2.0.0.dev20" description = "A remote GUI for pyobs" authors = [{ name = "Tim-Oliver Husser", email = "thusser@uni-goettingen.de" }] requires-python = ">=3.11" @@ -12,7 +12,7 @@ dependencies = [ "sunpy[all]>=7.0.1", "QtAwesome>=1.4.0,<2", "qfitswidget>=0.13.1", - "pyobs-core>=2.0.0.dev78,<3", + "pyobs-core>=2.0.0.dev93,<3", "pyside6>=6.10.1", "astroplan>=0.10.1", "astropy>=7.0.1,<8", diff --git a/specs/2026-08-21-gui-widget-startup-responsiveness.md b/specs/2026-08-21-gui-widget-startup-responsiveness.md new file mode 100644 index 0000000..3d487fd --- /dev/null +++ b/specs/2026-08-21-gui-widget-startup-responsiveness.md @@ -0,0 +1,397 @@ +# Plan: pyobs-gui — make module widgets appear and respond immediately at startup + +Status: implemented, closed 2026-08-23 (PR #141, `123161b`) +Audited: 2026-08-21 + +## Problem + +When pyobs-gui starts (or a new module connects), the module's nav-list entry appears in the +sidebar **before** the widget behind it is ready, and there is no loading feedback anywhere: + +1. **Clicks are silently dropped.** `MainWindow._add_client()` (`mainwindow.py:415`) adds the + nav item first, then `await widget.open(...)`, and only afterwards adds the widget to the + `stackedWidget` and registers it in `self._widgets`. `_change_page()` (`mainwindow.py:445`) + early-returns when `client not in self._widgets`, so clicking a module name right after it + appears does nothing at all — the row highlights, the page never changes, and the user has to + click again later with no hint. +2. **The widget is blank and disabled until state arrives.** No widget passes `update_func`, so + every widget is purely state-subscription driven. Content is fetched only on first show + (`BaseWidget.showEvent` → `_init()`, `base.py:261-268`), which is deferred until the page is + clicked. Until the first state callback, widgets are `setEnabled(False)` and show `N/A`/empty + fields (e.g. `TelescopeWidget.__init__` disables itself, `telescopewidget.py:77`, and only + `update_gui()`, triggered by state callbacks, re-enables it). +3. **The telescope is the worst case.** Its `open()` chain is the heaviest in the codebase — + compass widget + `get_interfaces` + ACL `get_permitted_methods` + up to three sidebar widgets + (Filter/Focus/Temperatures, each opened) + a `get_capabilities(IModule)` observer fallback in + login/standalone mode — all sequential (`telescopewidget.py:142-199`). On first click its + `_init()` then fires five sequential `subscribe_state` calls (`:201-210`). Camera is similar: + `_init()` has several `wait_for_state()` calls with the default **10 s timeout** + (`interface.py:53-54`, e.g. `camerawidget.py:120,140,148,162,170,178,188`). +4. **Startup itself is serialized.** `_init_clients()` (`mainwindow.py:353`) awaits + `_client_connected()` for each module sequentially, and each call additionally rebuilds the + Shell command model over **all** clients (`_update_client_list` → `CommandModel.init`, + `shellwidget.py:40` — O(N) per module, O(N²) total, with synchronous interface introspection on + the UI thread) and re-scans all clients for autonomous/weather modules (`_check_warnings`). + +## Current state (audited 2026-08-21) + +- `mainwindow.py:415-443` — `_add_client()`: nav item added before `widget.open()`; widget added + to `stackedWidget` and `self._widgets` only afterwards. +- `mainwindow.py:445-465` — `_change_page()`: silent `return` for clients not yet in `_widgets`. +- `mainwindow.py:353-356` — `_init_clients()`: strictly sequential per-client awaits. +- `mainwindow.py:626-690` — `_client_connected()`: per-module chain includes + `await self._update_client_list()` (Shell command-model rebuild over all clients) and + `await self._check_warnings()` (all-clients scan ×2). +- `base.py:261-287` — `showEvent` → `_init()` once; content is subscription-driven, no `update_func` + is used by any widget; `_update_loop` (`base.py:289`) never actually runs. +- `telescopewidget.py:142-210`, `camerawidget.py:59-198` — heavy sequential `open()`/`_init()` + chains (see Problem §3). +- No loading indicator exists anywhere in `pyobs_gui/`. + +## Design + +### 1. `_add_client()` becomes non-blocking: placeholder page + background open + +The nav item, the page, and the widget registry all exist the moment the module connects; only the +remote work is deferred: + +```python +# mainwindow.py — new state +self._pages: Dict[str, QtWidgets.QWidget] = {} # client -> current page (placeholder or real) +self._pending_opens: Dict[str, asyncio.Task[None]] = {} # client -> in-flight open task +``` + +```python +async def _add_client(self, client: str, icon: QtGui.QIcon, widget: BaseWidget) -> None: + # nav item first, exactly as today + item = PagesListWidgetItem() + item.setIcon(icon) + item.setText(client) + self.listPages.addItem(item) + self.listPages.sortItems() + + # register immediately so _change_page and the shortcuts always find the client + self._widgets[client] = widget + + # placeholder page: clickable now, explains the delay, 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() +``` + +`_make_loading_page(client, icon)` returns a plain `QWidget` with a centered vertical layout: the +module's icon and a grey "Loading …" label. No new dependency (no spinner package). + +`_change_page()` switches on `self._pages[client]` instead of `self._widgets[client]`, so a click +during `open()` switches to the placeholder instantly — **the click is never dropped**, and the +"Loading…" text tells the user why the content is not there yet. Section headers still fall out +naturally: they are in neither map. + +`_client_disconnected()` (`mainwindow.py:692`) gains a cancel-and-await of the pending open task +and placeholder teardown (see §3). + +### 2. Parallel initial discovery + +```python +async def _init_clients(self) -> None: + await asyncio.gather(*(self._client_connected(Event(), c) for c in self.comm.clients)) +``` + +Each `_client_connected` now returns after its placeholder is registered; the per-widget `open()` +chains — the dominant startup cost (telescope/camera) — run concurrently as their own tasks (see +§1). Still inline per client: the ACL `get_permitted_methods` fetch, the `comm.proxy()` +interface-detection loop, and any custom-sidebar `add_to_sidebar()` opens — but those are now +concurrent across clients via the gather instead of serialized. Later connects (ModuleOpenedEvent +handlers) are already dispatched as separate tasks by the comm layer (comm.py:687-692), so they +stay concurrent too. + +### 3. Remove the redundant per-connect global work from `_client_connected` / `_client_disconnected` + +- **Drop `await self._update_client_list()` and `await self._check_warnings()` from + `_client_connected()`, and the per-disconnect `_check_warnings()` from `_client_disconnected()`.** + Warnings are already covered by the periodic `_check_warning_task()` (`mainwindow.py:594`, every + 5 s) plus the initial call in `open()`; add one `_check_warnings()` after the `_init_clients` + gather so the state is fresh immediately at startup. This kills the O(N²) Shell command-model + rebuild at startup and the duplicate all-clients scans per module. +- **Keep the log-filter client menu current cheaply**: `_update_client_list()` is split into a + sync `_update_clients_menu()` (the `_clients_menu` rebuild — cheap, purely local) that stays + called per connect/disconnect, and the `shell.update_client_list()` → `CommandModel.init()` half + that is dropped from the per-connect path entirely. The Shell widget rebuilds its own model via + its `ModuleOpenedEvent`/`ModuleClosedEvent` handler (`shellwidget.py:203`), debounced and gated + on the Shell page being visible — see the `shellwidget.py` bullet in §5, which is a **required + dependency of this section, not a fast-follow**: without the shell-side gate, the O(N²) rebuild + still happens on every module change regardless of what the mainwindow does. +- `_client_disconnected()` reworked — cancel **and await** the pending open before discarding, and + remove the placeholder page from the stack (a dict pop alone would leave a ghost QWidget in the + stack, and if the placeholder was the current page it would stay visible after the module + vanished): + ```python + self._update_clients_menu() # cheap, local; the shell model is the shell's own job now + + 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 this plan: registering + # the widget early (see §1) means _client_disconnected() no longer early-returns while + # open() is in flight. Today the widget isn't in self._widgets mid-open, so discard() + # can never run concurrently with open() at all — it is not pre-existing. + task = self._pending_opens.pop(client, None) + if task is not None: + task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await task + + widget = self._widgets[client] + + # 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 + + placeholder = self._pages.pop(client, None) + if placeholder is not None: + self.stackedWidget.removeWidget(placeholder) + placeholder.deleteLater() + self.stackedWidget.removeWidget(widget) + + # ... existing nav-item removal and widget.discard() / del self._widgets[client] unchanged + ``` +- **`discard_all_widgets()` (`mainwindow.py:375`) must also cancel `self._pending_opens`.** It + exists specifically to stop async callbacks from firing after Qt starts tearing down widgets + during `GUI._logout()`'s reconnect flow ("libshiboken: Internal C++ object already deleted"). + The placeholder design adds exactly the kind of background task that can trigger this: an + `_open_client` task can still be running — touching `self.stackedWidget` and the widget it's + about to swap in — when logout starts. Add, at the top of `discard_all_widgets()`: + ```python + for task in self._pending_opens.values(): + task.cancel() + for task in list(self._pending_opens.values()): + with contextlib.suppress(asyncio.CancelledError): + await task + self._pending_opens.clear() + ``` + before the existing per-widget `discard()` loop, so no `_open_client` task is still mutating + `stackedWidget`/a widget after teardown begins. + +### 4. Harden `_showEvent` against the pre-existing double-init race + +Two rapid show/hide/show cycles can spawn two concurrent `_showEvent` tasks, both seeing +`_initialized is False` and both running `_init()` → duplicate subscriptions. Memoize the init run: + +```python +async def _showEvent(self, event: QtGui.QShowEvent) -> None: + if not self._initialized and hasattr(self, "_init"): + 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 + +async def _run_init(self) -> None: + await self._init() + self._initialized = True +``` + +`self._init_task: asyncio.Task[Any] | None = None` must be added to `BaseWidget.__init__` — it +does not exist today, so the snippet's `is None` check would otherwise AttributeError on the very +first show. Failure semantics are preserved from today: a raising `_init()` leaves `_initialized` +False and `_init_task` cleared, so the next show retries — the naive +`finally: self._initialized = True` would instead mark a half-initialized widget done forever and +leave the exception unretrieved in the memoized task. + +(Small, isolated; the placeholder design already guarantees `_init()` only ever runs after +`open()` finished, since the real widget is only inserted/shown post-open — no new +open/init concurrency is introduced.) + +### 5. Widget cleanups (shell = required dependency of §3; telescope/camera = fast-follows) + +- `shellwidget.py` — debounce `CommandModel.init()` rebuilds and only rebuild when the Shell page + is visible, instead of on every module connect. **Required for §3's cost claim, same PR**: + `ShellWidget._module_changed` (`shellwidget.py:203`) fires on every module open/close regardless + of visibility and rebuilds unconditionally, so gating only the mainwindow side would leave the + O(N²) rebuilds running anyway. Concretely: skip the rebuild when `self.isVisible()` is False (a + stacked-page widget is hidden whenever it isn't the current page), debounce rapid module events + (a short timer/sleep that resets on each event), and rebuild on first show — e.g. a `showEvent` + override that rebuilds when the model is stale — so a Shell page that was never opened isn't left + with an empty command list. +- `telescopewidget.py:_init()` — fire the five `subscribe_state` calls concurrently with + `asyncio.gather` (each is independent; XmppComm's `_subscribe_state` already returns + immediately and subscribes in background tasks, so this mainly removes the sequential await + hops). Fast-follow, same PR or next. +- `camerawidget.py:_init()` — pass an explicit short `timeout=` to each `wait_for_state()` + (e.g. 2 s instead of the 10 s default) and gather the independent capability/state fetches, so a + slow-publishing camera can't hold the page blank for ~70 s in the worst case. Two notes: keep + each interface's caps → state → subscribe ordering intact inside the gather (they populate the + same control, e.g. `comboBinning` is filled from capabilities before its current value is set + from state); and with a short timeout the widget briefly shows default values for interfaces + that haven't published yet — acceptable, the subscription callback corrects them as soon as + state arrives. Fast-follow, same PR or next. + +## Decisions + +- **Placeholder page, not "register early + show the half-built widget".** Showing the real widget + before `open()` finishes would render a half-built UI (empty combo boxes, missing sidebar) and + reintroduce an open/`_init` race (e.g. `TelescopeWidget._init` reads `self._interfaces`, which + `open()` populates). A dedicated "Loading…" page keeps per-widget code untouched and makes the + delay self-explanatory. +- **Loading indicator = simple centered label + module icon.** No new dependency (no + `QProgressIndicator` package); the label is replaced by the real widget the moment `open()` + completes. +- **`_update_client_list` / `_check_warnings` per connect: dropped.** The periodic warning task + plus one post-gather call covers warnings; the Shell command model rebuild is gated on + visibility. Per-connect O(N) work was the dominant startup cost behind the serialized + `_init_clients`. +- **Pre-warming `_init()` at open time is explicitly out of scope.** It would make first-click + content instant but adds state subscriptions for pages the user may never open (traffic and + churn on every module the GUI is subscribed to). Revisit later if first-click latency after the + placeholder swap still feels slow. +- **Open failure = teardown, not a permanent "Loading…" page.** If `widget.open()` raises, the + client is removed (nav item, placeholder, registry entries) and the widget discarded (§1 + `_fail_open`). Today the same failure leaves a nav item that silently does nothing on click; the + new behavior must not trade that for an equally dead page that *looks* intentional. + +## Per-file change list + +All files under `pyobs_gui/` (plus `tests/`): + +1. `mainwindow.py` — `_add_client` restructure (§1: placeholder + background open + swap + + `_fail_open` teardown), `_change_page` → `self._pages`, `_init_clients` → `asyncio.gather` (§2), + drop per-connect `_update_client_list`/`_check_warnings` and add the post-gather call, extract + sync `_update_clients_menu()` (§3), `_client_disconnected` rework: cancel+await pending open, + remove the placeholder from the stack, drop per-disconnect `_check_warnings` (§3), + `_make_loading_page` helper (§1), `discard_all_widgets()` cancels and drains `_pending_opens` + before discarding widgets (§3), plus `import contextlib` and a module logger (used by the + open-failure path). +2. `base.py` — `self._init_task = None` in `__init__`; memoized init task in `_showEvent` with + retry-on-failure (§4). +3. `shellwidget.py` — visibility-gated + debounced `CommandModel` rebuild with a first-show + rebuild hook (§5, required by §3). +4. `telescopewidget.py` — `_init()` gather (§5, fast-follow). +5. `camerawidget.py` — explicit `wait_for_state` timeouts + gather in `_init()` (§5, fast-follow). +6. `tests/` — new tests (see Verification). + +No `pyobs-core` changes. + +## Verification + +### New pytest coverage (offscreen Qt, existing `tests/` infrastructure) + +- `MainWindow._add_client` with a fake slow-opening `BaseWidget` (its `open()` sleeps or awaits a + controllable event): assert the nav item exists and `_pages[client]` is the placeholder + immediately; `_change_page` to it shows the placeholder; when `open()` completes the real widget + replaces the placeholder; if the page was current, `stackedWidget.currentWidget()` becomes the + real widget (and `_init` fires). Fake `Comm` with `clients`, `proxy`, `register_event`, + `subscribe_state`, `get_interfaces`, `clients_with_interface` mocks — the existing `FakeComm` in + `tests/test_camerawidget.py` only implements `safe_proxy`, so this is a new, fuller fake built + on that pattern. +- `_open_client` failure path: fake `BaseWidget.open()` that raises → the nav item and placeholder + are removed, the client leaves `_widgets`/`_pages`, `discard()` is called, and no "Loading…" + page remains. Also: a mid-open disconnect that cancels the task must not double-tear-down + (teardown runs exactly once, whether the disconnect or the failure happens first). +- `_client_disconnected` mid-open: pending open task is cancelled **and awaited** before + `discard()` (no concurrent open-tail/discard window), placeholder removed from the stack, no + ghost page — including when the placeholder was the current page. +- `_init_clients` parallel: two clients whose `open()` blocks on different events both reach the + placeholder stage before either open completes (assert via event ordering, not wall-clock). +- `TelescopeWidget._init` gather: fake comm records that all five `subscribe_state` calls are + in flight before any completes. +- Regression: existing `tests/` suite stays green (`pytest tests/`). + +### Integration (LocalComm fixtures) + +Drive the real `GUI` module headlessly against the existing `test/*.yaml` fixtures (`full.yaml`, +`telescope.yaml`, `camera.yaml`, ...) per the `verify` skill recipe: connect, assert each nav item +appears immediately, click a module name during its open window, assert the "Loading…" page shows, +then assert the real widget and its content arrive without a second click. For `telescope.yaml`, +assert the page fills (status labels populated, controls enabled) within a bounded time of the +first click. + +### Manual (XMPP) + +Real-network run: click each module right after it appears — page switches instantly to +"Loading…", then auto-fills; telescope page fills within a few seconds of the first click. + +## Rollout / out of scope + +- pyobs-gui only; no config, API, or pyobs-core changes. Rollback = revert the widget edits + (the placeholder/`_pages` machinery is confined to `mainwindow.py`). +- Out of scope: pre-warming `_init()` at open time (Decision), moving `CommandModel.init`'s + `inspect` work off the UI thread, `StatusWidget`'s own per-module RPC chains + (`statuswidget.py:_add_module_details` — same "fills in later" UX but not part of the click + problem), and any pyobs-core `wait_for_state` default-timeout change. Also out of scope (noted, + pre-existing, unrelated to this plan): `ShellWidget.open()` (`shellwidget.py:164-165`) registers + its `ModuleOpenedEvent`/`ModuleClosedEvent` handlers via `self.comm.register_event()` directly + instead of the tracked `BaseWidget.register_event()` wrapper, so `discard()` never unregisters + them — a leak on every logout/reconnect. (Once §3/§5 land, the mainwindow no longer triggers the + `CommandModel` rebuild per connect — the shell's own `_module_changed` handler becomes the sole + rebuild trigger, so it must carry the debounce/visibility gate.) Worth its own fix later. +- Follow-up candidates (noted, not planned): auto-switch to a newly connected module when its + nav item is already selected; show a spinner instead of the static label if the open takes + long; pre-warm `_init` for the most recently used pages. diff --git a/specs/index.md b/specs/index.md index ce65c2a..adc2f24 100644 --- a/specs/index.md +++ b/specs/index.md @@ -8,6 +8,10 @@ ADRs that concern `pyobs-gui` live in `pyobs-core`'s `specs/` tree instead (`spe ## Local plans +- `2026-08-21-gui-widget-startup-responsiveness.md` — **implemented, closed 2026-08-23 (PR #141, + `123161b`)**. Make module widgets appear and respond immediately at startup: non-blocking + `_add_client` with a "Loading…" placeholder page (clicks are never dropped), parallel + `_init_clients`, drop per-connect O(N) work, plus telescope/camera `_init` fast-follows. - `2026-08-20-gui-remote-call-error-handling.md` — **implemented, closed 2026-08-20 (PR #138, `bfc0a87`)**. Catch exceptions on remote method calls and show them in a messagebox (issue #134): route every user-triggered remote call through `run_background`, log throttled background @@ -43,3 +47,6 @@ ADRs that concern `pyobs-gui` live in `pyobs-core`'s `specs/` tree instead (`spe - `pyobs-core/specs/plans/2026-07-29-gui-iautoguiding-widget.md` — **implemented, closed**. The `IAutoGuiding` widget; the follow-up refinement (`OffsetResult`/`OffsetFrame`, arcsec-based `GuidingState`) has shipped in both pyobs-core and pyobs-gui. +- `pyobs-core/specs/plans/2026-08-21-basevideo-http-token-auth.md` — **proposed**. Shared-token + auth + browser login for `BaseVideo` (pyobs-core side); pyobs-gui side is a one-header change + in `VideoWidget`'s raw-socket GET (design: `pyobs-core/specs/design/basevideo-http-auth.md`). 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() diff --git a/tests/test_videowidget.py b/tests/test_videowidget.py index 4d33ac9..858c515 100644 --- a/tests/test_videowidget.py +++ b/tests/test_videowidget.py @@ -1,4 +1,4 @@ -from unittest.mock import AsyncMock +from unittest.mock import AsyncMock, MagicMock import pytest @@ -6,6 +6,19 @@ from pyobs_gui.videowidget import VideoWidget +def _widget_with_stream_socket() -> tuple[VideoWidget, MagicMock]: + """VideoWidget with a mocked raw stream socket, skipping _init.""" + widget = VideoWidget() + widget._initialized = True + widget.host = "localhost" + widget.port = 37077 + widget.path = "/webcam/video.mjpg" + widget.scheme = "http" + socket = MagicMock() + widget.socket = socket + return widget, socket + + @pytest.mark.asyncio async def test_grab_image_awaits_expose_task_and_surfaces_failure(qapp) -> None: """grab_image() must run the expose sequence as an awaited task so failures surface (issue @@ -36,3 +49,34 @@ async def test_grab_image_sets_exposure_count_before_grabbing(qapp) -> None: assert widget.datadisplay.grab_data.await_count == 3 widget.close() + + +# ── raw-socket stream auth ───────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_show_event_sends_authorization_header_when_token_configured(qapp) -> None: + """With an HttpFile carrying a token, the raw-socket GET must include Authorization.""" + widget, socket = _widget_with_stream_socket() + widget._auth_header = "Bearer secret" + + await widget._showEvent(MagicMock()) + + written = socket.write.call_args[0][0] + assert written.startswith(b"GET /webcam/video.mjpg HTTP/1.0\r\nHost: localhost:37077\r\n") + assert b"Authorization: Bearer secret\r\n" in written + assert written.endswith(b"\r\n") + widget.close() + + +@pytest.mark.asyncio +async def test_show_event_sends_no_authorization_header_without_token(qapp) -> None: + """Without a token, the bytes written to the socket are unchanged from today.""" + widget, socket = _widget_with_stream_socket() + widget._auth_header = None + + await widget._showEvent(MagicMock()) + + written = socket.write.call_args[0][0] + assert written == b"GET /webcam/video.mjpg HTTP/1.0\r\nHost: localhost:37077\r\n\r\n" + widget.close() diff --git a/uv.lock b/uv.lock index 337f184..221f4c6 100644 --- a/uv.lock +++ b/uv.lock @@ -3256,7 +3256,7 @@ wheels = [ [[package]] name = "pyobs-core" -version = "2.0.0.dev78" +version = "2.0.0.dev93" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "aiohttp" }, @@ -3288,14 +3288,14 @@ dependencies = [ { name = "tenacity" }, { name = "typing-extensions" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/47/bd/f9cae7fc00382cfeedde9f8975a72a2f0c5a464e15ce7e2561482773fae7/pyobs_core-2.0.0.dev78.tar.gz", hash = "sha256:9b748a8af94b2cb1787e33fdead1f677021878b86897690ae6d5643d5ad53f93", size = 361666, upload-time = "2026-08-18T10:45:40.129Z" } +sdist = { url = "https://files.pythonhosted.org/packages/6a/af/9430d1f7e917c748c219f0fce4a1760b08829559877008e9dc006fba8c3d/pyobs_core-2.0.0.dev93.tar.gz", hash = "sha256:d88b6312ca701da6bfc3f0ad3c3ddd25a4fa7eff488acded04721a1d38527184", size = 370770, upload-time = "2026-08-23T13:09:23.719Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/01/4b/0be191afcadc30ac49d39042249af637f221c3301297e2624ad2b6ef1353/pyobs_core-2.0.0.dev78-py3-none-any.whl", hash = "sha256:7833147de3b291767120b32333a384be66286f86d9be301ca20e194d6d867dac", size = 584432, upload-time = "2026-08-18T10:45:38.3Z" }, + { url = "https://files.pythonhosted.org/packages/97/0f/4944d705950263bc8f615fd2069a1ef9c9fc7a3a99cbf965eb02de721f60/pyobs_core-2.0.0.dev93-py3-none-any.whl", hash = "sha256:a92e3b64bc5a93e7ae4f2a3c2c191c81d4a2098c0c6a5b77bc234320a132bc62", size = 593808, upload-time = "2026-08-23T13:09:22.045Z" }, ] [[package]] name = "pyobs-gui" -version = "2.0.0.dev19" +version = "2.0.0.dev20" source = { editable = "." } dependencies = [ { name = "astroplan" }, @@ -3338,7 +3338,7 @@ requires-dist = [ { name = "colour", specifier = ">=0.1.5,<0.2" }, { name = "keyring", specifier = ">=25.7.0,<26" }, { name = "matplotlib", specifier = ">=3.10.1,<4" }, - { name = "pyobs-core", specifier = ">=2.0.0.dev78,<3" }, + { name = "pyobs-core", specifier = ">=2.0.0.dev93,<3" }, { name = "pyside6", specifier = ">=6.10.1" }, { name = "qasync", specifier = ">=0.27.1,<0.29" }, { name = "qfitswidget", specifier = ">=0.13.1" },