Repository navigation
fix(tests): render the headphone gate test's text without antialiasing. Principle XI. - #6424
Conversation
…g. Principle XI. Full Suite has been red on main since #6413: radio_setup_headphone_output_gate_test fails "Headphone: label: not the unavailable token" in both themes. The dimmed label does take the unavailable color; the test counts pixels of that exact color, and under the Ubuntu CI image's subpixel antialiasing at the new 12px caption size no pixel of "Headphone:" is fully covered. Reproduced locally with a DejaVu + rgb/lcddefault fontconfig (461/303 differing pixels against CI's 460/303); hintnone breaks the Mute check the same way. The test now sets QFont::NoAntialias on the application font, so every glyph pixel is the label's color under any fontconfig. Passes under the LCD, hintnone and native configs; pointing the disabled caption rule at another token still fails it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ten9876
left a comment
There was a problem hiding this comment.
Reviewed head 24b2aaa6dd9c7a87f2bb2852c5ab11951f8b5316. Issue-fit: yes. No closing issue is linked; the stated problem is the font-dependent Full Suite failure following #6413. The explicit QFont include and process-local NoAntialias setting address it without changing shipping UI, assertions, registration, or production defaults. This is a clear-root-cause test/CI fix, which GOVERNANCE.md exempts from an RFC. The PR merged while this review was in progress.
Scope
| File/group | Change | Claimed? | Verdict |
|---|---|---|---|
tests/radio_setup_headphone_output_gate_test.cpp:37,465–471 |
Include QFont; disable text antialiasing for this test application before constructing widgets | Yes, explicitly test-only | In scope |
Everything in the diff is explained by the stated fix. One commit, no unrelated files, no new public surface, no removed guards, and no production UI preference change. No user-doc or changelog update is required.
Blockers
None.
Nits (non-blocking)
None.
Verification and attacks survived
- Socket preflight passed: the injected backend never dials a peer; the Demo uses its synthetic in-process connection. No added socket ownership or changed registration.
- Independently compiled the exact head and parent test sources into scratch binaries, reusing unchanged objects/libraries from the clean build at this exact PR head (static-core linkage). No shared checkout or existing build was modified. Full scratch checkout attempts hit the temporary-file quota; their incomplete worktree was removed. A sandbox audio-service stall disappeared outside the sandbox; only the review's stalled test process was terminated.
- With DejaVu-only RGB/LCD-default fontconfig, both hintslight and hintnone: parent returned
Totals: 5 passed, 1 failed; head returnedTotals: 6 passed, 0 failed. Parent failed on the missing unavailable-token pixels, including Headphone labels in both themes. Native Arch fonts also gave head6 passed, 0 failed. Exact author/CI pixel counts were not reproduced, but the failure class was. - Mutation: overriding only
QLabel[rsTone="caption"]:disabledwithcolor.canon.mutedmakes the fixed test fail for both captions in both themes:Default Dark | Headphone: label: not the unavailable token #3a4a5a, and the corresponding light-theme#b0bcc8failure. The assertion still detects the wrong token. Existing tests also passed separate HL2/ANAN gating, disconnect/reconnect, Demo positive control, repeatable rendering, and disabled engaged-Mute checks. - Inspected the Full Suite run log: target passed;
100% tests passed, 0 tests failed out of 793. Required PR checks passed. Earliest head run creation was 2026-10-12 22:50:04 UTC; current main's observed commit timestamp was 23:17:21 UTC. PR-triggered green checks therefore describe the older merge tree, while the dispatched Full Suite describes the branch head. Neither proves current main's post-merge suite. - No standalone app/bridge session: this changes only test rendering and was exercised through the real RadioSetupDialog in the focused GUI test with in-process Demo state. No operator app or live radio was driven. The optional automated code-review skill was unavailable; the equivalent audit was performed manually. Local macOS/Windows font rendering was not executed.
Recommendation: Approve. The before/after reproduction and wrong-color mutation independently support the fix, with no blockers or nits. Already merged; monitor the post-merge Full Suite on main for independent confirmation.
There was a problem hiding this comment.
Status first
This PR is closed and unmerged — 24b2aaa6d is not an ancestor of origin/main, and #6424 is absent from the open-PR list, while #6419 is still open. So this review is retrospective: it reads as though you withdrew #6424 in favour of #6419's pixelsInkedIn approach. If that's right, nothing here asks you to act — but the trade-off between the two is worth recording on the thread, because the approach you kept is the weaker of the two on one specific axis (below). Everything I checked on the code itself came out clean.
1. Issue fit
Yes — this is a correct and correctly-diagnosed fix for #6418 (fp 45383ccd1a1a, open, priority: high, maintainer-review). I confirmed the mechanism rather than taking the body's word for it:
The failing predicate is tests/radio_setup_headphone_output_gate_test.cpp:383:
if (pixelsOfColour(dimmed, QColor(unavailable)) <= 0)
failures << where + QStringLiteral(": not the unavailable token ")
+ unavailable;It runs over six parts — label, value, mute × Headphone:/Line Out: (:361-365). I traced every one of those three widget kinds to the QSS rule that paints it, and in all three the unavailable token arrives as text colour only, with no solid fill anywhere in the probed region:
src/gui/RadioSetupDialog.cpp:847-851—QLabel:disabled { color: {{color.control.unavailable}}; }src/gui/RadioSetupDialog.cpp:1069-1071—QLabel[rsTone="value"]:disabled, …:disabled { color: {{color.control.unavailable}}; }src/gui/RadioSetupDialog.cpp:1019-1027—QPushButton:disabled, … { color: {{color.control.unavailable}}; border-color: {{color.canon.line}}; background: transparent; }
Note the Mute's border is canon.line and its background is transparent, so the Mute is glyph-ink-dependent too — it is not rescued by a solid edge. The one place the token is a solid fill, QSlider::handle:horizontal:disabled { background: {{color.control.unavailable}}; } (:1096-1098), is not among the probed parts. So "the token only ever appears as glyph ink" is confirmed, and glyph coverage is exactly the right lever. The diagnosis in the body is sound.
One hygiene nit: the body describes #6418 in detail but contains no Fixes #6418 keyword, so merging would not have closed the sticky. #6419's body does have it.
2. Scope
Clean — one file, one include, seven lines, all of it explained by the stated fix.
| File / group | What it changes | Claimed by title/body? | Verdict |
|---|---|---|---|
tests/radio_setup_headphone_output_gate_test.cpp:37 |
#include <QFont> |
Yes (implied) | In scope — required by the new code |
tests/radio_setup_headphone_output_gate_test.cpp:465-471 |
Comment + QFont::NoAntialias on the app font in main() |
Yes — "main() sets QFont::NoAntialias on the application font" |
In scope |
No production code, no build/CI changes, no new public surface, no deleted guard, no settings or capability surface, no dead additions. No commit on the branch is unrelated to #6418. The body's "Test-only" and "No CI or workflow changes" self-certifications are both true as written.
Governance: a test-only fix for a CI failure with an identified root cause needs no RFC (GOVERNANCE.md exempts bug fixes with a clear root cause). No CHANGELOG.md entry was added — correct. Nothing here is a personal-preference change: it alters no user-visible default, only the test binary's own font rendering, and it points at an authority (the #6418 full-suite failure) as a fix must.
3. Blockers
None.
4. Needs maintainer decision (recorded, not a blocker)
tests/radio_setup_headphone_output_gate_test.cpp:465-471 vs #6419 — two open/competing fixes for the same #6418, same author, different seam. This PR's body does not mention #6419, so a maintainer reading either one alone would not know the other existed. The trade-off, stated plainly because it outlives this PR:
- #6424 (this one) keeps the assertion exact.
pixelsOfColourstill demands a pixel equal tocolor.control.unavailable, andNoAntialiasjust guarantees such a pixel exists. Your mutation evidence (repointingQLabel[rsTone="caption"]:disabledatcolor.canon.mutedstill fails) shows it stays discriminating. The cost is that it asserts against a rendering mode no operator ever sees, and it leans onQFont::NoAntialiasbeing honoured — it is documented as a hint, not a guarantee. - #6419 keeps real antialiased rendering but relaxes the predicate to "on the background→ink segment and ≥60% of the way there". That accepts neighbouring grey tokens: in Default Light the dimmed Mute's
border-color: {{color.canon.line}}#c0c8d0sits ~71% fromcolor.canon.nested#dde5edtowardcolor.control.unavailable#b0bcc8, so it can be counted from a different token than the one under test. Dark is immune (canon.linethere is 12%-alpha and blends to near-background). So #6419's check means "greyish toward the token", not "the token".
Neither is wrong; they trade strictness against renderer-independence in opposite directions. #6419 carries the much stronger evidence (96-config font sweep, old check failing 59 of them, against this PR's three font setups), which is presumably why you kept it — reasonable. Worth noting for the maintainer that the strictness #6424 preserved is real and #6419 gives it up, so if #6419 lands, the "is it this token" claim is best pinned separately — the polished-palette route in tests/radio_setup_label_theme_token_test.cpp:69-73 reads the Disabled group that QStyleSheetStyle fills from the matched :disabled rules, and is renderer-independent either way.
5. Nits (non-blocking)
tests/radio_setup_headphone_output_gate_test.cpp:465-468— the comment overstates slightly: "so 'the unavailable token is in the render' holds under any fontconfig".QFont::NoAntialiasis a style-strategy hint; it is honoured byQFontEngineFT(which is what theoffscreenplatform gets on Linux via the generic-unix fontconfig database), but "under any fontconfig" is stronger than a hint can promise. "under the fontconfigs we exercise" would be exact, and would not weaken the fix.- Body hygiene, as above: add
Fixes #6418so the sticky auto-closes, and cross-link #6419 so the two fixes are visibly alternatives.
6. Socket tests
Nothing to record. This PR adds and modifies no socket-owning test. I read the test source itself rather than tests.cmake alone: the file contains no QTcpServer/QTcpSocket/QUdpSocket/QLocalServer/QWebSocketServer, no bind()/listen()/connectToHost(), and no peer process. connectDemo() (:164-174) builds a RadioInfo with QHostAddress::LocalHost and port 4992, but the family is SimBackend::familyName() — the in-process demo backend, which opens no socket; the inline // never dialled comment is accurate. NoCommandPlaneBackend (:60-73) is an injected in-process IRadioBackend with no-op methods, not a fake radio peer speaking firmware, so it does not trip the no-synthetic-peer rule. Registration is unchanged at tests/tests.cmake:3824-3846 (QT_QPA_PLATFORM=offscreen, TIMEOUT 60 — an explicit opt-out of the 300 s retrofit default, pre-existing).
7. What I tried to break — and could not
Findings above are reasoned from code and from CI metadata; I am headless, with no build and no test run, so I reproduced nothing at runtime. Specifically:
- "The QSS resets the style strategy." The rules that paint the probed widgets also declare
font-size: 12pxandfont-weight: 700(src/gui/RadioSetupDialog.cpp:853-860,1069-1071), and a stylesheet-declared font is a plausible way to loseNoAntialias. It survives:QStyleSheetStyleresolves its rule font against the widget's font, andQFont::resolve()only overrides resolved properties — the rules set no style strategy, soStyleStrategyResolvedis unset andNoAntialiasinherits from the app font. Held. - "Something re-sets the app font after line 471."
grep -rn "QApplication::setFont\|qApp->setFont\|QGuiApplication::setFont\|setStyleStrategy" src/returns nothing — no UI-scale pass, no theme font pass.ThemeManagernever touches fonts and no theme JSON declaresfont-family, so the per-themesetActiveTheme()calls inside the slot cannot clobber it. Held. - "A probed widget has its own explicitly-constructed font." That would drop the strategy for that widget and leave
valueormutestill failing. The rows atsrc/gui/RadioSetupDialog.cpp:5193-5265constructhpLabel/lineoutLabel/hpMute/lineoutMutewith nosetFontcall, so all inherit the app font. Held. - "The fix breaks the adjacent controls in the same slot."
NoAntialiasremoves AA fringes, which lowers everydifferingPixelscount — so I checked whether any could collapse to the failing side.diff == 0("dimmed renders as live",:381) cannot trigger because the colours still differ;repeat != 0(:376) compares a widget to itself;redDimmed != 0/redLive <= 0(:417-421) sit on the engaged-Mute danger border, which is solid and axis-aligned and was never the fragile case; and the disconnect-restore checks (:447-453) are relative dimmed-vs-live comparisons. Size mismatches are guarded bydifferingPixelsreturning< 0(:379,:423). Both dialogs get the same app font, so the mono metrics shift identically and no region changes size relative to its twin. Held. - "The branch is stale and re-fixes something already fixed." My own notes claimed #6419 had already landed
pixelsInkedIn; it has not — #6419 is open,mainstill carriespixelsOfColourattests/radio_setup_headphone_output_gate_test.cpp:382, and574f77a9e(main tip) is an ancestor of this head. The branch is current and the bug was live. Held (my prior note was the thing that was wrong). - CodeGuard
CS-VLN-CMDI-BACKTICKat:75— refuted and dropped. The line is a//comment, "The capabilities the production backend forfamilydeclares", whose backticks are Markdown quoting of the parameter name. It is pre-existing onmainat the identical line number and untouched by this diff, and the file contains noQProcess/system()/popen()/exec*()— no command sink exists. The security audit found nothing else: no credentials (demoSerial()/demoModelName()are demo identity constants), no crypto, no file writes (QWidget::grab()is in-memory), no network boundary. Credit: security-audit (CodeGuard).
What I could not verify. The body's local evidence — the three font setups, the rgba=rgb/lcdfilter=lcddefault repro at 461/303 against CI's 460/303, and the hintnone Mute claim — I cannot reproduce. The evidence I do have is better than code reading, though, and it is worth putting on the record: the Full test suite check passed on this head (24b2aaa6d, run 38318904581). That is the same full-suite.yml lane whose push-to-main run 38295821226 opened #6418, dispatched here via workflow_dispatch against the branch — i.e. the fix was exercised on the exact Ubuntu lane that was red, and went green. I could not open that log to quote test 392's Passed line, because get_ci_run_log only returns failed-job output. Note this test appears in no ctest -R filter in ci.yml, so the ordinary PR lanes prove compile-only for it; that dispatched full-suite run is the whole of the runtime evidence, and it is the right evidence.
8. Recommendation
Approve with nits — but moot as withdrawn; the maintainer's live decision is #6419. The change is correct, minimal, in scope, well-diagnosed, and validated on the lane that was failing; had it stayed open I would have recommended merging it with the Fixes #6418 keyword added. Since it is closed and #6419 is the surviving fix, the useful next step is a one-line comment on #6419 recording that #6424 was the exact-match alternative and that #6419 trades token-exactness for renderer-independence — so a future reader of #6418 can see both branches were considered rather than rediscovering the neighbouring-grey weakness the hard way.
Thanks for chasing this to an actual rasterizer-level root cause with a local repro that matched CI's pixel counts to within one pixel, and for mutation-testing the predicate instead of just turning it green — that is what made it possible to review this without a build.
🤖 aethersdr-agent · cost: $9.0090 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (1 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.4892 · model: claude-opus-5 |
Full Suite has been red on main since #6413 (37fbdbc). Every run since then fails one test:
Cause
The app is correct: the dimmed Headphone label does change color (460 of 2160 pixels differ from the live one). The test, though, also checks that pixels of the exact
color.control.unavailablevalue appear in the render. That only happens when a glyph pixel is fully covered. #6413 moved captions to the canon 12px style. The CI image (Ubuntu 24.04, DejaVu) renders with subpixel antialiasing, and under it no pixel of "Headphone:" is fully covered. "Line Out:" happens to keep one.Reproduced locally with a DejaVu-only fontconfig set to
rgba=rgb,lcdfilter=lcddefault. It gives 461/303 differing pixels, against CI's 460/303, and the same failure. Withhintnone, the Mute check breaks the same way.Fix
Test-only.
main()setsQFont::NoAntialiason the application font, so every glyph pixel is the label's color under any fontconfig.Verification
hintnone, and native Arch fonts.QLabel[rsTone="caption"]:disabledatcolor.canon.mutedstill fails the test with "not the unavailable token".🤖 Generated with Claude Code