Repository navigation
fix(theme): tree-view rows use the theme's backgrounds, so the Theme Editor's token list is readable (#5934). Principle VIII. - #6312
Conversation
…Editor's token list is readable (aethersdr#5934). Principle VIII. The app stylesheet gave QListWidget a background and an alternate-background-color but had no rule for tree views. Qt only takes AlternateBase from a stylesheet when alternate-background-color is set, so a QTreeView with alternating rows kept the native AlternateBase (near-white on Windows) under the theme's light color.text.primary. Every other row of the Theme Editor's Object list was unreadable, which is why font.family.freq looked missing. - QTreeView (and so QTreeWidget) now gets background.0, background.1 for alternate rows, text.primary and border.strong. Headers inside tree views get background.1 and text.primary. The header rule is scoped to tree views, so table headers elsewhere are unchanged. - Selected rows are left to the palette. The rule proposed in aethersdr#5938 (accent background with black text) was unreadable with a dark custom accent (1.56:1) and added a hard-coded colour literal. - The Theme Editor's "inherited" cells use color.text.secondary instead of a fixed slate. They're repainted when a token is edited in place, so editing color.text.secondary itself updates them without rebuilding the list. theme_manager_test renders an alternating QTreeWidget under the app stylesheet and checks rows 0 and 1 against background.0 and background.1, and checks that no tree-view selection rule is back. Builds on the analysis and review of aethersdr#5938. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cus tint (aethersdr#5934). Principle VIII. Row 0 of a freshly shown QTreeWidget is its current item, which the style tints (#131e2f against background.0 #0f0f1a in the default dark theme), so comparing it with background.0 failed on the fixed stylesheet. Rows 2 and 3 keep the same base/alternate pairing without that state. Run locally against Qt 6.12: passes; removing the tree view's alternate-background-color fails it; restoring the black-on-accent selection rule fails it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
1. Issue fit
Yes. #5934 opens as "font.family.freq is missing from the Theme Editor", but the thread redefines it: ve7atj's screenshot shows the token is present and the alternating rows are light-grey text on near-white. The maintainer-reviewed triage pinned the root cause to appStylesheetTemplate() styling QListWidget but no tree view, so QTreeWidget with setAlternatingRowColors(true) fell back to the native QPalette::AlternateBase. Both hunks map onto that:
| Requirement (from the thread) | Hunk |
|---|---|
| Alternate rows must use the theme's background | src/gui/Theme.h:95-100 — QTreeView { background-color; alternate-background-color; color; border } |
The hardcoded slate #607080 on "inherited" cells ignores the theme |
src/gui/ThemeEditorDialog.cpp:114-124, :492-496 — color.text.secondary |
| Fix should reach every themed tree, not just this dialog | type selector QTreeView matches QTreeWidget and subclasses |
I verified the tokens actually resolve rather than trusting the grep (per the nested-token trap): color.text.secondary is seeded at src/core/ThemeSeedGenerated.cpp:177 (#8ea8c0) and present in both bundled themes (scopes.root.tokens.color.text.secondary → {color.gray.400} dark, {color.gray.100} light), so the new brush resolves in Light as well as Dark.
Relationship to #5938 is clean. #5938 is still open on the same issue and the same two files; the author has asked for it to be closed as superseded and this body says so up front. This is the "invited successor" shape, not a silent duplicate — but #5938 needs a maintainer to close it, and the two will conflict on Theme.h if both land.
The three deltas from #5938 are all responses to its review feedback, not new preferences:
- dropping
QTreeView::item:selected { … color: #000 }(reviewers measured 1.56:1 on a dark custom accent), - scoping the header rule to
QTreeView QHeaderView::section, - repainting the "inherited" cells on an in-place edit.
That last one is the interesting part and it's done the safe way: the existing comment at ThemeEditorDialog.cpp:1127-1136 warns that rebuilding the list mid-edit SEGVs in QPixmapIconEngine because onTokenRowClicked still holds a row pointer. The new path only calls setForeground() — nothing destructive — so the warned-about hazard doesn't apply.
2. Scope
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/gui/Theme.h:91-106 |
New QTreeView block + QTreeView QHeaderView::section |
Yes — both in the body | In scope. The header rule is a global restyle the title doesn't mention, but it's a fix, not a preference: a native-painted header section under color.text.primary has the same contrast failure as the rows. Disclosed in the body. |
src/gui/ThemeEditorDialog.cpp:114-124 |
inheritedCellText() / inheritedCellBrush() helpers |
Yes | In scope — extraction needed by the repaint loop. |
src/gui/ThemeEditorDialog.cpp:492-496 |
De-hardcode the "inherited" foreground | Yes | In scope; exactly the triage's item 2. |
src/gui/ThemeEditorDialog.cpp:1140-1160 |
Repaint "inherited" cells on in-place edit + early return |
Yes | In scope. The return is a guard for the rebuild branch, not a removed code path — nothing previously ran after refreshTokenList(). |
tests/theme_manager_test.cpp:1661-1691 |
New pixel-sampling block | Yes | In scope. |
Nothing in the diff is unexplained by the issue thread. No new public surface, no settings key, no protocol verb, no CHANGELOG.md entry (correct — it must not have one). No removed guard: the only - lines are the three replaced statements in populateRow().
No socket test is added or modified. The new block constructs a QTreeWidget and grabs its viewport; there is no QTcpServer/QUdpSocket/bind()/listen()/connectToHost() and no fake peer anywhere in tests/theme_manager_test.cpp.
3. Blockers
None.
4. Nits (all non-blocking)
-
src/gui/ThemeEditorDialog.cpp:1154— the repaint keys off display text, so a real override that reads "inherited" gets recoloured.if ((*rows)->text(col) == inheritedCellText()) {
For
font.family.*columnspopulateRow()writestm.valueAt(scopePath, key)verbatim (:487), so a font family literally namedinheritedwould be painted as a placeholder. Vanishingly unlikely in practice — but the row already knows the answer, so a role flag is both cheaper and exact.Qt::UserRoleis free on columns ≥ 1 (only column 0 carries the token key,:431). In theelsebranch ofpopulateRow():} else { item->setText(col, inheritedCellText()); item->setFont(col, inheritedFont); item->setForeground(col, inheritedCellBrush()); item->setData(col, Qt::UserRole, true); }
the matching
setData(col, Qt::UserRole, {})in the overridden branch next to the existingsetForeground(col, QBrush()), and then:if ((*rows)->data(col, Qt::UserRole).toBool()) { (*rows)->setForeground(col, inherited); }
-
tests/theme_manager_test.cpp:1689— this assertion is a string-absence guard, not a behaviour test.EXPECT_TRUE(!appStylesheetTemplate().contains(QStringLiteral("QTreeView::item:selected")));
It pins the #5938 review outcome, which is a reasonable thing to want, but it tests the implementation back to itself and it's narrow:
QTreeWidget::item:selectedorQTreeView::item:selected:!activewould both reintroduce the black-on-accent text and pass. Worth a comment saying it's a deliberate regression latch for the #5938 blocker rather than a contrast check — contrast can't be measured honestly offscreen, so I'm not asking for that test. -
tests/theme_manager_test.cpp:1669— the header rule ships unexercised.tree->setHeaderHidden(true)meansQTreeView QHeaderView::section(Theme.h:101-106) is never painted by the test, while the production Theme Editor shows its header (rebuildColumns()sets per-section resize modes atThemeEditorDialog.cpp:398-406). DroppingsetHeaderHidden(true)and sampling a header pixel would cover it; low value, your call. -
src/gui/Theme.h:95— one blast-radius surface is missing from the body's list. The body enumeratesRadioSetupDialog,SettingsBrowserDialogandNetworkDiagnosticsDialog. It omitsImageFileDialog:getBackgroundImagePath()builds aQFileDialogwithDontUseNativeDialogandViewMode::Detail(ImageFileDialog.cpp:263-273), parented viaScopedChildWidget<QFileDialog> dialogOwner(parent, …), and reaches into its internaltreeView(:280). That tree is aQTreeViewunder a themed ancestor, so the image picker's detail view newly getsbackground.0,text.primaryand a new1px solid border.strong. Probably an improvement — a dark-themed picker beats a white one — but it's an undisclosed user-visible change; worth a line in the body. -
The body overstates the
NetworkDiagnosticsDialogeffect, in the safe direction. It says the nav tree "newly gets the themed background and alternate rows". Its own ID-selector ruleQTreeWidget#networkDiagnosticsNavigationalready setscolor,background,border,border-radius,paddingandoutline(NetworkDiagnosticsDialog.cpp:110-117), and an ID selector outranks a type selector, so none of those change. And the nav never callssetAlternatingRowColors()— the only call in that file ism_audioStreamsTableat:935, aQTableWidget, which isn't aQTreeViewsubclass and so isn't touched at all. The new rule is effectively inert there. Smaller blast radius than claimed, but the body should match. -
src/gui/Theme.h:90— the sibling keeps the pattern this PR deliberately dropped.QListWidget::item:selected { background-color: {{color.accent}}; color: #000; }This is exactly the
accent+ hardcoded#000combination #5938's reviewers measured at 1.56:1 on a dark custom accent. Trees now avoid it; lists still carry it, and the two surfaces now render selection differently. Pre-existing and genuinely out of scope here — but the PR reads as "selection is left to the palette" when that's true of only half the item views. Worth its own issue. -
src/gui/ThemeEditorDialog.cpp:493— one interaction I could not settle headless. The overridden branch resets the cell withitem->setForeground(col, QBrush()). A default-constructedQBrushisQt::NoBrushwith colour black, and whetherQStyledItemDelegate::initStyleOption()treats that stored-but-default brush as "no override" or as "paint black" decides whether those cells staycolor.text.primaryor go black. It never mattered before because the fallback background was white; now it'sbackground.0. Narrow:rebuildColumns()emits onlyObject | Valuewhen no scope is selected (:375-391), solastCol == 1and the whole intermediate-column loop is skipped — these cells exist only with a container scope picked. Qt sources aren't on this box, so I'm flagging it as a thing to eyeball in the Theme Editor with a scope selected, not as a confirmed defect. The line is pre-existing and unchanged by this PR.
5. Security audit
I ran the required security-audit subagent over the three files. All 13 harness findings are false positives, and I refuted each against the code:
CS-VLN-CMDI-BACKTICK×10 (Theme.h:195,244,280;ThemeEditorDialog.cpp:317,553,614;theme_manager_test.cpp:506,543,1019,1233) — every hit is a markdown backtick inside a//comment, e.g.Theme.h:195"Use this instead ofwidget->setStyleSheet(...)". There is no shell sink of any kind in these three files: nosystem,popen,QProcessorexec*.CG-PATH-001×3 (theme_manager_test.cpp:1551,1557,1566) — these are negative assertions proving the traversal guard works, e.g.EXPECT_TRUE(!tm.saveCurrentThemeAs(QStringLiteral("../escape")));. The../is the test input being rejected.
Two genuine items came back, both credited to security-audit (CodeGuard) and both in unchanged code — not introduced, worsened or reachable-differently because of this PR, so neither is a finding against it. Flagging so they can be routed separately rather than lost:
- Theme token values are spliced into the Qt stylesheet unvalidated.
flattenTokens()stores any JSON string verbatim (ThemeManager.cpp:163-164) andcolorHexToCssFragment()returns an invalid colour string unchanged (:97-100), so an imported.aetherthemewith"color": {"background": {"0": "red; } * { border-image: url(...)"}}injects arbitrary QSS app-wide. Bounded — QSS carries no script andurl()resolves throughQFile/qrc, not the network — so the realistic worst case is UI spoofing (e.g. making a TX-armed indicator look idle). This PR adds consumers of that template but the sink is insrc/core/ThemeManager.cpp, which isn't in the diff. ThemeEditorDialog.cpp:689-700—dragEnterEvent()checks each URL's suffix isaethertheme/json(:679-684), butdropEvent()re-reads the URLs and callsimportThemeFromPath()with no suffix re-check. Low severity (importThemeFromFilestill requires valid theme JSON), and also outside this diff.
6. What I tried to break
- "The test would pass against the unfixed code." It wouldn't, and I traced why rather than assuming. Assertion 1 (
base == background.0) passes either way — the author says so himself, and his own mutation run confirms removing the tree rule'sbackground-colorstill passes becauseQWidget { background-color }already covers it. Assertion 2 is the load-bearing one: Qt only populatesQPalette::AlternateBasefrom a stylesheet whenalternate-background-coloris set, so without that line row 3 falls to the platform palette's near-whiteAlternateBase≠background.1. The reporter's screenshot is the evidence that role isn't derived from the QSS background. Reasoned from code — I did not run it. - "Is it tautological, comparing the stylesheet to itself?" No. Both sides read the same
ThemeManager, which makes it robust rather than circular: if theme resources were missing the painted pixel andtm.color()would move together. And resources aren't missing —theme_manager_testlinks${THEME_TEST_RESOURCES}(tests/tests.cmake:2068) and runs withQT_QPA_PLATFORM=offscreen(:2073-2074), so this isn't the font/theme-resource flake class that has bitten pixel tests on the full-suite lane. - The repaint loop's column range and reentrancy.
for (int col = 1; col < lastCol; ++col)mirrorspopulateRow()'s range exactly (:467), and the last column ("Value") never shows the placeholder — it always carries a resolved value — so skipping it is right, not an off-by-one. With no scope selected there are 2 columns and the loop body never executes. It only callssetForeground(), so the dangling-row SEGV the:1127comment warns about can't be triggered from it. - Did the new rule break a sibling tree? I checked all five
QTreeView/QTreeWidgetusers insrc/gui/.SettingsBrowserDialogsets a dialog-levelQTreeWidgetrule includingborder(:58-62) andNetworkDiagnosticsDialoguses a higher-specificity ID selector (:110-117) — both win over the new type rule.RadioSetupDialog's tree likewise has closer rules. The one surface that genuinely changes and isn't in the body isImageFileDialog'sQFileDialogdetail view (nit above). - CI is green but did not run this test. All five checks pass on
c69963c, and.github/workflows/ci.yml:3-18states the test gate is frozen: the Linuxbuildjob runs no tests, and the onlyctest -Rsteps are the macOS/Windows platform-pinned ones in.github/ci-test-gate.txt.theme_manager_testappears nowhere inci.yml. So the new pixel assertions first execute post-merge onfull-suite.yml, and again weekly under the sanitizers. Per that same canon comment ("a new test needs no step here — do not add one"), this is correct and I'm not asking for a CI step — just naming that green here is not evidence this test passed. - Could not check: anything runtime. No build, no GUI, headless throughout. Specifically unverified by me: the Windows light-palette before/after screenshots, the four mutation runs, and the
QBrush()delegate question in the last nit. Every finding above is reasoned from code in the head checkout at/tmp/aetherclaude/pr-6312.
7. Recommendation
Approve with nits. The fix matches the root cause the issue thread established, the tokens resolve in both bundled themes, the new test discriminates against the unfixed code for the right reason, and the one global side effect (the scoped header rule) is a contrast fix rather than a smuggled preference. The three deltas from #5938 are documented answers to that PR's review blockers, which is the right way to supersede. Nothing here warrants withholding a merge.
Two next steps, both for a maintainer rather than the author: close #5938 (the author has asked; the two touch the same Theme.h hunk and will conflict if both land), and decide whether the QListWidget::item:selected black-on-accent sibling gets its own issue. The author may want to fix the NetworkDiagnosticsDialog sentence and add ImageFileDialog to the blast-radius list; neither needs a new review round.
Nice iteration on the test, by the way — the second commit moving the sample off row 0 because the current item carries the style's focus tint is the kind of thing that usually surfaces as a CI flake three weeks later.
🤖 aethersdr-agent · cost: $15.2281 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (13 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.4083 · model: claude-opus-5 |
|
Antares Detector — candidate vulnerable file(s)
The gradient parser in ThemeManager::parseGradient reads arbitrary JSON scalar values (type, angle, center components, radius, stops[].color) from theme JSON without validating their types or values, effectively deserializing untrusted theme data. This is a dangerous external-input deserialization pattern (CWE-502/unsafe deserialization) and can lead to code execution if theme JSON is malicious. Localized by Cisco Foundation AI Antares-1B running locally in the AetherClaude sandbox, seeded by the Cartographer security map. Advisory only — please verify before acting. 🤖 aethersdr-agent · cost: $15.4083 · model: claude-opus-5 |
ten9876
left a comment
There was a problem hiding this comment.
Issue fit
Yes. #5934's thread redefined "font.family.freq is missing" as "every other row in the Theme Editor's Object list is light text on the native near-white AlternateBase". The root cause in the body holds: before this PR appStylesheetTemplate() had alternate-background-color for QListWidget only, and the Object list is a QTreeWidget with setAlternatingRowColors(true) (src/gui/ThemeEditorDialog.cpp:261). The new QTreeView rule (src/gui/Theme.h:95-100) fixes it. The PR also drops the two things that blocked #5938: the hard-coded black-on-accent selection rule, and the unscoped QHeaderView::section.
I reproduced the bug on Linux, not only on Windows: the merge base (888ac58bf, offscreen, Default Dark) paints rows 2, 4 and 6 near-white with unreadable text. The PR head paints them background.1, and they are readable. Details are below.
Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/gui/Theme.h:91-100: QTreeView rule |
Tree backgrounds, alternate rows, text and border, everywhere applyAppTheme() reaches |
Yes | The fix |
src/gui/Theme.h:101-106: QTreeView QHeaderView::section |
Tree-view headers get background.1 and text.primary, with border: none |
Yes (scoped to trees, as disclosed) | In scope; see nit 1 |
src/gui/ThemeEditorDialog.cpp:114-124, 495-497: "inherited" cells |
#607080 becomes color.text.secondary |
Yes | In scope. It removes a hard-coded colour, which docs/style/theme-style-guide.md asks for |
src/gui/ThemeEditorDialog.cpp:1143-1159: in-place repaint |
Recolours inherited cells on a same-theme themeChanged, without rebuilding the list |
Yes | In scope; see nit 4 |
tests/theme_manager_test.cpp:1661-1691 |
Pixel check of alternate rows, plus a check that the selection rule stays out | Yes | Regression coverage, socket-free |
Everything in the diff is explained by the issue and the #5938 reviews. There are two commits, both authored 2026-10-08 and both about #5934, with no date outliers. The PR adds no settings keys, public surface, CHANGELOG entry, CI edits or dependencies, and no guards are removed (the - side is two lines, both replaced). The body's "+82 / −2 in 3 files" is accurate.
Socket preflight: the only test change is theme_manager_test, which builds a QTreeWidget and grabs pixels. It has no sockets and no fake peers.
Blockers
None.
Nits (non-blocking)
src/gui/Theme.h:104:border: noneremoves the column dividers from tree headers. I grabbed the Theme Editor on the merge base and on the head, same offscreen demo session and Default Dark. On the base, the native header draws a separator between Object and Value. On the head there is none, and the header background (background.1) is the same colour as the alternate rows directly beneath it. With a scope selected the list grows extra stretch columns, so the column boundaries and resize handles become harder to find. A drop-in that keeps the theming and restores the separators:QTreeView QHeaderView::section { background-color: {{color.background.1}}; color: {{color.text.primary}}; border: none; border-right: 1px solid {{color.border.strong}}; border-bottom: 1px solid {{color.border.strong}}; padding: 3px 6px; }
- Sibling left with the same defect:
src/gui/RadioHealthDialog.cpp:57-60. ThatQTableWidgetusessetAlternatingRowColors(true), and its own stylesheet setsbackgroundandcolor: {{color.text.primary}}but noalternate-background-color. It therefore keeps the nativeAlternateBase, which is the #5934 mechanism in another dialog. I checked everysetAlternatingRowColors(true)insrc/. The others either setalternate-background-coloror::item:alternatein their own sheet (AudioDeviceChange, Ax25, DxCluster, RadioSetup Peripherals, Memory, NetworkDiagnostics), or are aQListWidgetalready covered by the app rule (WfmDevice). RadioHealth is the only one left. This is fine as a follow-up, but the PR doesn't read as fixing the defect class. - The body says
NetworkDiagnosticsDialog's navigation tree "newly gets the themed background and alternate rows". That tree has its own, closerQTreeWidget#networkDiagnosticsNavigation { background: … }rule (src/gui/NetworkDiagnosticsDialog.cpp:110-117), and it doesn't callsetAlternatingRowColors. So the new background and alternate colour don't appear there in practice. The PR does reach a surface the body doesn't list:ImageFileDialog's non-nativeQFileDialogin Detail mode (src/gui/ImageFileDialog.cpp:272-280), whosetreeViewand header now take the tree and header rules. It's probably an improvement, but it's worth listing in the blast-radius paragraph. src/gui/ThemeEditorDialog.cpp:1153-1159: inherited cells are found by their display text. A real override whose value is the literal stringinherited(afont.family.*value can be any string) would be recoloured as inherited on the next in-place edit. Separately, the loop runssetForegroundon every inherited cell for every same-themethemeChanged, including each tick of a live colour edit, even whencolor.text.secondarydidn't change. Both are cheap to fix: tag the cell inpopulateRow(for exampleitem->setData(col, Qt::UserRole + 1, true)) and test the tag, and skip the loop when the brush colour equals the last one applied.- Test reach (
tests/theme_manager_test.cpp:1686-1690). The selection guard is a substring check, so it would missQTreeWidget::item:selectedorQTreeView::item:selected:active. Nothing covers the new in-place repaint path (editcolor.text.secondary, then read an inherited cell's foreground), and the header rule isn't asserted either. None of this blocks, because the load-bearing assertion (the alternate row) does pin the fix. See below. - Housekeeping: #5938 is still open. Once this lands it should be closed as superseded.
What was verified empirically vs. read
- Built PR head
c69963c96and its merge base888ac58bf(lean RelWithDebInfo, shared core, Qt 6.12, Linux), targetsAetherSDRandtheme_manager_test. theme_manager_testpasses on the head.- Mutation: with
alternate-background-colordeleted from theQTreeViewrule, the test fails attheme_manager_test.cpp:1687(the alternate-row check). The regression test pins the fix. Restored afterwards and confirmed bygit diff. - Bridge, demo only: two offscreen instances with isolated
AETHER_SETTINGS_DIR,AETHER_AUTOMATION_NO_TX=1and explicit sockets, each connected toDEMO-0001(get radio→"model":"AetherSDR Demo", familysim).invoke "Theme Editor" trigger, thengrab ThemeEditorDialog.- Merge base: alternate rows near-white, text illegible. #5934 reproduced on Linux.
- PR head: all rows readable, alternate rows
background.1, tree border drawn, header themed, no column divider (nit 1).
- What I tried to break that held up:
- Selection: no stylesheet selection rule remains, so
Highlight/HighlightedTextcome from the palette as a pair. - Item foreground overrides: inherited cells still set
ForegroundRole, which beats the view'scolor. - Dialogs with their own tree rules (RadioSetup, SettingsBrowser, NetworkDiagnostics) keep them, because a widget's own sheet beats the inherited app sheet.
- The in-place repaint path only mutates foregrounds and never deletes items, so it does not reintroduce the dangling-row SEGV that the
onActiveThemeChangedcomment warns about. - Merge:
git merge-treeagainst currentmain(4f1691959) is clean, and none of the three files changed onmainsince the merge base.
- Selection: no stylesheet selection rule remains, so
- Not verified:
- The scope-column "inherited" repaint after editing
color.text.secondary, reasoned from code only. The scope combo has no object or accessible name, so the bridge could not select a container. - Windows native styles (hover tints), the Default Light theme, and macOS.
- The scope-column "inherited" repaint after editing
- CI: green, created 04:12Z on a merge base one commit behind current
main(#6311, spots, which touches none of these files).theme_manager_testisn't in the per-PRci.ymlfilter, so the local run above is the evidence for it. - Automated pass:
/code-reviewat medium raised the header-divider,RadioHealthDialog, text-match, per-edit-loop and test-reach points; I verified those, and they appear above. I dropped its hover/selection-contrast claim as not a regression: the merge base had no hover or selection rule either, and the PR leaves both to the palette. I also dropped its root-scopecolor.text.secondarypoint as speculative.
…lternates on the theme, inherited cells are marked by role (aethersdr#5934). Principle VIII. The tree-scoped header rule drew no section borders, so the header blended into the color.background.1 alternate rows beneath it; right and bottom borders in color.border.strong restore the separator. RadioHealthDialog's table alternated rows without an alternate-background-color and showed the native AlternateBase under the theme's text — the same defect as aethersdr#5934. The Theme Editor now marks "inherited" scope cells with an item data role instead of matching their text, so an override whose value reads "inherited" is never recoloured, and the in-place repaint runs only when color.text.secondary actually moved. theme_manager_test pins the header separators (fails with either border removed) and widens the selection latch to any QTreeView ::item...:selected rule. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…-checks the file type (aethersdr#5934). Principle VIII. The app stylesheet painted a selected QListWidget row as black text on color.accent, which is unreadable on a dark custom accent; trees already leave selection to the palette, and lists now do the same. ThemeEditorDialog::dropEvent imported every local URL in the drop without the .aethertheme / .json check dragEnterEvent applies, and a drop is not guaranteed to follow an accepted dragEnter. Both now go through ThemeManager::isImportableThemeFile(). theme_manager_test's selection latch covers list and tree selectors, and the predicate is pinned for accepted, rejected and non-local paths. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Answers to the aethersdr-agent review and the CodeGuard scan, at head Nits
Security pass
|
ten9876
left a comment
There was a problem hiding this comment.
Approving at 359cd18d6 after a maintainer remediation pass.
Fixed
- Header separators lost (
Theme.h:101-108) → right/bottomcolor.border.strongon the tree-scoped header section, per maintainer decision —08bd1804b;theme_manager_testfails with either border removed (:1719,:1720), demo grab shows the Object | Value separator. - RadioHealthDialog alternates rows on the native AlternateBase (
RadioHealthDialog.cpp:61) →alternate-background-color: color.background.1, folded in per maintainer decision —08bd1804b; reasoned (the demo reports no health registers), same mechanism the tree test pins. - Inherited cells found by text / repaint on every edit (
ThemeEditorDialog.cpp) → item data role + early return unlesscolor.text.secondarymoved —08bd1804b; demo withapplet/acomscope shows inherited cells italictext.secondaryand overrides in primary text. - Black-on-accent list selection (
Theme.h:90) → rule deleted, lists use the palette like trees, per maintainer decision —359cd18d6; guard fails at:1692with the rule restored; demo radio list shows white on the palette highlight in Default Dark. dropEventskipped the file-type check →ThemeManager::isImportableThemeFile()on drag-enter and drop —359cd18d6; pinned at:1699-1705, fails when forced totrue.- Stylesheet token injection (unchanged
ThemeManager.cpp) → filed #6320 per maintainer decision.
Verified: theme_manager_test + theme_seed_test pass on Linux / Qt 6.12; engine-boundary findings identical to the PR's original head; manifest, capability, command-plane and QSettings gates clean; all four required checks green on this head; all commits verified.
Not verified: the in-place inherited-cell repaint (no UI path leaves scope columns on screen while the root text.secondary changes in place), Windows/macOS rendering of the palette selection.
…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>
… the accent — Customize Button Bar, menus, Radio Setup, MIDI. Principle VIII. (#6322) ## Summary #6312 ruled that a selected row draws the palette highlight, because dark text on `color.accent` is 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: | Where | Was | |---|---| | Customize Button Bar lists (`FavoritesPickerDialog.cpp`) | black on `color.accent` (rule removed) | | Every menu that uses the app stylesheet (`Theme.h`, `QMenu::item:selected`) | black on `color.accent` | | CW history context menu (`CwxPanel.cpp`) | `background.spectrum` on `color.accent` | | VFO frequency context menu (`VfoWidget.cpp`) | `background.0` on `color.accent` | | Radio Setup category tree (`RadioSetupDialog.cpp`) | `background.0` on `color.accent.bright` | | Radio Setup USB cable and bit lists (`RadioSetupDialog.cpp`) | `background.0` on `color.accent` | | Radio Setup Peripherals device list (`RadioSetupDialog_Peripherals.cpp`) | `background.0` on `color.accent` | | MIDI Mapping binding table (`MidiMappingDialog.cpp`) | `background.0` on `color.accent` | The 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 `::item` rule 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.onAccent` pair 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 in `tests/tests.cmake`). - **Render check:** it builds the real Customize Button Bar dialog, selects row 1 and samples it against the **application** palette. It does not use the list's own palette, because a dialog stylesheet's `selection-color` rewrites that. - **Source scan:** it also scans `src/` for any selected-item rule whose background is a `color.accent` token. - With this PR: `favorites_picker_selection_test: all checks passed`. - Mutations, each red: - Old dialog rule restored: `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`. - The pre-PR sources of the other seven rows restored: eight `FAIL: selection drawn on the accent: …` lines, one per rule (the cable and bit lists are two). - A rule of only `::item:selected { color: #000; }` renders unchanged (white on the palette highlight, checked by grabbing the row), so the test correctly lets it pass. - **Peripherals list:** an offscreen render of its exact stylesheet, with alternating rows. A selected normal row and a selected alternate row both draw `#308cc6` (the palette highlight); unselected rows keep `#304050` / `#1a2a3a`. - **Demo-simulator bridge session** (offscreen, isolated settings, private network namespace, NO_TX, `DEMO-0001`). Radio Setup opened with `invoke "Radio Setup..." trigger`. The category tree's selected row is the palette blue with white text and keeps its rounded corners. - `theme_manager_test` passes. - Static gates (engine boundary, touchpoint manifest, capability records, command plane): per-file findings identical to the merge base. `audit_colours --strict` against the merge base: total references −1, setStyleSheet unchanged. ## Not verified - Menu selection was not rendered: the bridge cannot hover a menu item. The three menu rules use the same QSS `palette()` roles as the lists, and the source scan pins them. - Windows and macOS drawing of the palette highlight; that is the same path #6312 already uses for lists. - `favorites_picker_selection_test` runs in the full-suite and sanitizer lanes, not the per-PR CI test filter. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Fixes #5934. Supersedes #5938: it keeps that PR's fix and drops the rule its three reviews blocked.
What
The Theme Editor's Object list showed every other row as light grey text on a near-white background, so tokens such as
font.family.freqlooked missing (screenshot in the issue thread). The token was always there.Root cause
appStylesheetTemplate()(src/gui/Theme.h) givesQListWidgeta background and analternate-background-color, but had no rule for tree views. Qt only takesQPalette::AlternateBasefrom a stylesheet whenalternate-background-coloris set, so aQTreeViewwith alternating rows kept the native AlternateBase. Under a light Windows palette that colour is near-white, while the text is the dark theme's lightcolor.text.primary. The Object list is aQTreeWidgetwithsetAlternatingRowColors(true).It only shows when the OS palette is light. On Windows in dark mode the native AlternateBase is already dark, which is why it wasn't reproducible here until Qt was forced to the light palette.
How
Theme.h:QTreeView(which coversQTreeWidget) getsbackground.0,background.1for alternate rows,text.primaryandborder.strong. Headers inside tree views (QTreeView QHeaderView::section) getbackground.1,text.primary, and right/bottomborder.strongseparators so the header stays distinct from thebackground.1alternate rows under it.QTreeView::item:selected { background: accent; color: #000 }, which reviewers measured at 1.56:1 on a dark custom accent; it also added a hard-coded colour. Not included. The template's existingQListWidget::item:selectedrule had the same black-on-accent shape and is removed, so a selected row in any list that relies on the app stylesheet now draws the palette highlight (white on blue in Default Dark) instead of black on the accent. Lists with their own selection rule (Radio Setup, Favorites picker, Profile Manager, Audio Device Change) are unchanged.QHeaderView::sectionreached five other dialogs.ThemeEditorDialog.cpp: "inherited" cells usecolor.text.secondaryinstead of the fixed slate#607080. Theme the tree-view rows so Theme Editor tokens stay readable (#5934). Principle VIII. #5938's review measured that change at 5.93:1 in the dark theme and 5.08:1 in the light one. On an in-place token edit the dialog repaints those cells without rebuilding the list, so editingcolor.text.secondaryitself updates them. A rebuild there is what the existing comment warns crashes; this path repaints only, finds the cells by an item data role rather than by their text, and runs only whencolor.text.secondaryactually changed.RadioHealthDialog.cpp: the register table alternated rows without analternate-background-color— the same defect — and now usesbackground.1.ThemeEditorDialog::dropEventimported every local URL without the.aethertheme/.jsoncheckdragEnterEventapplies. Both now useThemeManager::isImportableThemeFile(). The related stylesheet-injection finding from the security pass is tracked separately in Theme token values are spliced into the app stylesheet unvalidated (QSS injection from an imported theme) #6320.Blast radius of the tree rule: it applies wherever
applyAppTheme()reaches.RadioSetupDialogandSettingsBrowserDialoghave their own, closerQTreeWidgetrules and are unchanged.NetworkDiagnosticsDialog's navigation tree has its own ID-selector rule and doesn't alternate rows, so it is effectively unchanged.ImageFileDialog's non-native file dialog (background-image picker) in Detail view is aQTreeViewunder a themed ancestor, so it newly gets the themed background, text and border.Testing
Proof with the agent automation bridge
Runs used the fork's Windows Installer build of this branch (Qt 6.12, run 37878449329) and the same-day
main-based build as "before". Both ran on Windows 10 22H2, in isolated settings profiles (AETHER_SETTINGS_DIR, no radio, Default Dark theme), with Qt forced to the light system palette (QT_QPA_PLATFORM=windows:darkmode=0), which is the reporter's condition. Steps:invoke "Theme Editor" trigger,grab_widget QTreeWidget, thenclickAta row and grab again.background.1Unit test (local, Qt 6.12.0 pinned, MSVC 2022, offscreen)
theme_manager_testrenders aQTreeWidgetwith alternating rows under the app stylesheet. It checks that rows 2 and 3 are exactlybackground.0andbackground.1, and that no list or tree::item…:selectedrule is in the template. A second block renders a two-column header and checks the right and bottom separators areborder.strong, and a third pinsisImportableThemeFile()for accepted, rejected and non-local paths. (Rows 0 and 1 aren't used, because row 0 is the current item and carries the style's focus tint.)alternate-background-colorremoved from the tree rule: fails (the alternate-row check).background-colorremoved: still passes. The app-wideQWidget { background-color }rule already paints normal rowsbackground.0, so that line is explicit rather than load-bearing.theme_manager_testisn't in the frozen per-PR test gate; it runs on pushes tomain.Maintainer remediation pass (Linux, Qt 6.12, demo simulator)
theme_manager_testandtheme_seed_testpass. Mutations: either header border removed → fails (:1719,:1720); theQListWidget::item:selectedrule put back → fails (:1692); the import predicate forced totrue→ fails (:1703,:1704).applet/acomscope selected the inherited cells are italictext.secondaryand overridden cells keep the primary text colour; the connection panel's radio list (connectionLocalRadioList) shows a selected row as white text on the palette's blue highlight in Default Dark.text.secondaryleaves scope columns on screen).Not verified
%LOCALAPPDATA%\AetherSDR\themes, whichAETHER_SETTINGS_DIRdoesn't isolate, so I didn't write a test theme into the operator's profile. Selection now comes from the palette, so the accent colour no longer affects the selected row's contrast; the unit test guards against the rule coming back.alternate-background-colormechanism applies there.🤖 Generated with Claude Code