Repository navigation
fix(gui): selected items draw the palette highlight, not dark text on the accent — Customize Button Bar, menus, Radio Setup, MIDI. Principle VIII. - #6322
Conversation
…tte, not black on the accent. Principle VIII.
FavoritesPickerDialog's list stylesheet carried its own
`::item:selected { background: accent; color: #000 }`, the combination
#6312 removed from the app-wide lists and trees: black text is unreadable
on a dark custom accent, and it made this dialog's selection differ from
every other list. The rule is gone; selection comes from the palette.
favorites_picker_selection_test drives the real dialog offscreen, selects
a row and samples it: the pixel must be the palette highlight and not
color.accent. It fails with the rule restored.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Reviewed head f5149c3595b3ebb75a4574d47bacea33ec00fd1b. Recommendation: Approve with nits. No blockers.
There is no linked closing issue; I reviewed the stated selection-rendering defect and the accepted #6312 follow-up. Removing the dialog-specific selected-item rule restores palette selection in both lists. The change solves the stated defect; it introduces no architecture or protocol change requiring an RFC.
Scope
| File | Change | Claimed? | Verdict |
|---|---|---|---|
src/gui/FavoritesPickerDialog.cpp |
Remove accent/black selection override from the shared list stylesheet | Yes | In scope |
tests/favorites_picker_selection_test.cpp |
Render the real dialog and check selected-row background | Yes | In scope |
tests/tests.cmake |
Register the socket-free offscreen test | Yes | In scope |
Everything in the diff is explained by the stated fix. The single commit's author date and content match the PR; there is no unrelated churn, new persistence key, or changelog entry.
Blockers
None.
Nits — non-blocking
tests/favorites_picker_selection_test.cpp:48: The assertions cover the background but not the white foreground that makes selection readable. I restored onlyQListWidget::item:selected { color: #000; }, leaving palette selection in place, and the test still passed. Consider checking selected-text rendering too, ideally with a dark custom accent. This is additional coverage, not a defect in the submitted rendering: the real GUI showed white text on palette blue.
What I tried to break
- Built
AetherSDR,favorites_picker_selection_test, andtheme_manager_testwith Qt 6.12 on Linux. Both CTests passed. Restoring the complete old rule failed both checks:selected row is drawn in color.accent (#00b4d8)andselected row #00b4d8 is not the palette highlight #308cc6. Restored the submitted source and reran both tests successfully. - Drove merge base
4e818e24fand this head through isolated offscreen demo instances (DEMO-0001, NO_TX, private network namespace).contextMenu 'Toggle button drawer', selection clicks, and dialog grabs reproduced black-on-cyan before and white-on-palette-blue after. Also moved PWR into Hidden, selected it, canceled and reopened; both lists remained readable. Repeated with a custom#082030accent. All owned instances were closed. - Strict engine-boundary, command-plane, capability-record, QSettings, colour, test-registration, touchpoint and CI-gate checks passed. Per-file engine/command/capability findings and the touchpoint check were identical to the merge base. Colour references decrease by one; stylesheet calls stay flat. No governance, ownership or dependency violation found. An automated
code-reviewskill was unavailable; this was a manual adversarial pass.
Linux offscreen pixels were verified; Windows/macOS drawing and hardware convergence were not exercised. These tests run in the full-suite/sanitizer lanes rather than the frozen per-PR test gate. CI status and merge-base freshness are recorded below.
All required PR checks (build, check-windows, check-macos, Static checks) passed, and GitHub verifies the head commit signature. The earliest head run was created at 2026-10-09T06:29:37Z, after current main 4e818e24f at 2026-10-09T06:07:47Z; main is still the reviewed merge base, so these checks are not stale. The filtered PR gate does not imply the full suite passed.
Recommendation: Approve with nits. The rendering fix survives the original-rule mutation and the demo checks. The foreground-only mutation identifies a non-blocking opportunity to strengthen the regression test; formal approval remains the maintainer’s decision.
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
1. Issue fit
Yes — and with an unusually well-grounded authority. There's no linked issue; this is a declared follow-up to the merged #6312, which is the right shape (GOVERNANCE.md does not want an RFC for a bug fix with a known root cause). I verified the precedent rather than taking the body's word for it: #6312's commit body (ba3052622) states that QTreeView::item:selected { background: accent; color: #000 } was "measured at 1.56:1 on a dark custom accent", that the rule "its three reviews blocked" was dropped, that the app-wide QListWidget::item:selected rule of "the same black-on-accent shape" was removed — and it names "Favorites picker" among the lists that kept their own copy. This PR deletes exactly that copy. That is a fix applying a merged maintainer ruling, not a preference (§6: clean — the authority is citable and I quoted it).
The "Principle VIII" citation checks out: CONSTITUTION.md:226 is "Evidence Over Assertion", and the body carries mutation output plus a bridge session.
2. Scope
Everything in the diff is explained by the stated fix. One row per group:
| File | Changes | Claimed? | Verdict |
|---|---|---|---|
src/gui/FavoritesPickerDialog.cpp:23-24 |
Deletes the ::item:selected rule, leaves an explanatory comment |
Yes — the entire point | In scope |
tests/favorites_picker_selection_test.cpp (new, 71 lines) |
Renders the real dialog, samples the selected row's pixel | Yes, body §Evidence | In scope |
tests/tests.cmake:2145-2161 |
Registers the new target | Yes | In scope |
No new public surface — no protocol verb, settings key, capability field or CLI flag. No CHANGELOG.md entry (correct; please keep it that way). The one - line is the defect itself, so there's no removed guard whose symptom can recur. No formatting churn.
3. Blockers
None.
4. Nits (all non-blocking)
-
src/gui/FavoritesPickerDialog.cpp:23-24— "the last remaining instance" is not accurate; please correct the body. The PR body says #6312 "flagged this as the last remaining instance of the pattern." #6312's body actually lists four: "Lists with their own selection rule (Radio Setup, Favorites picker, Profile Manager, Audio Device Change) are unchanged." This PR closes one of the four. Still carrying their own selection rule after merge:Location Rule Accent-dependent? src/gui/RadioSetupDialog.cpp:6771,:7161background: {{color.accent}}; color: {{color.background.0}}Yes — same exposure class src/gui/RadioSetupDialog.cpp:705background: {{color.accent.bright}}; color: {{color.background.0}}Yes src/gui/RadioSetupDialog_Peripherals.cpp:1704background: {{color.accent}}Yes src/gui/MidiMappingDialog.cpp:195background: {{color.accent}}; color: {{color.background.0}}Yes src/gui/ProfileManagerDialog.cpp:33background: #0070c0No (hard-coded, readable) src/gui/AudioDeviceChangeDialog.cpp:56background: #12384a; color: #ffffffNo (hard-coded, readable) And the exact deleted shape survives in the app-wide template #6312 itself edited, for a different widget:
src/gui/Theme.h:184(insideappStylesheetTemplate(), declared atTheme.h:47):QMenu::item:selected { background-color: {{color.accent}}; color: #000; }Nothing here asks you to widen this PR — incremental is the right call, and the title is honestly scoped to the Customize Button Bar. Just soften the body sentence, and a tracking issue for the residue (especially
Theme.h:184, which is app-wide and reachable on every menu) would be worth opening. -
Coverage disclosure for the maintainer:
favorites_picker_selection_testis in noci.yml -Rfilter (the only filtered ctest steps are at.github/workflows/ci.yml:819,:832,:1174,:1189). The eight green checks onf5149c3prove the new target compiles and links on Linux (build) and macOS (check-macos); nothing in this PR's CI ran its assertions. That's the project's standing gate design, not a defect in the PR — #6312 made the same disclosure fortheme_manager_test— but it means the mutation evidence in the body is the evidence standing behind those assertions. The body says "registered intests/tests.cmake", which is accurate and doesn't overclaim; worth stating the CI reality explicitly all the same. -
src/gui/FavoritesPickerDialog.cpp:25— "Hover is unchanged" is very slightly off. The hover background is unchanged. For a row that is both hovered and selected, the text colour was#000(supplied by the deleted rule) and is now the inherited/palette colour. In Default Dark that's an improvement, since#000on{{color.background.1}}was poor to begin with. Precision point, not a defect. -
CodeGuard: all four CRITICAL
CS-VLN-CMDI-AMPfindings are false positives — dropped, recorded here for the audit trail. The security-audit (CodeGuard) pass confirmed the flagged lines are C++ reference sigils, not shell metacharacters::216for (const auto& id : activeOrder) {,:224for (const auto& id : hiddenIds) {,:233auto allListed = [this](const QString& id) {,:234auto matches = [&id](QListWidget* w) {. The file contains noQProcess,system(,exec*,popen,QDesktopServicesorstartDetached— there is no command string to inject into. All four lines are also untouched by this PR (they shifted +1 from the comment). No genuine security issue found in the file; the only data reaching the dialog is the in-process applet registry fromsrc/gui/AppletPanel.cpp:2327-2334, used solely as item text,Qt::UserRoledata and tooltips.
5. What I tried to break
Findings below are reasoned from the code and from CI metadata; I ran no build and executed no test, so nothing here was reproduced at runtime.
- "Removing the rule gives you the palette highlight" — does it actually, given this template keeps a
::itemrule? This was my main line of attack, because the template here differs from #6312's precedent:appStylesheetTemplate()hasQListWidget { … }andQTreeView { … }but no::itemsubcontrol rule at all (Theme.h:85-104), whereasFavoritesPickerDialog.cpp:22keepsQListWidget::item { padding: 4px 6px; }. A::itemrule can makeQStyleSheetStyledraw the item box itself and lose the highlight entirely rather than fall through to the palette. Reading Qt'sPE_PanelItemViewItempath, a padding-only rule is not "drawable", so it delegates to the base style and the palette highlight is drawn — and the body's mutation output (#308cc6with the fix,#00b4d8with the rule restored) confirms it empirically. Survived, and credit to the author for adding a render test for this template rather than leaning on #6312's evidence. - Does the test discriminate, or assert the implementation back to itself? It discriminates on both sides. With the old rule restored, assertion 1 fails (pixel ==
color.accent). With a different wrong re-fix — saybackground: {{color.background.2}}; color: #000— assertion 1 passes but assertion 2 fails (pixel ≠ paletteHighlight). So it pins the intent, not the literal deletion. Survived. - Can the test go vacuous if the theme fails to load? This was my second-best attack: an unloaded theme makes
color.accentan invalidQColor, whosergb()is opaque black, degrading assertion 1 to "≠ black". But${THEME_TEST_RESOURCES}is compiled into the target (tests.cmake:2152, generated at:1969) andThemeManager's constructor callsseedBuiltinDefaults()(src/core/ThemeManager.cpp:483) which populatescolor.accentregardless of resources. The body's#00b4d8confirms a real accent. Survived. - Current-item focus tint at the sample point. The test does
setCurrentRow(1)and samples row 1, so the sampled row is both selected and current — and #6312's own test deliberately avoided row 0 "because row 0 is the current item and carries the style's focus tint." The mutation output shows the sampled pixel was exactly#00b4d8and exactly#308cc6, i.e. uncontaminated atrow.right() - 4under Linux offscreen, so the concern doesn't bite there. Windows/macOS styles could tint differently; the body already lists those platforms as not verified, which covers it. Survived, with that caveat named. - CMake contract and link hazards. The target links
aethercoreand does not compileAppSettings.cpp, so no${AETHER_SETTINGS_SOURCES}/AETHER_SETTINGS_CONSUMERSjoin is required (the obligation attests.cmake:68-83is explicitly only for targets that don't linkaethercore); it doesn't trip the_aether_shared_source_count EQUAL 3configure guard;TestSettingsProfileis constructed beforeQApplication, matching house style. I also checked the duplicate-symbol risk from compilingPersistentDialog.cpp/FramelessResizer.cppwhile the deferred retrofit linksaetherdesktop_support(which contains them viaGUI_SOURCES,CMakeLists.txt:1215,:1324,:1339) — there are 22 precedents for that exact combination intests.cmake, including the immediate neighbours, andbuild/check-macosare green. Survived. - Constructor/arity check on the new test.
FavoritesPickerDialog(entries, activeOrder, {}, 4)matches the real 5-arg signature with a defaultedparent(FavoritesPickerDialog.h:31-35), andfavoriteSplit = 4does put the divider past the last row, soFavoritesDividerDelegate::paintadds no 22px strip to row 1. ThefindChildren+count() == 4disambiguation picks the Active list unambiguously (Hidden has 0 items). Survived. - Governance sweep. No settings writes, no credentials, no capability declarations, no radio-authoritative state, no TX path — Constitution I/II/III/V/VI are not implicated. The dialog already rides
PersistentDialogperdocs/style/dialog-patterns.md; this PR doesn't touch that. No a11y surface changed (a stylesheet rule, no new interactive widgets). Clean.
Socket tests: none. tests/favorites_picker_selection_test.cpp uses only QApplication, QListWidget, QColor/QImage — no QTcpServer/QTcpSocket/QUdpSocket/QLocalServer, no bind/listen/connectToHost, no peer process, no Fake* radio or amplifier class. Nothing to record beyond that, and no socket test removed either.
6. Recommendation
Approve with nits. This is a small, well-targeted change that applies a ruling a merged PR already made and three prior reviews already blocked the opposite of, and the new test is the genuinely valuable part — it fails on the unfixed code and on the plausible wrong re-fix, and it covers a Qt stylesheet interaction (::item padding rule present, no :selected rule) that #6312's own evidence did not reach. No blockers, no governance hit, no regression path I could construct.
Next step for the author: fix the "last remaining instance" sentence in the body, since #6312 named four lists and the {{color.accent}}; color: #000 shape still lives at src/gui/Theme.h:184 — then, separately, open a tracking issue for the residue. Neither needs to hold this merge. Thanks for the mutation output and the bridge before/after; having both made this one straightforward to check.
🤖 aethersdr-agent · cost: $7.1172 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (4 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: $7.2337 · model: claude-opus-5 |
…lected row's text is the palette's highlighted text. Principle VIII. A selected-item rule that kept the palette background but forced dark text passed the background-only checks; the text check now fails on it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ark text on the accent. Principle VIII. #6312 ruled that selection comes from the palette, because dark text on color.accent is unreadable on a dark custom accent. Seven more rules still drew it: the app-wide QMenu rule (Theme.h), the CW history and VFO frequency context menus, Radio Setup's category tree, USB cable and bit lists, the Peripherals device list and the MIDI binding table. Each now uses palette(highlight) / palette(highlighted-text). An explicit rule, rather than none, keeps selection visible where the item rule draws its own background (Peripherals) or shape (the category tree). favorites_picker_selection_test reads the expected colours from the application palette rather than the list's own, which a dialog's selection-color rewrites, and scans src/ for any selected-item rule on a color.accent background. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ten9876
changed the title
|
Landing pass on Foreground coverage (review 5466713823). "Last remaining instance" / the residue (review 5466787715, nit 1). On the maintainer's call, the residue lands here rather than as follow-up work.
The test now scans "Hover is unchanged" (nit 3). The body now says the hovered row's background is unchanged. CI coverage disclosure (nit 2). The body states that the test runs in the full-suite and sanitizer lanes, not the per-PR filter. CodeGuard Static gates: per-file findings identical to the merge base. |
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
1 participant
Summary
#6312 ruled that a selected row draws the palette highlight, because dark text on
color.accentis unreadable on a dark custom accent. It removed that rule from the app-wide lists and trees. Eight places still drew a selected item on the accent, and they now draw from the palette:FavoritesPickerDialog.cpp)color.accent(rule removed)Theme.h,QMenu::item:selected)color.accentCwxPanel.cpp)background.spectrumoncolor.accentVfoWidget.cpp)background.0oncolor.accentRadioSetupDialog.cpp)background.0oncolor.accent.brightRadioSetupDialog.cpp)background.0oncolor.accentRadioSetupDialog_Peripherals.cpp)background.0oncolor.accentMidiMappingDialog.cpp)background.0oncolor.accentThe Customize Button Bar's rule is deleted outright, the same as #6312. The others become
palette(highlight)/palette(highlighted-text). An explicit rule is needed there because a list whose::itemrule draws its own background (Peripherals) or shape (the rounded category tree) would otherwise draw no selection at all.Lists with a fixed, readable selection colour that does not depend on the accent (Profile Manager, Audio Device Change, Memories, D-STAR traffic) and the style-guide
canon.cyan/canon.onAccentpair are unchanged.Visible change: in Default Dark, a selected item in these places goes from cyan with dark text to the palette highlight with white text, the same as the connection panel's radio list after #6312. A hovered row's background is unchanged.
Evidence
favorites_picker_selection_test(socket-free, offscreen; registered intests/tests.cmake).selection-colorrewrites that.src/for any selected-item rule whose background is acolor.accenttoken.favorites_picker_selection_test: all checks passed.FAIL: selected row is drawn in color.accent (#00b4d8),FAIL: selected row #00b4d8 is not the palette highlight #308cc6,FAIL: selected row text #171d1e is not the palette highlighted text #ffffff.selection-color: #000:FAIL: selected row text #050f15 is not the palette highlighted text #ffffff.selection-background-color: {{color.background.2}}:FAIL: selected row #304050 is not the palette highlight #308cc6.FAIL: selection drawn on the accent: …lines, one per rule (the cable and bit lists are two).::item:selected { color: #000; }renders unchanged (white on the palette highlight, checked by grabbing the row), so the test correctly lets it pass.#308cc6(the palette highlight); unselected rows keep#304050/#1a2a3a.DEMO-0001). Radio Setup opened withinvoke "Radio Setup..." trigger. The category tree's selected row is the palette blue with white text and keeps its rounded corners.theme_manager_testpasses.audit_colours --strictagainst the merge base: total references −1, setStyleSheet unchanged.Not verified
palette()roles as the lists, and the source scan pins them.favorites_picker_selection_testruns in the full-suite and sanitizer lanes, not the per-PR CI test filter.🤖 Generated with Claude Code