Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150) - #157

Merged
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets
Sep 1, 2026
Merged

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150)#157
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets

Conversation

@thusser

Copy link
Copy Markdown
Member

Summary

Implements #150 per specs/2026-08-28-gui-main-vs-sidebar-widgets.md (D1-D6).
Replaces the DEFAULT_WIDGETS/DEFAULT_ICONS/DEFAULT_CONFIG first-match-wins dict (a module
implementing several interfaces only ever showed one widget, silently dropping the rest) with:

  • MAIN_WIDGETS registry (MainWidgetEntry): interface, widget class, tab label/icon,
    declared sidebar fills, sidebar_preferred flag, and a paired_sidebar_widget slot (D6, no
    consumer yet -- see below).
  • collect_main_widgets(): matches a module's interfaces against the registry, applies the
    sidebar_preferred promotion rule (a cross-interface interface like filters/focuser/
    temperatures/cooling demotes into the sidebar when a "real" main widget also matched, and
    promotes back to its own page/tabs when nothing else did -- fixes a double-display bug found
    while re-deriving the design), and layers the widgets: custom-config merge/overwrite/extra-tab
    semantics on top (D3).
  • ModulePage: the universal page host, one match or many. Renders bare (no tab chrome) for
    one match, a QTabWidget for several; the sidebar column is a page-level property shared
    across every tab (previously per-widget-class, and only actually visible on the three widgets
    whose .ui happened to declare a widgetSidebar -- now every module gets one, so a custom
    sidebar: entry is finally visible regardless of module type).
  • ALWAYS_SIDEBAR_WIDGETS: FitsHeadersWidget lands in every module's sidebar
    unconditionally, including a module whose only matches were promoted into tabs.
  • D5 partial-open-failure isolation: main widgets open concurrently; one failing widget is
    logged, discarded, and its tab dropped -- the rest of the page stays up. All failing tears the
    client down exactly like today's single-widget path.
  • ModuleWindow (standalone mode) now shares the same collect_main_widgets/ModulePage/
    open_module_page assembly as MainWindow, instead of its own copy of the first-match loop.
  • Sidebar fills move out of CameraWidget.open()/TelescopeWidget.open() into sidebar_fills
    class attributes, consulted by the assembler via getattr(widget_class, "sidebar_fills", ...)
    so wiring either class in via custom widgets: config keeps its built-in sidebar.

Out of scope (tracked separately, specs/2026-09-01-gui-video-widget-split.md): actually
splitting VideoWidget into a main widget + paired_sidebar_widget pair. The mechanism itself is
implemented and wired in open_module_page(), just unused -- no MAIN_WIDGETS entry sets
paired_sidebar_widget yet.

One deviation from the plan's literal sketch: _add_client's signature is unchanged (still
takes a single widget: BaseWidget) rather than a List[WidgetChoice]. _open_client instead
dispatches on isinstance(widget, ModulePage) -- a ModulePage gets the new gather-open +
sidebar-fill treatment, anything else (Shell/Events/Status, and the existing
test_mainwindow_startup.py fakes) takes the untouched original single-widget path. This kept
every existing test passing unmodified and minimized blast radius.

Test plan

  • ruff check . clean
  • pyrefly check clean (0 errors)
  • black --check clean on all touched files
  • pytest -q: 74 passed (59 pre-existing + 15 new in tests/test_multiwidget_pages.py,
    covering the promotion rule both directions, universal ModulePage for a single match,
    ALWAYS_SIDEBAR_WIDGETS, declared-sidebar-fill gating via has_proxy, all three
    widgets: config modes (extra tab / interface-replace / overwrite), custom sidebar:
    visibility on a bare module, disconnect teardown of every tab + sidebar widget,
    get_fits_headers aggregation, D5 partial- and all-failure handling, and ModuleWindow
    parity)
  • Manual smoke against a real/dummy fleet (a camera-also-focuser fixture showing one "Camera"
    page with focuser demoted to sidebar; a standalone filter-wheel-only fixture still getting
    its own page) -- not done in this environment (headless, no live fleet); the fake-registry
    test suite exercises the equivalent code paths with assertions instead.

🤖 Generated with Claude Code

Replaces the interface -> widget dict (first-match-wins, silently dropping
every other interface a module implements) with an ordered MAIN_WIDGETS
registry plus collect_main_widgets(): a module matching several main
widgets now gets one nav entry whose page is a tab widget, one tab per
widget, with a shared sidebar (ModulePage) that's the universal page host
regardless of match count. Sidebar-preferred interfaces (filters, focuser,
temperatures, cooling) demote into the sidebar when another main widget
already matched, and promote back to their own page/tabs when nothing else
did. Partial widget-open failures drop just that tab instead of tearing
down the whole page. Custom widgets:/sidebar: config gained real
interface-targeted replace and overwrite semantics. The VideoWidget split
(D6's first paired_sidebar_widget consumer) is tracked separately.
Fake interface/widget registry so the promotion rule, universal
ModulePage, custom widgets:/sidebar: config, disconnect teardown,
FITS-header aggregation and D5 partial-open-failure handling are all
exercised without needing real comm plumbing. Updates the two existing
DEFAULT_WIDGETS/DEFAULT_ICONS assertions for the new registry shape.
…ract
Also records that the DEFAULT_CONFIG deletion (D1) has no maintainer
objection -- deleting outright, no fallback constant.
ALWAYS_SIDEBAR_WIDGETS unconditionally added FITS-header controls to
every module's sidebar (roof, weather, standalone filter wheel, ...),
but that panel only makes sense for modules that actually write FITS
files. Move it to a per-entry sidebar=((None, FitsHeadersWidget), ...)
declaration on the ICamera and IVideo MAIN_WIDGETS rows instead (and
CameraWidget.sidebar_fills, restoring its pre-registry behavior), and
leave ALWAYS_SIDEBAR_WIDGETS empty -- kept as a mechanism for genuinely
interface-agnostic sidebar content, per D2, just unused for now.
@thusser

Copy link
Copy Markdown
MemberAuthor

Reviewed the full diff at head (2824b06) plus the surrounding base.py/modulegui.py code and the spec, and independently re-ran the claimed checks: pytest -q → 74 passed, ruff check . clean, black --check clean on all touched files, pyrefly check → 0 errors. The architecture (registry + promotion rule, universal ModulePage, D3 merge/overwrite, D5 main-widget isolation, ModuleWindow parity) is a clear improvement over first-match-wins, docs are accurate, and the tests are thoughtful. However, I found one confirmed functional bug and one spec deviation I'd want addressed before merge.

1. HIGH — duplicate sidebar widgets when a sidebar_preferred interface is also a declared fill (confirmed)

open_module_page() applies sidebar fills in two steps with no dedup:

  • (b) each surviving main widget's declared sidebar tuple, gated by has_proxymainwindow.py:347-350
  • (c) every demoted sidebar_preferred match — mainwindow.py:352-354

In the production registry these overlap completely: all six declared fills (Camera's IFilters/ICooling/ITemperatures, Telescope's IFilters/IFocuser/ITemperatures) are themselves sidebar_preferred registry entries. So e.g. an ICamera + ITemperatures module gets TemperaturesWidget twice (once per step), ICamera + IFilters gets FilterWidget twice, ITelescope + IFocuser gets FocusWidget twice. This is a regression vs. the old code, where CameraWidget.open()/TelescopeWidget.open() filled each sidebar widget exactly once.

I verified against the real registry with a scratch test reusing the PR's fake-comm infra:

>>> REAL registry, ICamera+ITemperatures: TemperaturesWidget instances in sidebar: 2 <- BUG

The new tests can't catch this: the fake IMainA's fill gate (IFillGate) is a distinct interface that is never also sidebar_preferred, so the overlap is never exercised.

Suggested fix: drop the sidebar_preferred interfaces from the Camera/Telescope sidebar tuples (the demoted entries already cover those cases), or dedupe step (b) against sidebar_preferred_choices — plus a regression test whose main entry's declared fill is a sidebar_preferred interface.

2. MEDIUM — D5 sidebar-failure isolation promised in the spec, not implemented

Spec D5: "Sidebar-widget open failures (inside add_to_sidebar_open_child) are logged and the widget dropped from the sidebar — a deliberate small improvement over today." Not implemented: add_to_sidebar_open_child (base.py:144-145, 232-249) has no try/except, so a failing sidebar widget — steps (a)/(b)/(d)/(e), or step (c)'s explicit open() — propagates to _open_client's catch-all → _fail_open, tearing down the entire page including successfully opened tabs. Only main-widget failures get the isolation. Either implement the catch-and-drop or note the deviation in the PR description.

3. LOW — interface: custom config can't target a demoted (sidebar) widget

collect_main_widgets looks up interface_name only among main_entries (mainwindow.py:215). For a camera+filter module, interface: IFilters finds no slot and logs "ignored: module doesn't implement it" — misleading (the module does implement it; the entry is just demoted) — and the custom widget is silently dropped instead of replacing the sidebar block.

4. LOW — overwrite: true + interface: silently becomes interface-replace

overwrite_entries requires interface is None (mainwindow.py:195), so an entry with both keys falls into the replace branch with overwrite ignored. Undocumented ambiguity; a warning would help.

5. LOW — ModuleWindow total-failure path leaves a silent empty page

MainWindow raises → _fail_open removes nav item + page. ModuleWindow.open() ignores open_module_page()'s False return (modulegui.py:50), leaving an empty ModulePage as central widget with no error surfaced (the old code propagated the failure).

Nits

  • sidebar_choice.widget.open() runs twice for demoted widgets (explicit call at mainwindow.py:353 + add_to_sidebar_open_child). Harmless today since all four demoted classes use the idempotent BaseWindow.open, but a footgun for future sidebar_preferred widgets with side-effecting open()s.
  • base.py:254 comment still references the deleted DEFAULT_WIDGETS.
  • specs/index.md still lists the plan as "draft" — expected to flip to implemented on landing (per the spec's own checklist), not a blocker.

Overall: solid, well-tested refactor — but #1 is a real user-visible regression on common module combos (camera+temperature, camera+filter, telescope+focuser, …) that the suite can't currently catch. I'd fix that (plus the regression test) before merging, and clarify or implement #2.

1. HIGH, confirmed: Camera's/Telescope's declared sidebar tuples fully
overlapped their own sidebar_preferred registry entries (IFilters,
ICooling, ITemperatures, IFocuser), so every one of those widgets was
added to the sidebar twice. Drop the redundant declarations (registry
and sidebar_fills class attributes) and add an interface-keyed dedup
in open_module_page() as a second line of defense, so a future entry
can't silently reintroduce the same duplication. WidgetChoice now
carries the MAIN_WIDGETS interface it originated from (or the slot
it's replacing, for a custom widgets: entry) so the dedup still holds
when a demoted slot has been custom-replaced.
2. MEDIUM, spec deviation: implement D5's promised sidebar-widget
failure isolation in BaseWidget.add_to_sidebar() -- a failing sidebar
widget is now logged and dropped instead of propagating and tearing
down the whole page. Also drops the redundant explicit .open() call
on demoted sidebar_preferred widgets (add_to_sidebar already opens
them via _open_child), which is what makes their failures go through
the same isolation as every other sidebar fill.
3. LOW: interface: custom config can now target an interface that's
currently demoted into the sidebar (sidebar_preferred), not just a
plain main-widget slot -- collect_main_widgets() falls back to
looking it up there before logging "module doesn't implement it".
4. LOW: interface: combined with overwrite: true now logs a warning
that interface-replace wins and overwrite is ignored, instead of
silently doing so.
5. LOW: ModuleWindow.open() now raises when open_module_page() reports
every main widget failed, matching how the old single-widget code
surfaced a total failure, instead of leaving a silent empty page.
Nit: base.py's register_event() docstring still referenced the deleted
DEFAULT_WIDGETS.
Adds 5 regression tests (79 total, up from 74).
@thusser

Copy link
Copy Markdown
MemberAuthor

Pushed fixes for all 5 findings plus the nits, in fccf0e0.

1. HIGH, confirmed — dropped the redundant sidebar_preferred interfaces (IFilters/ICooling/ITemperatures on Camera, IFilters/IFocuser/ITemperatures on Telescope) from both MAIN_WIDGETS' declared sidebar tuples and the sidebar_fills class attributes -- the demoted-match path already covers them. Also added a defense-in-depth dedup in open_module_page(), keyed by interface (not widget class, so it still holds when interface: custom config has replaced a demoted slot): a declared fill is skipped if its interface is already covered by a sidebar_preferred_choices match. Added test_declared_fill_overlapping_sidebar_preferred_not_duplicated, which deliberately reintroduces the overlap in the fake registry and asserts exactly one instance.

2. MEDIUM — implemented the promised D5 catch-and-drop in BaseWidget.add_to_sidebar(): a failing sidebar widget is now logged and dropped instead of propagating. Also removed the redundant explicit .open() call on demoted sidebar_preferred widgets before add_to_sidebar() (it already opens them via _open_child) -- that's what makes step (c)'s failures go through the same isolation as everything else. Added test_sidebar_widget_open_failure_is_dropped_not_fatal.

3. LOWinterface: custom config now falls back to looking up a demoted (sidebar_preferred) slot when it's not found among the plain main entries, before logging "module doesn't implement it". Added test_custom_widget_interface_replaces_demoted_sidebar_slot.

4. LOWinterface: + overwrite: true together now logs a warning that interface-replace wins and overwrite is ignored (behavior unchanged, just no longer silent). Added test_custom_widget_overwrite_with_interface_warns_and_interface_wins.

5. LOWModuleWindow.open() now raises when open_module_page() reports every main widget failed, matching how the old single-widget code surfaced a total failure, instead of leaving a silent empty page. Added test_module_window_raises_on_total_open_failure.

Nit — fixed the stale DEFAULT_WIDGETS reference in base.py's register_event() docstring. Left specs/index.md as draft, per the plan's own convention of flipping it on merge.

79 tests passing (74 + 5 new), ruff check . / pyrefly check / black --check all clean on every touched file.

horizontalLayout had no explicit margins, so its QGroupBox border sat
inset by Qt's default layout margin instead of flush with the sidebar
column like the other two sidebar-preferred widgets.
The shared sidebar (D2) aggregates fills across every tab, so it can
grow taller than any single old widget's hand-picked sidebar did.
Wrap widgetSidebar in a QScrollArea: vertical-as-needed, horizontal
always off, frameless to keep the same visual look when it fits.
@thusser
thusser merged commit b7a14a6 into developSep 1, 2026
2 checks passed
thusser added a commit that referenced this pull request Sep 1, 2026
Check off the implementation checklist, note the deviation from the
literal _add_client sketch and the post-merge follow-up fixes, and
unblock the VideoWidget-split follow-up plan.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@thusser
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150) - #157

Merged
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets
Sep 1, 2026
Merged

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150)#157
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets

Conversation

@thusser

Copy link
Copy Markdown
Member

Summary

Implements #150 per specs/2026-08-28-gui-main-vs-sidebar-widgets.md (D1-D6).
Replaces the DEFAULT_WIDGETS/DEFAULT_ICONS/DEFAULT_CONFIG first-match-wins dict (a module
implementing several interfaces only ever showed one widget, silently dropping the rest) with:

  • MAIN_WIDGETS registry (MainWidgetEntry): interface, widget class, tab label/icon,
    declared sidebar fills, sidebar_preferred flag, and a paired_sidebar_widget slot (D6, no
    consumer yet -- see below).
  • collect_main_widgets(): matches a module's interfaces against the registry, applies the
    sidebar_preferred promotion rule (a cross-interface interface like filters/focuser/
    temperatures/cooling demotes into the sidebar when a "real" main widget also matched, and
    promotes back to its own page/tabs when nothing else did -- fixes a double-display bug found
    while re-deriving the design), and layers the widgets: custom-config merge/overwrite/extra-tab
    semantics on top (D3).
  • ModulePage: the universal page host, one match or many. Renders bare (no tab chrome) for
    one match, a QTabWidget for several; the sidebar column is a page-level property shared
    across every tab (previously per-widget-class, and only actually visible on the three widgets
    whose .ui happened to declare a widgetSidebar -- now every module gets one, so a custom
    sidebar: entry is finally visible regardless of module type).
  • ALWAYS_SIDEBAR_WIDGETS: FitsHeadersWidget lands in every module's sidebar
    unconditionally, including a module whose only matches were promoted into tabs.
  • D5 partial-open-failure isolation: main widgets open concurrently; one failing widget is
    logged, discarded, and its tab dropped -- the rest of the page stays up. All failing tears the
    client down exactly like today's single-widget path.
  • ModuleWindow (standalone mode) now shares the same collect_main_widgets/ModulePage/
    open_module_page assembly as MainWindow, instead of its own copy of the first-match loop.
  • Sidebar fills move out of CameraWidget.open()/TelescopeWidget.open() into sidebar_fills
    class attributes, consulted by the assembler via getattr(widget_class, "sidebar_fills", ...)
    so wiring either class in via custom widgets: config keeps its built-in sidebar.

Out of scope (tracked separately, specs/2026-09-01-gui-video-widget-split.md): actually
splitting VideoWidget into a main widget + paired_sidebar_widget pair. The mechanism itself is
implemented and wired in open_module_page(), just unused -- no MAIN_WIDGETS entry sets
paired_sidebar_widget yet.

One deviation from the plan's literal sketch: _add_client's signature is unchanged (still
takes a single widget: BaseWidget) rather than a List[WidgetChoice]. _open_client instead
dispatches on isinstance(widget, ModulePage) -- a ModulePage gets the new gather-open +
sidebar-fill treatment, anything else (Shell/Events/Status, and the existing
test_mainwindow_startup.py fakes) takes the untouched original single-widget path. This kept
every existing test passing unmodified and minimized blast radius.

Test plan

  • ruff check . clean
  • pyrefly check clean (0 errors)
  • black --check clean on all touched files
  • pytest -q: 74 passed (59 pre-existing + 15 new in tests/test_multiwidget_pages.py,
    covering the promotion rule both directions, universal ModulePage for a single match,
    ALWAYS_SIDEBAR_WIDGETS, declared-sidebar-fill gating via has_proxy, all three
    widgets: config modes (extra tab / interface-replace / overwrite), custom sidebar:
    visibility on a bare module, disconnect teardown of every tab + sidebar widget,
    get_fits_headers aggregation, D5 partial- and all-failure handling, and ModuleWindow
    parity)
  • Manual smoke against a real/dummy fleet (a camera-also-focuser fixture showing one "Camera"
    page with focuser demoted to sidebar; a standalone filter-wheel-only fixture still getting
    its own page) -- not done in this environment (headless, no live fleet); the fake-registry
    test suite exercises the equivalent code paths with assertions instead.

🤖 Generated with Claude Code

Replaces the interface -> widget dict (first-match-wins, silently dropping
every other interface a module implements) with an ordered MAIN_WIDGETS
registry plus collect_main_widgets(): a module matching several main
widgets now gets one nav entry whose page is a tab widget, one tab per
widget, with a shared sidebar (ModulePage) that's the universal page host
regardless of match count. Sidebar-preferred interfaces (filters, focuser,
temperatures, cooling) demote into the sidebar when another main widget
already matched, and promote back to their own page/tabs when nothing else
did. Partial widget-open failures drop just that tab instead of tearing
down the whole page. Custom widgets:/sidebar: config gained real
interface-targeted replace and overwrite semantics. The VideoWidget split
(D6's first paired_sidebar_widget consumer) is tracked separately.
Fake interface/widget registry so the promotion rule, universal
ModulePage, custom widgets:/sidebar: config, disconnect teardown,
FITS-header aggregation and D5 partial-open-failure handling are all
exercised without needing real comm plumbing. Updates the two existing
DEFAULT_WIDGETS/DEFAULT_ICONS assertions for the new registry shape.
…ract
Also records that the DEFAULT_CONFIG deletion (D1) has no maintainer
objection -- deleting outright, no fallback constant.
ALWAYS_SIDEBAR_WIDGETS unconditionally added FITS-header controls to
every module's sidebar (roof, weather, standalone filter wheel, ...),
but that panel only makes sense for modules that actually write FITS
files. Move it to a per-entry sidebar=((None, FitsHeadersWidget), ...)
declaration on the ICamera and IVideo MAIN_WIDGETS rows instead (and
CameraWidget.sidebar_fills, restoring its pre-registry behavior), and
leave ALWAYS_SIDEBAR_WIDGETS empty -- kept as a mechanism for genuinely
interface-agnostic sidebar content, per D2, just unused for now.
@thusser

Copy link
Copy Markdown
MemberAuthor

Reviewed the full diff at head (2824b06) plus the surrounding base.py/modulegui.py code and the spec, and independently re-ran the claimed checks: pytest -q → 74 passed, ruff check . clean, black --check clean on all touched files, pyrefly check → 0 errors. The architecture (registry + promotion rule, universal ModulePage, D3 merge/overwrite, D5 main-widget isolation, ModuleWindow parity) is a clear improvement over first-match-wins, docs are accurate, and the tests are thoughtful. However, I found one confirmed functional bug and one spec deviation I'd want addressed before merge.

1. HIGH — duplicate sidebar widgets when a sidebar_preferred interface is also a declared fill (confirmed)

open_module_page() applies sidebar fills in two steps with no dedup:

  • (b) each surviving main widget's declared sidebar tuple, gated by has_proxymainwindow.py:347-350
  • (c) every demoted sidebar_preferred match — mainwindow.py:352-354

In the production registry these overlap completely: all six declared fills (Camera's IFilters/ICooling/ITemperatures, Telescope's IFilters/IFocuser/ITemperatures) are themselves sidebar_preferred registry entries. So e.g. an ICamera + ITemperatures module gets TemperaturesWidget twice (once per step), ICamera + IFilters gets FilterWidget twice, ITelescope + IFocuser gets FocusWidget twice. This is a regression vs. the old code, where CameraWidget.open()/TelescopeWidget.open() filled each sidebar widget exactly once.

I verified against the real registry with a scratch test reusing the PR's fake-comm infra:

>>> REAL registry, ICamera+ITemperatures: TemperaturesWidget instances in sidebar: 2 <- BUG

The new tests can't catch this: the fake IMainA's fill gate (IFillGate) is a distinct interface that is never also sidebar_preferred, so the overlap is never exercised.

Suggested fix: drop the sidebar_preferred interfaces from the Camera/Telescope sidebar tuples (the demoted entries already cover those cases), or dedupe step (b) against sidebar_preferred_choices — plus a regression test whose main entry's declared fill is a sidebar_preferred interface.

2. MEDIUM — D5 sidebar-failure isolation promised in the spec, not implemented

Spec D5: "Sidebar-widget open failures (inside add_to_sidebar_open_child) are logged and the widget dropped from the sidebar — a deliberate small improvement over today." Not implemented: add_to_sidebar_open_child (base.py:144-145, 232-249) has no try/except, so a failing sidebar widget — steps (a)/(b)/(d)/(e), or step (c)'s explicit open() — propagates to _open_client's catch-all → _fail_open, tearing down the entire page including successfully opened tabs. Only main-widget failures get the isolation. Either implement the catch-and-drop or note the deviation in the PR description.

3. LOW — interface: custom config can't target a demoted (sidebar) widget

collect_main_widgets looks up interface_name only among main_entries (mainwindow.py:215). For a camera+filter module, interface: IFilters finds no slot and logs "ignored: module doesn't implement it" — misleading (the module does implement it; the entry is just demoted) — and the custom widget is silently dropped instead of replacing the sidebar block.

4. LOW — overwrite: true + interface: silently becomes interface-replace

overwrite_entries requires interface is None (mainwindow.py:195), so an entry with both keys falls into the replace branch with overwrite ignored. Undocumented ambiguity; a warning would help.

5. LOW — ModuleWindow total-failure path leaves a silent empty page

MainWindow raises → _fail_open removes nav item + page. ModuleWindow.open() ignores open_module_page()'s False return (modulegui.py:50), leaving an empty ModulePage as central widget with no error surfaced (the old code propagated the failure).

Nits

  • sidebar_choice.widget.open() runs twice for demoted widgets (explicit call at mainwindow.py:353 + add_to_sidebar_open_child). Harmless today since all four demoted classes use the idempotent BaseWindow.open, but a footgun for future sidebar_preferred widgets with side-effecting open()s.
  • base.py:254 comment still references the deleted DEFAULT_WIDGETS.
  • specs/index.md still lists the plan as "draft" — expected to flip to implemented on landing (per the spec's own checklist), not a blocker.

Overall: solid, well-tested refactor — but #1 is a real user-visible regression on common module combos (camera+temperature, camera+filter, telescope+focuser, …) that the suite can't currently catch. I'd fix that (plus the regression test) before merging, and clarify or implement #2.

1. HIGH, confirmed: Camera's/Telescope's declared sidebar tuples fully
overlapped their own sidebar_preferred registry entries (IFilters,
ICooling, ITemperatures, IFocuser), so every one of those widgets was
added to the sidebar twice. Drop the redundant declarations (registry
and sidebar_fills class attributes) and add an interface-keyed dedup
in open_module_page() as a second line of defense, so a future entry
can't silently reintroduce the same duplication. WidgetChoice now
carries the MAIN_WIDGETS interface it originated from (or the slot
it's replacing, for a custom widgets: entry) so the dedup still holds
when a demoted slot has been custom-replaced.
2. MEDIUM, spec deviation: implement D5's promised sidebar-widget
failure isolation in BaseWidget.add_to_sidebar() -- a failing sidebar
widget is now logged and dropped instead of propagating and tearing
down the whole page. Also drops the redundant explicit .open() call
on demoted sidebar_preferred widgets (add_to_sidebar already opens
them via _open_child), which is what makes their failures go through
the same isolation as every other sidebar fill.
3. LOW: interface: custom config can now target an interface that's
currently demoted into the sidebar (sidebar_preferred), not just a
plain main-widget slot -- collect_main_widgets() falls back to
looking it up there before logging "module doesn't implement it".
4. LOW: interface: combined with overwrite: true now logs a warning
that interface-replace wins and overwrite is ignored, instead of
silently doing so.
5. LOW: ModuleWindow.open() now raises when open_module_page() reports
every main widget failed, matching how the old single-widget code
surfaced a total failure, instead of leaving a silent empty page.
Nit: base.py's register_event() docstring still referenced the deleted
DEFAULT_WIDGETS.
Adds 5 regression tests (79 total, up from 74).
@thusser

Copy link
Copy Markdown
MemberAuthor

Pushed fixes for all 5 findings plus the nits, in fccf0e0.

1. HIGH, confirmed — dropped the redundant sidebar_preferred interfaces (IFilters/ICooling/ITemperatures on Camera, IFilters/IFocuser/ITemperatures on Telescope) from both MAIN_WIDGETS' declared sidebar tuples and the sidebar_fills class attributes -- the demoted-match path already covers them. Also added a defense-in-depth dedup in open_module_page(), keyed by interface (not widget class, so it still holds when interface: custom config has replaced a demoted slot): a declared fill is skipped if its interface is already covered by a sidebar_preferred_choices match. Added test_declared_fill_overlapping_sidebar_preferred_not_duplicated, which deliberately reintroduces the overlap in the fake registry and asserts exactly one instance.

2. MEDIUM — implemented the promised D5 catch-and-drop in BaseWidget.add_to_sidebar(): a failing sidebar widget is now logged and dropped instead of propagating. Also removed the redundant explicit .open() call on demoted sidebar_preferred widgets before add_to_sidebar() (it already opens them via _open_child) -- that's what makes step (c)'s failures go through the same isolation as everything else. Added test_sidebar_widget_open_failure_is_dropped_not_fatal.

3. LOWinterface: custom config now falls back to looking up a demoted (sidebar_preferred) slot when it's not found among the plain main entries, before logging "module doesn't implement it". Added test_custom_widget_interface_replaces_demoted_sidebar_slot.

4. LOWinterface: + overwrite: true together now logs a warning that interface-replace wins and overwrite is ignored (behavior unchanged, just no longer silent). Added test_custom_widget_overwrite_with_interface_warns_and_interface_wins.

5. LOWModuleWindow.open() now raises when open_module_page() reports every main widget failed, matching how the old single-widget code surfaced a total failure, instead of leaving a silent empty page. Added test_module_window_raises_on_total_open_failure.

Nit — fixed the stale DEFAULT_WIDGETS reference in base.py's register_event() docstring. Left specs/index.md as draft, per the plan's own convention of flipping it on merge.

79 tests passing (74 + 5 new), ruff check . / pyrefly check / black --check all clean on every touched file.

horizontalLayout had no explicit margins, so its QGroupBox border sat
inset by Qt's default layout margin instead of flush with the sidebar
column like the other two sidebar-preferred widgets.
The shared sidebar (D2) aggregates fills across every tab, so it can
grow taller than any single old widget's hand-picked sidebar did.
Wrap widgetSidebar in a QScrollArea: vertical-as-needed, horizontal
always off, frameless to keep the same visual look when it fits.
@thusser
thusser merged commit b7a14a6 into developSep 1, 2026
2 checks passed
thusser added a commit that referenced this pull request Sep 1, 2026
Check off the implementation checklist, note the deviation from the
literal _add_client sketch and the post-merge follow-up fixes, and
unblock the VideoWidget-split follow-up plan.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@thusser
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150) - #157

Merged
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets
Sep 1, 2026
Merged

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150)#157
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets

Conversation

@thusser

Copy link
Copy Markdown
Member

Summary

Implements #150 per specs/2026-08-28-gui-main-vs-sidebar-widgets.md (D1-D6).
Replaces the DEFAULT_WIDGETS/DEFAULT_ICONS/DEFAULT_CONFIG first-match-wins dict (a module
implementing several interfaces only ever showed one widget, silently dropping the rest) with:

  • MAIN_WIDGETS registry (MainWidgetEntry): interface, widget class, tab label/icon,
    declared sidebar fills, sidebar_preferred flag, and a paired_sidebar_widget slot (D6, no
    consumer yet -- see below).
  • collect_main_widgets(): matches a module's interfaces against the registry, applies the
    sidebar_preferred promotion rule (a cross-interface interface like filters/focuser/
    temperatures/cooling demotes into the sidebar when a "real" main widget also matched, and
    promotes back to its own page/tabs when nothing else did -- fixes a double-display bug found
    while re-deriving the design), and layers the widgets: custom-config merge/overwrite/extra-tab
    semantics on top (D3).
  • ModulePage: the universal page host, one match or many. Renders bare (no tab chrome) for
    one match, a QTabWidget for several; the sidebar column is a page-level property shared
    across every tab (previously per-widget-class, and only actually visible on the three widgets
    whose .ui happened to declare a widgetSidebar -- now every module gets one, so a custom
    sidebar: entry is finally visible regardless of module type).
  • ALWAYS_SIDEBAR_WIDGETS: FitsHeadersWidget lands in every module's sidebar
    unconditionally, including a module whose only matches were promoted into tabs.
  • D5 partial-open-failure isolation: main widgets open concurrently; one failing widget is
    logged, discarded, and its tab dropped -- the rest of the page stays up. All failing tears the
    client down exactly like today's single-widget path.
  • ModuleWindow (standalone mode) now shares the same collect_main_widgets/ModulePage/
    open_module_page assembly as MainWindow, instead of its own copy of the first-match loop.
  • Sidebar fills move out of CameraWidget.open()/TelescopeWidget.open() into sidebar_fills
    class attributes, consulted by the assembler via getattr(widget_class, "sidebar_fills", ...)
    so wiring either class in via custom widgets: config keeps its built-in sidebar.

Out of scope (tracked separately, specs/2026-09-01-gui-video-widget-split.md): actually
splitting VideoWidget into a main widget + paired_sidebar_widget pair. The mechanism itself is
implemented and wired in open_module_page(), just unused -- no MAIN_WIDGETS entry sets
paired_sidebar_widget yet.

One deviation from the plan's literal sketch: _add_client's signature is unchanged (still
takes a single widget: BaseWidget) rather than a List[WidgetChoice]. _open_client instead
dispatches on isinstance(widget, ModulePage) -- a ModulePage gets the new gather-open +
sidebar-fill treatment, anything else (Shell/Events/Status, and the existing
test_mainwindow_startup.py fakes) takes the untouched original single-widget path. This kept
every existing test passing unmodified and minimized blast radius.

Test plan

  • ruff check . clean
  • pyrefly check clean (0 errors)
  • black --check clean on all touched files
  • pytest -q: 74 passed (59 pre-existing + 15 new in tests/test_multiwidget_pages.py,
    covering the promotion rule both directions, universal ModulePage for a single match,
    ALWAYS_SIDEBAR_WIDGETS, declared-sidebar-fill gating via has_proxy, all three
    widgets: config modes (extra tab / interface-replace / overwrite), custom sidebar:
    visibility on a bare module, disconnect teardown of every tab + sidebar widget,
    get_fits_headers aggregation, D5 partial- and all-failure handling, and ModuleWindow
    parity)
  • Manual smoke against a real/dummy fleet (a camera-also-focuser fixture showing one "Camera"
    page with focuser demoted to sidebar; a standalone filter-wheel-only fixture still getting
    its own page) -- not done in this environment (headless, no live fleet); the fake-registry
    test suite exercises the equivalent code paths with assertions instead.

🤖 Generated with Claude Code

Replaces the interface -> widget dict (first-match-wins, silently dropping
every other interface a module implements) with an ordered MAIN_WIDGETS
registry plus collect_main_widgets(): a module matching several main
widgets now gets one nav entry whose page is a tab widget, one tab per
widget, with a shared sidebar (ModulePage) that's the universal page host
regardless of match count. Sidebar-preferred interfaces (filters, focuser,
temperatures, cooling) demote into the sidebar when another main widget
already matched, and promote back to their own page/tabs when nothing else
did. Partial widget-open failures drop just that tab instead of tearing
down the whole page. Custom widgets:/sidebar: config gained real
interface-targeted replace and overwrite semantics. The VideoWidget split
(D6's first paired_sidebar_widget consumer) is tracked separately.
Fake interface/widget registry so the promotion rule, universal
ModulePage, custom widgets:/sidebar: config, disconnect teardown,
FITS-header aggregation and D5 partial-open-failure handling are all
exercised without needing real comm plumbing. Updates the two existing
DEFAULT_WIDGETS/DEFAULT_ICONS assertions for the new registry shape.
…ract
Also records that the DEFAULT_CONFIG deletion (D1) has no maintainer
objection -- deleting outright, no fallback constant.
ALWAYS_SIDEBAR_WIDGETS unconditionally added FITS-header controls to
every module's sidebar (roof, weather, standalone filter wheel, ...),
but that panel only makes sense for modules that actually write FITS
files. Move it to a per-entry sidebar=((None, FitsHeadersWidget), ...)
declaration on the ICamera and IVideo MAIN_WIDGETS rows instead (and
CameraWidget.sidebar_fills, restoring its pre-registry behavior), and
leave ALWAYS_SIDEBAR_WIDGETS empty -- kept as a mechanism for genuinely
interface-agnostic sidebar content, per D2, just unused for now.
@thusser

Copy link
Copy Markdown
MemberAuthor

Reviewed the full diff at head (2824b06) plus the surrounding base.py/modulegui.py code and the spec, and independently re-ran the claimed checks: pytest -q → 74 passed, ruff check . clean, black --check clean on all touched files, pyrefly check → 0 errors. The architecture (registry + promotion rule, universal ModulePage, D3 merge/overwrite, D5 main-widget isolation, ModuleWindow parity) is a clear improvement over first-match-wins, docs are accurate, and the tests are thoughtful. However, I found one confirmed functional bug and one spec deviation I'd want addressed before merge.

1. HIGH — duplicate sidebar widgets when a sidebar_preferred interface is also a declared fill (confirmed)

open_module_page() applies sidebar fills in two steps with no dedup:

  • (b) each surviving main widget's declared sidebar tuple, gated by has_proxymainwindow.py:347-350
  • (c) every demoted sidebar_preferred match — mainwindow.py:352-354

In the production registry these overlap completely: all six declared fills (Camera's IFilters/ICooling/ITemperatures, Telescope's IFilters/IFocuser/ITemperatures) are themselves sidebar_preferred registry entries. So e.g. an ICamera + ITemperatures module gets TemperaturesWidget twice (once per step), ICamera + IFilters gets FilterWidget twice, ITelescope + IFocuser gets FocusWidget twice. This is a regression vs. the old code, where CameraWidget.open()/TelescopeWidget.open() filled each sidebar widget exactly once.

I verified against the real registry with a scratch test reusing the PR's fake-comm infra:

>>> REAL registry, ICamera+ITemperatures: TemperaturesWidget instances in sidebar: 2 <- BUG

The new tests can't catch this: the fake IMainA's fill gate (IFillGate) is a distinct interface that is never also sidebar_preferred, so the overlap is never exercised.

Suggested fix: drop the sidebar_preferred interfaces from the Camera/Telescope sidebar tuples (the demoted entries already cover those cases), or dedupe step (b) against sidebar_preferred_choices — plus a regression test whose main entry's declared fill is a sidebar_preferred interface.

2. MEDIUM — D5 sidebar-failure isolation promised in the spec, not implemented

Spec D5: "Sidebar-widget open failures (inside add_to_sidebar_open_child) are logged and the widget dropped from the sidebar — a deliberate small improvement over today." Not implemented: add_to_sidebar_open_child (base.py:144-145, 232-249) has no try/except, so a failing sidebar widget — steps (a)/(b)/(d)/(e), or step (c)'s explicit open() — propagates to _open_client's catch-all → _fail_open, tearing down the entire page including successfully opened tabs. Only main-widget failures get the isolation. Either implement the catch-and-drop or note the deviation in the PR description.

3. LOW — interface: custom config can't target a demoted (sidebar) widget

collect_main_widgets looks up interface_name only among main_entries (mainwindow.py:215). For a camera+filter module, interface: IFilters finds no slot and logs "ignored: module doesn't implement it" — misleading (the module does implement it; the entry is just demoted) — and the custom widget is silently dropped instead of replacing the sidebar block.

4. LOW — overwrite: true + interface: silently becomes interface-replace

overwrite_entries requires interface is None (mainwindow.py:195), so an entry with both keys falls into the replace branch with overwrite ignored. Undocumented ambiguity; a warning would help.

5. LOW — ModuleWindow total-failure path leaves a silent empty page

MainWindow raises → _fail_open removes nav item + page. ModuleWindow.open() ignores open_module_page()'s False return (modulegui.py:50), leaving an empty ModulePage as central widget with no error surfaced (the old code propagated the failure).

Nits

  • sidebar_choice.widget.open() runs twice for demoted widgets (explicit call at mainwindow.py:353 + add_to_sidebar_open_child). Harmless today since all four demoted classes use the idempotent BaseWindow.open, but a footgun for future sidebar_preferred widgets with side-effecting open()s.
  • base.py:254 comment still references the deleted DEFAULT_WIDGETS.
  • specs/index.md still lists the plan as "draft" — expected to flip to implemented on landing (per the spec's own checklist), not a blocker.

Overall: solid, well-tested refactor — but #1 is a real user-visible regression on common module combos (camera+temperature, camera+filter, telescope+focuser, …) that the suite can't currently catch. I'd fix that (plus the regression test) before merging, and clarify or implement #2.

1. HIGH, confirmed: Camera's/Telescope's declared sidebar tuples fully
overlapped their own sidebar_preferred registry entries (IFilters,
ICooling, ITemperatures, IFocuser), so every one of those widgets was
added to the sidebar twice. Drop the redundant declarations (registry
and sidebar_fills class attributes) and add an interface-keyed dedup
in open_module_page() as a second line of defense, so a future entry
can't silently reintroduce the same duplication. WidgetChoice now
carries the MAIN_WIDGETS interface it originated from (or the slot
it's replacing, for a custom widgets: entry) so the dedup still holds
when a demoted slot has been custom-replaced.
2. MEDIUM, spec deviation: implement D5's promised sidebar-widget
failure isolation in BaseWidget.add_to_sidebar() -- a failing sidebar
widget is now logged and dropped instead of propagating and tearing
down the whole page. Also drops the redundant explicit .open() call
on demoted sidebar_preferred widgets (add_to_sidebar already opens
them via _open_child), which is what makes their failures go through
the same isolation as every other sidebar fill.
3. LOW: interface: custom config can now target an interface that's
currently demoted into the sidebar (sidebar_preferred), not just a
plain main-widget slot -- collect_main_widgets() falls back to
looking it up there before logging "module doesn't implement it".
4. LOW: interface: combined with overwrite: true now logs a warning
that interface-replace wins and overwrite is ignored, instead of
silently doing so.
5. LOW: ModuleWindow.open() now raises when open_module_page() reports
every main widget failed, matching how the old single-widget code
surfaced a total failure, instead of leaving a silent empty page.
Nit: base.py's register_event() docstring still referenced the deleted
DEFAULT_WIDGETS.
Adds 5 regression tests (79 total, up from 74).
@thusser

Copy link
Copy Markdown
MemberAuthor

Pushed fixes for all 5 findings plus the nits, in fccf0e0.

1. HIGH, confirmed — dropped the redundant sidebar_preferred interfaces (IFilters/ICooling/ITemperatures on Camera, IFilters/IFocuser/ITemperatures on Telescope) from both MAIN_WIDGETS' declared sidebar tuples and the sidebar_fills class attributes -- the demoted-match path already covers them. Also added a defense-in-depth dedup in open_module_page(), keyed by interface (not widget class, so it still holds when interface: custom config has replaced a demoted slot): a declared fill is skipped if its interface is already covered by a sidebar_preferred_choices match. Added test_declared_fill_overlapping_sidebar_preferred_not_duplicated, which deliberately reintroduces the overlap in the fake registry and asserts exactly one instance.

2. MEDIUM — implemented the promised D5 catch-and-drop in BaseWidget.add_to_sidebar(): a failing sidebar widget is now logged and dropped instead of propagating. Also removed the redundant explicit .open() call on demoted sidebar_preferred widgets before add_to_sidebar() (it already opens them via _open_child) -- that's what makes step (c)'s failures go through the same isolation as everything else. Added test_sidebar_widget_open_failure_is_dropped_not_fatal.

3. LOWinterface: custom config now falls back to looking up a demoted (sidebar_preferred) slot when it's not found among the plain main entries, before logging "module doesn't implement it". Added test_custom_widget_interface_replaces_demoted_sidebar_slot.

4. LOWinterface: + overwrite: true together now logs a warning that interface-replace wins and overwrite is ignored (behavior unchanged, just no longer silent). Added test_custom_widget_overwrite_with_interface_warns_and_interface_wins.

5. LOWModuleWindow.open() now raises when open_module_page() reports every main widget failed, matching how the old single-widget code surfaced a total failure, instead of leaving a silent empty page. Added test_module_window_raises_on_total_open_failure.

Nit — fixed the stale DEFAULT_WIDGETS reference in base.py's register_event() docstring. Left specs/index.md as draft, per the plan's own convention of flipping it on merge.

79 tests passing (74 + 5 new), ruff check . / pyrefly check / black --check all clean on every touched file.

horizontalLayout had no explicit margins, so its QGroupBox border sat
inset by Qt's default layout margin instead of flush with the sidebar
column like the other two sidebar-preferred widgets.
The shared sidebar (D2) aggregates fills across every tab, so it can
grow taller than any single old widget's hand-picked sidebar did.
Wrap widgetSidebar in a QScrollArea: vertical-as-needed, horizontal
always off, frameless to keep the same visual look when it fits.
@thusser
thusser merged commit b7a14a6 into developSep 1, 2026
2 checks passed
thusser added a commit that referenced this pull request Sep 1, 2026
Check off the implementation checklist, note the deviation from the
literal _add_client sketch and the post-merge follow-up fixes, and
unblock the VideoWidget-split follow-up plan.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@thusser
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150) - #157

Merged
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets
Sep 1, 2026
Merged

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150)#157
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets

Conversation

@thusser

Copy link
Copy Markdown
Member

Summary

Implements #150 per specs/2026-08-28-gui-main-vs-sidebar-widgets.md (D1-D6).
Replaces the DEFAULT_WIDGETS/DEFAULT_ICONS/DEFAULT_CONFIG first-match-wins dict (a module
implementing several interfaces only ever showed one widget, silently dropping the rest) with:

  • MAIN_WIDGETS registry (MainWidgetEntry): interface, widget class, tab label/icon,
    declared sidebar fills, sidebar_preferred flag, and a paired_sidebar_widget slot (D6, no
    consumer yet -- see below).
  • collect_main_widgets(): matches a module's interfaces against the registry, applies the
    sidebar_preferred promotion rule (a cross-interface interface like filters/focuser/
    temperatures/cooling demotes into the sidebar when a "real" main widget also matched, and
    promotes back to its own page/tabs when nothing else did -- fixes a double-display bug found
    while re-deriving the design), and layers the widgets: custom-config merge/overwrite/extra-tab
    semantics on top (D3).
  • ModulePage: the universal page host, one match or many. Renders bare (no tab chrome) for
    one match, a QTabWidget for several; the sidebar column is a page-level property shared
    across every tab (previously per-widget-class, and only actually visible on the three widgets
    whose .ui happened to declare a widgetSidebar -- now every module gets one, so a custom
    sidebar: entry is finally visible regardless of module type).
  • ALWAYS_SIDEBAR_WIDGETS: FitsHeadersWidget lands in every module's sidebar
    unconditionally, including a module whose only matches were promoted into tabs.
  • D5 partial-open-failure isolation: main widgets open concurrently; one failing widget is
    logged, discarded, and its tab dropped -- the rest of the page stays up. All failing tears the
    client down exactly like today's single-widget path.
  • ModuleWindow (standalone mode) now shares the same collect_main_widgets/ModulePage/
    open_module_page assembly as MainWindow, instead of its own copy of the first-match loop.
  • Sidebar fills move out of CameraWidget.open()/TelescopeWidget.open() into sidebar_fills
    class attributes, consulted by the assembler via getattr(widget_class, "sidebar_fills", ...)
    so wiring either class in via custom widgets: config keeps its built-in sidebar.

Out of scope (tracked separately, specs/2026-09-01-gui-video-widget-split.md): actually
splitting VideoWidget into a main widget + paired_sidebar_widget pair. The mechanism itself is
implemented and wired in open_module_page(), just unused -- no MAIN_WIDGETS entry sets
paired_sidebar_widget yet.

One deviation from the plan's literal sketch: _add_client's signature is unchanged (still
takes a single widget: BaseWidget) rather than a List[WidgetChoice]. _open_client instead
dispatches on isinstance(widget, ModulePage) -- a ModulePage gets the new gather-open +
sidebar-fill treatment, anything else (Shell/Events/Status, and the existing
test_mainwindow_startup.py fakes) takes the untouched original single-widget path. This kept
every existing test passing unmodified and minimized blast radius.

Test plan

  • ruff check . clean
  • pyrefly check clean (0 errors)
  • black --check clean on all touched files
  • pytest -q: 74 passed (59 pre-existing + 15 new in tests/test_multiwidget_pages.py,
    covering the promotion rule both directions, universal ModulePage for a single match,
    ALWAYS_SIDEBAR_WIDGETS, declared-sidebar-fill gating via has_proxy, all three
    widgets: config modes (extra tab / interface-replace / overwrite), custom sidebar:
    visibility on a bare module, disconnect teardown of every tab + sidebar widget,
    get_fits_headers aggregation, D5 partial- and all-failure handling, and ModuleWindow
    parity)
  • Manual smoke against a real/dummy fleet (a camera-also-focuser fixture showing one "Camera"
    page with focuser demoted to sidebar; a standalone filter-wheel-only fixture still getting
    its own page) -- not done in this environment (headless, no live fleet); the fake-registry
    test suite exercises the equivalent code paths with assertions instead.

🤖 Generated with Claude Code

Replaces the interface -> widget dict (first-match-wins, silently dropping
every other interface a module implements) with an ordered MAIN_WIDGETS
registry plus collect_main_widgets(): a module matching several main
widgets now gets one nav entry whose page is a tab widget, one tab per
widget, with a shared sidebar (ModulePage) that's the universal page host
regardless of match count. Sidebar-preferred interfaces (filters, focuser,
temperatures, cooling) demote into the sidebar when another main widget
already matched, and promote back to their own page/tabs when nothing else
did. Partial widget-open failures drop just that tab instead of tearing
down the whole page. Custom widgets:/sidebar: config gained real
interface-targeted replace and overwrite semantics. The VideoWidget split
(D6's first paired_sidebar_widget consumer) is tracked separately.
Fake interface/widget registry so the promotion rule, universal
ModulePage, custom widgets:/sidebar: config, disconnect teardown,
FITS-header aggregation and D5 partial-open-failure handling are all
exercised without needing real comm plumbing. Updates the two existing
DEFAULT_WIDGETS/DEFAULT_ICONS assertions for the new registry shape.
…ract
Also records that the DEFAULT_CONFIG deletion (D1) has no maintainer
objection -- deleting outright, no fallback constant.
ALWAYS_SIDEBAR_WIDGETS unconditionally added FITS-header controls to
every module's sidebar (roof, weather, standalone filter wheel, ...),
but that panel only makes sense for modules that actually write FITS
files. Move it to a per-entry sidebar=((None, FitsHeadersWidget), ...)
declaration on the ICamera and IVideo MAIN_WIDGETS rows instead (and
CameraWidget.sidebar_fills, restoring its pre-registry behavior), and
leave ALWAYS_SIDEBAR_WIDGETS empty -- kept as a mechanism for genuinely
interface-agnostic sidebar content, per D2, just unused for now.
@thusser

Copy link
Copy Markdown
MemberAuthor

Reviewed the full diff at head (2824b06) plus the surrounding base.py/modulegui.py code and the spec, and independently re-ran the claimed checks: pytest -q → 74 passed, ruff check . clean, black --check clean on all touched files, pyrefly check → 0 errors. The architecture (registry + promotion rule, universal ModulePage, D3 merge/overwrite, D5 main-widget isolation, ModuleWindow parity) is a clear improvement over first-match-wins, docs are accurate, and the tests are thoughtful. However, I found one confirmed functional bug and one spec deviation I'd want addressed before merge.

1. HIGH — duplicate sidebar widgets when a sidebar_preferred interface is also a declared fill (confirmed)

open_module_page() applies sidebar fills in two steps with no dedup:

  • (b) each surviving main widget's declared sidebar tuple, gated by has_proxymainwindow.py:347-350
  • (c) every demoted sidebar_preferred match — mainwindow.py:352-354

In the production registry these overlap completely: all six declared fills (Camera's IFilters/ICooling/ITemperatures, Telescope's IFilters/IFocuser/ITemperatures) are themselves sidebar_preferred registry entries. So e.g. an ICamera + ITemperatures module gets TemperaturesWidget twice (once per step), ICamera + IFilters gets FilterWidget twice, ITelescope + IFocuser gets FocusWidget twice. This is a regression vs. the old code, where CameraWidget.open()/TelescopeWidget.open() filled each sidebar widget exactly once.

I verified against the real registry with a scratch test reusing the PR's fake-comm infra:

>>> REAL registry, ICamera+ITemperatures: TemperaturesWidget instances in sidebar: 2 <- BUG

The new tests can't catch this: the fake IMainA's fill gate (IFillGate) is a distinct interface that is never also sidebar_preferred, so the overlap is never exercised.

Suggested fix: drop the sidebar_preferred interfaces from the Camera/Telescope sidebar tuples (the demoted entries already cover those cases), or dedupe step (b) against sidebar_preferred_choices — plus a regression test whose main entry's declared fill is a sidebar_preferred interface.

2. MEDIUM — D5 sidebar-failure isolation promised in the spec, not implemented

Spec D5: "Sidebar-widget open failures (inside add_to_sidebar_open_child) are logged and the widget dropped from the sidebar — a deliberate small improvement over today." Not implemented: add_to_sidebar_open_child (base.py:144-145, 232-249) has no try/except, so a failing sidebar widget — steps (a)/(b)/(d)/(e), or step (c)'s explicit open() — propagates to _open_client's catch-all → _fail_open, tearing down the entire page including successfully opened tabs. Only main-widget failures get the isolation. Either implement the catch-and-drop or note the deviation in the PR description.

3. LOW — interface: custom config can't target a demoted (sidebar) widget

collect_main_widgets looks up interface_name only among main_entries (mainwindow.py:215). For a camera+filter module, interface: IFilters finds no slot and logs "ignored: module doesn't implement it" — misleading (the module does implement it; the entry is just demoted) — and the custom widget is silently dropped instead of replacing the sidebar block.

4. LOW — overwrite: true + interface: silently becomes interface-replace

overwrite_entries requires interface is None (mainwindow.py:195), so an entry with both keys falls into the replace branch with overwrite ignored. Undocumented ambiguity; a warning would help.

5. LOW — ModuleWindow total-failure path leaves a silent empty page

MainWindow raises → _fail_open removes nav item + page. ModuleWindow.open() ignores open_module_page()'s False return (modulegui.py:50), leaving an empty ModulePage as central widget with no error surfaced (the old code propagated the failure).

Nits

  • sidebar_choice.widget.open() runs twice for demoted widgets (explicit call at mainwindow.py:353 + add_to_sidebar_open_child). Harmless today since all four demoted classes use the idempotent BaseWindow.open, but a footgun for future sidebar_preferred widgets with side-effecting open()s.
  • base.py:254 comment still references the deleted DEFAULT_WIDGETS.
  • specs/index.md still lists the plan as "draft" — expected to flip to implemented on landing (per the spec's own checklist), not a blocker.

Overall: solid, well-tested refactor — but #1 is a real user-visible regression on common module combos (camera+temperature, camera+filter, telescope+focuser, …) that the suite can't currently catch. I'd fix that (plus the regression test) before merging, and clarify or implement #2.

1. HIGH, confirmed: Camera's/Telescope's declared sidebar tuples fully
overlapped their own sidebar_preferred registry entries (IFilters,
ICooling, ITemperatures, IFocuser), so every one of those widgets was
added to the sidebar twice. Drop the redundant declarations (registry
and sidebar_fills class attributes) and add an interface-keyed dedup
in open_module_page() as a second line of defense, so a future entry
can't silently reintroduce the same duplication. WidgetChoice now
carries the MAIN_WIDGETS interface it originated from (or the slot
it's replacing, for a custom widgets: entry) so the dedup still holds
when a demoted slot has been custom-replaced.
2. MEDIUM, spec deviation: implement D5's promised sidebar-widget
failure isolation in BaseWidget.add_to_sidebar() -- a failing sidebar
widget is now logged and dropped instead of propagating and tearing
down the whole page. Also drops the redundant explicit .open() call
on demoted sidebar_preferred widgets (add_to_sidebar already opens
them via _open_child), which is what makes their failures go through
the same isolation as every other sidebar fill.
3. LOW: interface: custom config can now target an interface that's
currently demoted into the sidebar (sidebar_preferred), not just a
plain main-widget slot -- collect_main_widgets() falls back to
looking it up there before logging "module doesn't implement it".
4. LOW: interface: combined with overwrite: true now logs a warning
that interface-replace wins and overwrite is ignored, instead of
silently doing so.
5. LOW: ModuleWindow.open() now raises when open_module_page() reports
every main widget failed, matching how the old single-widget code
surfaced a total failure, instead of leaving a silent empty page.
Nit: base.py's register_event() docstring still referenced the deleted
DEFAULT_WIDGETS.
Adds 5 regression tests (79 total, up from 74).
@thusser

Copy link
Copy Markdown
MemberAuthor

Pushed fixes for all 5 findings plus the nits, in fccf0e0.

1. HIGH, confirmed — dropped the redundant sidebar_preferred interfaces (IFilters/ICooling/ITemperatures on Camera, IFilters/IFocuser/ITemperatures on Telescope) from both MAIN_WIDGETS' declared sidebar tuples and the sidebar_fills class attributes -- the demoted-match path already covers them. Also added a defense-in-depth dedup in open_module_page(), keyed by interface (not widget class, so it still holds when interface: custom config has replaced a demoted slot): a declared fill is skipped if its interface is already covered by a sidebar_preferred_choices match. Added test_declared_fill_overlapping_sidebar_preferred_not_duplicated, which deliberately reintroduces the overlap in the fake registry and asserts exactly one instance.

2. MEDIUM — implemented the promised D5 catch-and-drop in BaseWidget.add_to_sidebar(): a failing sidebar widget is now logged and dropped instead of propagating. Also removed the redundant explicit .open() call on demoted sidebar_preferred widgets before add_to_sidebar() (it already opens them via _open_child) -- that's what makes step (c)'s failures go through the same isolation as everything else. Added test_sidebar_widget_open_failure_is_dropped_not_fatal.

3. LOWinterface: custom config now falls back to looking up a demoted (sidebar_preferred) slot when it's not found among the plain main entries, before logging "module doesn't implement it". Added test_custom_widget_interface_replaces_demoted_sidebar_slot.

4. LOWinterface: + overwrite: true together now logs a warning that interface-replace wins and overwrite is ignored (behavior unchanged, just no longer silent). Added test_custom_widget_overwrite_with_interface_warns_and_interface_wins.

5. LOWModuleWindow.open() now raises when open_module_page() reports every main widget failed, matching how the old single-widget code surfaced a total failure, instead of leaving a silent empty page. Added test_module_window_raises_on_total_open_failure.

Nit — fixed the stale DEFAULT_WIDGETS reference in base.py's register_event() docstring. Left specs/index.md as draft, per the plan's own convention of flipping it on merge.

79 tests passing (74 + 5 new), ruff check . / pyrefly check / black --check all clean on every touched file.

horizontalLayout had no explicit margins, so its QGroupBox border sat
inset by Qt's default layout margin instead of flush with the sidebar
column like the other two sidebar-preferred widgets.
The shared sidebar (D2) aggregates fills across every tab, so it can
grow taller than any single old widget's hand-picked sidebar did.
Wrap widgetSidebar in a QScrollArea: vertical-as-needed, horizontal
always off, frameless to keep the same visual look when it fits.
@thusser
thusser merged commit b7a14a6 into developSep 1, 2026
2 checks passed
thusser added a commit that referenced this pull request Sep 1, 2026
Check off the implementation checklist, note the deviation from the
literal _add_client sketch and the post-merge follow-up fixes, and
unblock the VideoWidget-split follow-up plan.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@thusser
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150) - #157

Merged
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets
Sep 1, 2026
Merged

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150)#157
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets

Conversation

@thusser

Copy link
Copy Markdown
Member

Summary

Implements #150 per specs/2026-08-28-gui-main-vs-sidebar-widgets.md (D1-D6).
Replaces the DEFAULT_WIDGETS/DEFAULT_ICONS/DEFAULT_CONFIG first-match-wins dict (a module
implementing several interfaces only ever showed one widget, silently dropping the rest) with:

  • MAIN_WIDGETS registry (MainWidgetEntry): interface, widget class, tab label/icon,
    declared sidebar fills, sidebar_preferred flag, and a paired_sidebar_widget slot (D6, no
    consumer yet -- see below).
  • collect_main_widgets(): matches a module's interfaces against the registry, applies the
    sidebar_preferred promotion rule (a cross-interface interface like filters/focuser/
    temperatures/cooling demotes into the sidebar when a "real" main widget also matched, and
    promotes back to its own page/tabs when nothing else did -- fixes a double-display bug found
    while re-deriving the design), and layers the widgets: custom-config merge/overwrite/extra-tab
    semantics on top (D3).
  • ModulePage: the universal page host, one match or many. Renders bare (no tab chrome) for
    one match, a QTabWidget for several; the sidebar column is a page-level property shared
    across every tab (previously per-widget-class, and only actually visible on the three widgets
    whose .ui happened to declare a widgetSidebar -- now every module gets one, so a custom
    sidebar: entry is finally visible regardless of module type).
  • ALWAYS_SIDEBAR_WIDGETS: FitsHeadersWidget lands in every module's sidebar
    unconditionally, including a module whose only matches were promoted into tabs.
  • D5 partial-open-failure isolation: main widgets open concurrently; one failing widget is
    logged, discarded, and its tab dropped -- the rest of the page stays up. All failing tears the
    client down exactly like today's single-widget path.
  • ModuleWindow (standalone mode) now shares the same collect_main_widgets/ModulePage/
    open_module_page assembly as MainWindow, instead of its own copy of the first-match loop.
  • Sidebar fills move out of CameraWidget.open()/TelescopeWidget.open() into sidebar_fills
    class attributes, consulted by the assembler via getattr(widget_class, "sidebar_fills", ...)
    so wiring either class in via custom widgets: config keeps its built-in sidebar.

Out of scope (tracked separately, specs/2026-09-01-gui-video-widget-split.md): actually
splitting VideoWidget into a main widget + paired_sidebar_widget pair. The mechanism itself is
implemented and wired in open_module_page(), just unused -- no MAIN_WIDGETS entry sets
paired_sidebar_widget yet.

One deviation from the plan's literal sketch: _add_client's signature is unchanged (still
takes a single widget: BaseWidget) rather than a List[WidgetChoice]. _open_client instead
dispatches on isinstance(widget, ModulePage) -- a ModulePage gets the new gather-open +
sidebar-fill treatment, anything else (Shell/Events/Status, and the existing
test_mainwindow_startup.py fakes) takes the untouched original single-widget path. This kept
every existing test passing unmodified and minimized blast radius.

Test plan

  • ruff check . clean
  • pyrefly check clean (0 errors)
  • black --check clean on all touched files
  • pytest -q: 74 passed (59 pre-existing + 15 new in tests/test_multiwidget_pages.py,
    covering the promotion rule both directions, universal ModulePage for a single match,
    ALWAYS_SIDEBAR_WIDGETS, declared-sidebar-fill gating via has_proxy, all three
    widgets: config modes (extra tab / interface-replace / overwrite), custom sidebar:
    visibility on a bare module, disconnect teardown of every tab + sidebar widget,
    get_fits_headers aggregation, D5 partial- and all-failure handling, and ModuleWindow
    parity)
  • Manual smoke against a real/dummy fleet (a camera-also-focuser fixture showing one "Camera"
    page with focuser demoted to sidebar; a standalone filter-wheel-only fixture still getting
    its own page) -- not done in this environment (headless, no live fleet); the fake-registry
    test suite exercises the equivalent code paths with assertions instead.

🤖 Generated with Claude Code

Replaces the interface -> widget dict (first-match-wins, silently dropping
every other interface a module implements) with an ordered MAIN_WIDGETS
registry plus collect_main_widgets(): a module matching several main
widgets now gets one nav entry whose page is a tab widget, one tab per
widget, with a shared sidebar (ModulePage) that's the universal page host
regardless of match count. Sidebar-preferred interfaces (filters, focuser,
temperatures, cooling) demote into the sidebar when another main widget
already matched, and promote back to their own page/tabs when nothing else
did. Partial widget-open failures drop just that tab instead of tearing
down the whole page. Custom widgets:/sidebar: config gained real
interface-targeted replace and overwrite semantics. The VideoWidget split
(D6's first paired_sidebar_widget consumer) is tracked separately.
Fake interface/widget registry so the promotion rule, universal
ModulePage, custom widgets:/sidebar: config, disconnect teardown,
FITS-header aggregation and D5 partial-open-failure handling are all
exercised without needing real comm plumbing. Updates the two existing
DEFAULT_WIDGETS/DEFAULT_ICONS assertions for the new registry shape.
…ract
Also records that the DEFAULT_CONFIG deletion (D1) has no maintainer
objection -- deleting outright, no fallback constant.
ALWAYS_SIDEBAR_WIDGETS unconditionally added FITS-header controls to
every module's sidebar (roof, weather, standalone filter wheel, ...),
but that panel only makes sense for modules that actually write FITS
files. Move it to a per-entry sidebar=((None, FitsHeadersWidget), ...)
declaration on the ICamera and IVideo MAIN_WIDGETS rows instead (and
CameraWidget.sidebar_fills, restoring its pre-registry behavior), and
leave ALWAYS_SIDEBAR_WIDGETS empty -- kept as a mechanism for genuinely
interface-agnostic sidebar content, per D2, just unused for now.
@thusser

Copy link
Copy Markdown
MemberAuthor

Reviewed the full diff at head (2824b06) plus the surrounding base.py/modulegui.py code and the spec, and independently re-ran the claimed checks: pytest -q → 74 passed, ruff check . clean, black --check clean on all touched files, pyrefly check → 0 errors. The architecture (registry + promotion rule, universal ModulePage, D3 merge/overwrite, D5 main-widget isolation, ModuleWindow parity) is a clear improvement over first-match-wins, docs are accurate, and the tests are thoughtful. However, I found one confirmed functional bug and one spec deviation I'd want addressed before merge.

1. HIGH — duplicate sidebar widgets when a sidebar_preferred interface is also a declared fill (confirmed)

open_module_page() applies sidebar fills in two steps with no dedup:

  • (b) each surviving main widget's declared sidebar tuple, gated by has_proxymainwindow.py:347-350
  • (c) every demoted sidebar_preferred match — mainwindow.py:352-354

In the production registry these overlap completely: all six declared fills (Camera's IFilters/ICooling/ITemperatures, Telescope's IFilters/IFocuser/ITemperatures) are themselves sidebar_preferred registry entries. So e.g. an ICamera + ITemperatures module gets TemperaturesWidget twice (once per step), ICamera + IFilters gets FilterWidget twice, ITelescope + IFocuser gets FocusWidget twice. This is a regression vs. the old code, where CameraWidget.open()/TelescopeWidget.open() filled each sidebar widget exactly once.

I verified against the real registry with a scratch test reusing the PR's fake-comm infra:

>>> REAL registry, ICamera+ITemperatures: TemperaturesWidget instances in sidebar: 2 <- BUG

The new tests can't catch this: the fake IMainA's fill gate (IFillGate) is a distinct interface that is never also sidebar_preferred, so the overlap is never exercised.

Suggested fix: drop the sidebar_preferred interfaces from the Camera/Telescope sidebar tuples (the demoted entries already cover those cases), or dedupe step (b) against sidebar_preferred_choices — plus a regression test whose main entry's declared fill is a sidebar_preferred interface.

2. MEDIUM — D5 sidebar-failure isolation promised in the spec, not implemented

Spec D5: "Sidebar-widget open failures (inside add_to_sidebar_open_child) are logged and the widget dropped from the sidebar — a deliberate small improvement over today." Not implemented: add_to_sidebar_open_child (base.py:144-145, 232-249) has no try/except, so a failing sidebar widget — steps (a)/(b)/(d)/(e), or step (c)'s explicit open() — propagates to _open_client's catch-all → _fail_open, tearing down the entire page including successfully opened tabs. Only main-widget failures get the isolation. Either implement the catch-and-drop or note the deviation in the PR description.

3. LOW — interface: custom config can't target a demoted (sidebar) widget

collect_main_widgets looks up interface_name only among main_entries (mainwindow.py:215). For a camera+filter module, interface: IFilters finds no slot and logs "ignored: module doesn't implement it" — misleading (the module does implement it; the entry is just demoted) — and the custom widget is silently dropped instead of replacing the sidebar block.

4. LOW — overwrite: true + interface: silently becomes interface-replace

overwrite_entries requires interface is None (mainwindow.py:195), so an entry with both keys falls into the replace branch with overwrite ignored. Undocumented ambiguity; a warning would help.

5. LOW — ModuleWindow total-failure path leaves a silent empty page

MainWindow raises → _fail_open removes nav item + page. ModuleWindow.open() ignores open_module_page()'s False return (modulegui.py:50), leaving an empty ModulePage as central widget with no error surfaced (the old code propagated the failure).

Nits

  • sidebar_choice.widget.open() runs twice for demoted widgets (explicit call at mainwindow.py:353 + add_to_sidebar_open_child). Harmless today since all four demoted classes use the idempotent BaseWindow.open, but a footgun for future sidebar_preferred widgets with side-effecting open()s.
  • base.py:254 comment still references the deleted DEFAULT_WIDGETS.
  • specs/index.md still lists the plan as "draft" — expected to flip to implemented on landing (per the spec's own checklist), not a blocker.

Overall: solid, well-tested refactor — but #1 is a real user-visible regression on common module combos (camera+temperature, camera+filter, telescope+focuser, …) that the suite can't currently catch. I'd fix that (plus the regression test) before merging, and clarify or implement #2.

1. HIGH, confirmed: Camera's/Telescope's declared sidebar tuples fully
overlapped their own sidebar_preferred registry entries (IFilters,
ICooling, ITemperatures, IFocuser), so every one of those widgets was
added to the sidebar twice. Drop the redundant declarations (registry
and sidebar_fills class attributes) and add an interface-keyed dedup
in open_module_page() as a second line of defense, so a future entry
can't silently reintroduce the same duplication. WidgetChoice now
carries the MAIN_WIDGETS interface it originated from (or the slot
it's replacing, for a custom widgets: entry) so the dedup still holds
when a demoted slot has been custom-replaced.
2. MEDIUM, spec deviation: implement D5's promised sidebar-widget
failure isolation in BaseWidget.add_to_sidebar() -- a failing sidebar
widget is now logged and dropped instead of propagating and tearing
down the whole page. Also drops the redundant explicit .open() call
on demoted sidebar_preferred widgets (add_to_sidebar already opens
them via _open_child), which is what makes their failures go through
the same isolation as every other sidebar fill.
3. LOW: interface: custom config can now target an interface that's
currently demoted into the sidebar (sidebar_preferred), not just a
plain main-widget slot -- collect_main_widgets() falls back to
looking it up there before logging "module doesn't implement it".
4. LOW: interface: combined with overwrite: true now logs a warning
that interface-replace wins and overwrite is ignored, instead of
silently doing so.
5. LOW: ModuleWindow.open() now raises when open_module_page() reports
every main widget failed, matching how the old single-widget code
surfaced a total failure, instead of leaving a silent empty page.
Nit: base.py's register_event() docstring still referenced the deleted
DEFAULT_WIDGETS.
Adds 5 regression tests (79 total, up from 74).
@thusser

Copy link
Copy Markdown
MemberAuthor

Pushed fixes for all 5 findings plus the nits, in fccf0e0.

1. HIGH, confirmed — dropped the redundant sidebar_preferred interfaces (IFilters/ICooling/ITemperatures on Camera, IFilters/IFocuser/ITemperatures on Telescope) from both MAIN_WIDGETS' declared sidebar tuples and the sidebar_fills class attributes -- the demoted-match path already covers them. Also added a defense-in-depth dedup in open_module_page(), keyed by interface (not widget class, so it still holds when interface: custom config has replaced a demoted slot): a declared fill is skipped if its interface is already covered by a sidebar_preferred_choices match. Added test_declared_fill_overlapping_sidebar_preferred_not_duplicated, which deliberately reintroduces the overlap in the fake registry and asserts exactly one instance.

2. MEDIUM — implemented the promised D5 catch-and-drop in BaseWidget.add_to_sidebar(): a failing sidebar widget is now logged and dropped instead of propagating. Also removed the redundant explicit .open() call on demoted sidebar_preferred widgets before add_to_sidebar() (it already opens them via _open_child) -- that's what makes step (c)'s failures go through the same isolation as everything else. Added test_sidebar_widget_open_failure_is_dropped_not_fatal.

3. LOWinterface: custom config now falls back to looking up a demoted (sidebar_preferred) slot when it's not found among the plain main entries, before logging "module doesn't implement it". Added test_custom_widget_interface_replaces_demoted_sidebar_slot.

4. LOWinterface: + overwrite: true together now logs a warning that interface-replace wins and overwrite is ignored (behavior unchanged, just no longer silent). Added test_custom_widget_overwrite_with_interface_warns_and_interface_wins.

5. LOWModuleWindow.open() now raises when open_module_page() reports every main widget failed, matching how the old single-widget code surfaced a total failure, instead of leaving a silent empty page. Added test_module_window_raises_on_total_open_failure.

Nit — fixed the stale DEFAULT_WIDGETS reference in base.py's register_event() docstring. Left specs/index.md as draft, per the plan's own convention of flipping it on merge.

79 tests passing (74 + 5 new), ruff check . / pyrefly check / black --check all clean on every touched file.

horizontalLayout had no explicit margins, so its QGroupBox border sat
inset by Qt's default layout margin instead of flush with the sidebar
column like the other two sidebar-preferred widgets.
The shared sidebar (D2) aggregates fills across every tab, so it can
grow taller than any single old widget's hand-picked sidebar did.
Wrap widgetSidebar in a QScrollArea: vertical-as-needed, horizontal
always off, frameless to keep the same visual look when it fits.
@thusser
thusser merged commit b7a14a6 into developSep 1, 2026
2 checks passed
thusser added a commit that referenced this pull request Sep 1, 2026
Check off the implementation checklist, note the deviation from the
literal _add_client sketch and the post-merge follow-up fixes, and
unblock the VideoWidget-split follow-up plan.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@thusser
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150) - #157

Merged
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets
Sep 1, 2026
Merged

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150)#157
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets

Conversation

@thusser

Copy link
Copy Markdown
Member

Summary

Implements #150 per specs/2026-08-28-gui-main-vs-sidebar-widgets.md (D1-D6).
Replaces the DEFAULT_WIDGETS/DEFAULT_ICONS/DEFAULT_CONFIG first-match-wins dict (a module
implementing several interfaces only ever showed one widget, silently dropping the rest) with:

  • MAIN_WIDGETS registry (MainWidgetEntry): interface, widget class, tab label/icon,
    declared sidebar fills, sidebar_preferred flag, and a paired_sidebar_widget slot (D6, no
    consumer yet -- see below).
  • collect_main_widgets(): matches a module's interfaces against the registry, applies the
    sidebar_preferred promotion rule (a cross-interface interface like filters/focuser/
    temperatures/cooling demotes into the sidebar when a "real" main widget also matched, and
    promotes back to its own page/tabs when nothing else did -- fixes a double-display bug found
    while re-deriving the design), and layers the widgets: custom-config merge/overwrite/extra-tab
    semantics on top (D3).
  • ModulePage: the universal page host, one match or many. Renders bare (no tab chrome) for
    one match, a QTabWidget for several; the sidebar column is a page-level property shared
    across every tab (previously per-widget-class, and only actually visible on the three widgets
    whose .ui happened to declare a widgetSidebar -- now every module gets one, so a custom
    sidebar: entry is finally visible regardless of module type).
  • ALWAYS_SIDEBAR_WIDGETS: FitsHeadersWidget lands in every module's sidebar
    unconditionally, including a module whose only matches were promoted into tabs.
  • D5 partial-open-failure isolation: main widgets open concurrently; one failing widget is
    logged, discarded, and its tab dropped -- the rest of the page stays up. All failing tears the
    client down exactly like today's single-widget path.
  • ModuleWindow (standalone mode) now shares the same collect_main_widgets/ModulePage/
    open_module_page assembly as MainWindow, instead of its own copy of the first-match loop.
  • Sidebar fills move out of CameraWidget.open()/TelescopeWidget.open() into sidebar_fills
    class attributes, consulted by the assembler via getattr(widget_class, "sidebar_fills", ...)
    so wiring either class in via custom widgets: config keeps its built-in sidebar.

Out of scope (tracked separately, specs/2026-09-01-gui-video-widget-split.md): actually
splitting VideoWidget into a main widget + paired_sidebar_widget pair. The mechanism itself is
implemented and wired in open_module_page(), just unused -- no MAIN_WIDGETS entry sets
paired_sidebar_widget yet.

One deviation from the plan's literal sketch: _add_client's signature is unchanged (still
takes a single widget: BaseWidget) rather than a List[WidgetChoice]. _open_client instead
dispatches on isinstance(widget, ModulePage) -- a ModulePage gets the new gather-open +
sidebar-fill treatment, anything else (Shell/Events/Status, and the existing
test_mainwindow_startup.py fakes) takes the untouched original single-widget path. This kept
every existing test passing unmodified and minimized blast radius.

Test plan

  • ruff check . clean
  • pyrefly check clean (0 errors)
  • black --check clean on all touched files
  • pytest -q: 74 passed (59 pre-existing + 15 new in tests/test_multiwidget_pages.py,
    covering the promotion rule both directions, universal ModulePage for a single match,
    ALWAYS_SIDEBAR_WIDGETS, declared-sidebar-fill gating via has_proxy, all three
    widgets: config modes (extra tab / interface-replace / overwrite), custom sidebar:
    visibility on a bare module, disconnect teardown of every tab + sidebar widget,
    get_fits_headers aggregation, D5 partial- and all-failure handling, and ModuleWindow
    parity)
  • Manual smoke against a real/dummy fleet (a camera-also-focuser fixture showing one "Camera"
    page with focuser demoted to sidebar; a standalone filter-wheel-only fixture still getting
    its own page) -- not done in this environment (headless, no live fleet); the fake-registry
    test suite exercises the equivalent code paths with assertions instead.

🤖 Generated with Claude Code

Replaces the interface -> widget dict (first-match-wins, silently dropping
every other interface a module implements) with an ordered MAIN_WIDGETS
registry plus collect_main_widgets(): a module matching several main
widgets now gets one nav entry whose page is a tab widget, one tab per
widget, with a shared sidebar (ModulePage) that's the universal page host
regardless of match count. Sidebar-preferred interfaces (filters, focuser,
temperatures, cooling) demote into the sidebar when another main widget
already matched, and promote back to their own page/tabs when nothing else
did. Partial widget-open failures drop just that tab instead of tearing
down the whole page. Custom widgets:/sidebar: config gained real
interface-targeted replace and overwrite semantics. The VideoWidget split
(D6's first paired_sidebar_widget consumer) is tracked separately.
Fake interface/widget registry so the promotion rule, universal
ModulePage, custom widgets:/sidebar: config, disconnect teardown,
FITS-header aggregation and D5 partial-open-failure handling are all
exercised without needing real comm plumbing. Updates the two existing
DEFAULT_WIDGETS/DEFAULT_ICONS assertions for the new registry shape.
…ract
Also records that the DEFAULT_CONFIG deletion (D1) has no maintainer
objection -- deleting outright, no fallback constant.
ALWAYS_SIDEBAR_WIDGETS unconditionally added FITS-header controls to
every module's sidebar (roof, weather, standalone filter wheel, ...),
but that panel only makes sense for modules that actually write FITS
files. Move it to a per-entry sidebar=((None, FitsHeadersWidget), ...)
declaration on the ICamera and IVideo MAIN_WIDGETS rows instead (and
CameraWidget.sidebar_fills, restoring its pre-registry behavior), and
leave ALWAYS_SIDEBAR_WIDGETS empty -- kept as a mechanism for genuinely
interface-agnostic sidebar content, per D2, just unused for now.
@thusser

Copy link
Copy Markdown
MemberAuthor

Reviewed the full diff at head (2824b06) plus the surrounding base.py/modulegui.py code and the spec, and independently re-ran the claimed checks: pytest -q → 74 passed, ruff check . clean, black --check clean on all touched files, pyrefly check → 0 errors. The architecture (registry + promotion rule, universal ModulePage, D3 merge/overwrite, D5 main-widget isolation, ModuleWindow parity) is a clear improvement over first-match-wins, docs are accurate, and the tests are thoughtful. However, I found one confirmed functional bug and one spec deviation I'd want addressed before merge.

1. HIGH — duplicate sidebar widgets when a sidebar_preferred interface is also a declared fill (confirmed)

open_module_page() applies sidebar fills in two steps with no dedup:

  • (b) each surviving main widget's declared sidebar tuple, gated by has_proxymainwindow.py:347-350
  • (c) every demoted sidebar_preferred match — mainwindow.py:352-354

In the production registry these overlap completely: all six declared fills (Camera's IFilters/ICooling/ITemperatures, Telescope's IFilters/IFocuser/ITemperatures) are themselves sidebar_preferred registry entries. So e.g. an ICamera + ITemperatures module gets TemperaturesWidget twice (once per step), ICamera + IFilters gets FilterWidget twice, ITelescope + IFocuser gets FocusWidget twice. This is a regression vs. the old code, where CameraWidget.open()/TelescopeWidget.open() filled each sidebar widget exactly once.

I verified against the real registry with a scratch test reusing the PR's fake-comm infra:

>>> REAL registry, ICamera+ITemperatures: TemperaturesWidget instances in sidebar: 2 <- BUG

The new tests can't catch this: the fake IMainA's fill gate (IFillGate) is a distinct interface that is never also sidebar_preferred, so the overlap is never exercised.

Suggested fix: drop the sidebar_preferred interfaces from the Camera/Telescope sidebar tuples (the demoted entries already cover those cases), or dedupe step (b) against sidebar_preferred_choices — plus a regression test whose main entry's declared fill is a sidebar_preferred interface.

2. MEDIUM — D5 sidebar-failure isolation promised in the spec, not implemented

Spec D5: "Sidebar-widget open failures (inside add_to_sidebar_open_child) are logged and the widget dropped from the sidebar — a deliberate small improvement over today." Not implemented: add_to_sidebar_open_child (base.py:144-145, 232-249) has no try/except, so a failing sidebar widget — steps (a)/(b)/(d)/(e), or step (c)'s explicit open() — propagates to _open_client's catch-all → _fail_open, tearing down the entire page including successfully opened tabs. Only main-widget failures get the isolation. Either implement the catch-and-drop or note the deviation in the PR description.

3. LOW — interface: custom config can't target a demoted (sidebar) widget

collect_main_widgets looks up interface_name only among main_entries (mainwindow.py:215). For a camera+filter module, interface: IFilters finds no slot and logs "ignored: module doesn't implement it" — misleading (the module does implement it; the entry is just demoted) — and the custom widget is silently dropped instead of replacing the sidebar block.

4. LOW — overwrite: true + interface: silently becomes interface-replace

overwrite_entries requires interface is None (mainwindow.py:195), so an entry with both keys falls into the replace branch with overwrite ignored. Undocumented ambiguity; a warning would help.

5. LOW — ModuleWindow total-failure path leaves a silent empty page

MainWindow raises → _fail_open removes nav item + page. ModuleWindow.open() ignores open_module_page()'s False return (modulegui.py:50), leaving an empty ModulePage as central widget with no error surfaced (the old code propagated the failure).

Nits

  • sidebar_choice.widget.open() runs twice for demoted widgets (explicit call at mainwindow.py:353 + add_to_sidebar_open_child). Harmless today since all four demoted classes use the idempotent BaseWindow.open, but a footgun for future sidebar_preferred widgets with side-effecting open()s.
  • base.py:254 comment still references the deleted DEFAULT_WIDGETS.
  • specs/index.md still lists the plan as "draft" — expected to flip to implemented on landing (per the spec's own checklist), not a blocker.

Overall: solid, well-tested refactor — but #1 is a real user-visible regression on common module combos (camera+temperature, camera+filter, telescope+focuser, …) that the suite can't currently catch. I'd fix that (plus the regression test) before merging, and clarify or implement #2.

1. HIGH, confirmed: Camera's/Telescope's declared sidebar tuples fully
overlapped their own sidebar_preferred registry entries (IFilters,
ICooling, ITemperatures, IFocuser), so every one of those widgets was
added to the sidebar twice. Drop the redundant declarations (registry
and sidebar_fills class attributes) and add an interface-keyed dedup
in open_module_page() as a second line of defense, so a future entry
can't silently reintroduce the same duplication. WidgetChoice now
carries the MAIN_WIDGETS interface it originated from (or the slot
it's replacing, for a custom widgets: entry) so the dedup still holds
when a demoted slot has been custom-replaced.
2. MEDIUM, spec deviation: implement D5's promised sidebar-widget
failure isolation in BaseWidget.add_to_sidebar() -- a failing sidebar
widget is now logged and dropped instead of propagating and tearing
down the whole page. Also drops the redundant explicit .open() call
on demoted sidebar_preferred widgets (add_to_sidebar already opens
them via _open_child), which is what makes their failures go through
the same isolation as every other sidebar fill.
3. LOW: interface: custom config can now target an interface that's
currently demoted into the sidebar (sidebar_preferred), not just a
plain main-widget slot -- collect_main_widgets() falls back to
looking it up there before logging "module doesn't implement it".
4. LOW: interface: combined with overwrite: true now logs a warning
that interface-replace wins and overwrite is ignored, instead of
silently doing so.
5. LOW: ModuleWindow.open() now raises when open_module_page() reports
every main widget failed, matching how the old single-widget code
surfaced a total failure, instead of leaving a silent empty page.
Nit: base.py's register_event() docstring still referenced the deleted
DEFAULT_WIDGETS.
Adds 5 regression tests (79 total, up from 74).
@thusser

Copy link
Copy Markdown
MemberAuthor

Pushed fixes for all 5 findings plus the nits, in fccf0e0.

1. HIGH, confirmed — dropped the redundant sidebar_preferred interfaces (IFilters/ICooling/ITemperatures on Camera, IFilters/IFocuser/ITemperatures on Telescope) from both MAIN_WIDGETS' declared sidebar tuples and the sidebar_fills class attributes -- the demoted-match path already covers them. Also added a defense-in-depth dedup in open_module_page(), keyed by interface (not widget class, so it still holds when interface: custom config has replaced a demoted slot): a declared fill is skipped if its interface is already covered by a sidebar_preferred_choices match. Added test_declared_fill_overlapping_sidebar_preferred_not_duplicated, which deliberately reintroduces the overlap in the fake registry and asserts exactly one instance.

2. MEDIUM — implemented the promised D5 catch-and-drop in BaseWidget.add_to_sidebar(): a failing sidebar widget is now logged and dropped instead of propagating. Also removed the redundant explicit .open() call on demoted sidebar_preferred widgets before add_to_sidebar() (it already opens them via _open_child) -- that's what makes step (c)'s failures go through the same isolation as everything else. Added test_sidebar_widget_open_failure_is_dropped_not_fatal.

3. LOWinterface: custom config now falls back to looking up a demoted (sidebar_preferred) slot when it's not found among the plain main entries, before logging "module doesn't implement it". Added test_custom_widget_interface_replaces_demoted_sidebar_slot.

4. LOWinterface: + overwrite: true together now logs a warning that interface-replace wins and overwrite is ignored (behavior unchanged, just no longer silent). Added test_custom_widget_overwrite_with_interface_warns_and_interface_wins.

5. LOWModuleWindow.open() now raises when open_module_page() reports every main widget failed, matching how the old single-widget code surfaced a total failure, instead of leaving a silent empty page. Added test_module_window_raises_on_total_open_failure.

Nit — fixed the stale DEFAULT_WIDGETS reference in base.py's register_event() docstring. Left specs/index.md as draft, per the plan's own convention of flipping it on merge.

79 tests passing (74 + 5 new), ruff check . / pyrefly check / black --check all clean on every touched file.

horizontalLayout had no explicit margins, so its QGroupBox border sat
inset by Qt's default layout margin instead of flush with the sidebar
column like the other two sidebar-preferred widgets.
The shared sidebar (D2) aggregates fills across every tab, so it can
grow taller than any single old widget's hand-picked sidebar did.
Wrap widgetSidebar in a QScrollArea: vertical-as-needed, horizontal
always off, frameless to keep the same visual look when it fits.
@thusser
thusser merged commit b7a14a6 into developSep 1, 2026
2 checks passed
thusser added a commit that referenced this pull request Sep 1, 2026
Check off the implementation checklist, note the deviation from the
literal _add_client sketch and the post-merge follow-up fixes, and
unblock the VideoWidget-split follow-up plan.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@thusser
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150) - #157

Merged
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets
Sep 1, 2026
Merged

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150)#157
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets

Conversation

@thusser

Copy link
Copy Markdown
Member

Summary

Implements #150 per specs/2026-08-28-gui-main-vs-sidebar-widgets.md (D1-D6).
Replaces the DEFAULT_WIDGETS/DEFAULT_ICONS/DEFAULT_CONFIG first-match-wins dict (a module
implementing several interfaces only ever showed one widget, silently dropping the rest) with:

  • MAIN_WIDGETS registry (MainWidgetEntry): interface, widget class, tab label/icon,
    declared sidebar fills, sidebar_preferred flag, and a paired_sidebar_widget slot (D6, no
    consumer yet -- see below).
  • collect_main_widgets(): matches a module's interfaces against the registry, applies the
    sidebar_preferred promotion rule (a cross-interface interface like filters/focuser/
    temperatures/cooling demotes into the sidebar when a "real" main widget also matched, and
    promotes back to its own page/tabs when nothing else did -- fixes a double-display bug found
    while re-deriving the design), and layers the widgets: custom-config merge/overwrite/extra-tab
    semantics on top (D3).
  • ModulePage: the universal page host, one match or many. Renders bare (no tab chrome) for
    one match, a QTabWidget for several; the sidebar column is a page-level property shared
    across every tab (previously per-widget-class, and only actually visible on the three widgets
    whose .ui happened to declare a widgetSidebar -- now every module gets one, so a custom
    sidebar: entry is finally visible regardless of module type).
  • ALWAYS_SIDEBAR_WIDGETS: FitsHeadersWidget lands in every module's sidebar
    unconditionally, including a module whose only matches were promoted into tabs.
  • D5 partial-open-failure isolation: main widgets open concurrently; one failing widget is
    logged, discarded, and its tab dropped -- the rest of the page stays up. All failing tears the
    client down exactly like today's single-widget path.
  • ModuleWindow (standalone mode) now shares the same collect_main_widgets/ModulePage/
    open_module_page assembly as MainWindow, instead of its own copy of the first-match loop.
  • Sidebar fills move out of CameraWidget.open()/TelescopeWidget.open() into sidebar_fills
    class attributes, consulted by the assembler via getattr(widget_class, "sidebar_fills", ...)
    so wiring either class in via custom widgets: config keeps its built-in sidebar.

Out of scope (tracked separately, specs/2026-09-01-gui-video-widget-split.md): actually
splitting VideoWidget into a main widget + paired_sidebar_widget pair. The mechanism itself is
implemented and wired in open_module_page(), just unused -- no MAIN_WIDGETS entry sets
paired_sidebar_widget yet.

One deviation from the plan's literal sketch: _add_client's signature is unchanged (still
takes a single widget: BaseWidget) rather than a List[WidgetChoice]. _open_client instead
dispatches on isinstance(widget, ModulePage) -- a ModulePage gets the new gather-open +
sidebar-fill treatment, anything else (Shell/Events/Status, and the existing
test_mainwindow_startup.py fakes) takes the untouched original single-widget path. This kept
every existing test passing unmodified and minimized blast radius.

Test plan

  • ruff check . clean
  • pyrefly check clean (0 errors)
  • black --check clean on all touched files
  • pytest -q: 74 passed (59 pre-existing + 15 new in tests/test_multiwidget_pages.py,
    covering the promotion rule both directions, universal ModulePage for a single match,
    ALWAYS_SIDEBAR_WIDGETS, declared-sidebar-fill gating via has_proxy, all three
    widgets: config modes (extra tab / interface-replace / overwrite), custom sidebar:
    visibility on a bare module, disconnect teardown of every tab + sidebar widget,
    get_fits_headers aggregation, D5 partial- and all-failure handling, and ModuleWindow
    parity)
  • Manual smoke against a real/dummy fleet (a camera-also-focuser fixture showing one "Camera"
    page with focuser demoted to sidebar; a standalone filter-wheel-only fixture still getting
    its own page) -- not done in this environment (headless, no live fleet); the fake-registry
    test suite exercises the equivalent code paths with assertions instead.

🤖 Generated with Claude Code

Replaces the interface -> widget dict (first-match-wins, silently dropping
every other interface a module implements) with an ordered MAIN_WIDGETS
registry plus collect_main_widgets(): a module matching several main
widgets now gets one nav entry whose page is a tab widget, one tab per
widget, with a shared sidebar (ModulePage) that's the universal page host
regardless of match count. Sidebar-preferred interfaces (filters, focuser,
temperatures, cooling) demote into the sidebar when another main widget
already matched, and promote back to their own page/tabs when nothing else
did. Partial widget-open failures drop just that tab instead of tearing
down the whole page. Custom widgets:/sidebar: config gained real
interface-targeted replace and overwrite semantics. The VideoWidget split
(D6's first paired_sidebar_widget consumer) is tracked separately.
Fake interface/widget registry so the promotion rule, universal
ModulePage, custom widgets:/sidebar: config, disconnect teardown,
FITS-header aggregation and D5 partial-open-failure handling are all
exercised without needing real comm plumbing. Updates the two existing
DEFAULT_WIDGETS/DEFAULT_ICONS assertions for the new registry shape.
…ract
Also records that the DEFAULT_CONFIG deletion (D1) has no maintainer
objection -- deleting outright, no fallback constant.
ALWAYS_SIDEBAR_WIDGETS unconditionally added FITS-header controls to
every module's sidebar (roof, weather, standalone filter wheel, ...),
but that panel only makes sense for modules that actually write FITS
files. Move it to a per-entry sidebar=((None, FitsHeadersWidget), ...)
declaration on the ICamera and IVideo MAIN_WIDGETS rows instead (and
CameraWidget.sidebar_fills, restoring its pre-registry behavior), and
leave ALWAYS_SIDEBAR_WIDGETS empty -- kept as a mechanism for genuinely
interface-agnostic sidebar content, per D2, just unused for now.
@thusser

Copy link
Copy Markdown
MemberAuthor

Reviewed the full diff at head (2824b06) plus the surrounding base.py/modulegui.py code and the spec, and independently re-ran the claimed checks: pytest -q → 74 passed, ruff check . clean, black --check clean on all touched files, pyrefly check → 0 errors. The architecture (registry + promotion rule, universal ModulePage, D3 merge/overwrite, D5 main-widget isolation, ModuleWindow parity) is a clear improvement over first-match-wins, docs are accurate, and the tests are thoughtful. However, I found one confirmed functional bug and one spec deviation I'd want addressed before merge.

1. HIGH — duplicate sidebar widgets when a sidebar_preferred interface is also a declared fill (confirmed)

open_module_page() applies sidebar fills in two steps with no dedup:

  • (b) each surviving main widget's declared sidebar tuple, gated by has_proxymainwindow.py:347-350
  • (c) every demoted sidebar_preferred match — mainwindow.py:352-354

In the production registry these overlap completely: all six declared fills (Camera's IFilters/ICooling/ITemperatures, Telescope's IFilters/IFocuser/ITemperatures) are themselves sidebar_preferred registry entries. So e.g. an ICamera + ITemperatures module gets TemperaturesWidget twice (once per step), ICamera + IFilters gets FilterWidget twice, ITelescope + IFocuser gets FocusWidget twice. This is a regression vs. the old code, where CameraWidget.open()/TelescopeWidget.open() filled each sidebar widget exactly once.

I verified against the real registry with a scratch test reusing the PR's fake-comm infra:

>>> REAL registry, ICamera+ITemperatures: TemperaturesWidget instances in sidebar: 2 <- BUG

The new tests can't catch this: the fake IMainA's fill gate (IFillGate) is a distinct interface that is never also sidebar_preferred, so the overlap is never exercised.

Suggested fix: drop the sidebar_preferred interfaces from the Camera/Telescope sidebar tuples (the demoted entries already cover those cases), or dedupe step (b) against sidebar_preferred_choices — plus a regression test whose main entry's declared fill is a sidebar_preferred interface.

2. MEDIUM — D5 sidebar-failure isolation promised in the spec, not implemented

Spec D5: "Sidebar-widget open failures (inside add_to_sidebar_open_child) are logged and the widget dropped from the sidebar — a deliberate small improvement over today." Not implemented: add_to_sidebar_open_child (base.py:144-145, 232-249) has no try/except, so a failing sidebar widget — steps (a)/(b)/(d)/(e), or step (c)'s explicit open() — propagates to _open_client's catch-all → _fail_open, tearing down the entire page including successfully opened tabs. Only main-widget failures get the isolation. Either implement the catch-and-drop or note the deviation in the PR description.

3. LOW — interface: custom config can't target a demoted (sidebar) widget

collect_main_widgets looks up interface_name only among main_entries (mainwindow.py:215). For a camera+filter module, interface: IFilters finds no slot and logs "ignored: module doesn't implement it" — misleading (the module does implement it; the entry is just demoted) — and the custom widget is silently dropped instead of replacing the sidebar block.

4. LOW — overwrite: true + interface: silently becomes interface-replace

overwrite_entries requires interface is None (mainwindow.py:195), so an entry with both keys falls into the replace branch with overwrite ignored. Undocumented ambiguity; a warning would help.

5. LOW — ModuleWindow total-failure path leaves a silent empty page

MainWindow raises → _fail_open removes nav item + page. ModuleWindow.open() ignores open_module_page()'s False return (modulegui.py:50), leaving an empty ModulePage as central widget with no error surfaced (the old code propagated the failure).

Nits

  • sidebar_choice.widget.open() runs twice for demoted widgets (explicit call at mainwindow.py:353 + add_to_sidebar_open_child). Harmless today since all four demoted classes use the idempotent BaseWindow.open, but a footgun for future sidebar_preferred widgets with side-effecting open()s.
  • base.py:254 comment still references the deleted DEFAULT_WIDGETS.
  • specs/index.md still lists the plan as "draft" — expected to flip to implemented on landing (per the spec's own checklist), not a blocker.

Overall: solid, well-tested refactor — but #1 is a real user-visible regression on common module combos (camera+temperature, camera+filter, telescope+focuser, …) that the suite can't currently catch. I'd fix that (plus the regression test) before merging, and clarify or implement #2.

1. HIGH, confirmed: Camera's/Telescope's declared sidebar tuples fully
overlapped their own sidebar_preferred registry entries (IFilters,
ICooling, ITemperatures, IFocuser), so every one of those widgets was
added to the sidebar twice. Drop the redundant declarations (registry
and sidebar_fills class attributes) and add an interface-keyed dedup
in open_module_page() as a second line of defense, so a future entry
can't silently reintroduce the same duplication. WidgetChoice now
carries the MAIN_WIDGETS interface it originated from (or the slot
it's replacing, for a custom widgets: entry) so the dedup still holds
when a demoted slot has been custom-replaced.
2. MEDIUM, spec deviation: implement D5's promised sidebar-widget
failure isolation in BaseWidget.add_to_sidebar() -- a failing sidebar
widget is now logged and dropped instead of propagating and tearing
down the whole page. Also drops the redundant explicit .open() call
on demoted sidebar_preferred widgets (add_to_sidebar already opens
them via _open_child), which is what makes their failures go through
the same isolation as every other sidebar fill.
3. LOW: interface: custom config can now target an interface that's
currently demoted into the sidebar (sidebar_preferred), not just a
plain main-widget slot -- collect_main_widgets() falls back to
looking it up there before logging "module doesn't implement it".
4. LOW: interface: combined with overwrite: true now logs a warning
that interface-replace wins and overwrite is ignored, instead of
silently doing so.
5. LOW: ModuleWindow.open() now raises when open_module_page() reports
every main widget failed, matching how the old single-widget code
surfaced a total failure, instead of leaving a silent empty page.
Nit: base.py's register_event() docstring still referenced the deleted
DEFAULT_WIDGETS.
Adds 5 regression tests (79 total, up from 74).
@thusser

Copy link
Copy Markdown
MemberAuthor

Pushed fixes for all 5 findings plus the nits, in fccf0e0.

1. HIGH, confirmed — dropped the redundant sidebar_preferred interfaces (IFilters/ICooling/ITemperatures on Camera, IFilters/IFocuser/ITemperatures on Telescope) from both MAIN_WIDGETS' declared sidebar tuples and the sidebar_fills class attributes -- the demoted-match path already covers them. Also added a defense-in-depth dedup in open_module_page(), keyed by interface (not widget class, so it still holds when interface: custom config has replaced a demoted slot): a declared fill is skipped if its interface is already covered by a sidebar_preferred_choices match. Added test_declared_fill_overlapping_sidebar_preferred_not_duplicated, which deliberately reintroduces the overlap in the fake registry and asserts exactly one instance.

2. MEDIUM — implemented the promised D5 catch-and-drop in BaseWidget.add_to_sidebar(): a failing sidebar widget is now logged and dropped instead of propagating. Also removed the redundant explicit .open() call on demoted sidebar_preferred widgets before add_to_sidebar() (it already opens them via _open_child) -- that's what makes step (c)'s failures go through the same isolation as everything else. Added test_sidebar_widget_open_failure_is_dropped_not_fatal.

3. LOWinterface: custom config now falls back to looking up a demoted (sidebar_preferred) slot when it's not found among the plain main entries, before logging "module doesn't implement it". Added test_custom_widget_interface_replaces_demoted_sidebar_slot.

4. LOWinterface: + overwrite: true together now logs a warning that interface-replace wins and overwrite is ignored (behavior unchanged, just no longer silent). Added test_custom_widget_overwrite_with_interface_warns_and_interface_wins.

5. LOWModuleWindow.open() now raises when open_module_page() reports every main widget failed, matching how the old single-widget code surfaced a total failure, instead of leaving a silent empty page. Added test_module_window_raises_on_total_open_failure.

Nit — fixed the stale DEFAULT_WIDGETS reference in base.py's register_event() docstring. Left specs/index.md as draft, per the plan's own convention of flipping it on merge.

79 tests passing (74 + 5 new), ruff check . / pyrefly check / black --check all clean on every touched file.

horizontalLayout had no explicit margins, so its QGroupBox border sat
inset by Qt's default layout margin instead of flush with the sidebar
column like the other two sidebar-preferred widgets.
The shared sidebar (D2) aggregates fills across every tab, so it can
grow taller than any single old widget's hand-picked sidebar did.
Wrap widgetSidebar in a QScrollArea: vertical-as-needed, horizontal
always off, frameless to keep the same visual look when it fits.
@thusser
thusser merged commit b7a14a6 into developSep 1, 2026
2 checks passed
thusser added a commit that referenced this pull request Sep 1, 2026
Check off the implementation checklist, note the deviation from the
literal _add_client sketch and the post-merge follow-up fixes, and
unblock the VideoWidget-split follow-up plan.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@thusser
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150) - #157

Merged
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets
Sep 1, 2026
Merged

Main widgets vs. sidebar widgets, automatic tab pages for multi-widget modules (#150)#157
thusser merged 7 commits into
developfrom
feature/gui-main-vs-sidebar-widgets

Conversation

@thusser

Copy link
Copy Markdown
Member

Summary

Implements #150 per specs/2026-08-28-gui-main-vs-sidebar-widgets.md (D1-D6).
Replaces the DEFAULT_WIDGETS/DEFAULT_ICONS/DEFAULT_CONFIG first-match-wins dict (a module
implementing several interfaces only ever showed one widget, silently dropping the rest) with:

  • MAIN_WIDGETS registry (MainWidgetEntry): interface, widget class, tab label/icon,
    declared sidebar fills, sidebar_preferred flag, and a paired_sidebar_widget slot (D6, no
    consumer yet -- see below).
  • collect_main_widgets(): matches a module's interfaces against the registry, applies the
    sidebar_preferred promotion rule (a cross-interface interface like filters/focuser/
    temperatures/cooling demotes into the sidebar when a "real" main widget also matched, and
    promotes back to its own page/tabs when nothing else did -- fixes a double-display bug found
    while re-deriving the design), and layers the widgets: custom-config merge/overwrite/extra-tab
    semantics on top (D3).
  • ModulePage: the universal page host, one match or many. Renders bare (no tab chrome) for
    one match, a QTabWidget for several; the sidebar column is a page-level property shared
    across every tab (previously per-widget-class, and only actually visible on the three widgets
    whose .ui happened to declare a widgetSidebar -- now every module gets one, so a custom
    sidebar: entry is finally visible regardless of module type).
  • ALWAYS_SIDEBAR_WIDGETS: FitsHeadersWidget lands in every module's sidebar
    unconditionally, including a module whose only matches were promoted into tabs.
  • D5 partial-open-failure isolation: main widgets open concurrently; one failing widget is
    logged, discarded, and its tab dropped -- the rest of the page stays up. All failing tears the
    client down exactly like today's single-widget path.
  • ModuleWindow (standalone mode) now shares the same collect_main_widgets/ModulePage/
    open_module_page assembly as MainWindow, instead of its own copy of the first-match loop.
  • Sidebar fills move out of CameraWidget.open()/TelescopeWidget.open() into sidebar_fills
    class attributes, consulted by the assembler via getattr(widget_class, "sidebar_fills", ...)
    so wiring either class in via custom widgets: config keeps its built-in sidebar.

Out of scope (tracked separately, specs/2026-09-01-gui-video-widget-split.md): actually
splitting VideoWidget into a main widget + paired_sidebar_widget pair. The mechanism itself is
implemented and wired in open_module_page(), just unused -- no MAIN_WIDGETS entry sets
paired_sidebar_widget yet.

One deviation from the plan's literal sketch: _add_client's signature is unchanged (still
takes a single widget: BaseWidget) rather than a List[WidgetChoice]. _open_client instead
dispatches on isinstance(widget, ModulePage) -- a ModulePage gets the new gather-open +
sidebar-fill treatment, anything else (Shell/Events/Status, and the existing
test_mainwindow_startup.py fakes) takes the untouched original single-widget path. This kept
every existing test passing unmodified and minimized blast radius.

Test plan

  • ruff check . clean
  • pyrefly check clean (0 errors)
  • black --check clean on all touched files
  • pytest -q: 74 passed (59 pre-existing + 15 new in tests/test_multiwidget_pages.py,
    covering the promotion rule both directions, universal ModulePage for a single match,
    ALWAYS_SIDEBAR_WIDGETS, declared-sidebar-fill gating via has_proxy, all three
    widgets: config modes (extra tab / interface-replace / overwrite), custom sidebar:
    visibility on a bare module, disconnect teardown of every tab + sidebar widget,
    get_fits_headers aggregation, D5 partial- and all-failure handling, and ModuleWindow
    parity)
  • Manual smoke against a real/dummy fleet (a camera-also-focuser fixture showing one "Camera"
    page with focuser demoted to sidebar; a standalone filter-wheel-only fixture still getting
    its own page) -- not done in this environment (headless, no live fleet); the fake-registry
    test suite exercises the equivalent code paths with assertions instead.

🤖 Generated with Claude Code

Replaces the interface -> widget dict (first-match-wins, silently dropping
every other interface a module implements) with an ordered MAIN_WIDGETS
registry plus collect_main_widgets(): a module matching several main
widgets now gets one nav entry whose page is a tab widget, one tab per
widget, with a shared sidebar (ModulePage) that's the universal page host
regardless of match count. Sidebar-preferred interfaces (filters, focuser,
temperatures, cooling) demote into the sidebar when another main widget
already matched, and promote back to their own page/tabs when nothing else
did. Partial widget-open failures drop just that tab instead of tearing
down the whole page. Custom widgets:/sidebar: config gained real
interface-targeted replace and overwrite semantics. The VideoWidget split
(D6's first paired_sidebar_widget consumer) is tracked separately.
Fake interface/widget registry so the promotion rule, universal
ModulePage, custom widgets:/sidebar: config, disconnect teardown,
FITS-header aggregation and D5 partial-open-failure handling are all
exercised without needing real comm plumbing. Updates the two existing
DEFAULT_WIDGETS/DEFAULT_ICONS assertions for the new registry shape.
…ract
Also records that the DEFAULT_CONFIG deletion (D1) has no maintainer
objection -- deleting outright, no fallback constant.
ALWAYS_SIDEBAR_WIDGETS unconditionally added FITS-header controls to
every module's sidebar (roof, weather, standalone filter wheel, ...),
but that panel only makes sense for modules that actually write FITS
files. Move it to a per-entry sidebar=((None, FitsHeadersWidget), ...)
declaration on the ICamera and IVideo MAIN_WIDGETS rows instead (and
CameraWidget.sidebar_fills, restoring its pre-registry behavior), and
leave ALWAYS_SIDEBAR_WIDGETS empty -- kept as a mechanism for genuinely
interface-agnostic sidebar content, per D2, just unused for now.
@thusser

Copy link
Copy Markdown
MemberAuthor

Reviewed the full diff at head (2824b06) plus the surrounding base.py/modulegui.py code and the spec, and independently re-ran the claimed checks: pytest -q → 74 passed, ruff check . clean, black --check clean on all touched files, pyrefly check → 0 errors. The architecture (registry + promotion rule, universal ModulePage, D3 merge/overwrite, D5 main-widget isolation, ModuleWindow parity) is a clear improvement over first-match-wins, docs are accurate, and the tests are thoughtful. However, I found one confirmed functional bug and one spec deviation I'd want addressed before merge.

1. HIGH — duplicate sidebar widgets when a sidebar_preferred interface is also a declared fill (confirmed)

open_module_page() applies sidebar fills in two steps with no dedup:

  • (b) each surviving main widget's declared sidebar tuple, gated by has_proxymainwindow.py:347-350
  • (c) every demoted sidebar_preferred match — mainwindow.py:352-354

In the production registry these overlap completely: all six declared fills (Camera's IFilters/ICooling/ITemperatures, Telescope's IFilters/IFocuser/ITemperatures) are themselves sidebar_preferred registry entries. So e.g. an ICamera + ITemperatures module gets TemperaturesWidget twice (once per step), ICamera + IFilters gets FilterWidget twice, ITelescope + IFocuser gets FocusWidget twice. This is a regression vs. the old code, where CameraWidget.open()/TelescopeWidget.open() filled each sidebar widget exactly once.

I verified against the real registry with a scratch test reusing the PR's fake-comm infra:

>>> REAL registry, ICamera+ITemperatures: TemperaturesWidget instances in sidebar: 2 <- BUG

The new tests can't catch this: the fake IMainA's fill gate (IFillGate) is a distinct interface that is never also sidebar_preferred, so the overlap is never exercised.

Suggested fix: drop the sidebar_preferred interfaces from the Camera/Telescope sidebar tuples (the demoted entries already cover those cases), or dedupe step (b) against sidebar_preferred_choices — plus a regression test whose main entry's declared fill is a sidebar_preferred interface.

2. MEDIUM — D5 sidebar-failure isolation promised in the spec, not implemented

Spec D5: "Sidebar-widget open failures (inside add_to_sidebar_open_child) are logged and the widget dropped from the sidebar — a deliberate small improvement over today." Not implemented: add_to_sidebar_open_child (base.py:144-145, 232-249) has no try/except, so a failing sidebar widget — steps (a)/(b)/(d)/(e), or step (c)'s explicit open() — propagates to _open_client's catch-all → _fail_open, tearing down the entire page including successfully opened tabs. Only main-widget failures get the isolation. Either implement the catch-and-drop or note the deviation in the PR description.

3. LOW — interface: custom config can't target a demoted (sidebar) widget

collect_main_widgets looks up interface_name only among main_entries (mainwindow.py:215). For a camera+filter module, interface: IFilters finds no slot and logs "ignored: module doesn't implement it" — misleading (the module does implement it; the entry is just demoted) — and the custom widget is silently dropped instead of replacing the sidebar block.

4. LOW — overwrite: true + interface: silently becomes interface-replace

overwrite_entries requires interface is None (mainwindow.py:195), so an entry with both keys falls into the replace branch with overwrite ignored. Undocumented ambiguity; a warning would help.

5. LOW — ModuleWindow total-failure path leaves a silent empty page

MainWindow raises → _fail_open removes nav item + page. ModuleWindow.open() ignores open_module_page()'s False return (modulegui.py:50), leaving an empty ModulePage as central widget with no error surfaced (the old code propagated the failure).

Nits

  • sidebar_choice.widget.open() runs twice for demoted widgets (explicit call at mainwindow.py:353 + add_to_sidebar_open_child). Harmless today since all four demoted classes use the idempotent BaseWindow.open, but a footgun for future sidebar_preferred widgets with side-effecting open()s.
  • base.py:254 comment still references the deleted DEFAULT_WIDGETS.
  • specs/index.md still lists the plan as "draft" — expected to flip to implemented on landing (per the spec's own checklist), not a blocker.

Overall: solid, well-tested refactor — but #1 is a real user-visible regression on common module combos (camera+temperature, camera+filter, telescope+focuser, …) that the suite can't currently catch. I'd fix that (plus the regression test) before merging, and clarify or implement #2.

1. HIGH, confirmed: Camera's/Telescope's declared sidebar tuples fully
overlapped their own sidebar_preferred registry entries (IFilters,
ICooling, ITemperatures, IFocuser), so every one of those widgets was
added to the sidebar twice. Drop the redundant declarations (registry
and sidebar_fills class attributes) and add an interface-keyed dedup
in open_module_page() as a second line of defense, so a future entry
can't silently reintroduce the same duplication. WidgetChoice now
carries the MAIN_WIDGETS interface it originated from (or the slot
it's replacing, for a custom widgets: entry) so the dedup still holds
when a demoted slot has been custom-replaced.
2. MEDIUM, spec deviation: implement D5's promised sidebar-widget
failure isolation in BaseWidget.add_to_sidebar() -- a failing sidebar
widget is now logged and dropped instead of propagating and tearing
down the whole page. Also drops the redundant explicit .open() call
on demoted sidebar_preferred widgets (add_to_sidebar already opens
them via _open_child), which is what makes their failures go through
the same isolation as every other sidebar fill.
3. LOW: interface: custom config can now target an interface that's
currently demoted into the sidebar (sidebar_preferred), not just a
plain main-widget slot -- collect_main_widgets() falls back to
looking it up there before logging "module doesn't implement it".
4. LOW: interface: combined with overwrite: true now logs a warning
that interface-replace wins and overwrite is ignored, instead of
silently doing so.
5. LOW: ModuleWindow.open() now raises when open_module_page() reports
every main widget failed, matching how the old single-widget code
surfaced a total failure, instead of leaving a silent empty page.
Nit: base.py's register_event() docstring still referenced the deleted
DEFAULT_WIDGETS.
Adds 5 regression tests (79 total, up from 74).
@thusser

Copy link
Copy Markdown
MemberAuthor

Pushed fixes for all 5 findings plus the nits, in fccf0e0.

1. HIGH, confirmed — dropped the redundant sidebar_preferred interfaces (IFilters/ICooling/ITemperatures on Camera, IFilters/IFocuser/ITemperatures on Telescope) from both MAIN_WIDGETS' declared sidebar tuples and the sidebar_fills class attributes -- the demoted-match path already covers them. Also added a defense-in-depth dedup in open_module_page(), keyed by interface (not widget class, so it still holds when interface: custom config has replaced a demoted slot): a declared fill is skipped if its interface is already covered by a sidebar_preferred_choices match. Added test_declared_fill_overlapping_sidebar_preferred_not_duplicated, which deliberately reintroduces the overlap in the fake registry and asserts exactly one instance.

2. MEDIUM — implemented the promised D5 catch-and-drop in BaseWidget.add_to_sidebar(): a failing sidebar widget is now logged and dropped instead of propagating. Also removed the redundant explicit .open() call on demoted sidebar_preferred widgets before add_to_sidebar() (it already opens them via _open_child) -- that's what makes step (c)'s failures go through the same isolation as everything else. Added test_sidebar_widget_open_failure_is_dropped_not_fatal.

3. LOWinterface: custom config now falls back to looking up a demoted (sidebar_preferred) slot when it's not found among the plain main entries, before logging "module doesn't implement it". Added test_custom_widget_interface_replaces_demoted_sidebar_slot.

4. LOWinterface: + overwrite: true together now logs a warning that interface-replace wins and overwrite is ignored (behavior unchanged, just no longer silent). Added test_custom_widget_overwrite_with_interface_warns_and_interface_wins.

5. LOWModuleWindow.open() now raises when open_module_page() reports every main widget failed, matching how the old single-widget code surfaced a total failure, instead of leaving a silent empty page. Added test_module_window_raises_on_total_open_failure.

Nit — fixed the stale DEFAULT_WIDGETS reference in base.py's register_event() docstring. Left specs/index.md as draft, per the plan's own convention of flipping it on merge.

79 tests passing (74 + 5 new), ruff check . / pyrefly check / black --check all clean on every touched file.

horizontalLayout had no explicit margins, so its QGroupBox border sat
inset by Qt's default layout margin instead of flush with the sidebar
column like the other two sidebar-preferred widgets.
The shared sidebar (D2) aggregates fills across every tab, so it can
grow taller than any single old widget's hand-picked sidebar did.
Wrap widgetSidebar in a QScrollArea: vertical-as-needed, horizontal
always off, frameless to keep the same visual look when it fits.
@thusser
thusser merged commit b7a14a6 into developSep 1, 2026
2 checks passed
thusser added a commit that referenced this pull request Sep 1, 2026
Check off the implementation checklist, note the deviation from the
literal _add_client sketch and the post-merge follow-up fixes, and
unblock the VideoWidget-split follow-up plan.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@thusser