Repository navigation
fix(gui): AetherRX and AetherTX check boxes and radio buttons draw their canon indicators again. Principle VIII. - #6345
Conversation
…eir canon indicators again. Principle VIII.
ModemChrome's canon look takes the sheet's own QCheckBox/QRadioButton
indicator rules out before appending the painted canon indicators. The
pattern ended each rule at the first "}", but in the canon look those rules
hold {{color.canon.*}} placeholders, so every match stopped inside a token
and left "}};" fragments behind. Qt rejects a stylesheet it cannot parse as a
whole, so the stage column and the AetherNR page lost the entire canon chrome
sheet: unchecked boxes and radios drew nothing, a checked box was a bare tick
and a selected radio a small dot. A rule body now ends at the first brace
outside a token.
modem_chrome_test:
- canonSheetLeavesNoRuleFragments: with tokens set aside, every brace in the
canon sheet opens or closes a rule at the top level.
- canonIndicatorsRenderTheirImages: a check box and a radio button under the
canon sheet draw the same pixels as under the canon indicator rules alone,
checked and unchecked.
Both fail with the old pattern.
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. No separate bug issue; the body names this as part of #6239 and a regression fix for #6339 (2eb01f48a, merged the same day the regex landed). GOVERNANCE.md does not want an RFC for a bug fix with a clear root cause, and the root cause here is exact and reproducible.
I reproduced the defect without a build. Taking the five indicator rules at src/gui/ModemChrome.cpp:165-189, substituting the kCanon values from src/gui/ModemChrome.cpp:51-70 (%17→{{color.canon.lineHi}}, @radioFill@→{{color.canon.control}}, …) and running the old pattern (?:QCheckBox|QRadioButton)::indicator[^{]*\{[^}]*\}\s* leaves exactly what the body claims:
};
background: {{color.canon.control}};
}
};
background: {{color.canon.nested}};
}
};
}
...
Five orphan fragments, each starting with a top-level }. The new pattern removes all five rules cleanly and leaves the neighbouring QRadioButton:disabled, QCheckBox:disabled and QPushButton rules untouched. Fed the Modem-look hex literals instead of tokens, old and new produce byte-identical output — the new pattern is a strict superset of the old behaviour, so the Modem look (which does not run this branch anyway) cannot shift.
The body's explanation of why #6339 shipped broken is also correct and worth crediting: the old pattern did remove the ::indicator selector text, so canonLookPaintsItsIndicators (tests/modem_chrome_test.cpp:155-166) compared count("::indicator") on a sheet Qt would reject outright and passed. canonSheetLeavesNoRuleFragments closes precisely that gap, deterministically and without depending on how Qt recovers from a parse error.
Scope
| File | What it changes | Claimed by the PR? | Verdict |
|---|---|---|---|
src/gui/ModemChrome.cpp:396-408 |
One regex widened to skip {{token}} braces, plus the comment explaining why |
Yes — this is the fix | In scope |
tests/modem_chrome_test.cpp |
Two new test slots + one indicatorPixels helper and its includes |
Yes — the Principle VIII evidence | In scope |
Everything in the diff is explained by the stated fix. No new public surface, no config key, no settings write, no protocol verb, no CHANGELOG.md entry, no behaviour-flag default moved. The only - lines are the replaced regex and the reflowed comment — no guard, early return or symptom-naming comment was deleted. Nothing bundled.
Blockers
None.
Nits (non-blocking)
-
The body's "never affected" list is wrong about two windows. It says "Windows whose canon sheet doesn't go through ModemChrome were never affected: Connect to Radio, AetherModem, AetherMap, Network Diagnostics and the Settings dialogs." But
src/gui/AetherRxSettingsDialog.cpp:34-36andsrc/gui/AetherTxSettingsDialog.cpp:35both do go through it:applyCanonSheet(bodyWidget(), [] { return ModemChrome::styleSheet(ModemChrome::Scale::Dialog, ModemChrome::Look::Canon); });
Those are the AetherRX/AetherTX Settings dialogs #6339 moved to
CanonWindow— by the same mechanism they lost their whole canon sheet too, and this fix restores them. That makes the fix better than advertised, but the claim as written would send someone looking in the wrong place next time. Worth correcting in the body (the title's "AetherRX and AetherTX" covers it either way). -
tests/modem_chrome_test.cpp:84— the backdrop comment overstates whatDrawChildrensuppresses. "Over one fixed backdrop, without the widget's own background" — omittingDrawWindowBackgroundskips thebackgroundRolefill, but a QSSbackgroundonQCheckBoxis still painted in the widget's ownpaintEvent. The test passes today only because canon gives bothQWidgetandQRadioButton, QCheckBoxbackground: transparent(src/gui/ModemChrome.cpp:105,156). If the canon palette ever gives a check box a non-transparent background, this test fails even though the indicators are perfectly correct. Not worth changing now; worth knowing when it next goes red. -
tests/modem_chrome_test.cpp:76—option.initFrom(&button)does not carryState_On, sosubElementRectis computed state-blind for both the checked and unchecked passes. Harmless today (the canon:checkedrules keep the same 18×18 geometry), but it means the render test would not notice a size change confined to a:checkedrule.
What I tried to break
- The new pattern against unbalanced input.
(?:\{\{[^{}]*\}\}|[^{}])*only accepts a brace as part of a complete{{…}}pair, so a body carrying a single stray{or}would fail to match and the rule would survive — but no rule body in the sheet contains one, and the Modem look's values are plain hex. The alternation is also unambiguous (the first branch requires{{), so there is no catastrophic-backtracking path on a long body. - Whether the regex can over-reach.
[^{]*between::indicatorand\{cannot cross a brace, so it cannot swallow a following selector; and the multi-selector rules that are not indicator rules (QRadioButton,\nQCheckBox {atsrc/gui/ModemChrome.cpp:155,QRadioButton:disabled,\nQCheckBox:disabled {at:161) do not contain::indicatorand survive — confirmed in the reproduction above. - Ordering. The removal at
:406runs after thec.literalssubstitution loop at:393, which is what puts the{{…}}tokens into those bodies in the first place — so the fix is at the right seam. Worth saying explicitly that the "cleaner" alternative of never emitting the rules for Canon is not better here:%15/%16/%20appear only in indicator rules, andQString::argchaining fills the lowest remaining%N, so deleting them from the template would silently shift every later positional argument. Post-substitution removal is the right call. - Fail-closed path.
canonIndicatorRules()returns{}whenThemedImageCachecannot write (src/gui/CanonIndicators.h:119-124), and the newQVERIFY(!indicators.isEmpty())attests/modem_chrome_test.cpp:197would then fail rather than skip. That is not a new environment dependency: the pre-existingcanonLookPaintsItsIndicatorsalready assertscanon.contains("canon-indicators/check_on_"), so an unwritable cache already reds this target. And the production fallback is intact — with empty rules the canon sheet simply carries no indicator rules and Qt's own are used, asCanonIndicators.h:14-16documents. - Sibling sheets. The other six callers of
canonIndicatorRules()(ConnectionPanel.cpp:365,WaveformsDialog.cpp:928,Ax25HfPacketDecodeDialog.cpp:961,TailnetShimDialog.cpp:133,PskReporterMapDialog.cpp:471,NetworkDiagnosticsDialog.cpp:343) append to static sheets. Grepped all six for::indicator: zero hits, so none carries its own indicator rules that would border the painted image, and none needs the regex.src/gui/ModemChrome.cpp:407is the only place insrc/with this pattern — no sibling left unfixed. - Whether the new tests would pass against the unfixed code.
canonSheetLeavesNoRuleFragmentswould not: the first orphan}drives--depthto-1and tripsQVERIFY2.canonIndicatorsRenderTheirImagesis weaker than it looks — it only reds on the old code if Qt drops the whole sheet on the parse error (which the body's symptom implies it does). The depth test carries the regression coverage regardless, so I would not treat the render test as the proof.
Honesty notes. I am headless: no build, no test run, no GUI. The regex behaviour above is reproduced in re, which matches Qt's PCRE semantics for these constructs, but the claims about what Qt does with the resulting sheet are reasoned from code, not observed. Separately: modem_chrome_test appears in no ctest -R step in .github/workflows/ci.yml — the only filtered runs there are the DV/thumbdv group, cross_needle_meter_test, mac_nr_filter_test and asr_gpu_probe_test. All eight checks green on f0d05f3 proves these two tests compile, not that they pass. The target is registered correctly (tests/tests.cmake:2044-2057, with QT_QPA_PLATFORM=offscreen), and the body checks the box — pasting the actual ctest -R modem_chrome_test --output-on-failure output would settle it. No socket, no fake peer, no network behaviour anywhere in this diff.
Security audit. Ran the security-audit subagent over the nine files the harness listed. Verdict: no findings. All of CodeGuard's hits are on files this PR does not touch, and each is a pattern false positive — markdown backticks quoting radio wire commands inside // comments, C++ & references and || operators read as shell metacharacters, 4294967296.0 (the 2^32 DDS phase-word scaler) read as a bank account, RFC 5737 TEST-NET-1 documentation addresses and loopback in a UDP test, and typographic dashes read as homoglyphs. Nothing to act on in this PR. One incidental observation for later, not a finding: #include <QProcess> at src/gui/RadioSetupDialog.cpp:87 is unused (QProcess appears nowhere else in that file) and is what gives the command-injection rule a plausible anchor there — dropping it would stop the file re-flagging on every scan.
Recommendation
Approve. One-regex fix at the correct seam, with the failure mode reproduced exactly as the body describes and a deterministic regression test that reds on the old pattern — which is more than the test it supersedes could do. The two nits are about test fragility and one inaccurate sentence in the body, neither worth a round trip on its own. Concrete next step for the maintainer: since modem_chrome_test is not in any CI ctest -R filter, ask @ten9876 to paste the local ctest -R modem_chrome_test --output-on-failure run (and, while editing, correct the "Settings dialogs were never affected" line — AetherRxSettingsDialog.cpp:34 and AetherTxSettingsDialog.cpp:35 both route through ModemChrome's canon look and were broken too).
Nice root-cause write-up, and thank you for making the why a test rather than a comment.
🤖 aethersdr-agent · cost: $9.2299 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (142 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: $9.3423 · model: claude-opus-5 |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
The fix satisfies its stated part of #6239: it repairs the stylesheet regression introduced by #6339. The token-aware rule matcher removes complete indicator rules instead of stopping inside a {{color.canon.*}} placeholder. There is no standalone closing issue; this is a demonstrated bug fix restoring the approved canon appearance, with no new feature or architecture decision.
Scope
| File | Change | Claimed? | Verdict |
|---|---|---|---|
src/gui/ModemChrome.cpp |
Remove complete canon indicator rules while preserving token braces | Yes | In scope |
tests/modem_chrome_test.cpp |
Check rule structure and actual checked/unchecked checkbox/radio pixels | Yes | In scope |
Everything in the diff is explained by the stated fix. No settings, defaults, public API, protocol, dependency, transmit, or CI-gate changes were found. The commit has a verified good signature. User documentation does not need a behavior update for this restoration of the documented appearance.
Blockers
None.
Nits — non-blocking
-
Correct the unaffected-windows claim in the PR body.
src/gui/AetherRxSettingsDialog.cpp:35andsrc/gui/AetherTxSettingsDialog.cpp:35both callModemChrome::styleSheet(Scale::Dialog, Look::Canon). Consequently, the sentence saying Settings dialogs do not pass through ModemChrome and were never affected is incorrect. They also receive the repaired full stylesheet, although they do not themselves contain checkbox/radio controls. Replace that paragraph with:Windows whose canon sheet does not go through ModemChrome were never affected: Connect to Radio, AetherModem, AetherMap and Network Diagnostics. The AetherRX and AetherTX Settings dialogs also use ModemChrome and receive the repaired canon stylesheet.
Verification and attacks
- Built the actual merge base
deab0a763and exact headf0d05f345in owned scratch worktrees, using Qt 6.12 on Linux with the lean shared-core build. Built only AetherSDR and the three named test targets. Headmodem_chrome_test,stage_tab_bar_drag_test, andcanon_window_testall pass (3/3). These inspected targets own no sockets; the modified rendering test is socket-free. - Compiled the new assertions against the merge-base production object, reverting the actual fix rather than merely inspecting the assertions. Both new tests fail: the brace check reports
a closing brace has no rule to close; the rendering check reports an actual 14×14 fallback checkbox versus the expected 18×18 image. Both pass on the head. This demonstrates that the tests detect the regression. - Drove merge-base and head AetherSDR through explicitly selected
aethersdr-pr6345-baseandaethersdr-pr6345-headbridge sockets, each with fresh isolated settings, offscreen rendering, and TX disabled. Connected only toDEMO-0001; the returned model wasAetherSDR Demo, with one slice and pan andtransmitting: false. Before captures show missing checkbox outlines and bare radio dots; head captures show full outlines, rings, and checked indicators. Clicked stage Gate checkboxes, checked AetherNR controls, exercised Default/Light themes, captured both Settings dialogs, and closed/reopened panels. All review-owned app instances were closed and their exit confirmed; the operator session was untouched. - Inspected token substitution, both Dialog/Compact sheets, the untouched Modem look, sibling call sites, and the rendering helper's checked-state handling. The helper uses the style option to locate the crop; the actual checked widget renders its own state, so the bot concern about a missing
State_Onin that crop option does not demonstrate a defect. - Independent canon/scope and lifecycle/test audits found no code blocker. The model-invocable automated
code-reviewskill was unavailable, so this was a manual review with independent passes. Existing CodeGuard findings are outside this two-file diff; inspected representative critical entries were ordinary comments, not vulnerabilities introduced here. I did not individually validate every one of its 142 entries.
Boundary/capability/command-plane gate results are unchanged between base and head: 102 existing engine-boundary findings, 70 capability booleans, and 32 command-plane files / 368 commands; touchpoint manifest checks pass on both sides. No new boundary findings were introduced.
CI and limits
All eight reported checks are successful, including Linux, Windows, macOS, and Static checks. However, the earliest run was created at 2026-10-10 16:16:42 UTC, while current main is 5538b8b7af1cfaffc1423a6fdf14a7191754d977 dated 16:39:58 UTC. Those green pull-request checks describe an older merge result. With strict: false, a refresh is not forced. Local verification covers the exact PR head and its actual merge base, not the latest-main merge candidate. Windows/macOS runtime rendering and real-radio behavior were not exercised; their build checks passed, and this change contains no radio path.
Recommendation: Approve with nits. The original defect is reproduced, the new tests fail without the fix, and the corrected GUI survives checked/unchecked rendering, theme changes, and panel reopening. Correct the PR-body affected-window list, then refresh/verify the merge candidate against current main before merging. This COMMENT review leaves formal approval to the maintainer.
|
Both reviews have no blockers. Here's where each point landed, all at head
|
Summary
Part of #6239. Fixes a regression from #6339: in AetherRX and AetherTX, unchecked check boxes and radio buttons drew nothing. A checked box showed only a bare tick and a selected radio only a small dot, so it wasn't clear what was selected.
Cause: ModemChrome's canon look takes the sheet's own
QCheckBox/QRadioButtonindicator rules out before appending the painted canon indicators (canonIndicatorRules()). The pattern ended each rule at its first}. In the canon look those rules hold{{color.canon.*}}placeholders, so every match stopped inside a token and left}};fragments behind. Qt rejects a stylesheet it cannot parse as a whole, so the stage column and the AetherNR page lost the entire canon chrome sheet, not only their indicators.Fix: the pattern now ends a rule body at the first brace outside a token (
src/gui/ModemChrome.cpp).Visible result: the canon chrome sheet applies to the stage column and the AetherNR page again. That restores:
The AetherRX and AetherTX Settings dialogs also use ModemChrome's canon look, and they get the repaired sheet too. Windows whose canon sheet doesn't go through ModemChrome were never affected: Connect to Radio, AetherModem, AetherMap and Network Diagnostics.
Constitution principle honored
Principle VIII (Evidence Over Assertion). The existing test only counted
::indicatorselectors, so it passed while the sheet was broken. The new checks test what Qt parses and draws.Test plan
modem_chrome_testgains two checks, and both fail with the old pattern:canonSheetLeavesNoRuleFragments: with tokens set aside, every brace in the canon sheet opens or closes a top-level rule.canonIndicatorsRenderTheirImages: a check box and a radio button under the canon sheet draw the same pixels as under the canon indicator rules alone, both checked and unchecked.modem_chrome_test,stage_tab_bar_drag_testandcanon_window_testpass.check_engine_boundary.py --strictandgen_touchpoint_manifest.py --checkpass. No new touchpoints.Checklist
AppSettingskeysMeterSmoother(unchanged)🤖 Generated with Claude Code