fix(theme): editor UX polish — filter persistence + reset semantics + factory-loader v2

[nigelfenton]

Four targeted fixes to the Theme Editor surface picking up the Tier 3 "editor UX polish" bullets from #3184. Hand-tested across both bundled themes and a forked user theme; theme_manager_test gains coverage for the per-applet cascade and the new v2-aware factory snapshot.

1. Filter persistence across scope changes

refreshTokenList() rebuilds the token tree from scratch but never re-applied the current text/inspector subset filter — so switching the scope dropdown (or any path that triggers a refresh) caused the filter to silently fall back to "show everything" while the QLineEdit kept the text it was apparently filtering by.

One-line fix: call filterTokensTo(m_activeSubset) at the end of refreshTokenList so the rebuilt rows immediately honour the filter.

2. Theme-delete-time cleanup of recent-color buckets

Per-theme recent-color grids persist under the AppSettings key ThemeEditor.RecentColors/<theme name>. Deleting the theme file used to leave those entries behind forever — a small but real leak called out in #3184.

onDeleteThemeClicked now removes the matching key after the theme file deletion succeeds. TokenEditorWidget::recentColorsKeyFor is promoted from private to public-static so the dialog can call the canonical key formatter directly (no string-duplication risk).

3. Reset-to-default semantics at nested scopes

At root scope Reset still loads the bundled factory value into the edit buffer for the user to confirm via OK — that behaviour is preserved.

At nested scopes (applet/tx, applet/rx, applet/comp, …) the previous behaviour was incorrect: Reset loaded the factory value into the buffer, and OK then wrote that value as a new override at the nested scope. Per #3184 the intended semantic at a non-root scope is "clear my override here so the scope falls back to inheriting from its parent" — i.e. exactly what right-click → Clear Override on the scope-chain column already does.

onResetClicked now branches:

• root scope: existing factory-load-into-buffer behaviour
• nested scope: calls removeOverride() directly, reloads the editor's buffer + picker from the now-inherited value, emits tokenChanged so the parent dialog repopulates the matching row (so the scope column flips to italic "inherited" AND the Value column reflects the now-resolved value — without that signal the Value column would stay stale, as observed during testing).

refreshResetButton also branches: at root scope it enables based on hasFactoryValue (existing), at nested scope it enables only when there's actually an override at this scope to clear — mirrors the right-click → Clear Override gating, so the button visually reflects whether clicking it has any effect.

4. Factory-snapshot v2 schema awareness (uncovered during testing)

While verifying Fix #3 at root scope, discovered that Reset there was also broken — but for an unrelated reason that predates this PR.

ThemeManager::ensureFactoryLoaded() reads :/themes/default-dark.json to build the m_factoryTokens map that factoryColor / factoryString / factorySizing / hasFactoryValue all read from. Its single flattenTokens call read from doc.object().value("tokens") — which is the v1 schema shape. In v2 (the canonical shape since #3176) the tokens live under scopes.root.tokens; the v1 path produces an empty m_factoryTokens against any v2 theme.

Downstream effect: hasFactoryValue returns false for every token in a v2 theme → refreshResetButton disables the Reset button at root scope (or with the existing styling, leaves it visually indistinguishable from enabled — silent failure either way) → factoryColor returns invalid QColor. Silently broken across every v2 theme since #3176 landed in March.

ensureFactoryLoaded now branches on schemaVersion:

• v2: reads primitives + scopes.root.tokens, then resolves {primitive} aliases inline before storing in m_factoryTokens, so factoryColor sees concrete hex values without needing a second lookup pass (matches the live runtime resolver — single-hop).
• v1: existing path preserved for older themes still on disk pending auto-migration.

theme_manager_test gains assertions on hasFactoryValue + factoryColor for color.accent (alias-resolved) and color.background.0 to lock this in against future schema bumps.

Verification

Windows 11, MSVC + Ninja + Qt 6.10.3, branched from aethersdr/AetherSDR main at 4ee99f78.

Check	Result
Filter persistence — type "accent", switch scope to applet/tx	Filter stays applied (list does NOT re-expand) ✓
Nested-scope Reset — forked theme, applet/tx, color.slider.foreground (#ff4d4d override) → click Reset	Picker reloaded #ff4d4d → #00b4d8, tx column flipped from override to italic "inherited", Value column updated to #00b4d8, Reset button disabled ✓
Root-scope Reset — same theme, (root), changed color.accent to magenta #a12c66, OK'd, then Reset	Picker flipped magenta → factory blue #00b4d8, • unsaved indicator appeared, OK + Cancel re-enabled, token list row still showed #a12c66 (uncommitted buffer) ✓
theme_manager_test — new factory-snapshot + per-applet cascade assertions	Pass ✓
theme_manager_test — 4 pre-existing failures (importThemeFromFile-related, Windows-vs-CI env issue)	Also present on clean main at the same source line numbers (398/400/451/590 there → 419/421/472/611 here, +21 from inserted test block). Not introduced by this PR. Happy to investigate separately.

Out of scope (follow-ups)

• The importThemeFromFile test failures are not investigated here — they predate this PR and the root-cause looks Windows-environment-specific (QTemporaryDir / XDG_CONFIG_HOME interaction).
• Tooltips / hint text explaining what Reset does at root vs nested could further close the UX gap, but the enable-state gating already gives correct feedback.

cc @ten9876 @jensenpat @chibondking

73 Nigel G0JKN

🤖 Generated with Claude Code

[aethersdr-agent]

Thanks @nigelfenton — this is a tidy, well-scoped fix bundle with a thoughtful "while I was here" find on the v2 factory loader. All four changes look correct.

Verified against the codebase:

• Fix 1 (filter persistence): filterTokensTo(m_activeSubset) is the right hook — it reads the QLineEdit text and m_activeSubset directly, so a single call at the tail of refreshTokenList is the minimum necessary change.
• Fix 2 (recent-colors cleanup): AppSettings::contains/remove/save pattern matches the API (AppSettings.h:31–37). Promoting recentColorsKeyFor to a public static avoids string duplication — good call.
• Fix 3 (nested-scope Reset): removeOverride + setToken + emit tokenChanged chain is right. The isOverriddenAt guard pairs cleanly with the new refreshResetButton branch, so the button accurately reflects whether the click has any effect. Symmetric with the right-click → Clear Override path.
• Fix 4 (v2 factory loader): Inline {primitive} resolution is single-hop, matching resolveAlias at ThemeManager.cpp:245–256. The userType() != QMetaType::QString guard correctly skips gradient/font entries that flattenTokens stores as QVariant<ThemeGradient> / QVariant<ThemeFont>. v1 path preserved for pre-migration files.

Conventions: AppSettings (not QSettings) ✓, no obvious null deref / leak risk, all five files in scope.

One small thought (not blocking): the new factory-snapshot test exercises one alias-resolved token (color.accent) and one direct-string token (color.background.0). If you wanted to lock in the gradient/font passthrough path under v2 too, a third assertion against a gradient token (e.g. factoryColor on a token that's a gradient → first stop fallback at ThemeManager.cpp:849–851) would round it out — but the current coverage is enough to catch the regression that motivated the fix.

Good catch on the pre-existing importThemeFromFile failures being a clean-main issue rather than introduced here.

LGTM.

---

_{🤖 aethersdr-agent · cost: $9.1271 · model: claude-opus-4-7}

[nigelfenton]

Thanks for the review @aethersdr-agent — addressed the gradient-passthrough suggestion in e249406b. Added an assertion against color.meter.bar.fillGradient (a bundled v2 gradient token) that exercises both the alias-resolution loop's userType() != QMetaType::QString guard and factoryColor's first-stop-fallback branch. Locks in the path against a future schema bump.

The three observations on #3198 (warning primitive load-bearing-for-future-use, light disabled-state contrast, hover state deferred) are all intentional design choices on this pass and will be revisited in the per-site QSS migration sweep that follows that PR. Thanks for flagging them.

73 Nigel

[aethersdr-agent]

Thanks Nigel — e249406b reads well. color.meter.bar.fillGradient is a good pick for that assertion: it's a literal nested object in scopes.root.tokens rather than a {primitive} alias string, so it concretely exercises the userType() != QMetaType::QString skip in the alias-resolution loop (any future regression that tried to toString() a gradient and re-feed it through the primitives map would be caught), and the first-stop-fallback branch in factoryColor is now pinned to #2f9e6a — matching the live ThemeManager::color() behaviour for gradient tokens at lines 845–852. Locks both paths in cleanly against the schema bump scenario you called out.

Spot-checked the v2 path against resources/themes/default-dark.json while reviewing — bar.fillGradient and the alias chains for color.accent/color.background.0 all resolve as the test expects, so the assertions match the bundled baseline.

Re the three #3198 observations — fair enough, all three are clearly scoped as design choices for the per-site QSS migration sweep rather than this PR. The warning primitive being load-bearing-for-future-use in particular is the kind of thing that's easier to justify in the sweep PR's diff than out-of-band here. Happy to revisit when that lands.

Re the importThemeFromFile Windows-env failures — agreed that's a separate investigation. If it does come back to bite, the QTemporaryDir + XDG_CONFIG_HOME interaction on Windows can sometimes be sidestepped by setting QStandardPaths::setTestModeEnabled(true) in the test harness, which forces all GenericConfigLocation / AppConfigLocation lookups to a deterministic per-process temp prefix on every platform — but worth confirming the actual symptom first before reaching for that.

73, Jeremy KK7GWY & Claude (AI dev partner)

---

_{🤖 aethersdr-agent · cost: $2.3285 · model: claude-opus-4-7}