Repository navigation
feat(gui): SpotHub is a CanonWindow workspace re-skinned to the canon palette. Principle VIII. - #6401
Conversation
… palette. Principle VIII. Part of #6239. SpotHub (DxClusterDialog) moves from PersistentDialog to a CanonWindow workspace, following AetherGPS and AetherMap: - No title bar: the ambient ground, the canon header and the corner close button; it resizes from every edge and closes with Esc or Ctrl+W. It no longer follows View -> Frameless Window, so MainWindow creates it directly instead of through showOrRaisePersistent(). - Geometry persists under its old key (DxClusterDialogGeometry). Minimum 360x600 and first-open 760x680, 40 px taller for the header. - Look: the canon/spotHub theme scope plus one canon sheet. The tab pages are a raised card with cyan selected tabs, group boxes nested with cyan titles, fields on control, consoles and the spot list nested, canon check boxes. Connect / Start are the filled cyan primary; a checked toggle keeps the green fill (toggle success tokens). - Per-widget field, console, table, tab and toggle sheets are gone in favor of that sheet; the remaining hex literals (swatch borders, label colors) are tokens. Docs: the SpotHub page describes the window; docs/style lists SpotHub among the canon windows and its scope. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Reviewed head 78052c6e85e0d3d4c6deb6165c4fb3d4dac26a96. Approve with nits; no blockers. This implements the stated SpotHub canon migration under RFC #6226 and rollout #6239: workspace chrome, scoped palette, old geometry-key reuse, consolidated control styling and matching user documentation. No closing bug issue is linked, so this review evaluates the migration against its stated requirements. The maintainer-authored body explicitly chooses green checked toggles; that disclosed design decision is preserved.
Scope
| File/group | Change | Claimed? | Verdict |
|---|---|---|---|
src/gui/DxClusterDialog.{h,cpp} |
CanonWindow workspace, header/resizer, same geometry key, disclosed size increases, canon sheet and primary-button properties | Yes | In scope |
src/gui/MainWindow_Menus.cpp |
Direct lazy construction with QPointer and WA_DeleteOnClose; retain per-open FreeDV refresh and fresh-instance wiring | Yes | In scope |
docs/style/{aethersdr-style-guide,dialog-patterns,theme-style-guide}.md |
Register SpotHub and its canon scope/workspace/persistence exceptions | Yes | In scope |
docs/user/docs/spothub.md |
Document moving, resizing, closing, taskbar behavior and Frameless Window exception | Yes | In scope |
Everything in the diff is explained by the migration. One signed commit, authored October 12 UTC, with no unrelated commits, engine/protocol changes, new settings keys, dependency changes, base-token edits or CHANGELOG entry. GitHub verifies the signature. The screenshot staleness is explicitly disclosed in the body.
Blockers
None.
Nits — non-blocking
- Two FreeDV fields still override the common canon field chrome.
src/gui/DxClusterDialog.cpp:2375and:2409: callsign and grid retain complete local QLineEdit sheets. Those take precedence over the body sheet, leaving raised-ground rectangular fields beside the rounded control-ground Station Msg field and overriding the common focus-border treatment. This is visible in the review-owned FreeDV grab. Keep their read-only text treatment while allowing the body sheet to own the field chrome:
AetherSDR::ThemeManager::instance().applyStyleSheet(m_fdvCallsignEdit,
"QLineEdit[readOnly=\"true\"] { color: {{color.text.label}}; }");Use the corresponding replacement for the grid field:
AetherSDR::ThemeManager::instance().applyStyleSheet(m_fdvGridEdit,
"QLineEdit[readOnly=\"true\"] { color: {{color.text.label}}; }");- Window integration coverage remains manual.
src/gui/MainWindow_Menus.cpp:626/src/gui/DxClusterDialog.cpp:561: the existing socket-freecanon_window_testcovers generic workspace keyboard, movement and persistence behavior, but no durable test constructs SpotHub through its menu and verifies repeated opening plus the old geometry key. The PR honestly discloses this, and independent bridge checks below passed. A focused, socket-free window-lifecycle test would preserve those guarantees; this migration does not require a synthetic feed peer.
Verification and attacks that held
- Built AetherSDR at the exact head on Linux/Qt 6.12 in a scratch checkout, using shared core and
-O2 -g1 -DNDEBUG. Built and ran the existing socket-free canon_window_test at the same head: 1/1 passed, including platform Close/Escape, hide-time persistence, workspace Return/Enter semantics and canon controls. No test is added or modified by this PR, so there is no new regression test to mutate. - Drove only the isolated offscreen instance
pr-6401-head, explicit socket/tmp/aethersdr-pr6401-head, private settings and TX disabled. Connected exclusively toDEMO-0001; the bridge returned"model":"AetherSDR Demo","serial":"DEMO-0001","transmitting":false. Existing operator/author instances were left untouched, and the review instance is now closed. - Invoked Settings → SpotHub… twice: exactly one SpotHub window, initially
{"h":680,"w":760,"x":360,"y":60}. Selected and grabbed all ten tabs, including FreeDV, Spot List and Display. Canon header/close button, selected cyan tab, source primaries, checkbox indicators and checked green toggles rendered; Freeze fit its button. Ground dragging moved the window. Resized to 900×760, closed and reopened it, then restarted the process: the geometry restored as{"h":760,"w":900,"x":445,"y":80}and the persisted old key contained445,80,900,760. A separate restart seeded with Qt's legacy base64 geometry restored{"h":720,"w":850,"x":200,"y":180}. - Source audits checked deferred log callbacks/timers during teardown, source connection/default preservation, fresh-instance signal wiring, scoped QSS precedence, disabled/selected states and theme indicator refresh. The engine-boundary check has 0 strict blockers; touchpoint manifest, capability records, command plane, a11y, generated reference and screenshot embedding checks pass. The color ratchet reproduces −43 references / −40 stylesheet calls against the parent.
Limits: no live radio or spot traffic; native taskbar/compositor behavior, physical edge resizing and macOS/Windows GUI appearance were not exercised. The bridge cannot reach FramelessResizer's native-window event filter, so its edge-drag result was not counted as a resize test. The resizer installation and workspace flags were inspected. The optional code-review skill is unavailable; equivalent manual and parallel source audits were performed. Local captures/logs are under /tmp/pr6401-runtime/head/; the existing user screenshot remains the disclosed follow-up.
All required checks (build, check-windows, check-macos, Static checks) and docs CI passed at this head. Earliest head run creation was 2026-10-12T02:01:35Z; current main 82077256 was committed 2026-10-12T01:12:06Z, so these checks have no newer-main freshness gap at review time. Recommendation: approve with the two non-blocking nits; formal approval remains the operator/CODEOWNER's decision.
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
Yes. The PR tracks #6239 ("Part of #6239"), which has no SpotHub-specific child issue, so I reviewed against the tracking issue's rulings plus the PR's own stated intent. Every bullet in the commit message maps to a hunk and each one holds up: DxClusterDialog now derives CanonWindow with Kind::Workspace (src/gui/DxClusterDialog.h:84, src/gui/DxClusterDialog.cpp:546), geometry persists under the old key (:561), FramelessResizer::install(this) matches NetworkDiagnosticsDialog.cpp:340 and PskReporterMapDialog.cpp:469 exactly, and MainWindow creates it directly instead of through showOrRaisePersistent() so View → Frameless Window no longer reaches it (src/gui/MainWindow_Menus.cpp:626-640). The three style docs and the user page are updated consistently; I found no doc left describing SpotHub as a persistent/frameless-title-bar window.
One bookkeeping gap: #6239 says "Each item below is its own issue and its own PR" and "Items under Candidates are not approved scope until the maintainer picks them." SpotHub is not in the Picked list — the nearest entry is Candidates → Persistent tool dialogs, which is scoped to a base-token→color.canon.* migration, not a CanonWindow conversion, and docs/style/dialog-patterns.md:275 still reads "Use CanonWindow only for … the tool windows the maintainer has moved to it." Since the author is the maintainer (GOVERNANCE.md:82, @ten9876), this PR is the pick, so it is not a governance blocker — but please add the SpotHub row to #6239's Picked list so the tracking issue and the style docs don't disagree.
Scope
| File / group | What it changes | Claimed by the body? | Verdict |
|---|---|---|---|
src/gui/DxClusterDialog.{h,cpp} |
PersistentDialog → CanonWindow workspace; one kSpotHubStyle sheet replaces ~30 per-widget sheets; remaining hexes → color.canon.* tokens |
Yes | In scope |
src/gui/MainWindow_Menus.cpp:621-648 |
Direct construction + WA_DeleteOnClose in place of showOrRaisePersistent() |
Yes ("MainWindow creates it directly") | In scope |
docs/style/{aethersdr-style-guide,dialog-patterns,theme-style-guide}.md |
SpotHub added to the canon-window, geometry-opt-in, Workspace-kind and canon/spotHub scope lists |
Yes ("docs/style lists SpotHub") | In scope |
docs/user/docs/spothub.md:27 |
New paragraph describing the chrome | Yes | In scope — wording matches the AetherGPS/AetherMap paragraphs verbatim |
Nothing in the diff is unexplained: no CI/build/vendored churn, no new settings key (the geometry key is reused), no new public or protocol surface, no CHANGELOG.md entry. Removals are all stylesheet strings — I read every - line and the only deleted comments were the kSpotHubToggle block comment and the "green checked / red unchecked" note on the Display-tab toggles (which was already wrong: kSpotHubToggle had no red state). No guard, early return or confirmation was removed. No sibling implementation is left behind — SpotHub was the only DxClusterDialog surface, and grep finds no other reference to it outside MainWindow_Menus.cpp and a stale-comment mention in src/core/ThemeManager.cpp:1544.
Security audit (security-audit (CodeGuard)): no findings in this change. The canon sheet is a static constexpr literal, applyCanonSheet(bodyWidget(), …) (:570) takes no runtime data, and every dynamic sheet interpolates only QColor::name() (a normalised #rrggbb), so the hex→token rewrite opened no stylesheet-injection path. The CodeGuard list is not PR-scoped — 19 files were scanned against 7 changed, and MainWindow.cpp, RadioModel.*, tools/check_command_plane.py and the tests/ files are not in this diff; their CS-VLN-CMDI-*/CS-PII-IP/CS-INJ-* hits are the known lexical false positives (C++ &&/||, Markdown backticks in // comments, 224.0.0.1 as the WSJT-X multicast default at DxClusterDialog.cpp:1450-1451, box-drawing rules read as homoglyphs). The audit did raise one pre-existing, out-of-diff item for a separate issue: src/gui/RadioSetupDialog.cpp:2080,2119 writes the MCP bridge token to the clipboard on bridge-enable and on Rotate, not only from the Copy handler.
Blockers
None.
Nits (all non-blocking)
src/gui/DxClusterDialog.cpp:353-355— the sheet has noQMenubackground, so built-in context menus inheritbackground: transparent. The sheet opens withwhich cascades to every descendant ofQWidget { color: {{color.canon.inkSoft}}; background: transparent; }bodyWidget(). The window's ownQMenu(:2620,ScopedChildWidget<QMenu> menuOwner(this)) is parented tothis, so it escapes — but Qt's standard context menus forQLineEditandQPlainTextEditare created with the widget as parent, so right-clicking a console or any field lands a menu inside the sheeted subtree whose only matching background declaration istransparent(and a widget-chain sheet outruns the app stylesheet that normally makes menus opaque). The sheet this one cites as its model guards exactly this,src/gui/PskReporterMapDialog.cpp:208-211:Reasoned from code, not reproduced — I cannot run the GUI. Drop-in, after theQMenu, QComboBoxPrivateContainer { background: {{color.canon.raised}}; }QLabelrule:QMenu, QComboBoxPrivateContainer { background: {{color.canon.raised}}; }src/gui/DxClusterDialog.cpp:2375and:2409— two fields keep base-token sheets, against the body's "Per-widget field, console, table, tab and toggle sheets are gone in favor of that sheet":They survive because they carry a read-only dimming rule the canon sheet lacks, so the FreeDV callsign and grid fields will read square-cornered and a different shade from every other field in the window. Add the state toAetherSDR::ThemeManager::instance().applyStyleSheet(m_fdvCallsignEdit, "QLineEdit { background: {{color.background.0}}; color: {{color.text.primary}}; border: 1px solid {{color.background.1}}; padding: 3px; }" "QLineEdit[readOnly=\"true\"] { color: {{color.text.label}}; }");
kSpotHubStyleand delete both calls:QLineEdit[readOnly="true"] { color: {{color.canon.muted}}; }src/gui/DxClusterDialog.cpp:433-461— the sheet's fourQComboBoxrule blocks are dead: SpotHub creates no combo box (grep QComboBoxmatches only the sheet). Harmless, but it also means the missingQComboBox::drop-downrule thatNetworkDiagnosticsDialog.cpp:196carries costs nothing here.src/gui/DxClusterDialog.cpp:3075—dxccTitlenow mixes vocabularies:color: {{color.canon.cyan}}withborder-top: 1px solid {{color.background.2}}.{{color.canon.line}}would finish the conversion.src/gui/DxClusterDialog.cpp:1923— the EiBi server label is still{{color.text.label}}while the POTA (:1774) and FreeDV (:2256) server labels moved to{{color.canon.muted}}.- No test accompanies the change. Correctly so, in my reading: this is window chrome and a stylesheet, with no deterministic socket-free seam, and
tests/canon_window_test.cpphas no adoptee inventory that needs a row. Naming it so "no test" and "didn't check" don't look alike.
What I tried to break
- Dangling pointer on the new
WA_DeleteOnClosepath.MainWindow_Menus.cpp:635sets it on a hand-built dialog, butm_spotHubDialogis aQPointer<DxClusterDialog>(MainWindow.h:1666), thewasFreshguard re-reads it, and the whole re-wiring block is gated onwasFresh— so a destroyed window recreates and reconnects cleanly. This is also not new:showOrRaisePersistent()(MainWindow.h:2263-2270) already setWA_DeleteOnClose, andPersistentDialog::closeEvent()accepts the event, so close already destroyed the old dialog. No service dies with the window either — every spot client lives onMainWindow, unlike AetherModem's KISS server (MainWindow_DigitalModes.cpp:217-224), which is why that one deliberately avoids the attribute. - Geometry migration — the body's "saved geometry carries over" claim.
PersistentDialogsaved a base64saveGeometry()blob;CanonWindowsaves"x,y,w,h".CanonWindow::restoreGeometryFromSettings()(src/gui/CanonWindow.cpp:152-182) falls through torestoreGeometry(QByteArray::fromBase64(...))when the value is not four comma-separated ints, so an existingDxClusterDialogGeometryis read once and rewritten on the next save. A window saved at the old 560-tall floor comes back clamped byexpandedTo(minimumSize())to the new 600. The claim holds. Both close paths flush:✕→close()and Esc/Ctrl+W →reject()both hide, andCanonWindow::hideEvent()saves and callsAppSettings::save(). - Every token the sheet names. All 12
color.canon.*keys pluscolor.toggle.success.{background,border}.checkedexist in bothresources/themes/default-dark.jsonanddefault-light.json(and inThemeSeedGenerated.cpp:82-96, 213-214);color.toggle.success.border.checkedhas precedent atGreenHeronApplet.cpp:62.canon/spotHubis not a defined scope in either theme — but neither arecanon/aetherMap(PskReporterMapDialog.cpp:464) orcanon/aetherGps(GpsLocationDialog.cpp:329), so resolution falls back to thecanonscope, which is exactly what the comment at:556-558relies on for the tabs' remainingcolor.text.*/color.accentsheets. - Contrast, dark theme, computed from the resolved values — all twelve pairs the sheet creates clear
docs/a11y.md's 4.5:1: checked toggleinkon#0060406.77, primaryonAccentoncyan12.71 (hover onaqua15.18),cyanoncontrol10.47, unselected tabmutedonraised6.39, selected tab 12.43, consoleinkSoftonnested11.61, disabled primarymutedoncontrol5.39, fieldinkoncontrol14.01, table 15.50. I ran dark only; #6239 records the light canon values as AI-generated and not human-managed, with dark as the canon priority. - What the window loses by leaving the tracked list.
m_persistentDialogsdrives nothing butsetFramelessMode()propagation (MainWindow.cpp:10305-10314) — no close-all, no shutdown flush — so dropping out of it costs only the thing the PR intends to drop, and matches AetherMap/AetherGPS. - Double-install and shortcut collisions.
CanonWindow's constructor does not installFramelessResizer(so the explicit install at:569is required, not doubled), and SpotHub declares noQShortcut/QKeySequenceof its own, soCanonWindow'sQKeySequence::Closecollides with nothing. InKind::Workspace, Return no longer auto-clicks the first button, which the cluster/RBN command line edits want. - CI: all 10 checks green on 78052c6, including
check-windows(the-DENABLE_ASR=OFFlane),check-macos,build,Static checksandBuild docs site. I also rantools/audit_colours.pyandtools/check_a11y.pyagainst the head checkout: neither reports anything inDxClusterDialog.cpp.
Everything above is reasoned from the head checkout at the PR's commit; I have no build and cannot drive the GUI, so no claim here was reproduced at runtime.
Recommendation
Approve with nits. The conversion follows the AetherMap/AetherGPS pattern down to the line, the geometry-migration and token claims both check out, and the net effect is ~250 lines of per-widget stylesheet replaced by one sheet. The QMenu background omission is the one thing I'd fix before merge — it's two lines and the sibling sheet shows the exact shape — and folding the two FreeDV field sheets in would finish what the body claims. Next step: add the QMenu/QComboBoxPrivateContainer and QLineEdit[readOnly="true"] rules, then tick SpotHub into #6239's Picked list. Thanks for keeping the user-doc paragraph consistent with the other canon windows.
🤖 aethersdr-agent · cost: $15.9456 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (235 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $16.0983 · model: claude-opus-5 |
Summary
Part of #6239. SpotHub (
DxClusterDialog) moves fromPersistentDialogto aCanonWindowworkspace, following the recipe AetherGPS (#6364) and AetherMap (#6340) use:CanonWindow::Kind::Workspace, an independent top-level window with its own taskbar entry. It no longer follows View → Frameless Window, so the Settings → SpotHub… handler creates it directly instead of throughshowOrRaisePersistent(), which would set frameless mode on it.DxClusterDialogGeometry), so saved positions carry over.Look. It sits under the
canon/spotHubtheme scope, which maps the tokens the tabs' remaining per-widget sheets use (status colors, labels) onto the canon palette, plus one canon sheet:control; the consoles and the spot list are nested, with a canon header row;color.toggle.success.*tokens instead of literals. The maintainer chose this over the canon aqua outline, because the fill is the only on/off signal on the Display tab's name-only toggles.Cleanup. About 60 per-widget field, console, table, tab, checkbox and toggle sheets are replaced by that one sheet, and the remaining hex literals (swatch borders, the server and DXCC/Signal History labels) are tokens.
kSpotHubToggleis gone.Docs:
docs/user/docs/spothub.mddescribes the window (move, resize, close, taskbar, Frameless Window).docs/style/*list SpotHub among the canon windows and its scope.spothub.pngshows the old window.Constitution principle honored
Principle VIII (Evidence Over Assertion) covers the checks below. The visual direction is RFC #6226's; the checked-toggle color was the maintainer's call.
Test plan
closeclosed the window and savedDxClusterDialogGeometry(570,237,760,680); reopening restored 760×680.check_engine_boundary,gen_touchpoint_manifest --check,check_capability_records,check_command_plane) andcheck_a11y.pypass. The color-audit ratchet is 43 references and 40setStyleSheetcalls belowmain.embed_screenshots.py --checkandgen_reference.py --checkpass.Checklist
AppSettingscalls; the existing geometry key is reuseddocs/user/docs/spothub.md,docs/style/*; noCHANGELOG.mdentry🤖 Generated with Claude Code