Skip to content

fix(tests): render the headphone gate test's text without antialiasing. Principle XI. - #6424

Merged
ten9876 merged 1 commit into
mainfrom
fix/headphone-gate-label
Oct 12, 2026
Merged

ten9876 merged 1 commit into
mainfrom
fix/headphone-gate-label

Conversation

@ten9876

@ten9876 ten9876 commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Full Suite has been red on main since #6413 (37fbdbc). Every run since then fails one test:

FAIL!  : RadioSetupHeadphoneOutputGateTest::dimmedRowsLookUnavailableInBothThemes()
  Default Dark  | Headphone: label: not the unavailable token #3a4a5a
  Default Light | Headphone: label: not the unavailable token #b0bcc8

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.unavailable value 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. With hintnone, the Mute check breaks the same way.

Fix

Test-only. main() sets QFont::NoAntialias on the application font, so every glyph pixel is the label's color under any fontconfig.

Verification

  • Passes under three font setups: the LCD (CI-like) config, LCD + hintnone, and native Arch fonts.
  • Mutation: pointing QLabel[rsTone="caption"]:disabled at color.canon.muted still fails the test with "not the unavailable token".

🤖 Generated with Claude Code

…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
ten9876 requested a review from a team as a code owner October 12, 2026 22:50
@ten9876
ten9876 merged commit 2452cb5 into main Oct 12, 2026
9 checks passed
@ten9876
ten9876 deleted the fix/headphone-gate-label branch October 12, 2026 23:17

@ten9876 ten9876 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 returned Totals: 6 passed, 0 failed. Parent failed on the missing unavailable-token pixels, including Headphone labels in both themes. Native Arch fonts also gave head 6 passed, 0 failed. Exact author/CI pixel counts were not reproduced, but the failure class was.
  • Mutation: overriding only QLabel[rsTone="caption"]:disabled with color.canon.muted makes the fixed test fail for both captions in both themes: Default Dark | Headphone: label: not the unavailable token #3a4a5a, and the corresponding light-theme #b0bcc8 failure. 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.

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. pixelsOfColour still demands a pixel equal to color.control.unavailable, and NoAntialias just guarantees such a pixel exists. Your mutation evidence (repointing QLabel[rsTone="caption"]:disabled at color.canon.muted still fails) shows it stays discriminating. The cost is that it asserts against a rendering mode no operator ever sees, and it leans on QFont::NoAntialias being 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}} #c0c8d0 sits ~71% from color.canon.nested #dde5ed toward color.control.unavailable #b0bcc8, so it can be counted from a different token than the one under test. Dark is immune (canon.line there 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::NoAntialias is a style-strategy hint; it is honoured by QFontEngineFT (which is what the offscreen platform 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 #6418 so 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: 12px and font-weight: 700 (src/gui/RadioSetupDialog.cpp:853-860, 1069-1071), and a stylesheet-declared font is a plausible way to lose NoAntialias. It survives: QStyleSheetStyle resolves its rule font against the widget's font, and QFont::resolve() only overrides resolved properties — the rules set no style strategy, so StyleStrategyResolved is unset and NoAntialias inherits 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. ThemeManager never touches fonts and no theme JSON declares font-family, so the per-theme setActiveTheme() 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 value or mute still failing. The rows at src/gui/RadioSetupDialog.cpp:5193-5265 construct hpLabel/lineoutLabel/hpMute/lineoutMute with no setFont call, so all inherit the app font. Held.
  • "The fix breaks the adjacent controls in the same slot." NoAntialias removes AA fringes, which lowers every differingPixels count — 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 by differingPixels returning < 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, main still carries pixelsOfColour at tests/radio_setup_headphone_output_gate_test.cpp:382, and 574f77a9e (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-BACKTICK at :75 — refuted and dropped. The line is a // comment, "The capabilities the production backend for family declares", whose backticks are Markdown quoting of the parameter name. It is pre-existing on main at the identical line number and untouched by this diff, and the file contains no QProcess/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

@aethersdr-agent

Copy link
Copy Markdown
Contributor

Cisco CodeGuard — static analysis of this PR (1 finding(s))

  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in tests/radio_setup_headphone_output_gate_test.cpp /tmp/aetherclaude/pr-6424/tests/radio_setup_headphone_output_gate_test.cpp:75

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant