Repository navigation
feat(gui): AetherControl is a CanonWindow workspace re-skinned to the canon palette. Principle VIII. - #6429
Conversation
… canon palette. Principle VIII. Part of #6239. AetherControl (FlexControlDialog) moves from PersistentDialog to a CanonWindow workspace, following SpotHub (#6401) and AetherGPS (#6364). - Chrome: no title bar; the ambient ground, the canon header and the corner close button. Drag any empty area to move it, resize it from any edge, close it with Esc or Ctrl+W. While the knob has the mouse captured, the first Esc still only releases it. - Kind: Workspace, an independent top-level window with its own taskbar entry. It no longer follows View -> Frameless Window, so it is created directly instead of through showOrRaisePersistent(). - Geometry persists under its old key, FlexControlDialogGeometry. - Header (maintainer's choice): the canon header replaces the "AetherControl / Virtual Tuning Controller" wordmark card; Detect, Settings... and Compact sit in a row under it, outside the scroll area. - Look: the canon/aetherControl scope and one sheet of canon tokens replace the window's hex literals. Cards are raised, panels nested, buttons and combos on control with cyan text. The "on" states (aux LEDs, the selected Aux, a connected FlexControl, checked options, an armed capture) keep AetherControl's green through the toggle.success and accent.success tokens (maintainer's choice). - Knob (maintainer's choice): the painted metal knob is unchanged; only its capture ring and "Press Esc" badge move from literals to tokens, and it repaints on a theme switch. - The minimum width now counts the body margins, border and vertical scrollbar, which used to cover the right edge of the device card. Tests: aethercontrol_window_test pins the workspace kind, geometry under the old key, Esc releasing a capture before it closes the window, and the captured ring in color.accent.success in both themes. Each fails when its guard is broken (ring literal, Esc release removed, geometry key renamed). Docs: flexcontrol-tuning-knob.md describes the window; docs/style lists AetherControl among the canon windows and scopes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
![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. #6239 is a tracker rather than a defect report, so the requirement list comes from the PR's own stated intent plus RFC #6226's canon, and every item maps to a hunk: CanonWindow::Kind::Workspace + frameless flag (FlexControlDialog.cpp:999), the canon/aetherControl scope and the token sheet (:74-254), geometry under the old key (:1006), edge resize (FramelessResizer::install, :1008), the canon header with the control row beneath it outside the scroll area (:1014-1089), direct construction so View → Frameless Window no longer applies (MainWindow_Controllers.cpp:265-278), and the docs rows. Nothing in the diff is unexplained, and nothing claimed is missing.
Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/gui/FlexControlDialog.{h,cpp} |
PersistentDialog → CanonWindow workspace; token sheet; canon header; windowChromeWidth() |
Yes | In scope |
src/gui/MainWindow_Controllers.cpp |
showOrRaisePersistent() → direct construction + WA_DeleteOnClose |
Yes | In scope — matches the established canon pattern verbatim (MainWindow.cpp:3576-3590 RadioSetup, :4681-4685 AGC-T) |
tests/aethercontrol_window_test.cpp, tests/tests.cmake |
New test; flex_control_dialog_size_test sources retargeted to CanonWindow.cpp |
Yes | In scope |
docs/style/*.md (3) |
Add AetherControl to the canon window / scope lists | Yes | In scope |
docs/user/docs/flexcontrol-tuning-knob.md, docs/user/screens.json |
Section rewrite + alt text | Yes | In scope |
Everything in the diff is explained by the stated intent. No CHANGELOG.md entry (correct). No new settings key — the existing FlexControlDialogGeometry is reused. No new public, protocol or capability surface. The one bundled behaviour change beyond the chrome swap — the minimum width now reserving the scrollbar, which previously overlapped the device card's right edge — is disclosed in the body and is a direct consequence of the new body margins, so it belongs here.
docs/user/versioned_docs/version-stable/flexcontrol-tuning-knob.md:48 still carries the old "Virtual Tuning Controller" wording. That is correct and not a finding: it is the frozen Stable snapshot, re-taken by 34363a381.
Blockers
None.
Nits (non-blocking)
-
docs/user/screens.json:2093— the alt text now describes the new window:"alt": "AetherControl window with Detect, Settings… and Compact buttons under its header. …"while
docs/user/static/img/screens/aethercontrol-window.pngstill shows the old wordmark card. You disclose this ("Stale screenshot, to recapture after merge"), so this is a question rather than a complaint: since the entry already carries"grab": {"target": "FlexControlDialog"}and the capture is automated, is there a reason the recapture can't ride in this PR? Until it does, screen-reader users get a description that disagrees with the image they can't see — the one audience for whom the alt text is the whole artifact. -
tests/aethercontrol_window_test.cpp:188-196— thereleased != 0control is a slack-6 per-channel RGB probe over antialiased vector art, the same test genre2452cb555had to repair on main two commits ago. I checked it rather than assuming: all 18 hardcoded stops in the knob's paint (#00040a…#e0e8ef) are a blue-grey ramp, and none lands within 6 on any channel ofcolor.accent.successin either theme (#4dd87adark,#1a8040light), so the control is sound today. It is only the future that worries me — any knob restyle toward green flips this red for a reason that has nothing to do with the assertion's subject. -
src/gui/FlexControlDialog.cpp:238-253— the new scrollbar rules style:vertical,::handle:verticaland zero outadd-line/sub-line, but leave::add-page/::sub-pageunstyled. With a partially-styled QSS scrollbar Qt falls back to the platform style for the trough segments, which may not read as canon on every platform. Cosmetic.
Bookkeeping, for you as the tracker owner
AetherControl is not on #6239's Picked list, and the sentence this PR edits in aethersdr-style-guide.md:113 still gates canon coverage on "the candidates listed on #6239, once the maintainer picks them". I am not treating that as a governance finding: you are #6239's author and assignee, the body attributes each look choice to the maintainer, and #6273's status comment establishes that picks happen outside the tracker. But the picked list is now lagging by at least three rows — AetherControl, SpotHub (#6401) and Radio Setup — which makes the next reviewer run the same check I just did. Worth a line in #6239.
What I tried to break
- The headline claim — that the canon palette actually applies. The diff touches no theme resource, and
ThemeManager.cpp:296-297falls back to root, not to the nearest ancestor, when a scope path is unregistered — so an unregisteredcanon/aetherControlwould silently skip the canon re-tint and the PR would be a no-op re-skin. It is registered:theme::setContainer→registerDeclaredContainer→scopeOrCreate(ThemeManager.cpp:1908-1916) walks the segments and createscanon/aetherControlas a child of the seededcanonscope, and the call precedesapplyStyleSheetin the constructor. Held. - Legacy geometry under the reused key.
PersistentDialogwrote base64saveGeometry();CanonWindowwrites"x,y,w,h". An existing operator's saved value is therefore the wrong format for the new reader, which would silently drop it.CanonWindow::restoreGeometryFromSettings()has an explicit base64 fallback (CanonWindow.cpp:179-181), and base64 contains no comma so it can never take the 4-part branch by accident. The comment's "saved geometry carries over" is true. Held. - The ✕ while the knob has the mouse captured.
FlexControlDialog::reject()swallows the first Escape to release the capture; if the corner ✕ also routed throughreject(), the ✕ would be dead on first click and the new user doc (which scopes the exception to Esc) would be wrong.CanonWindow.cpp:74connects it to&QDialog::close, andcloseEventreleases the capture before delegating. Doc is accurate. Held. - What
trackPersistentDialog()drops. The new comment claims the only loss is View → Frameless Window.m_persistentDialogsis walked in exactly one place (MainWindow.cpp:10238-10246) and does exactly one thing —dlg->setFramelessMode(on). Nothing else keyed off the tracked set. Claim is complete. - Lifetime.
WA_DeleteOnCloseon a member:MainWindow.h:1697isQPointer<FlexControlDialog>, and all ~20 call sites null-check, including the two lambdas at:285/:290. Escape routes throughreject()→hide()so it never deletes; the ✕ does. Both were already true undershowOrRaisePersistent, so this is not a change. windowChromeWidth()double-counting the scrollbar. Checked both states: with the bar hiddenwidth() - m_scrollArea->width()is the margins alone and the bar is deliberately reserved; with it shown the subtraction already removes it, so+ sizeHint().width()restores exactly one. The pre-show fallback2 * (kBodyMargin + kInset)= 30 is exact, not an estimate (14+14 body margins + 2×1 border). No path reaches theisVisible()branch before a layout pass —FlexControlDialoghas noshowEvent, and the only two call sites are the constructor and the Compact toggle.- Stuck maximum height after leaving Compact.
applyCompactWindowSize()callssetFixedHeight(), which pins the maximum;updateCompactMode()resetssetMaximumHeight(QWIDGETSIZE_MAX)andsetMinimumSize(0,0)on entry (:1722-1723). No trap. - Contrast on the new pairings, resolved per theme rather than eyeballed.
canon.inkontoggle.success.background.checkedis 6.86:1 dark (#eaf2fbon#006040) and 10.97:1 light (#1a2a3aon#c8e8d0) — the light ramp inverts (gray.50=#1a2a3a), so canon surfaces stay legible in both.canon.cyanoncanon.controlis the tightest at ~4.67:1 light, still overdocs/a11y.md's 4.5:1 for 12 px bold. No regression. - Tests that pass against unfixed code. They don't.
capturedRingIsTheSuccessTokenloops both themes, and the pre-PR literal#65d379is >6 offaccent.successin each (24 off dark, 75+ off light), so a revert to any single hex fails at least one theme. The workspace-kind and old-key-geometry slots both assert shapesPersistentDialogcannot produce. Your three mutation runs are author-reported and I can't reproduce them, but the assertions are structurally capable of failing. - Link gaps from the retargeted source lists.
FramelessMoveHelper.handTheme.hare header-only, so swappingPersistentDialog.cpp/FramelessWindowTitleBar.cppforCanonWindow.cppleaves no undefined symbol;flex_control_dialog_size_test.cppis unmodified and touches only the public surface plus the still-present "Compact" button text, so it keeps compiling.aethercontrol_window_testis inAETHER_SETTINGS_CONSUMERS(tests.cmake:8712) and usesTestSettingsProfile, per the CMake contract. - Security. security-audit (CodeGuard) returned no findings: all 19 leads are refuted — the CMDI hits are C++
||and reference declarators plus a Markdown backtick in a//comment, the SSRF/PII hits are RFC 5737/2606 documentation addresses inside"alt"strings — and none of the flagged lines intersects this diff.
Could not check: anything at runtime. I have no build and no GUI, so every statement above is reasoned from the head checkout at 1601eb3, not reproduced. CI is green on all 10 checks, but ci.yml has no ctest -R filter containing aethercontrol_window_test or flex_control_dialog_size_test — so green proves the new target compiles on Linux, macOS and Windows, and proves nothing about whether its four slots pass. They run post-merge in the full suite. The two gaps you name yourself (no physical knob, no live radio) I also cannot cover.
Recommendation
Approve with nits. This is a clean migration: the scope is exactly what the title says, the two mechanisms most likely to break silently — scope registration and legacy geometry under a reused key — both have real answers in the code rather than hopeful comments, and the new test is a genuine guard rather than a restatement of the implementation. The three nits are all non-blocking; the only one I'd like an answer on before merge is whether the screenshot recapture can ride along, since the alt text in this PR already describes the window the image doesn't show. Next step: a maintainer merge decision, plus the AetherControl/SpotHub/Radio Setup rows on #6239's picked list.
🤖 aethersdr-agent · cost: $19.7882 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (19 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: $19.9856 · model: claude-opus-5 |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Reviewed head 1601eb30d29b53cd1d78185190832b145cd0a1ca. Issue fit: mostly. #6239 is the canon migration tracker, rather than a defect report; RFC #6226 is approved, and the body attributes the surface's look choices to the maintainer. Workspace kind, token scope, header/control placement, old geometry key, scrollbar reservation and direct MainWindow construction map to the stated requirements. The migration works in the checks below, except the newly inherited close shortcut does not close a captured knob's window as documented. Recommendation: Request changes.
Scope
| File/group | Change | Claimed? | Verdict |
|---|---|---|---|
src/gui/FlexControlDialog.{h,cpp} |
CanonWindow workspace, scoped tokens, header, chrome sizing, capture accents and theme repaint | Yes | In scope |
src/gui/MainWindow_Controllers.cpp |
Direct construction, WA_DeleteOnClose and show/raise/activate | Yes | In scope |
tests/aethercontrol_window_test.cpp, tests/tests.cmake |
New socket-free window test and retarget existing size test to CanonWindow | Yes | In scope |
docs/style/aethersdr-style-guide.md, dialog-patterns.md, theme-style-guide.md |
Register the migrated window and scope in the canon docs | Yes | In scope |
docs/user/docs/flexcontrol-tuning-knob.md, docs/user/screens.json |
Workspace behavior, close instructions and screenshot alt text | Yes | In scope; screenshot explicitly disclosed as stale |
Everything in the diff is explained by the stated migration. One relevant October 13 authored commit; no unrelated work, new settings key, protocol/capability surface, theme-token value change, or CHANGELOG entry. The minimum-width change and green on-state choices are disclosed. The body checklist's scope/persistence/docs claims hold, subject to the close behavior finding below. #6239's picked list does not yet name this surface, but the maintainer-authored PR explicitly attributes its choices to the maintainer; I am not treating that bookkeeping lag as unauthorized design.
Blockers
-
Ctrl+W releases capture instead of closing —
src/gui/FlexControlDialog.cpp:1889–1894.CanonWindow.cpp:78–79adds a QKeySequence::Close shortcut that invokes virtualQDialog::reject(). The retained override suppresses every rejection while the wheel is captured, so Ctrl+W reaches it and only releases capture. The body anddocs/user/docs/flexcontrol-tuning-knob.md:50say Ctrl+W closes, with only Escape having the release-first exception. This shortcut is newly exposed by the base-class migration; the corner ✕/close-event path works.Reproduced offscreen with the production widget and a real Qt key sequence: show window → double-click the enabled knob → assert armed →
QTest::keyClick(knob, Qt::Key_W, Qt::ControlModifier):CTRLW visible true armed false FAIL! : AetherControlWindowTest::ctrlWClosesWhileCaptured() '!window.isVisible()' returned FALSE. (Ctrl+W should close the captured workspace)Restrict rejection suppression to the existing Escape deduplication flag. This drop-in replacement was compiled in scratch and passed all eight checks, including the new Ctrl+W probe, legacy base64 import and the original Escape-release test:
void FlexControlDialog::reject() { if (m_ignoreNextReject) { m_ignoreNextReject = false; releaseWheelCapture(); return; } CanonWindow::reject(); }
Corrected output:
CTRLW visible false armed false,Totals: 8 passed, 0 failed. Add the captured-close-shortcut test to the delivered suite. This is a Linux Ctrl+W reproduction; the same QKeySequence::Close routing is used on macOS, but ⌘W was not executed locally.
Nits — explicitly non-blocking
- Legacy migration is not pinned by the new test —
tests/aethercontrol_window_test.cpp:101–132. It empties the old key and writes a new CanonWindow CSV value, so it never tests the header comment's claim about saved pre-migration base64 geometry. My additional scratch test seededQDialog::saveGeometry().toBase64()underFlexControlDialogGeometry; head successfully restored 660×700. Add this as a regression case so removal of the CanonWindow base64 fallback cannot silently lose existing geometry. - Screenshot/alt mismatch —
docs/user/docs/flexcontrol-tuning-knob.md:52,docs/user/screens.json:2093. The alt text describes the new header/control arrangement, while the unchanged PNG still depicts the old wordmark card. The body explicitly identifies this stale shot, as AGENTS.md permits, so it is not a blocker; recapture the image as disclosed.
What I tried to break / verification
- Test-boundary preflight passed: the new window test and retargeted size test use standalone widgets/slice state, with no serial port, socket or synthetic firmware peer. The settings object-library wiring, settings-consumer registration, and TestSettingsProfile isolation are present.
- Ran all six named socket-free targets from the clean exact-head build:
aethercontrol_window_test,flex_control_dialog_size_test,canon_window_test,theme_manager_test,theme_seed_test,ulanzi_mapping_migration_testall exited 0. Separately compiled scratch Qt probes and alternate dialog objects, reusing unchanged objects from that build; no shared checkout/build was modified. The new production-widget probe found the blocker (7 passed, 1 failed); the focused correction passed all eight. - Independently repeated the three claimed mutations: replacing the captured ring with the old literal failed the ring slot; renaming the geometry key failed the save/restore slot; omitting capture release failed the Escape slot. The original test reports dark/light ring pixels
released 0, captured 782/864. These tests detect their subjects; I did not adopt the prior review's speculative future-green-knob concern as a current defect. - Full desktop bridge: offscreen, fresh isolated settings, NO_TX, identity
pr-6429-review, explicit socketaethersdr-pr6429-review; connected onlyDEMO-0001, confirmedmodel="AetherSDR Demo"andtransmitting=false. Opened via Settings → AetherControl, grabbed and inspected Dark, Light and Compact, toggled Compact both ways, captured the knob and closed/reopened. Reopened capture hint returned toDouble-click the knob to capture circular tuning.Afterresizereturned{"actual":{"h":700,"w":640}}, closed, restarted the process, and reopened: dumpTree restored{"h":700,"w":640,"x":449,"y":95}. Review instances were disconnected/closed; no operator session or physical radio was driven. The bridge does not expose arbitrary window-shortcut injection, so Ctrl+W evidence is from the running Qt widget fixture, not a bridge key injection. Physical-knob detection, native taskbar behavior, and macOS/Windows gestures remain unverified locally. - Ran all four engine/seam checks on exact source exports of base
34363a38and head, comparing complete per-file outputs: identical. 102 tracked engine-boundary findings, zero strict blockers; 70 capability booleans; 297 raw commands in 30 files; touchpoint manifests current and unchanged. Lifetime/sizing audit also survived: QPointer member ownership, receiver-bound timers/theme repaint, capture release on hide/destruction, and one scrollbar-width reservation. Prior CodeGuard warnings at MainWindow logical-OR/bit-operation/string/comment lines and unrelated screens.json endpoints are not shell-execution or network changes in this diff; no claimed vulnerability was substantiated.
CI: every required check and the Docs build passed. Earliest head run creation was 2026-10-13 03:48:39 UTC; current main's observed tip is 34363a38 from 2026-10-12 23:18:11 UTC, also the merge base. No stale-main timing gap was found. PR checks do not establish a full-suite run. The optional automated code-review skill was unavailable; this pass used manual audit plus an independent parallel lifetime/sizing review.
Next step: fix the captured close shortcut and add its socket-free regression case; then rerun the focused window tests and required checks. GitHub authentication is the PR author's account, which cannot submit a self REQUEST_CHANGES review, so this single review is posted as COMMENT with the Request changes recommendation preserved.
rfoust
left a comment
rfoust
left a comment
There was a problem hiding this comment.
Reviewed head 1601eb30d29b53cd1d78185190832b145cd0a1ca. Issue fit: mostly. Recommendation: Request changes. The workspace migration matches tracker #6239 and approved RFC #6226, including the stated maintainer choices, but the newly inherited Close shortcut does not close while the knob is captured.
Scope
| File/group | Change | Claimed? | Verdict |
|---|---|---|---|
src/gui/FlexControlDialog.{h,cpp} |
CanonWindow workspace, scoped tokens, header, chrome sizing and theme repaint | Yes | In scope |
src/gui/MainWindow_Controllers.cpp |
Direct construction, WA_DeleteOnClose, show/raise/activate | Yes | In scope |
tests/aethercontrol_window_test.cpp, tests/tests.cmake |
Socket-free window coverage and retargeted size test | Yes | In scope |
docs/style/*.md |
Document the migrated window and scope | Yes | In scope |
User guide and docs/user/screens.json |
Workspace behavior and screenshot alt text | Yes | In scope; stale screenshot disclosed |
Everything in the diff is explained by the stated migration. No unrelated changes, new settings keys, public protocol/capability additions, or CHANGELOG entry. The body's scope and persistence checklist holds, subject to the close-behavior blocker.
Blocker
- Captured knob swallows Ctrl+W — see the inline comment on
FlexControlDialog.cpp:1894. Independently reproduced in the running production widget: Ctrl+W releases capture but leaves the workspace visible, contradicting the documented Escape-only exception. Restrict rejection suppression to the Escape deduplication flag and add a captured-close regression test.
Nits — explicitly non-blocking
- Legacy geometry import is not pinned — see
aethercontrol_window_test.cpp:104. An additional scratch probe confirmed base64 import works; the delivered test exercises only newly saved CanonWindow geometry. - Screenshot/alt mismatch — see
flexcontrol-tuning-knob.md:52. The unchanged PNG shows the old window. This is disclosed in the body and is non-blocking; recapture it as planned.
Verification and attacks
- Constructed a clean scratch merge of head
1601eb30with observed current main47aa4098. GitHub merge refa06044e5still uses base34363a38; the green required checks predate that main advance and do not run the new window test. - Test-boundary preflight passed: the changed targets do not open sockets or serial ports or emulate firmware. ARM64/Qt 6.12 focused builds succeeded, with RADE left enabled; ARM64 processors and absence of RNNoise x86 sources were checked.
aethercontrol_window_test,flex_control_dialog_size_test, andcanon_window_testall passed. - Added a scratch-only production-widget Ctrl+W probe. Uncorrected routing returned
CAPTURED CLOSE "Ctrl+W" visible true armed falseand failed. Limiting suppression tom_ignoreNextRejectreturnedvisible false armed false; all eight checks passed, including the original Escape-release test and an added legacy-base64 import case. - Replacing the captured ring token with its old literal failed the ring slot in both themes. Restored production code produced released/captured token-colored pixel counts of
0/782dark and0/864light. The test detects that regression. - Test registration, generated reference pages, and focused accessibility checks passed. Manual lifecycle/scope review found QPointer ownership, receiver-bound theme repaint, capture release on hide/destruction, and legacy geometry fallback intact. Prior CodeGuard leads were not substantiated as vulnerabilities in this diff.
- Full desktop build was started but not completed; no demo bridge session was run. Runtime reproduction uses the offscreen production-widget fixture. Native macOS Command-W, physical-knob detection, and taskbar behavior remain unverified. No operator session or live radio was driven. The optional automated review pass was unavailable.
Next step: fix the captured Close shortcut, deliver its socket-free regression case, and rerun the focused tests and required checks. Scratch changes were restored; the operator's checkout was preserved.
NF0T
left a comment
NF0T
left a comment
There was a problem hiding this comment.
Reviewed head 1601eb30d29b53cd1d78185190832b145cd0a1ca. Recommendation: Request changes, one blocker.
The migration itself is clean. Every hunk maps to the stated intent, and the two mechanisms most likely to fail silently both hold up. canon/aetherControl is registered before applyStyleSheet, and the pre-migration base64 geometry still restores through CanonWindow::restoreGeometryFromSettings() (CanonWindow.cpp:179-181). The captured-ring test is a real guard: its released control reads 0 and the captured knob reads 782 (dark) and 864 (light) token-colored pixels. The one thing I found is the same one @rfoust and the earlier maintainer-account pass reported, and I reproduced it on a third toolchain.
Scope
| File / group | Claimed? | Verdict |
|---|---|---|
FlexControlDialog.{h,cpp}: PersistentDialog to CanonWindow workspace, token sheet, canon header, windowChromeWidth() |
Yes | In scope |
MainWindow_Controllers.cpp: direct construction + WA_DeleteOnClose |
Yes | In scope; same shape as Radio Setup and AGC-T |
tests/aethercontrol_window_test.cpp, tests.cmake |
Yes | In scope |
docs/style/*.md (3), docs/user/docs/flexcontrol-tuning-knob.md, screens.json |
Yes | In scope; stale screenshot disclosed |
One commit, authored 2026-10-13. No new settings key, no protocol or capability surface, no CHANGELOG entry. Preference-vs-fix: this is a deliberate look change, and the body attributes each choice to the maintainer under RFC #6226 / #6239. The PR is the maintainer's own, so I am not treating it as an unauthorised preference change.
Main is now one commit ahead of the merge base (47aa4098, CTR2) and touches MainWindow_Controllers.cpp. A trial merge of this head onto current main is clean.
Blocker
Ctrl+W (and Ctrl+F4 on Windows) release the knob's capture but never close the window. FlexControlDialog.cpp:1889 (reject()).
CanonWindow.cpp:78-79 wires QKeySequence::Close to QDialog::reject. That shortcut is new to this window, since PersistentDialog had none. FlexControlDialog::reject() swallows every rejection while m_wheel->captured(), so the shortcut only releases the capture. The body and flexcontrol-tuning-knob.md:50 both promise that Ctrl+W (⌘W) closes, with only Esc releasing first. The ✕ is unaffected because it connects to QDialog::close.
I reproduced it on Windows 11, MinGW 13.1 / Qt 6.12.0, offscreen, building the PR's own aethercontrol_window_test and adding a slot that double-clicks the knob and then sends each binding Qt reports for QKeySequence::Close:
CLOSEKEY Ctrl+F4 visible true armed false
CLOSEKEY Ctrl+W visible true armed false
FAIL! probeCloseShortcutWhileCaptured(): did not close: Ctrl+F4, Ctrl+W, Close
Restricting the suppression to the Escape-dedup flag fixes it. This is the same change @rfoust and @ten9876's pass proposed:
void FlexControlDialog::reject()
{
if (m_ignoreNextReject) {
m_ignoreNextReject = false;
releaseWheelCapture();
return;
}
CanonWindow::reject();
}With that applied, the same probe gives visible false armed false for all three bindings.
I wanted to check what the || m_wheel->captured() clause was protecting before agreeing to drop it. Every Escape route sets m_ignoreNextReject first (handleEscapeRelease() from event(), eventFilter() and keyPressEvent()), so I probed Escape sent to the window, to the knob, and to another focused child (an option button). With the fix, each one releases the capture, leaves the window open, and closes on the next Esc. Full run: 8 passed, 0 failed.
Please add a captured-close regression to the delivered test. Looping over QKeySequence::keyBindings(QKeySequence::Close) also covers the Windows binding without hard-coding Ctrl+W.
Sibling migrations (#6372, #6340) put "every close path" in their subject lines. This one is the close path that got missed.
Non-blocking
- Legacy-geometry import is unpinned (
aethercontrol_window_test.cpp:104). Agree with @rfoust. The fallback exists and I read it, but the delivered test only exercises the new CSV format. SeedingQDialog::saveGeometry().toBase64()under the old key would pin the "geometry carries over" claim. geometryPersistsUnderTheOldKeyAndRestoresdepends on font metrics. It assertswindow.minimumWidth() < 640as a precondition. On my machine, offscreen with no Qt font directory,minimumWidth()was 764 and the slot failed, and the Ctrl+W failure above is unrelated to it. WithQT_QPA_FONTDIR=C:/Windows/Fontsit passes. Choosing the target size relative tominimumWidth()(say minimum + 40) would remove the dependency. I did not check which CI lanes run this test.- Screenshot / alt text.
aethercontrol-window.pngis stale, and the alt text already describes the new window. It is disclosed, and AGENTS.md permits it. I'd lean toward recapturing in this PR sincecapture_screenshots.py --onlyis automated, but I am not asking for it.
Other reviews
- @rfoust and the maintainer-account pass: agree on the blocker and on the fix. I reproduced it myself, rather than relying on their output.
- aethersdr-agent ("Approve with nits"): I disagree with the verdict. Its ✕ check was right, since
closeEventreleases the capture. It stopped there and did not follow theQKeySequence::Closeshortcut intoreject(), so it vouched for a doc sentence that is false for Ctrl+W. Its slack-6 probe concern is not a current defect. All 18 hardcoded knob stops are blue-grey, and the ring slot measured 0 released against 782 and 864 captured. - CodeGuard's 19 leads sit on lines outside this diff, and I found nothing to substantiate them.
What I tried to break
- Test-boundary preflight: the added test and the retargeted size test construct bare widgets and a standalone
SliceModel. There are no sockets, serial ports or fake-firmware peers. - Reproduction: built
aethercontrol_window_testfrom the PR head in an isolated worktree and build dir. Unmodified: the PR's slots pass with fonts available, and my close-shortcut probe fails as shown. With the fix: all pass.flex_control_dialog_size_testalso passes (6/6). - Escape release-first, from window, knob and a sibling child, before and after the fix.
reject()vsclose(): the corner ✕ goes throughclose(), so it is unaffected.- Governance: Principle VIII matches the citation on #6401 and #6413. CODEOWNERS is Tier 3 only (
src/falls to*,docs/andtests/to@aethersdr/reviewers)..github/ci-test-gate.txtis unchanged. CI runs noctest -Rfor either window test, so green proves the target compiles, not that its slots pass. - Not tested: I did not drive the demo bridge. This is a key-routing defect reproduced at the widget level, and the maintainer-account pass already drove the bridge. macOS ⌘W, a physical knob, and native taskbar behaviour are unverified by me. One unverified side note: the
captured()clause also backstopped any Escape-equivalent that reachesQDialog::keyPressEventwithout passinghandleEscapeRelease(), such as macOS Ctrl+. as I recall the Qt source. I did not check it. Worth a look on a Mac after the change.
Next step: apply the reject() change, add the captured-close regression, and re-run aethercontrol_window_test and flex_control_dialog_size_test. I'll approve on the next push if nothing else turns up. The squash subject would be the current title plus (#6429): feat(gui): AetherControl is a CanonWindow workspace re-skinned to the canon palette. Principle VIII. (#6429).
Thanks for another careful canon migration. The scope discipline, the controls in the ring test, and the disclosure of the stale screenshot made this easy to review.
73, Ryan NF0T & Claude (AI dev partner)
👨🏼💻 Co-authored by Claude Sonnet 5.5
Retired by efad631: the captured Close shortcut now closes via the Escape-only reject guard at FlexControlDialog.cpp:1889. Delivered platform-binding regression failed on the old guard (Ctrl+W/Ctrl+F4/Close) and passes now; Escape window/knob/sibling routing, legacy base64 geometry, and font-independent geometry sizing pass. Final aethercontrol_window_test: 12 passed, 0 failed; six selected CTests pass, full desktop build passes, and a private TX-disabled DEMO-0001 bridge demonstrates capture-close/recreate, both themes, Compact, and geometry across restart. Stale screenshot recaptured and inspected; evidence posted in the review threads and PR body. Hosted CI is still running; no claim of post-merge validation.
|
Remediation is now on verified head I independently inspected the CodeGuard leads on the original head. The C++ lines are boolean The ring-test concern is hypothetical: on the final production widget, the released control has 0 success-colored pixels and the captured knob has 782 (Dark) / 864 (Light). The test’s current control holds, and both themes pass. No knob restyle is part of this PR. The bot’s potential scrollbar trough difference is an optional cosmetic suggestion rather than a reproduced defect; the maintainer-approved stylesheet is retained as explicitly directed by the operator ("Leave current scrollbar styling"). This does not broaden the accepted migration’s palette. Tracker #6239 bookkeeping is outside this PR’s implementation and is not a correctness finding, as the existing reviews explicitly note. Final local evidence: desktop build passed, 6/6 selected CTests passed, AetherControl fixture 12 passed / 0 failed, before-fix captured-close rows failed, documentation build passed, and the architecture gates have identical findings on main and head. The private TX-disabled DEMO-0001 bridge proved capture/close/recreation, Dark/Light/Compact rendering and 700×1200 geometry restoration across restart. All proof processes have stopped. Native Cocoa Command-W, taskbar behavior, a physical knob, VoiceOver, and live hardware remain unverified. PR CI is running; post-merge CI is still outstanding. |
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Verified head efad6319817eb2458a876e291fb94ccb7c23410d after the final push, integrated with main fc434510e45fe751e65e9cede63a29205c3c6198.
FlexControlDialog.cpp:1889: the Escape-only rejection guard fixes captured platform Close shortcuts. The delivered regression fails on the prior guard (Ctrl+W, Ctrl+F4, Close) and passes on this head. Escape from window, knob, and sibling control still releases first and closes on the next press.aethercontrol_window_test.cpp:105–156: legacy base64 geometry import is now exercised, and the CSV size assertion derives from the widget minimum rather than a font-dependent width limit.- The AetherControl screenshot is recaptured, visually inspected, and matches its alt text; before/after images are in the PR body.
Empirical proof: final ARM64/macOS Qt 6.12 desktop build passed; six headless CTests passed; AetherControl reports 12 passed, 0 failed. Authenticated, isolated-settings, TX-disabled DEMO-0001 bridge verification covered capture/close/recreation, Dark/Light, Compact (700×744), and geometry restoration (700×1200) across a process boundary. Both proof processes were stopped. Docs site and static checks pass locally; all four architecture gates produce identical main/head findings. All three review threads have evidence replies and are resolved; both blocking reviews were dismissed only after their requests were demonstrated.
CodeGuard leads are refuted by their actual source content (C++ expressions/declarations and passive example-address descriptions). The current ring control reads 0 released / 782 Dark / 864 Light captured pixels; its suggested future-restyle risk is not a current defect. The original approved scrollbar palette is retained; an optional cosmetic change was put to the operator, with no extra styling applied without direction. No new design or scope choice was needed for the fixes.
Manual adversarial pass checked capture cleanup on hide/destruction, receiver-bound repaint, QPointer recreation, legacy geometry fallback, scope registration and unchanged engine touchpoints. Automated code review was unavailable. Native Cocoa Command-W/taskbar behavior, physical hardware, live radio and VoiceOver remain unverified; widget and simulator evidence do not establish hardware behavior. This developer build has no QtKeychain/DFNR and is not a release-package proof.
CI freshness checked: earliest run on this head was created 2026-10-14T13:59:08Z; current main was committed 2026-10-14T06:55:52Z, so its tree precedes the fresh runs. Platform jobs are still running; this review does not claim they or post-merge main CI have passed. Landing remains contingent on all required checks and a final exact-head/main/thread/signature/merge-tree recheck.
|
Post-merge verification for squash commit
CodeQL and System Libraries Canary are still running; no success is claimed for either yet. The operator explicitly chose to retain the existing scrollbar styling. No live hardware or native Cocoa Command-W proof was performed. |
Summary
Part of #6239 and approved RFC #6226. AetherControl (
FlexControlDialog) uses aCanonWindow::Kind::Workspacewith the canon header and palette. Detect, Settings… and Compact remain above the scroll area; minimum width reserves the body margins, border, and scrollbar so the device card stays visible. The painted metal knob retains its appearance, while capture and on-state accents use theme tokens and repaint on theme changes.The workspace is always frameless and opens independently of View → Frameless Window. Its geometry uses the existing
FlexControlDialogGeometrykey, including import of legacy base64 geometry. Drag empty space to move it and resize from its edges. Escape releases an active knob capture first; subsequent Escape and the platform Close shortcuts close the window. Close shortcuts also close while captured.User and style documentation describe the migrated window. The AetherControl screenshot has been recaptured through the automation bridge on the built-in demo simulator and visually inspected.
Screenshots
Before:
After:
Validation
Verified final head
efad6319817eb2458a876e291fb94ccb7c23410d, integrated with mainfc434510e45fe751e65e9cede63a29205c3c6198:cmake --build build -j22with selected targets.aethercontrol_window_test,flex_control_dialog_size_test,canon_window_test,theme_manager_test,theme_seed_test, andulanzi_mapping_migration_test.Native macOS Command-W/taskbar behavior, a physical FlexControl, live radio operation, and VoiceOver were not exercised. Offscreen Qt exposes Ctrl+W/Ctrl+F4/Close; this is widget and simulator evidence. The optional automated code-review pass was unavailable. This developer app build has no QtKeychain or DFNR and is not a release-package validation.
Constitution
Principles VIII (Evidence Over Assertion) and XI (Fixes Are Demonstrated): local evidence above; PR CI and the squash commit’s post-merge CI are tracked separately during landing. No new settings key, protocol/capability change, workflow edit, or CHANGELOG entry.
Generated with OpenAI Codex (GPT-6 Astra)