Repository navigation
Conversation
![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, on every acceptance bullet of #6748 — and I checked them rather than taking the body's word. CanonWindow::Kind::Dialog with theme::setContainer(this, "canon/ulanziDial") and makeCanonHeader(); the CanonMessageBox swap with the wording byte-identical; docs/style/* and the user page updated. The acceptance line I expected to fail is the one that holds cleanest: grep '#[0-9a-fA-F]\{6\}' src/gui/UlanziDialMapperDialog.cpp returns nothing — not one hex literal left, including the painted fallback dial.
The one requirement that isn't in #6748 at all is the best part of the PR: setReturnClicksDefault(false). On main Return in a pill pressed Reset to Defaults, i.e. a keyboard user tabbing through the pills could silently rewrite every mapping. That is a real bug fix riding in a re-skin, and it is the thing here with no test (nit 2).
Scope
Everything in the diff is explained by the issue. No unrelated files, no new public surface, no build or CI churn, and the - lines delete only what the + lines replace.
| File | Changes | Claimed? | Verdict |
|---|---|---|---|
src/gui/UlanziDialMapperDialog.{h,cpp} |
PersistentDialog → CanonWindow, canon sheet, ulanziState property, pills re-parented to the canvas, CanonMessageBox |
Yes | In scope |
src/gui/MainWindow_Menus.cpp |
showOrRaisePersistent() → direct new + WA_DeleteOnClose |
Yes | In scope; required (a CanonWindow satisfies neither setFramelessMode() nor trackPersistentDialog()) |
docs/style/*.md (3 files) |
add the window to the canon lists and canon/ulanziDial to the scopes |
Yes | In scope — but see Needs maintainer decision |
docs/user/docs/ulanzi-dial.md |
+2 lines, the close paths | Yes | In scope |
Deleted behaviour I read as carefully as the additions: pillComboStyle(), setStatusStyle() and the ThemeManager::clearWidgetTracking() call all go, and the comment they carried named a real symptom ("a later theme change would repaint a connected or permission line in the disabled colour"). The property-selector replacement is immune to it by construction — the dynamic property survives applyStyleSheet()'s re-polish, so there is nothing left to clear. grep -rn "UlanziDialMapperGeom\|setStatusStyle" src/ tests/ docs/ returns nothing; no stale reader was left behind.
Needs maintainer decision
1. The pick itself — docs/style/aethersdr-style-guide.md:118, docs/style/dialog-patterns.md:237. This is not a code problem; it is the one thing I cannot settle, and the diff asserts it rather than citing it.
The sentence the PR edits says canon covers "the candidates listed on #6239, once the maintainer picks them, and the tool windows the maintainer has moved to it" — and the PR adds itself to the second clause, which is the maintainer's. Reading #6239 directly:
- Picked: tool windows on
CanonWindowends at AGC-T Calibration (#6372). Ulanzi Dial — Control Mapping is not on it. - Candidates: short-lived windows is a closed five (What's New, System Info, Help pages, SWR sweep licence,
FramelessMessageBox) and prefaces itself with "Tool and workspace dialogs keep the persistent pattern." - The only line that could cover this window, "Persistent tool dialogs (one PR per dialog)", sits under "surfaces whose widgets move from base tokens to
color.canon.*" — a re-skin that keepsPersistentDialog, not a chrome move. docs/style/dialog-patterns.md:273-277closes it: "UseCanonWindowonly for windows the style guide covers … Other tool and workspace dialogs keep the persistent pattern above."
#6748 is the contributor's own acceptance spec, labelled maintainer-review, with no maintainer comment on it. On the twin #6745 (Icom RC-28, same author, 15 minutes earlier, identical position) @w5jwp said it himself: "Finding 1 (the pick …) is the maintainer's call." Same answer applies here, and one ruling should cover the batch. @ten9876 — the token re-skin is unambiguously wanted and this PR delivers it; the question is only whether a controller-mapping tool dialog leaves PersistentDialog chrome. If the answer is "re-skin yes, chrome no", most of this diff survives and only the base class and the menu call site change back.
2. FramelessResizer is not installed — src/gui/UlanziDialMapperDialog.cpp:217. docs/style/dialog-patterns.md:275-276: "A tool window on it also installs FramelessResizer, since CanonWindow itself only moves."
setFixedSize(kDialogSize);The PR keeps the fixed size instead, which is a coherent choice for a pixel-placed canvas — you cannot usefully resize a dialog whose contents are absolute-positioned — but the PR body doesn't say so, and #6745 was asked for the resizer and took it. Either install it (and the pills need a real layout first) or state in the body that a fixed-size canon dialog opts out, so the next conversion doesn't have to guess.
Blockers
None. No correctness, lifetime, or safety defect found.
Nits (all non-blocking)
- No test, where the twin shipped one. #6745 added
rc28_mapping_dialog_test(21 checks, null encoder, socket-free) with mutation proofs. The seam exists here too — the constructor toleratesmanager == nullptr(UlanziDialMapperDialog.cpp:305, theUlanzi Dial manager unavailablebranch), so a widget-level QTest can assert, deterministically and socket-free:CanonWindowbase; Return in a pill does not click Reset to Defaults (the regression fix, which nothing currently pins); the fiveulanziStatevalues and theenabled()==falseshort-circuit inonConnectionChanged(:737-741); the pills' accessible names. Per the test-layer boundary this is GUI state, not wire or policy, so it isn't merge-gating — but the one behavioural fix in the PR deserves the same treatment its sibling got. src/gui/UlanziDialMapperDialog.cpp:400— half-translated accessible name:: tr("%1 key action").arg(p.defaultLabel));p.defaultLabelisQString::fromLatin1(kPillSpecs[i].label)(:391), so a localised build announces a translated frame around an untranslated "Top Left".tr()the label at the point of use, or keep an English-only name deliberately and say so.src/gui/UlanziDialMapperDialog.cpp:238-258— the status line and the Last event readout get object names but no accessible name. #6745 named its status lineRC-28 connection status. Both are live regions a screen-reader user has no other route to;docs/a11y.mddoesn't require it for a non-interactiveQLabel, hence a nit, andcheck_a11y.pywould not have caught it either way.- Light-theme amber,
src/gui/UlanziDialMapperDialog.cpp:198—QLabel#UlanziStatus[ulanziState='warning']resolves tocolor.accent.warning, which in Default Light measures 2.85 : 1 againstcolor.canon.ground(#ffb84d… light#d08010on#f5f5f8), underdocs/a11y.md:143's 4.5 : 1 for this 12 px/600 text. Computed by flattening both bundled theme JSONs and resolving the{alias}chains, not eyeballed. This is not the PR's defect — it used the shared token exactly asdocs/a11y.md:148asks, and dozens of widgets use the same one; it wants its own issue alongside #6239's carried contrast question. Worth flagging here only because the amber state is the single path the PR explicitly could not exercise (Linux, blocked dial). Every other pair I measured improves onmain: pill text 5.93 → 10.47 dark, disconnected 6.03, connected 10.72, popup 11.61, selection 12.71. - Geometry.
UlanziDialMapperGeomis now orphaned in every existing settings store — nothing reads or writes it. Disclosed in the body, and #6748 asked for centred placement, so this is the sanctioned behaviour; but #6745 went the other way (setGeometryKey(), legacy base64 read once and rewritten asx,y,w,h), andCanonWindow::restoreGeometryFromSettings()would have carried it under the same key. Two conversions, two patterns, same day — worth one ruling, not two. docs/style/dialog-patterns.md:275— the PR adds the window to two of the three canon-window lists indocs/style/but not this one. Arguably correct while the pick is open; flagging so it isn't missed if the pick lands.docs/user/screens.jsonshot 58 (ulanzi-mapper) still describes the old chrome. Disclosed as "recapture after merge"; itswait_visible/grabtarget isUlanziDialMapperDialog, which the newsetObjectName()at:216keeps valid, so the capture recipe itself doesn't break.
Security
CodeGuard's 90 findings were audited against the head checkout. All refuted; nothing confirmed. The file list is main's tip, not this PR's diff — PanadapterStream.{cpp,h}, RadioModel.cpp and tools/publish_update_manifest*.py are the work of the commits before this branch point and contain no #6749 line. By rule family:
CS-VLN-CMDI-PIPE/-AMP→ C++||and&&, aconst QString&, and a Qt accelerator (addMenu("&Window")). No shell.CS-VLN-CMDI-BACKTICK(9 sites) → every one inside a//comment quoting a Flex verb, e.g.// `pan close all`.CS-VLN-SSRF-HEX(24 sites) → protocol sentinels and constants:0xFFFFFFFF,kNeutralPanStreamIdBase = 0xE1000000u,0x00001C2D // OUI (FlexRadio).CG-PATH-001atUlanziDialMapperDialog.cpp:51→QCoreApplication::applicationDirPath()plus a literal; no external input in scope.- The Linux
pkexecpath (:772-852), checked because it is the only privileged code near the diff, is clean and unchanged in substance:proc->start(pkexec, {"/bin/sh", "-c", kScript, "aethersdr", kRule})is an argv list,kScript/kRulearestatic const QStringliterals with noarg()or concatenation, and the rule text reaches the shell only as a quoted"$1"intoprintf '%s'. The PR changes only the "pkexec was not found" box.
Credit: security-audit (CodeGuard).
What I tried to break
- Dialog lifetime, the usual way this pattern dies.
WA_DeleteOnCloseset by the caller plus a raw member would dangle on reopen.MainWindow.h:1772isQPointer<UlanziDialMapperDialog>, and the other toucher,MainWindow_Controllers.cpp:415, null-checks beforerefreshStatus().QDialog::done()honoursWA_DeleteOnClose, so Close (accept()), Esc and ⌘W/Ctrl+W all destroy it and null theQPointer— the same conclusion @w5jwp corrected himself to on #6745, and it matchesdocs/style/dialog-patterns.md:262-264's "WA_DeleteOnClose+ aQPointer". Backend signals are allconnect(m_manager, …, this, …), so destruction disconnects them; nothing fires into a dead dialog. - The pill re-parenting arithmetic, the one place "positions match the previous window" can silently break. Every moved coordinate is the old one minus exactly (10, 37): (120,100)→(110,63), (260,100)→(250,63), (400,100)→(390,63), (265,557)→(255,520), (185,525)→(175,488), (265,525)→(255,488), (185,557)→(175,520). Uniform, so the layout is internally consistent — and because the pills and the dial image now share the canvas's coordinate system, header height can no longer shift one relative to the other, which is the actual win. Reconstructing the old canvas origin from
main's layout (640×675, a ~30 px frameless title bar,AlignCenterin the leftover) gives ≈(10, 39), so any residual is a pixel or two; the screenshot is the real evidence and I cannot measure rendering headless. - The fixed-size height budget. 722 − 2·
kInset− 24 of margins − 590 canvas − 8 spacing leaves 98 px for the canon header and the button row.makeCanonHeader()is a 17 px/700 label + 8 spacing + a 1 px rule ≈ 32 px, and the button row ≈ 30 px, so ~36 px of slack — the fixed size will not clip the Close button. Width is exact rather than slack: 642 − 2·kInset− 20 = 620, the canvas's fixed width to the pixel. - Every theme token in the new sheet and the new
paintEvent. Flattened both bundled theme JSONs and resolved the{alias}chains: all 15 ofcanon.{cyan,aqua,control,nested,raised,ink,inkSoft,muted,line,lineHi,onAccent}andaccent.{success,danger,warning}exist as root tokens in both themes. A typo'd token would have left a literal{{…}}in the QSS and silently dropped the whole rule block; none does.canon/ulanziDialhas no entry in either theme, but neither does any of the thirteen existingcanon/*containers — resolution falls back throughcanonto root, so that's the established pattern, not an omission. - Whether the canvas's own sheet eats the body's.
QWidget#UlanziDialCanvas { background: transparent; }carries an id selector that can never match aQComboBox, so the body'sQComboBox#UlanziPillrules still reach the pills it now parents — the stated reason for the scoping holds, and the open-popup screenshot confirms the cascade reachesQAbstractItemViewtoo. - The status-state ordering trap I expected to find.
refreshStatus()callsshowDisabledStatus()and then queuesreportState(), so a reply arriving later could have repainted a disabled dial as a red "Disconnected".onConnectionChanged(:737-741) short-circuits toshowDisabledStatus()when!connected && !UlanziDialMappings::enabled(). Covered. - Checked and found nothing: no inventory test or generator enumerates canon windows or
canon/*scopes (grep -rn "agcCalibration" tests/ tools/is empty), so the three doc lists are the only registry and no gate was skipped. - Could not check, stated plainly: nothing here was run. There is no build, no test run and no way to drive the GUI from this seat, so every finding above is reasoned from the code at
9ae805bc6, not reproduced. Rendering, the Linux Grant-access flow, light-theme re-skin (which #6745 exercised and this PR's test plan does not mention) and Windows are unverified by me and by the PR.
Socket tests: none added, modified or removed. No QTcpServer/QUdpSocket/QLocalServer/bind/listen/connectToHost and no Fake* peer anywhere in the diff.
Recommendation
Needs maintainer decision — on scope, not on code. The implementation is the cleanest of these conversions I've read: zero hex literals left, the clearWidgetTracking() hack retired by construction rather than worked around, the pill re-parenting fixing the exact coordinate hazard the triage comment predicted, contrast improving on main in nine of ten measured pairs, and a genuine keyboard bug fixed on the way past. I found no blocker.
What it cannot supply for itself is the pick: #6239 doesn't list this window, dialog-patterns.md:276 currently says tool dialogs keep PersistentDialog, and the PR edits that documentation to say otherwise. Concrete next step: @ten9876 rules once on #6239 for the batch (#6745 and #6749 are in the identical position), and in the same breath on the FramelessResizer/fixed-size question and on which geometry pattern a fixed-size canon dialog follows. If the pick lands, this merges with nits — of which the test mirroring #6745's is the one I'd ask for.
Thanks @w5jwp — the token work and the a11y additions here are solid, and deferring the pick on #6745 yourself was the right call.
🤖 aethersdr-agent · cost: $14.4460 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (91 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: $15.5737 · model: claude-opus-5 |
|
Thanks for the review. Addressed in
|
CI: both reds are the same checker, and the fix is in the doc generator, not in your docsThanks for this — the conversion itself is fine, and the two red checks on What's red
Both run The cause I reproduced it on your head commit (
Please don't do what the error message says. Running Why your local run passed. It did, at Suggested fix — teach the parser the macro. STRING_WRAPPERS = ("QStringLiteral", "QLatin1String", "QString", "tr", "QObject::tr",
"QT_TR_NOOP")
# QT_TRANSLATE_NOOP(context, "text"): the text is the second argument.
QT_TRANSLATE_NOOP_RE = r'QT_TRANSLATE_NOOP\s*\(\s*"(?:[^"\\]|\\.)*"\s*,(.*)\)'and in if m:
e = m.group(1).strip()
else:
m = re.fullmatch(QT_TRANSLATE_NOOP_RE, e, re.S)
if m:
e = m.group(1).strip()I tried exactly that on your branch and Alternative, your call: drop the macro and keep
CodeGuard, for completeness: Nice piece of work, and the Return-key fix in particular is a real improvement. ~6 lines in the generator and both checks go green. 🤖 Generated with Claude Code 🤖 aethersdr-agent · cost: $7.2933 · model: claude-opus-5 |
…kinned to the canon palette. Principle VIII. The mapper moves from PersistentDialog to a CanonWindow dialog (Kind::Dialog), as the AetherRX / AetherTX Settings dialogs and AGC-T Calibration did: canon header, ambient ground, corner close button, Esc and Ctrl+W / Cmd+W to close, centred on the main window. It no longer follows View -> Frameless Window, so the menu action creates it directly instead of through showOrRaisePersistent(), and as a centred dialog it no longer saves its geometry. Look: the canon/ulanziDial theme scope, the ModemChrome Canon sheet the Settings dialogs use, and canon pills (cyan on control, aqua on hover and focus). The status line colours from a ulanziState property instead of hard-coded hex, which also retires the ThemeManager tracking workaround. The painted fallback dial uses color.canon.* and repaints on a theme change. The Linux "pkexec was not found" notice is a CanonMessageBox with its wording unchanged. Every pill is now a canvas child placed in canvas coordinates (the old dialog-coordinate pills were offset by the canvas position), so the post-show re-layout is gone. Positions match the previous window. The canvas's own transparent-background rule is scoped to the canvas, so it no longer overrides the canon sheet on the pills and their popup lists. Return in a pill no longer presses Reset to Defaults (setReturnClicksDefault(false)), and every pill has an accessible name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…its retired geometry key goes at startup, and a test pins Return and the status states. Principle VIII. - The status line and the Last event readout get accessible names; a pill's name translates its control label too (the labels are marked with QT_TRANSLATE_NOOP and read through tr()). - UlanziDialMapperGeom joins the keys main.cpp retires at startup: the canon mapper opens centred and nothing reads it. - dialog-patterns.md lists the mapper among the canon tool windows and says why it keeps a fixed size instead of installing FramelessResizer: its pills sit at fixed points on the dial picture. ulanzi_dial_mapper_dialog_test (19 checks, no backend or device): the mapper is a fixed-size CanonWindow with no resizer; Return in a pill leaves every mapping alone (fails with setReturnClicksDefault(true)); the status line reads Connected (without the Keyboard suffix), Disconnected, and off when the dial is turned off in Radio Setup (fails without that short-circuit); every control is named; and Close, Esc, Ctrl+W / Cmd+W and the corner button each destroy the WA_DeleteOnClose window. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…NOOP table entries as their text, so the Ulanzi button table names its buttons. Principle VIII. literal() in tools/docs/gen_reference.py unwrapped QStringLiteral/tr/... but not the translation no-op macros, so the kPillSpecs labels marked with QT_TRANSLATE_NOOP read as raw source and gen_reference.py --check failed in Static checks and Build docs site. QT_TRANSLATE_NOOP takes the context first, so it has its own pattern; QT_TR_NOOP joins STRING_WRAPPERS. tools/test_gen_reference_literal.py pins both, registered in tests/tests.cmake as gen_reference_literal_test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
76d5b30 to
0b84f76
Compare
|
Thanks, that was it. Fixed in
|
Summary
Closes #6748. Part of #6239 and approved RFC #6226.
The Ulanzi Dial mapping window (
UlanziDialMapperDialog) moves fromPersistentDialogto aCanonWindowdialog (Kind::Dialog), as theAetherRX / AetherTX Settings dialogs and AGC-T Calibration did:
corner close button. It closes with Close, the corner ✕, Esc or Ctrl+W / ⌘W
and opens centred on the main window. It no longer follows
View → Frameless Window, so the Settings → Controllers → Ulanzi Dial
Mapping... action creates it directly instead of through
showOrRaisePersistent().FramelessResizeras canon tool windows otherwise do: the pills sit at fixed points on the dial picture, so a larger window would only add empty ground.dialog-patterns.mdnow says so, so the next conversion doesn't have to guess.canon/ulanziDialtheme scope with theModemChrome Canon sheet the AetherRX / AetherTX Settings dialogs use, plus:
canon.control, alineHihairline, aqua on hover andfocus, the popup list on
canon.nestedwith a cyan selection;(
QWidget#UlanziDialCanvas), so it does not override the body sheet onthe pills it now holds;
ulanziStateproperty: connectedaccent.success, disconnected or failedaccent.danger, the Linuxpermission warning
accent.warning, turned offcanon.muted. Thisreplaces five raw hex stylesheets and the
clearWidgetTracking()workaround they needed;
through; the painted fallback dial (used when the photo is missing) is
drawn from
color.canon.*and repaints on a theme change.Reset to Defaults (
setReturnClicksDefault(false), as a page ofcontrols rather than a form;
mainpressed it), and every pill has anaccessible name ("Top Left key action", "Tuning action", "Single tap
action", …). The control labels in those names go through
tr()(markedwith
QT_TRANSLATE_NOOPinkPillSpecs). The status line ("Ulanzi Dialconnection status") and the Last event readout ("Last Ulanzi Dial
event") are named too.
CanonMessageBoxwith its wording unchanged, as the CTR2 popups became in feat(ctr2): the CTR2 window's questions in the canon style, and a restart that beats its answer is not an error. Principle VIII. #6736.
to be children of the dialog, placed in dialog coordinates that had the
canvas offset (10, 37) baked in, and needed a re-layout after show. Every
pill is now a canvas child placed in canvas coordinates, so the header
cannot shift them, the re-layout is gone, and the canon sheet on the body
reaches them. Positions match the previous window.
Docs:
docs/user/docs/ulanzi-dial.mdsays how to close the window.docs/style/*list it among the canon windows and thecanon/ulanziDialscope. That includes the tool-window list in
dialog-patterns.md, withthe fixed-size note above.
tools/docs/gen_reference.pyreads aQT_TRANSLATE_NOOP("context", "text")orQT_TR_NOOP("text")table entryas its text, so the Ulanzi button table in the generated reference keeps
reading "Top Left", … with the labels marked for translation. Pinned by
tools/test_gen_reference_literal.py(gen_reference_literal_test).ulanzi-mapper.pngshowsthe old window (the capture tool's passes are Linux-only).
Intent
After this merges the Ulanzi Dial mapper looks and closes like the other
canon windows. Out of scope: mappings, defaults, the backend and dispatch,
and the dial photo not loading from a macOS app bundle (it is not in a
.qrc; the dialog only looks next to the binary — onmaintoo).Screenshots
Before (
main, the current docs shot):After (this branch, macOS offscreen; dial photo placed in the build bundle
for the shot):
An open pill list, on
canon.nestedwith the cyan selection:Constitution principle honored
Principle VIII (Evidence Over Assertion) covers the checks below. The visual
direction is RFC #6226's.
Test plan
cmake --build build --target AetherSDR, macOSarm64, Qt 6.12)
canon_window_test,theme_manager_test,theme_seed_testandulanzi_mapping_migration_testpass locally (the last scans thisfile's
kPillSpecsfor TX defaults).gen_reference.py --check(on the rebased head, after the generatorfix), test registration, CI test gate, qsettings, theme seed,
network timeouts, command plane.
gen_reference_literal_test(11 cases):QT_TRANSLATE_NOOPwithspacing, a newline and a comma in the context;
QT_TR_NOOP; theexisting wrappers; and non-literals that must stay
None.Mutation-checked: without the
QT_TRANSLATE_NOOPbranch three cases failand
--checkreports the macro call in the button column; withoutQT_TR_NOOPits case fails.AETHER_AUTOMATION_NO_TX=1, offscreen and on screen: opened from themenu and grabbed (above); a pill's popup opened with
showPopupon"Tuning action" and grabbed on the canon ground.
ulanzi_dial_mapper_dialog_test(19 checks, socket-free, no backend or device):CanonWindowwith noFramelessResizer;WA_DeleteOnClosewindow.setReturnClicksDefault(true)fails the Return check, and removing the turned-off short-circuit inonConnectionChangedfails the off check.ci.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.tools/check_a11y.py: 0 findings.node tools/docs/test_log_rules.mjs(767 checks, 49 rules) and thedocs site build (
npm ci && npm run buildindocs/user) pass locally.CanonMessageBox, Windows. The Linux permission state draws incolor.accent.warning, which measures 2.85:1 on the canon ground inDefault Light, under 4.5:1. That's the shared warning token, used by many
widgets, not something this PR introduces.
Checklist
AppSettingscalls. As a centred dialog it no longersaves its geometry, so
main.cppretiresUlanziDialMapperGeomatstartup beside
AboutDialogGeometry. feat(gui): MQTT Settings is a CanonWindow dialog, re-skinned to the canon palette. Principle VIII. #6751 edits the same retired-keysline for MQTT Settings, so whichever lands second takes a one-line rebase.
MeterSmoother(n/a)docs/user/docs/ulanzi-dial.md,docs/style/*🤖 Generated with Claude Code