Repository navigation
feat(gui): MQTT Settings is a CanonWindow dialog, re-skinned to the canon palette. Principle VIII. - #6751
feat(gui): MQTT Settings is a CanonWindow dialog, re-skinned to the canon palette. Principle VIII.#6751w5jwp wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Reviewed at the PR head (5a2f76f) against the head checkout. The implementation is careful and the mechanical claims in the body hold up to checking — I verified the byte-identity claim, the ratchet, the retired key and the show-path precedent rather than taking them on trust. The one thing that is not settled is whether this window was picked for CanonWindow at all, and merging is that pick.
Everything below is reasoned from code: I am headless, with no build, no test run and no way to drive the GUI, so nothing here was reproduced at runtime.
1. Issue fit
Yes — #6750 is implemented point for point. CanonWindow with header and corner close (MqttSettingsDialog.cpp:135-137), canon/mqttSettings scope, the ModemChrome canon sheet plus the tab/group-box rules that sheet lacks, FramelessResizer::install(this) for the "still resizable" requirement, OK/Apply/Cancel save behaviour untouched (:310-320), the #556 literal gone from both sites, the retired key at src/main.cpp:613, and both shared helpers in CanonIndicators.h. Nothing in #6750 is left unaddressed.
The issue itself is one hour old, opened by the PR author, still labelled maintainer-review with no maintainer ruling on it — which is the subject of §3.
2. Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/gui/MqttSettingsDialog.{h,cpp} |
PersistentDialog → CanonWindow, canon sheet, port-field inset, #556 removed |
Yes | In scope (#6750) |
src/gui/MainWindow.cpp |
showMqttSettingsDialog() becomes the canon create-or-raise |
Yes | In scope — required; showOrRaisePersistent would not compile against a CanonWindow |
src/gui/CanonIndicators.h |
canonSpinArrowRules() moved in, new canonItemCheckRules(), shared detail::canonIndicatorImages() |
Yes | In scope — #6750 names the extraction explicitly |
src/gui/RadioSetupDialog.cpp |
drops the private spinArrowRules() + now-unused ThemedImageCache.h include, calls the shared copy |
Yes | In scope — consumer side of the move. themedImagePath has no other caller in the file, and CanonIndicators.h carries the include |
src/main.cpp |
MqttSettingsDialogGeometry joins the retired-key list |
Yes | In scope |
docs/user/docs/mqtt-station-automation.md |
documents OK/Apply/Cancel/✕/Esc/Ctrl+W | Yes | In scope |
docs/style/* (3 roster sentences) |
lists MQTT Settings among windows "the maintainer has moved to" CanonWindow |
Yes | Needs maintainer decision — see §3 |
Everything in the diff is explained by #6750. No unrelated files, no formatting churn, no new public/protocol surface, no settings key added, no guard or confirmation removed. The only - lines of substance are the two #556 stylesheets and the moved function, both accounted for.
FYI, not a finding: the three docs/style roster sentences are the same lines #6745, #6749 and #6735 each edit, so whichever lands first will conflict the others.
3. Needs maintainer decision (not a blocker, but it gates merge)
docs/style/aethersdr-style-guide.md:118, dialog-patterns.md:238, theme-style-guide.md:46 — these edits add MQTT Settings to the list of "the tool windows the maintainer has moved to it" and to canon/mqttSettings among the scopes. That sentence is only true once this merges, so the docs assert a pick that hasn't been made.
MQTT Settings appears nowhere on #6239 — not under Picked, and not in the CanonWindow candidate list (What's New, System Info, Help pages, SWR sweep licence, frameless message boxes). The line #6750 and this PR cite, "Persistent tool dialogs (one PR per dialog)", sits under the bullet for surfaces whose widgets move from base tokens to color.canon.* — a re-skin that keeps PersistentDialog chrome, which is a different change from moving the window's chrome to CanonWindow. #6239 says "Items under 'Candidates' are not approved scope until the maintainer picks them" (quoted in the #6750 triage comment, which reached the same conclusion independently).
So this is a self-nomination of the Network-Diagnostics / Radio-Setup class, not a reading of an existing bullet. That is a legitimate thing to ask for — #6735 asks for the same thing and says so in its body — but the body here presents it as settled ("Part of #6239", "as the AetherRX and AetherTX Settings dialogs did") without disclosing that the window is unlisted. Worth a sentence in the body either way.
Fold the body's own flagged visual calls into the same ruling: the 29 px fields / 31 px buttons, and the AlignRight | AlignVCenter label alignment.
4. Blockers
None.
5. Nits (all non-blocking)
-
src/gui/MqttSettingsDialog.cpp:41-100— the composed sheet has noQMenurule, so a field's right-click menu likely draws with no background.ModemChrome::styleSheet(…, Look::Canon)opens with a blanket rule whose canon background istransparent(ModemChrome.cpp:104-108,%2=c.ground="transparent"forkCanonat:52):QWidget { color: %1; background: %2; font-size: %3px; }A Qt type selector matches subclasses, and
QLineEdit::createStandardContextMenu()parents itsQMenuto the field — which is insidebodyWidget(), so the sheet reaches it. Host, User, Password and the CA-file edit, the Port spin's internal editor, and the tables' cell editors all have one. Neither ModemChrome's canon sheet,kMqttCanonRules,canonSpinArrowRules()norcanonItemCheckRules()contains aQMenuselector.Two independent in-repo data points:
RadioSetupDialog.cpp:1069-1082carries explicitQMenurules in its canon sheet, whoseQWidgetrule is alsobackground: transparent(:852-854); and #6735's body reports observing exactly this onTxBandDialog("the menu on the RF and Tune power fields drew no background at all") and fixing it in a follow-up commit. Onmainthis dialog had no body-wide sheet at all, so its context menus drew normally — this is a regression for this dialog, though the gap is pre-existing in every ModemChrome-canon consumer (AetherRxSettingsDialog,AetherTxSettingsDialog,FirmwareInstallDialog), which have far less text entry. Reasoned from code plus #6735's observation; I could not reproduce it.Drop-in — append to
kMqttCanonRules(the block isRadioSetupDialog.cpp:1069-1082verbatim):QMenu { color: {{color.canon.inkSoft}}; background: {{color.canon.nested}}; border: 1px solid {{color.canon.lineHi}}; selection-background-color: {{color.canon.cyan}}; selection-color: {{color.canon.onAccent}}; } QMenu::item:selected { color: {{color.canon.onAccent}}; background: {{color.canon.cyan}}; } QMenu::item:disabled { color: {{color.control.unavailable}}; }The better home is ModemChrome's
if (look == Look::Canon)branch (ModemChrome.cpp:405-419), beside theQListWidgetappend, so the sibling dialogs get it too — but that widens this PR's blast radius, so a follow-up is fine. -
No test references
MqttSettingsDialog.canon_window_testcovers the base class, not this dialog's wiring. The deterministic socket-free seam exists and the sibling PRs in this batch use it: #6745 addsrc28_mapping_dialog_test(is-a-CanonWindow,FramelessResizerinstalled, geometry key behaviour), #6735 extendstune_output_dialog_test. A few cheap checks here would be: the composed sheet is non-empty and contains aQTableWidget::indicatorrule;canonSpinArrowRules()output is stable; the retired key is gone after startup. Not merge-gating — neither would run in the frozen per-PRctest -Rgate — so this is a consistency nit, not a withhold. -
src/gui/MqttSettingsDialog.cpp:143-148— the composed sheet callsdetail::canonIndicatorImages()twice per rebuild (once inside ModemChrome's canon branch viacanonIndicatorRules(), once viacanonItemCheckRules()). Harmless — the images are cache-path lookups — and only on theme change. Mentioning it only so it isn't mistaken for a bug later. -
The body's test plan omits
tools/audit_colours.py, whichdocs/style/aethersdr-style-guide.md:202-204makes a requirement of every migration ("Every migration keeps the colour-audit ratchet flat or lower"). I ran it: it passes comfortably (figures below), so this is a body-completeness nit, not a gate failure.
6. What I tried to break (and could not)
- "The function body is byte-identical (compared programmatically)." Held. I extracted
spinArrowRulesfrom1afefafdb:src/gui/RadioSetupDialog.cppandcanonSpinArrowRulesfrom the headCanonIndicators.h, stripped the signature line: 42 lines each,diffclean. ThecanonIndicatorRules()template and its.arg()order are also unchanged, the aggregate-init order matches the struct field order, and the fail-closed path is preserved (return {}becamenullopt→return {}in both callers), so a failed image write still yields Qt's own indicators rather than blanks. - Dangling pointer on reopen.
WA_DeleteOnCloseplus a cached pointer is the classic use-after-free here;MainWindow.h:1760is aQPointer<MqttSettingsDialog>, soif (!m_mqttSettingsDialog)sees null after deletion and rebuilds, reconnectingsettingsSaved. The hoisting of that connect into the fresh-construction branch is equivalent to the oldwasFreshdance, and the new show path is character-for-character the AGC Calibration precedent atMainWindow.cpp:4663-4673. - "
trackPersistentDialog()only fed the Frameless Window toggle, so nothing else is lost." Held.m_persistentDialogshas exactly one consumer: the propagation loop atMainWindow.cpp:10264-10272, insidesetFramelessMode. Nothing else reads the list. - The retired key. No reader survives:
MqttSettingsDialogGeometryappears only atsrc/main.cpp:613, anddialog/mqttSettingsappears nowhere.docs/user/screens.jsonkeyswait_visible/targeton the class nameMqttSettingsDialog, which is unchanged, so the capture spec still finds the window as the body claims. - "The canon sheet's disabled rule colours them" (the
#556removal). Held, and it is an improvement, not a regression.ModemChrome.cpp:161-164hasQRadioButton:disabled, QCheckBox:disabled { color: %11 }, where%11isc.textDim=color.canon.muted(#8598b4). Against the group box'scolor.canon.nested(#0e1a2a) that is 5.96:1, versus the old#556on#0f0f1aat 2.61:1 — the first clearsdocs/a11y.md's 4.5:1 aim, the second never did. - Mixed check-box appearance. I expected the table's new PNG checks to clash with ModemChrome's QSS-drawn ones (solid cyan fill, no tick). They don't:
ModemChrome.cpp:405-410strips its own::indicatorrules underLook::Canonand appendscanonIndicatorRules(), so the dialog'sQCheckBoxes and the Display column now draw from the same 18 px images. - No new theme tokens needed? Confirmed —
resources/themes/default-dark.jsoncarries acanonscope with tokens and no child scopes, socanon/mqttSettingsre-palettes by prefix inheritance, exactly ascanon/agcCalibrationdoes. No theme file needed touching. - The colour ratchet. Ran
tools/audit_colours.py --compare-src <main>/src --strictfrom the head checkout: unique colours 486 (base 487), total references 1877 (base 1879),setStyleSheetsites 597 (base 599) — OK on all three. Also rantools/check_a11y.py src/gui/MqttSettingsDialog.cpp(0 findings) andtools/gen_touchpoint_manifest.py --check(up to date). - QSS escape / injection in the new helpers.
canonItemCheckRules' only call site passes the literalQStringLiteral("QTableWidget"), and the multi-argQString::arg(view, …)substitutes in one pass, so aviewcontaining%2could not re-enter later placeholders.kMqttCanonRules'%1/%2are the two padding ints and the string holds no other%. - The CA-certificate file dialog.
QFileDialog::getOpenFileName(this, …)at:220parents to the window, notbodyWidget(), so the blanket-transparent sheet does not reach it — this is why the §5 finding is limited to the body's own context menus. - CodeGuard's 50 findings: all 50 refuted. The
security-audit (CodeGuard)subagent confirmed every one as lexical — backticked identifiers in C++ doc comments (CanonIndicators.h:165,RadioSetupDialog.cpp:416/1428/1582),const QString&declarators and Qt&Class::signalsyntax read as shell&/|, the literal/proc/self/statusread,224.0.0.1as "PII", the Flex status code50000053as a bank account, "override" in a DSP comment as a prompt injection. Every MainWindow hit is also verbatim onmain, so untouched by this PR. It separately checked the keychain path: the password reaches onlyQKeychain::WritePasswordJob::setTextData(), logs carryerrorString()only (no secret, unlike the QrzClient case), the persistedMqttConnectionConfigholds no password, andsetEchoMode(Password)is intact. No genuine security defect. - CI, honestly:
build,check-macos,check-windows,Static checks,Build docs siteandSanitizer option configuresare all green on5a2f76f, andENABLE_MQTTdefaultsON(CMakeLists.txt:69) so this dialog really is compiled in those lanes. That proves compilation, not behaviour — no MQTT or canon dialog test is in the frozen per-PRctest -Rgate. - Could not check: anything at runtime. In particular the measured geometry (620×560, 29/31/32 px rows, the Port text lining up with Host's), the light-theme pass, and whether Esc / ⌘W actually delete the instance. On the last one,
QDialog::done()honoursWA_DeleteOnClosein Qt 6 and the body reports the bridge'scloseleaving the dialog deleted with edits discarded, so I have no reason to doubt it — but a surviving instance re-showing half-edited fields is the failure mode if that ever regresses, which is one more argument for the test nit above. - Also not checkable from here: the
#556→ canon change on the light theme, and the mono-font claim for the Port field — though that one is a consistency fix rather than a restyle:ModemChrome.cpp:266-274already sets the same"SF Mono", "Menlo", "Consolas", monospacestack onQLineEdit, so the spin box was the odd one out.
7. Recommendation
Needs maintainer decision. The engineering is clean: the refactor is provably behaviour-preserving, the show path copies an existing canon precedent exactly, the retired key has no reader left, the ratchet moved down, and I could not find a correctness, lifetime or threading defect. But MQTT Settings is on neither of #6239's approved lists, and the three docs/style roster edits state the pick as already made — so merging is the pick, and that is the maintainer's call, together with the field/button sizing and label alignment the body already flags.
Concrete next step: get the #6750 scope ruling (and the sizing call) from the maintainer; separately, add the QMenu block above so right-clicking a field doesn't show a transparent menu. The coverage nit is worth doing for parity with #6745 but shouldn't hold the merge.
Thanks for the unusually checkable body — the byte-identity claim, the retired-key count and the capture-spec note each pointed me straight at the thing to verify, which is the opposite of the usual experience.
🤖 aethersdr-agent · cost: $20.8488 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (55 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.9970 · model: claude-opus-5 |
|
Thanks for the review. It says it read
Colour audit in the test plan: added. At
The same commit also names the canon header and the corner close button in the |
…anon palette. Principle VIII. MqttSettingsDialog moves from PersistentDialog to a CanonWindow dialog, as the AetherRX / AetherTX Settings dialogs did: the canon header and corner close button, the canon/mqttSettings theme scope, and the ModemChrome canon sheet plus SpotHub's tab and group-box rules. Fields and buttons are sized for a multi-row form. It opens centered, closes with Esc or Ctrl+W, still resizes, and no longer follows View → Frameless Window, so showMqttSettingsDialog() creates it directly. The Port spin box gets canon steppers through canonSpinArrowRules(), moved unchanged from RadioSetupDialog.cpp into CanonIndicators.h, and a text inset the sheet's padding cannot give it. The table's Display check uses the canon check images. The hard-coded aethersdr#556 disabled colour is gone. Part of aethersdr#6239. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tings' retired geometry key goes at startup. Principle VIII. Review follow-ups to the MQTT Settings move to CanonWindow: - canonItemCheckRules(view) in CanonIndicators.h gives an item view's check column the canon check images. canonIndicatorRules() and it now share detail::canonIndicatorImages(); canonIndicatorRules()' output is unchanged. MQTT Settings uses it in place of regex-retargeting canonIndicatorRules()' text. - MQTT Settings' field padding and the port field's text inset come from named constants (padding + the 1 px border), so they cannot drift apart. - MqttSettingsDialogGeometry joins the retired keys removed at startup, as AboutDialogGeometry did: a CanonWindow dialog opens centred and nothing reads it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s it and the close paths. Principle VIII. The canon controls sheet clears every descendant to transparent, and Qt parents a field's standard right-click menu (Undo/Cut/Copy/Paste) to the field, so the menus on the broker, topic and certificate fields drew with no panel. QMenu gets its canon panel back, as on TX Band Settings. mqtt_settings_dialog_test (8 checks, socket-free, never accepted) covers the opaque field menu, failing without the QMenu rules, and records that Esc, Ctrl+W / Cmd+W and the corner button each destroy a WA_DeleteOnClose window, so a half-edited field never survives into the next open. The screenshot's alt text names the canon header and the corner close button. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
415bbf4 to
0190287
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 #6750. 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.*. #6750 has the scope question. Please don't merge before the pick. Merging is that pick:aethersdr-style-guide.mdanddialog-patterns.mdlist MQTT Settings among the windows the maintainer has moved toCanonWindow, which is true only once this lands.The MQTT Settings window (
MqttSettingsDialog) moves fromPersistentDialogto aCanonWindowdialog (Kind::Dialog), as the AetherRX and AetherTX Settings dialogs did:FramelessResizer). It no longer follows View → Frameless Window, soshowMqttSettingsDialog()creates it directly instead of throughshowOrRaisePersistent(). That helper'strackPersistentDialog()only fed the Frameless Window toggle, so nothing else is lost.canon/mqttSettingstheme scope:ModemChromecanon sheet the AetherRX/TX Settings dialogs use;setTextMargins(). The padding and the inset come from the same named constants (padding plus the 1 px border), so they cannot drift apart.QMenuits canon panel back, as on TX Band Settings (0190287f).#556colour on the always-on built-in topics; the canon sheet's disabled rule colours them.MqttSettingsDialogGeometryjoins the keysmain.cppremoves at startup, asAboutDialogGeometrydid when About moved to CanonWindow. Nothing reads it any more.Shared code (
src/gui/CanonIndicators.h):spinArrowRules()moves fromRadioSetupDialog.cppascanonSpinArrowRules(). The function body is byte-identical (compared programmatically), and Radio Setup now calls the shared copy.canonItemCheckRules(view): the canon check images for an item view's check column. It andcanonIndicatorRules()now sharedetail::canonIndicatorImages().canonIndicatorRules()' output is unchanged: same template, same eight images.There are no message boxes on the MQTT path, so
CanonMessageBoxis not involved.Docs:
docs/style/*list MQTT Settings among the canon windows andcanon/mqttSettingsamong the scopes.screens.json: themqtt-settings-dialogalt text names the canon header and the corner close button.mqtt-settings-dialog.pngshows the old window. It should be re-shot withtools/docs/capture_screenshots.py --only mqtt-settings-dialog(its spec still finds the window by class name). Native macOS captures were used during review; they are not replacements for the tool's shot.Intent
MQTT Settings looks and closes like the other canon dialogs. Out of scope: what it saves, the keychain handling, the MQTT applet, and Radio Setup's own spin box text, which also sits on the field's left edge (the same Qt behaviour).
Design review (maintainer): the field and button sizes and the label alignment are visual calls for the maintainer.
Constitution principle honored
Principle VIII (Evidence Over Assertion) covers the checks below. The visual direction is RFC #6226's.
Test plan
canon_window_testpasses.mqtt_settings_dialog_test(socket-free, the dialog is never accepted): 8 checks, all passing.CanonWindow. A field's standard right-click menu is parented inside the canon body and draws an opaque background; renaming theQMenuselectors so they match nothing fails this check.WA_DeleteOnClosewindow (Qt 6.12'sQDialog::done()closes; it does not only hide), so a half-edited field never comes back on the next open.ci.yml's frozen per-PRctest -Rsteps, so green means it compiled; it runs infull-suite.yml.gen_touchpoint_manifest.py --check,gen_reference.py --check,git diff --checkpass.tools/audit_colours.py --compare-src <base> --strict, at0190287f): unique colours 486 (−1), references 1877 (−2),setStyleSheet597 (−2).tools/check_a11y.py src/gui/MqttSettingsDialog.cpp: no findings.npm ci && npm run buildsucceeds (Node 24, as CI), including its link and anchor check.AETHER_SETTINGS_DIR),AETHER_AUTOMATION_NO_TX=1, connected to the demo radio (DEMO-0001, "AetherSDR Demo"), opened via MQTT...:setText mqttHost discard.me, then the bridge'sclose; the dialog was deleted, and reopening showedlocalhost;ActiveTheme = "Default Light"): every control readable, checked and disabled states distinct;MqttSettingsDialogGeometrypresent in the scratch store before launch (count 1), gone after (count 0).CanonIndicators.hrefactor.keyeventreaches only shortcuts;mqtt_settings_dialog_testdrives both on the real dialog); typing into a table cell; a live broker; Windows and Linux builds. The app sets the Fusion style on every platform (main.cpp), so the spin-box inset behaviour measured on macOS is the same style.Checklist
AppSettingscalls. As a centered dialog it no longer saves its geometry, and the old key is removed at startup.MeterSmoother(n/a)docs/user/docs/mqtt-station-automation.md,docs/style/*🤖 Generated with Claude Code