Repository navigation
Conversation
There was a problem hiding this comment.
1. Issue fit
Yes — every bullet of #6755 maps to a hunk, and I verified each against the head checkout rather than taking the body's word:
| #6755 asks | Verified |
|---|---|
CanonWindow + Kind::Workspace |
FreeDvReporterDialog.h:28, .cpp:263 |
| Resize from any edge | FramelessResizer::install(this) at .cpp:273 — required, since CanonWindow only moves |
Keep FreeDvReporterGeometry |
.cpp:271; CanonWindow::restoreGeometryFromSettings() falls back to restoreGeometry(QByteArray::fromBase64(...)) when the value isn't four comma-separated ints, then saveGeometryToSettings() rewrites x,y,w,h. Carry-over claim holds. |
| Closing hides it | Holds, and for a load-bearing reason: CanonWindow sets WA_DeleteOnClose nowhere, and Esc / ⌘W go through QDialog::reject() → hide (CanonWindow.cpp:129). MainWindow holds a raw, non-QPointer m_freedvReporterDialog (MainWindow.h:1343), so a WA_DeleteOnClose here would have been a use-after-free on the next showFreeDvReporter(). It isn't present. |
| One canon sheet for nine | .cpp:117-249 |
| Bottom Close + separator gone | Confirmed in the diff |
No CanonMessageBox |
No QMessageBox on the path |
The body's "showFreeDvReporter() already created it directly … so MainWindow is unchanged" is true: MainWindow_DigitalModes.cpp:1239-1241 is a direct show()/raise()/activateWindow(), never showOrRaisePersistent(), so unlike the sibling PRs in this batch this one owes no call-site conversion and loses no View → Frameless Window behaviour it had.
2. Scope
Everything in the diff is explained by #6755. No out-of-scope files; git diff --name-only 1afefafdb..HEAD is exactly seven.
| File | Changes | Claimed? | Verdict |
|---|---|---|---|
src/gui/FreeDvReporterDialog.{h,cpp} |
base class, canon sheet, nine per-widget sheets + Close + separator removed | Yes | In scope |
docs/style/dialog-patterns.md |
four canon lists + the hide-on-close bullet | Yes | In scope — see the decision item below |
docs/style/aethersdr-style-guide.md, theme-style-guide.md |
window + canon/freedvReporter scope |
Yes | In scope |
tests/freedv_reporter_message_test.cpp |
4 new cases | Yes | In scope |
tests/tests.cmake |
PersistentDialog.cpp/FramelessWindowTitleBar.cpp → CanonWindow.cpp + ${THEME_TEST_RESOURCES} |
Yes | In scope; link closure matches flex_control_dialog_size_test, Qt6::Test was already linked |
No new settings key, no protocol surface, no base-token value changed. CHANGELOG.md correctly untouched.
3. Blockers
None. The implementation is clean; the one thing holding it is a ruling, not a defect.
4. Needs maintainer decision — this is an unlisted CanonWindow pick, and the docs edit asserts the pick already happened
Not a code problem, and not a governance violation — but the body doesn't surface it, and its sibling does.
#6239's own text: "Items under 'Candidates' are not approved scope until the maintainer picks them." FreeDV Reporter is on no list there — not the picked "tool windows on CanonWindow" set, and not the "short-lived windows that could move to CanonWindow" candidates. The only bullet that covers it is "Persistent tool dialogs (one PR per dialog)", which sits under "surfaces whose widgets move from base tokens to color.canon.*" — a re-skin that keeps PersistentDialog chrome. So "Part of #6239" in the body shouldn't be read as authorisation for a full Kind::Workspace move. docs/style/dialog-patterns.md:275-277 is the sentence the move contradicts, and this PR edits around it while leaving it standing:
and workspace dialogs keep the persistent pattern above.
Two things make it worth naming rather than waving through:
- The docs edits make merging the ruling.
aethersdr-style-guide.md:118anddialog-patterns.md:238now list FreeDV Reporter among "the tool windows the maintainer has moved to it" — true only once this lands. Sibling #6753 (same author, same day, same shape) says so out loud: "Waiting on the maintainer's pick … please don't merge before the pick. Merging is that pick." #6757 carries no such line despite being in the same position. - The body's one appeal to a ruling isn't on the record. "The maintainer reviewed the running window and kept the selected-band fill as drawn" — I checked the comments on both #6755 and #6757; the only non-bot comments are the author's own screenshots, and #6755 is still open and labelled
maintainer-review. The triage bot on #6755 reached the same conclusion independently ("this is an unlisted pick, not a reading of #6239"). If that review happened out of band, fine — but it isn't citable, and it's the only thing in the body resembling the pick.
Counterweight, in fairness: SpotHub (#6401), Radio Setup, Network Diagnostics, Waveforms and Updates all appear in these same doc lists without being on #6239's picked list, so unlisted moves of exactly this class have landed before. The precedent is real; the call is @ten9876's. Per the triage bot, this arrives as one of a wave (#6735, #6753, #6757 workspace; #6745, #6749, #6751 dialog) that is cheaper to rule once as a batch than per surface. Concrete next step: add #6753's "don't merge before the pick" caveat to the body, or get the batch ruled.
5. Nits (all non-blocking)
-
src/gui/FreeDvReporterDialog.cpp:172— the deleted Send-button sheet carried a comment naming a symptom and a prior ruling, which §3 says to quote back:// Widget×state tokens rather than generic chrome (docs/a11y.md): dimming the text alone left the disabled button reading as absent rather than greyed-out, which undercuts the row's own gating (#4231 review).The replacement is the opposite shape — fill removed entirely:
QPushButton:disabled { color: {{color.canon.muted}}; border-color: {{color.canon.line}}; background: transparent; }I checked before calling it a regression, and it is byte-identical to SpotHub's (
DxClusterDialog.cpp), i.e. the ratified canon idiom, and the gating still reads: resolving both bundled themes,canon.mutedoncanon.groundis 6.71:1 dark / 5.94:1 light, well clear ofdocs/a11y.md's 4.5:1. So the #4231 concern is answered differently rather than reintroduced. One residue worth a line: thecanon.lineborder is 4.03:1 against ground in dark but 1.55:1 in light, below the 3:1 non-text floor — so in the light theme the disabled Send is effectively text-only. That's a pre-existing property of the canon palette shared by every canon window, not something this PR introduces; it's only worth noting because this PR deletes the comment that guarded it. -
tests/freedv_reporter_message_test.cpp:285-289— the comment block is attached to the wrong function. Its first sentence describes the focus behaviour, which istestBandButtonsTakeTabFocusOnly()at:299; only the second belongs totestMinimumFitsBandRow(). Move the first two lines down one function. -
tests/freedv_reporter_message_test.cpp:289—testMinimumFitsBandRow()assertsminimumWidth() >= minimumSizeHint().width()on a never-shown window, which is the expression.cpp:280just assigned, so it can only catch deletion of that line. Clipping is a post-show property; re-checking aftershow()(as the two cases above it do) would make it catch a polish-ordering regression too. The disclosed 780-vs-825 mutation measurement is the real evidence here, and the assertion is at least relational rather than a hard pixel figure, so it won't drift with platform fonts. -
Body/evidence — the sibling PRs each disclose "CI does not run this test before merge." This one doesn't, and it's true:
freedvappears nowhere in.github/workflows/ci.yml, so the 5/5 green on24e0344proves the new test compiles, not that its checks ran; they run post-merge infull-suite.yml. Worth stating, since Principle VIII is the stated frame. -
src/core/ThemeManager.cpp:1544—checkBoxIndicatorStyle()'s comment still listsFreeDvReporterDialogas a user ("RadioSetupDialog, DxClusterDialog, FreeDvReporterDialog"); after this PR it isn't one. One-word docs fix, out of scope to require. -
Undisclosed (welcome) behaviour change —
m_msgSendBtnhassetAutoDefault(false)(.cpp:325) but the band buttons never did, so underPersistentDialogReturn in the message field could also click the first band button.Kind::Workspaceneuters default buttons, and the explicitconnect(m_msgEdit, &QLineEdit::returnPressed, …)at.cpp:333still fires, so Return now sends and nothing else. Worth a line in the body.
6. What I tried to break (and couldn't)
All findings are reasoned from code; I have no build, no test run and no way to drive the GUI, so nothing below was reproduced at runtime.
- The lifetime trap. The most serious bug available here was a
WA_DeleteOnClosecanon window behind MainWindow's rawm_freedvReporterDialogpointer. ReadCanonWindow's constructor, close-button connect, theQKeySequence::Closeshortcut andhideEvent: noWA_DeleteOnCloseanywhere,reject()hides,WA_QuitOnCloseis cleared forWorkspace. The hide-on-close claim and the raw pointer are consistent. - The sibling call site the diff doesn't show. Grepped every reference to
FreeDvReporterDialogoutside its own files:MainWindow_DigitalModes.cpp(directshow()),MainWindow.h,DxClusterDialog.cpp(uses only theMessageMaxLengthconstant). Nothing passed it toshowOrRaisePersistent()/trackPersistentDialog(PersistentDialog*), so the base-class swap breaks no call site — the body's "MainWindow is unchanged" survives. - Token typos in a 130-line sheet. Extracted all 12
{{…}}tokens and resolved each against the flattened nested JSON of both bundled themes (a plain grep misses them): all 12 present.color.control.unavailableis the one non-canon.*token, andRadioSetupDialog.cpp:863,907already uses it inside a canon window, so it's established, not a slip. - The unexercised row highlights. The body flags that TX/RX/message highlights on the canon surface are unseen, and
FreeDvReporterModel.cpp:107-122paintsBackgroundRole/ForegroundRolethrough the non-widget-awareThemeManager::instance().color(), i.e. root scope — which is what thetheme-style-guide.mdline this PR edits warns against. But neither theme'scanonscope overridescolor.highlight.*(tx #fc4500,rx #379baf,message #e58be5,fg #000000), so root and canon resolve identically and the highlights carry no colour regression. Unhighlighted rows fall through to the sheet'scanon.inkoncanon.nested. - The
QWidget { background: transparent }cascade reaching the table viewport, and the absence of aQToolTiprule for the Band radio's tooltip. Both are identical to SpotHub's shipped sheet, so established behaviour rather than introduced. - An offscreen-screen trap in the new geometry test.
testCanonToolWindowsaves a legacy base64 rect ofminimumSize() + (0,60);QOffscreenScreenis 800×600 andrestoreGeometry()fits an oversized rect to the screen, which would defeat the assertion given the disclosed 825 px layout minimum. It survives because the width chosen is the minimum, so any clamp is undone by the window's own minimum — deliberate, and the inline comment says as much. - CodeGuard's 85 findings. None of the three flagged files (
PanadapterStream.{cpp,h},RadioModel.cpp) is in this diff —git diff --name-onlyreturns seven files, none of them those. A read-only audit of every flagged line found no shell/command/network sink (noQProcess,system(),popen,QNetworkAccessManagerin either file) and no literal credential:CS-VLN-CMDI-BACKTICKfires on backticks inside C++ comments quoting protocol verbs (// `stream remove 0x<id>`),CMDI-PIPE/AMPon||/&&/bit-masks,SSRF-HEXon hex sentinels like0xFFFFFFFF,SEC-PASSWORDon a runtime keychain read (IcomCredentials::sessionPassword()). Nothing attributable to this PR. Credit: security-audit (CodeGuard). - Couldn't check: anything visual (the canon sheet's rendered result, the opaque-menu grab, the 825 px measurement) and the docs claim that FreeDV Reporter is only the second canon window to hide on close — I verified it does hide, not that no third one already did.
No socket tests added or removed; the test is socket-free and never reaches qso.freedv.org.
7. Recommendation
Needs maintainer decision. The code is approve-with-nits: the migration is faithful to #6755, the mechanics a CanonWindow move owes are all present (FramelessResizer, geometry carry-over under the old key, no call-site conversion needed here), and the one lifetime hazard this pattern invites is absent. What it needs first is the pick — FreeDV Reporter is on none of #6239's lists, the only bullet covering it is the token-only re-skin, and the docs hunks assert a maintainer decision that isn't on the record in either thread.
Concretely: either add #6753's "don't merge before the pick" caveat to the body, or have @ten9876 rule the #6735/#6753/#6757 batch shape once — at which point this merges on the nits alone. Thanks for a clean, well-evidenced migration, and for keeping the geometry key and the object names stable.
🤖 aethersdr-agent · cost: $20.5443 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (85 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: $21.2814 · model: claude-opus-5 |
|
Thanks for the review. Addressed in
|
…herRX and AetherTX. Principle VIII. FreeDvReporterDialog moves from PersistentDialog to a CanonWindow workspace (RFC aethersdr#6226, part of aethersdr#6239): the canon ground, header and corner close button, a FramelessResizer, and the FreeDvReporterGeometry key, whose base64 rectangle CanonWindow reads once and replaces with x,y,w,h. Closing hides it, as with AetherTX, since MainWindow keeps feeding it stations. One canon sheet under canon/freedvReporter replaces the per-widget sheets: a nested station list with a cyan header row rounded to the table border, the message field on control, canon indicators, and a selected band in aqua on nested. The message field's right-click menu is a child of the field, so QMenu gets its panel back. Band buttons take Tab focus only, so a band clicked off does not keep the focus border that matches the checked one. The minimum width is the larger of 780 and the layout's own, since an explicit minimum overrides the layout and the semibold band row clipped Track and Band. The bottom Close button and its separator go; the corner close replaces them. freedv_reporter_message_test gains socket-free checks for the canon window, the resizer, legacy geometry restore and save, the opaque field menu, the minimum width and the band focus policy. The style docs list the window and its scope. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dow, and the disabled Send rule says why it stays legible. Principle VIII. testMinimumFitsBandRow also shows the window at its minimum size and checks no labelled control is narrower than its minimum size hint, so clipping after the canon sheet polishes is caught, not only deletion of the line that sets the minimum: without it, Track, Band and Freq clip (48 px for 58, 56 and 53). The comment that belonged to testBandButtonsTakeTabFocusOnly moves back above it. The disabled Send rule regains a comment naming why its muted ink matters: Send must read as greyed-out, not absent, since that is how the row explains its own gating (aethersdr#4231). checkBoxIndicatorStyle()'s comment names the one dialog that still uses it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
94105bc to
d9b270f
Compare
Resolves the conflicts in the three docs/style/* canon-window lists, where main added FFTW Wisdom / canon/nr2Wisdom. Every list keeps main's entries and this branch's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Summary
Closes #6755. Part of #6239. Waiting on the maintainer's pick: this full
CanonWindowmove is not on #6239'sCanonWindowlists; #6239's "Persistent tool dialogs (one PR per dialog)" candidate covers only moving a dialog's colours tocolor.canon.*. Please don't merge before the pick. Merging is that pick:aethersdr-style-guide.mdanddialog-patterns.mdlist FreeDV Reporter among the tool windows the maintainer has moved toCanonWindow, which is true only once this lands.FreeDV Reporter (
FreeDvReporterDialog) moves fromPersistentDialogto aCanonWindowworkspace, using the same approach as AetherRX, AetherTX and SpotHub (#6401):CanonWindow::Kind::Workspace, an independent top-level window with its own taskbar entry.showFreeDvReporter()already created it directly rather than throughshowOrRaisePersistent(), so MainWindow is unchanged.PersistentDialog, the band buttons were auto-default, so Return in the message field could also click the first band button. Now Return only sends, through the field'sreturnPressed.FreeDvReporterGeometry). CanonWindow savesx,y,w,h; it reads the base64saveGeometry()value the PersistentDialog left there once, and replaces it on the next save, so saved positions carry over.Look. It sits under a new
canon/freedvReportertheme scope with one canon sheet, based on SpotHub's:control; disabled (reporting off) it reads muted;canon.mutedink (6.71:1 dark, 5.94:1 light on the ground), so it reads as greyed-out rather than absent, which is how the row explains its gating (Allow custom status message configuration in FreeDV Reporter panel #4231). The rule is SpotHub's. Itscanon.lineborder is 4.03:1 in dark but 1.55:1 in light, under the 3:1 non-text floor, so in light the disabled button reads by its text alone. That's a property of the canon palette that every canon window shares, not something this PR introduces;Cleanup. The nine per-widget sheets (label, field, Send, table, separator, check box, two radio buttons, band buttons) are gone. So are the bottom Close button and the separator line above it, which the corner close button replaces as in other canon workspaces.
No
CanonMessageBox: the window never shows aQMessageBox.Docs:
docs/style/dialog-patterns.mdlists FreeDV Reporter among the canon workspaces that save their geometry, and as the second window, after AetherTX, that hides on close.docs/style/aethersdr-style-guide.mdanddocs/style/theme-style-guide.mdlist the window and itscanon/freedvReporterscope. The user docs don't describe this window's chrome or the Close button, so they are unchanged.Constitution principle honored
Principle VIII (Evidence Over Assertion) covers the checks below. The visual direction is RFC #6226's; the selected-band fill is part of what the maintainer is asked to review.
Test plan
AetherSDRandfreedv_reporter_message_test.freedv_reporter_message_testpasses. Its target now compilesCanonWindow.cppinstead ofPersistentDialog.cpp/FramelessWindowTitleBar.cpp, and gains window checks, all socket-free: the window is aCanonWindowwithFramelessResizer; a base64 rectangle saved by the old dialog restores, and hiding savesx,y,w,hunder the same key; the message field's menu grabs opaque; the minimum width fits the layout and, on the shown window at that minimum, no labelled control is narrower than its minimum size hint; and every band button takes Tab focus only.QMenurules fails the opaque-menu check, and dropping the focus policy fails the focus check. Measured before the fix, the minimum width was 780 against a layout minimum of 825. Dropping the line that raises the minimum to the layout's fails both minimum checks: Track, Band and Freq clip (48 px for 58, 56 and 53).canon_window_testpasses.freedv_reporter_message_testis not inci.yml's frozen per-PRctest -Rsteps (docs/agents/tests-ci.md), so the green checks prove it compiles; it runs post-merge infull-suite.yml. The local runs above are the evidence.screencapture -l(the automation bridge was unavailable). It draws the canon header and corner ✕, the canon header row, muted "Message (reporting off)" with Send dimmed, canon indicators, and "All" selected in aqua. The table scrolls horizontally at this width, as it did before. Offscreen grabs of the fixed build show the rounded header corner and the opaque field menu.check_engine_boundary,gen_touchpoint_manifest --check,check_capability_records,check_command_plane,check_a11y.pyandgen_reference.py --check. The colour-audit ratchet is unchanged againstmain(487 / 1879 / 599).embed_screenshots.py --checkreportsyour-first-session.mdout of step; this PR does not touch that file.Checklist
9100df8b, ED25519)AppSettingscalls; the existing geometry key is reuseddocs/style/dialog-patterns.md,aethersdr-style-guide.md,theme-style-guide.md; noCHANGELOG.mdentry.ThemeManager::checkBoxIndicatorStyle()'s comment now names the one dialog that still uses it (MidiMappingDialog).🤖 Generated with Claude Code