Skip to content

fix(theme): tree-view rows use the theme's backgrounds, so the Theme Editor's token list is readable (#5934). Principle VIII. - #6312

Merged
ten9876 merged 4 commits into
aethersdr:mainfrom
chibondking:fix/5934-theme-tree-rows
Oct 9, 2026
Merged

ten9876 merged 4 commits into
aethersdr:mainfrom
chibondking:fix/5934-theme-tree-rows

Conversation

@chibondking

@chibondking chibondking commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

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.freq looked missing (screenshot in the issue thread). The token was always there.

Root cause

appStylesheetTemplate() (src/gui/Theme.h) gives QListWidget a background and an alternate-background-color, but had no rule for tree views. Qt only takes QPalette::AlternateBase from a stylesheet when alternate-background-color is set, so a QTreeView with alternating rows kept the native AlternateBase. Under a light Windows palette that colour is near-white, while the text is the dark theme's light color.text.primary. The Object list is a QTreeWidget with setAlternatingRowColors(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 covers QTreeWidget) gets background.0, background.1 for alternate rows, text.primary and border.strong. Headers inside tree views (QTreeView QHeaderView::section) get background.1, text.primary, and right/bottom border.strong separators so the header stays distinct from the background.1 alternate rows under it.
  • Selection is left to the palette, for lists too. Theme the tree-view rows so Theme Editor tokens stay readable (#5934). Principle VIII. #5938 added 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 existing QListWidget::item:selected rule 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.
  • Header scope: the header rule is scoped to tree views, so table headers elsewhere don't change. Theme the tree-view rows so Theme Editor tokens stay readable (#5934). Principle VIII. #5938's unscoped QHeaderView::section reached five other dialogs.
  • ThemeEditorDialog.cpp: "inherited" cells use color.text.secondary instead 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 editing color.text.secondary itself 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 when color.text.secondary actually changed.
  • RadioHealthDialog.cpp: the register table alternated rows without an alternate-background-color — the same defect — and now uses background.1.
  • Theme drops: ThemeEditorDialog::dropEvent imported every local URL without the .aethertheme / .json check dragEnterEvent applies. Both now use ThemeManager::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. RadioSetupDialog and SettingsBrowserDialog have their own, closer QTreeWidget rules 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 a QTreeView under 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, then clickAt a row and grab again.

Light OS palette Light OS palette, row selected Dark OS palette, row selected
Before (main) Every other row near-white, text unreadable — —
After (this PR) All rows readable, alternate rows background.1 Light-grey highlight, dark text (palette) Dark-grey highlight, white text (palette)
5934-proof-before-after

Unit test (local, Qt 6.12.0 pinned, MSVC 2022, offscreen)

theme_manager_test renders a QTreeWidget with alternating rows under the app stylesheet. It checks that rows 2 and 3 are exactly background.0 and background.1, and that no list or tree ::item…:selected rule is in the template. A second block renders a two-column header and checks the right and bottom separators are border.strong, and a third pins isImportableThemeFile() 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.)

  • As written: passes.
  • Mutation, alternate-background-color removed from the tree rule: fails (the alternate-row check).
  • Mutation, the black-on-accent selection rule put back: fails.
  • Mutation, the tree rule's background-color removed: still passes. The app-wide QWidget { background-color } rule already paints normal rows background.0, so that line is explicit rather than load-bearing.

theme_manager_test isn't in the frozen per-PR test gate; it runs on pushes to main.

Maintainer remediation pass (Linux, Qt 6.12, demo simulator)

  • theme_manager_test and theme_seed_test pass. Mutations: either header border removed → fails (:1719, :1720); the QListWidget::item:selected rule put back → fails (:1692); the import predicate forced to true → fails (:1703, :1704).
  • Bridge, demo only, isolated settings: the Theme Editor header shows its column separator; with the applet/acom scope selected the inherited cells are italic text.secondary and 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.
  • Not reached on the demo: Radio Health (the demo reports no health registers, so the table is empty), and the in-place inherited-cell repaint (the UI rebuilds the list when the scope changes, so no in-place edit of the root text.secondary leaves scope columns on screen).

Not verified

  • A dark custom accent, screenshotted. User themes live in %LOCALAPPDATA%\AetherSDR\themes, which AETHER_SETTINGS_DIR doesn'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.
  • Windows 11 native, the reporter's platform. Reproduced with Qt's light palette on Windows 10 instead.
  • macOS and Linux, where the stylesheet change applies too. The same alternate-background-color mechanism applies there.

🤖 Generated with Claude Code

chibondking and others added 2 commits October 8, 2026 20:33
…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>

@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.

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.* columns populateRow() writes tm.valueAt(scopePath, key) verbatim (:487), so a font family literally named inherited would 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::UserRole is free on columns ≥ 1 (only column 0 carries the token key, :431). In the else branch of populateRow():

          } 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 existing setForeground(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:selected or QTreeView::item:selected:!active would 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) means QTreeView 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 at ThemeEditorDialog.cpp:398-406). Dropping setHeaderHidden(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 enumerates RadioSetupDialog, SettingsBrowserDialog and NetworkDiagnosticsDialog. It omits ImageFileDialog: getBackgroundImagePath() builds a QFileDialog with DontUseNativeDialog and ViewMode::Detail (ImageFileDialog.cpp:263-273), parented via ScopedChildWidget<QFileDialog> dialogOwner(parent, …), and reaches into its internal treeView (:280). That tree is a QTreeView under a themed ancestor, so the image picker's detail view newly gets background.0, text.primary and a new 1px 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 NetworkDiagnosticsDialog effect, in the safe direction. It says the nav tree "newly gets the themed background and alternate rows". Its own ID-selector rule QTreeWidget#networkDiagnosticsNavigation already sets color, background, border, border-radius, padding and outline (NetworkDiagnosticsDialog.cpp:110-117), and an ID selector outranks a type selector, so none of those change. And the nav never calls setAlternatingRowColors() — the only call in that file is m_audioStreamsTable at :935, a QTableWidget, which isn't a QTreeView subclass 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 #000 combination #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 with item->setForeground(col, QBrush()). A default-constructed QBrush is Qt::NoBrush with colour black, and whether QStyledItemDelegate::initStyleOption() treats that stored-but-default brush as "no override" or as "paint black" decides whether those cells stay color.text.primary or go black. It never mattered before because the fallback background was white; now it's background.0. Narrow: rebuildColumns() emits only Object | Value when no scope is selected (:375-391), so lastCol == 1 and 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 of widget->setStyleSheet(...)". There is no shell sink of any kind in these three files: no system, popen, QProcess or exec*.
  • 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:

  1. Theme token values are spliced into the Qt stylesheet unvalidated. flattenTokens() stores any JSON string verbatim (ThemeManager.cpp:163-164) and colorHexToCssFragment() returns an invalid colour string unchanged (:97-100), so an imported .aethertheme with "color": {"background": {"0": "red; } * { border-image: url(...)"}} injects arbitrary QSS app-wide. Bounded — QSS carries no script and url() resolves through QFile/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 in src/core/ThemeManager.cpp, which isn't in the diff.
  2. ThemeEditorDialog.cpp:689-700 — dragEnterEvent() checks each URL's suffix is aethertheme/json (:679-684), but dropEvent() re-reads the URLs and calls importThemeFromPath() with no suffix re-check. Low severity (importThemeFromFile still 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's background-color still passes because QWidget { background-color } already covers it. Assertion 2 is the load-bearing one: Qt only populates QPalette::AlternateBase from a stylesheet when alternate-background-color is set, so without that line row 3 falls to the platform palette's near-white AlternateBase ≠ 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 and tm.color() would move together. And resources aren't missing — theme_manager_test links ${THEME_TEST_RESOURCES} (tests/tests.cmake:2068) and runs with QT_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) mirrors populateRow()'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 calls setForeground(), so the dangling-row SEGV the :1127 comment warns about can't be triggered from it.
  • Did the new rule break a sibling tree? I checked all five QTreeView/QTreeWidget users in src/gui/. SettingsBrowserDialog sets a dialog-level QTreeWidget rule including border (:58-62) and NetworkDiagnosticsDialog uses 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 is ImageFileDialog's QFileDialog detail view (nit above).
  • CI is green but did not run this test. All five checks pass on c69963c, and .github/workflows/ci.yml:3-18 states the test gate is frozen: the Linux build job runs no tests, and the only ctest -R steps are the macOS/Windows platform-pinned ones in .github/ci-test-gate.txt. theme_manager_test appears nowhere in ci.yml. So the new pixel assertions first execute post-merge on full-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

@aethersdr-agent

Copy link
Copy Markdown
Contributor

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

  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/Theme.h /tmp/aetherclaude/pr-6312/src/gui/Theme.h:195
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/Theme.h /tmp/aetherclaude/pr-6312/src/gui/Theme.h:244
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/Theme.h /tmp/aetherclaude/pr-6312/src/gui/Theme.h:280
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/ThemeEditorDialog.cpp /tmp/aetherclaude/pr-6312/src/gui/ThemeEditorDialog.cpp:317
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/ThemeEditorDialog.cpp /tmp/aetherclaude/pr-6312/src/gui/ThemeEditorDialog.cpp:553
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/ThemeEditorDialog.cpp /tmp/aetherclaude/pr-6312/src/gui/ThemeEditorDialog.cpp:614
  • [MEDIUM] CG-PATH-001 — Potential path traversal in tests/theme_manager_test.cpp /tmp/aetherclaude/pr-6312/tests/theme_manager_test.cpp:1551
  • [MEDIUM] CG-PATH-001 — Potential path traversal in tests/theme_manager_test.cpp /tmp/aetherclaude/pr-6312/tests/theme_manager_test.cpp:1557
  • [MEDIUM] CG-PATH-001 — Potential path traversal in tests/theme_manager_test.cpp /tmp/aetherclaude/pr-6312/tests/theme_manager_test.cpp:1566
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in tests/theme_manager_test.cpp /tmp/aetherclaude/pr-6312/tests/theme_manager_test.cpp:506
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in tests/theme_manager_test.cpp /tmp/aetherclaude/pr-6312/tests/theme_manager_test.cpp:543
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in tests/theme_manager_test.cpp /tmp/aetherclaude/pr-6312/tests/theme_manager_test.cpp:1019
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in tests/theme_manager_test.cpp /tmp/aetherclaude/pr-6312/tests/theme_manager_test.cpp:1233

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

@aethersdr-agent

Copy link
Copy Markdown
Contributor

Antares Detector — candidate vulnerable file(s)

  • src/core/ThemeManager.cpp
  • src/core/ThemeManager.h

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 ten9876 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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)

  1. src/gui/Theme.h:104: border: none removes 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;
            }
  2. Sibling left with the same defect: src/gui/RadioHealthDialog.cpp:57-60. That QTableWidget uses setAlternatingRowColors(true), and its own stylesheet sets background and color: {{color.text.primary}} but no alternate-background-color. It therefore keeps the native AlternateBase, which is the #5934 mechanism in another dialog. I checked every setAlternatingRowColors(true) in src/. The others either set alternate-background-color or ::item:alternate in their own sheet (AudioDeviceChange, Ax25, DxCluster, RadioSetup Peripherals, Memory, NetworkDiagnostics), or are a QListWidget already 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.
  3. The body says NetworkDiagnosticsDialog's navigation tree "newly gets the themed background and alternate rows". That tree has its own, closer QTreeWidget#networkDiagnosticsNavigation { background: … } rule (src/gui/NetworkDiagnosticsDialog.cpp:110-117), and it doesn't call setAlternatingRowColors. 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-native QFileDialog in Detail mode (src/gui/ImageFileDialog.cpp:272-280), whose treeView and header now take the tree and header rules. It's probably an improvement, but it's worth listing in the blast-radius paragraph.
  4. src/gui/ThemeEditorDialog.cpp:1153-1159: inherited cells are found by their display text. A real override whose value is the literal string inherited (a font.family.* value can be any string) would be recoloured as inherited on the next in-place edit. Separately, the loop runs setForeground on every inherited cell for every same-theme themeChanged, including each tick of a live colour edit, even when color.text.secondary didn't change. Both are cheap to fix: tag the cell in populateRow (for example item->setData(col, Qt::UserRole + 1, true)) and test the tag, and skip the loop when the brush colour equals the last one applied.
  5. Test reach (tests/theme_manager_test.cpp:1686-1690). The selection guard is a substring check, so it would miss QTreeWidget::item:selected or QTreeView::item:selected:active. Nothing covers the new in-place repaint path (edit color.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.
  6. Housekeeping: #5938 is still open. Once this lands it should be closed as superseded.

What was verified empirically vs. read

  • Built PR head c69963c96 and its merge base 888ac58bf (lean RelWithDebInfo, shared core, Qt 6.12, Linux), targets AetherSDR and theme_manager_test.
  • theme_manager_test passes on the head.
  • Mutation: with alternate-background-color deleted from the QTreeView rule, the test fails at theme_manager_test.cpp:1687 (the alternate-row check). The regression test pins the fix. Restored afterwards and confirmed by git diff.
  • Bridge, demo only: two offscreen instances with isolated AETHER_SETTINGS_DIR, AETHER_AUTOMATION_NO_TX=1 and explicit sockets, each connected to DEMO-0001 (get radio → "model":"AetherSDR Demo", family sim). invoke "Theme Editor" trigger, then grab 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/HighlightedText come from the palette as a pair.
    • Item foreground overrides: inherited cells still set ForegroundRole, which beats the view's color.
    • 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 onActiveThemeChanged comment warns about.
    • Merge: git merge-tree against current main (4f1691959) is clean, and none of the three files changed on main since the merge base.
  • 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.
  • CI: green, created 04:12Z on a merge base one commit behind current main (#6311, spots, which touches none of these files). theme_manager_test isn't in the per-PR ci.yml filter, so the local run above is the evidence for it.
  • Automated pass: /code-review at 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-scope color.text.secondary point as speculative.

ten9876 and others added 2 commits October 8, 2026 22:05
…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>
@ten9876

ten9876 commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator

Answers to the aethersdr-agent review and the CodeGuard scan, at head 359cd18d6:

Nits

  • Inherited cells found by text (ThemeEditorDialog.cpp:1154) — fixed in 08bd1804b: populateRow() now sets an item data role (kInheritedCellRole = Qt::UserRole + 1, since column 0 already uses Qt::UserRole) and the repaint loop reads that role; the loop also returns early unless color.text.secondary actually changed.
  • Selection guard is a string match — widened in 08bd1804b/359cd18d6 to a regex over any QTreeView|QTreeWidget|QListView|QListWidget …::item…:selected selector. Putting the old list rule back fails it at tests/theme_manager_test.cpp:1692.
  • Header rule unexercised — fixed in 08bd1804b: a new block renders a two-column header and checks the section's right and bottom pixels are color.border.strong (the maintainer chose to restore the separators). Removing either border fails it (:1719, :1720).
  • ImageFileDialog missing from the blast radius / NetworkDiagnostics overstated — PR body corrected.
  • QListWidget::item:selected black-on-accent sibling (Theme.h:90) — folded in on the maintainer's ruling in 359cd18d6: the rule is deleted, so lists use the palette selection like trees. On the demo, the connection panel's radio list shows a selected row as white on the palette's blue highlight in Default Dark.
  • QBrush() on overridden cells — checked on the demo with the applet/acom scope selected: overridden scope cells draw in the primary text colour, not black.

Security pass

  • Stylesheet token injection (ThemeManager.cpp:97-100,163-164) — unchanged code; filed as Theme token values are spliced into the app stylesheet unvalidated (QSS injection from an imported theme) #6320.
  • dropEvent skips the suffix check (ThemeEditorDialog.cpp:689-700) — fixed in 359cd18d6: drag-enter and drop both go through ThemeManager::isImportableThemeFile(), pinned in theme_manager_test (forcing it to true fails :1703, :1704).
  • CS-VLN-CMDI-BACKTICK ×10, CG-PATH-001 ×3 — false positives, as the review established (backticks in comments; negative path-traversal assertions).

@ten9876 ten9876 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving at 359cd18d6 after a maintainer remediation pass.

Fixed

  1. Header separators lost (Theme.h:101-108) → right/bottom color.border.strong on the tree-scoped header section, per maintainer decision — 08bd1804b; theme_manager_test fails with either border removed (:1719, :1720), demo grab shows the Object | Value separator.
  2. 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.
  3. Inherited cells found by text / repaint on every edit (ThemeEditorDialog.cpp) → item data role + early return unless color.text.secondary moved — 08bd1804b; demo with applet/acom scope shows inherited cells italic text.secondary and overrides in primary text.
  4. Black-on-accent list selection (Theme.h:90) → rule deleted, lists use the palette like trees, per maintainer decision — 359cd18d6; guard fails at :1692 with the rule restored; demo radio list shows white on the palette highlight in Default Dark.
  5. dropEvent skipped the file-type check → ThemeManager::isImportableThemeFile() on drag-enter and drop — 359cd18d6; pinned at :1699-1705, fails when forced to true.
  6. 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.

@ten9876
ten9876 merged commit ba30526 into aethersdr:main Oct 9, 2026
5 checks passed
ten9876 added a commit that referenced this pull request Oct 9, 2026
…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 added a commit that referenced this pull request Oct 9, 2026
… 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>
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.

Font.Family.Freq object missing in theme editor in version 29.6.4

2 participants