Skip to content

Commit 03173c1

Browse files
nigelfentonclaudeten9876
authored
feat(theme): generate the compiled seed from the bundled JSON (#4582)
#3184 item 2 — *"the high-leverage one; kills the drift class permanently."* > ⚠ **Stacked on #4570** (`6c599007`), whose checker this repurposes into the freshness gate rather > than superseding. Review `7250974f` alone, or merge #4570 first. ## What was wrong `seedBuiltinDefaults()` was a hand-maintained copy of `default-dark.json`. Both failure modes its own comment warned about had already happened: - **9 tokens had drifted** — `color.accent.dim` and all eight `color.slice.a..h`. The seed said slice A was `#ff4040` (red); both bundled themes say `#00d4ff` (cyan). - **25 tokens were never seeded at all** — `slice.dim.a..h`, `highlight.*`, `button.*.disabled`, and the six `waterfall.colormap.*` gradients. These resolved **transparent** (alpha 0) on any theme predating them, which is strictly worse than a stale value. ## What this does `tools/gen_theme_seed.py` emits `src/core/ThemeSeedGenerated.cpp` from the resource, resolving `{primitive}` aliases to concrete values (the seed runs before any primitives palette exists). **209 hand-written lines become one call. All 128 tokens emitted, up from 104.** Per the design agreed on the issue: **commit-time, not build-time.** The generated file is checked in and greppable — `seedBuiltinDefaults()` is code people demonstrably read, and its *"Preliminary values — a dedicated slice-colour audit may tune these"* comment is **how the drift was diagnosed**. `tools/check_theme_seed.py` becomes a staleness gate; its `KNOWN_SEED_DRIFT` list is deleted because the class of bug is gone, not because the nine were individually patched. One structural note: the generated TU can't see `ThemeScope` (private to `ThemeManager.cpp`), so scoped tokens go through a new private `seedScopedToken()` helper. ## On testing — the honest version **The seed is not observable through the public API.** The constructor loads Default Dark immediately after seeding, so by the time `instance()` returns the JSON has overwritten everything. That is precisely the invisibility that let the drift go unnoticed. I wrote C++ assertions for it, discovered **they passed identically against the old hand-written seed**, and removed them rather than ship a test that proves nothing. `theme_manager_test` carries a note explaining why, so the next person doesn't repeat the attempt. Verification lives where the property *is* observable — `--check` now asserts **completeness** (every JSON token reaches the emitted table) independently of byte-equality, because byte-equality alone would happily pass a generator that silently dropped a family: | injected fault | result | |---|---| | drop the waterfall gradients | **FAIL** — "emitted 120 tokens but the theme defines 128" | | edit the theme, don't regenerate | **FAIL** `--strict`, exit 1 | | neither | pass | `ctest -R "theme\|s_meter\|colour\|color\|style\|applet"` **5/5**; full tree builds on Windows/MSVC. ## Behaviour change worth calling out A user whose theme predates the slice tokens currently renders slice A **red** from the seed and will flip to **cyan**. That is the correct value — both bundled themes agree — and the blast radius is limited to pre-v2 themes. Per the issue discussion this wants a `CHANGELOG.md` line under whichever release carries it; happy to add that here if you'd prefer it in-PR. Refs #3184. --- ## Review round (maintainer, 2026-07-31 — commit `eb4df8f5`) Reviewed, and the follow-ups were pushed to this branch rather than handed back. The generator design was right; what was missing was the scaffolding around it — four of the five blockers were in files this PR didn't touch. **Corrections to the description above:** the count of never-seeded tokens is **24**, not 25 (104 → 128); it was wrong in three source comments too, and is now fixed everywhere. The "stacked on #4570" note is obsolete — #4570 landed, and this branch has since been merged with current `main`. The CHANGELOG offer is taken up: the entry is in this PR. The injected-fault table's "emitted 120 tokens" is actually 121 (there are seven gradient tokens, not six — `color.meter.bar.fillGradient` is one), and that assertion has been replaced outright, see below. **Fixed in `eb4df8f5`:** - **`tests/theme_seed_test.cpp` (new).** The premise that "the seed is not observable through the public API" holds only for `theme_manager_test`, which links `resources.qrc`. `container_widget_test` already links ThemeManager *without* the theme resource — there the seed **is** the rendered palette, which is the resource-missing fallback path #3184 named. The new target pins the nine formerly-drifted values, alias resolution, one token from each previously unseeded family, all seven gradients, the `seedScopedToken()` scope tree, and the non-colour token types. Mutation-tested: reverting only the nine values → 9 failures; deleting only the six colormap gradients → 6 failures; unmodified → PASS. The CI gate proves the *generator* agrees with the JSON; it cannot prove the emitted C++ reaches `m_tokens` at runtime — a target that omits `ThemeSeedGenerated.cpp` from its source list passes the gate and ships a transparent UI. - **`theme-seed-check.yml` `paths:`** did not include `ThemeSeedGenerated.cpp` or `gen_theme_seed.py` — the two files the gate exists to protect. A hand-edit of the "DO NOT EDIT" file never ran the check. Both added; the header comment (still describing the hand-maintained seed and `KNOWN_SEED_DRIFT`) rewritten. - **`docs/theming/slider-knob-tokens.md` and `toggle-button-tokens.md`** are the "adding the next control namespace" playbooks, and both still instructed contributors to hand-add entries to `seedBuiltinDefaults()` — re-creating the dual source of truth this PR removes. Both now say: edit the JSON, regenerate. - **`CHANGELOG.md`** under `[Unreleased]`, per the ruling on #3184 — covering both the corrected slice colours and the 24 tokens that previously resolved transparent. - **Generator hardening.** An alias naming a missing primitive was passed through and emitted as `QString("{color.teal.500}")` — an invalid QColor, i.e. transparent — with the gate exiting 0; now a hard error. `resolve()` didn't descend into lists, so an alias in a gradient stop survived to `QColor("{...}")`; lists are now walked. `--check` compared token *counts* with `<`, so a fault duplicating one token and dropping another passed; it now compares the `(scope, token)` sets and names what's missing. Output is byte-identical on the current tree. - **Merged current `main`** (settings sources moved to `${AETHER_SETTINGS_SOURCES}` under RFC #4603) — resolved across all nine targets, which also removed the duplicated `ThemeSeedGenerated.cpp` line in the `ENABLE_ASR` block. - **Stale ctor comment** claiming the seed "lacks the `waterfall.colormap` gradients" — it no longer does; rewritten to the reason that still holds. **Verified:** full tree builds (2563/2563, zero errors, Linux/GCC/Qt6); `ctest -R "theme|s_meter|colour|color|style|applet|container"` 7/7; `gen_theme_seed.py --check` and `check_theme_seed.py --strict` pass, and fail on each injected fault above. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Jeremy [KK7GWY] <kk7gwy@aethersdr.com>
1 parent 50404bf commit 03173c1

12 files changed

Lines changed: 959 additions & 646 deletions

.github/workflows/theme-seed-check.yml

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,27 @@
11
name: Theme Seed Check
22

33
# ThemeManager::seedBuiltinDefaults() compiles a copy of the default theme into
4-
# the binary, kept in sync with resources/themes/default-dark.json BY HAND — the
5-
# source comments say so, and warn that a drifted seed diverges "silently".
6-
# Nine tokens have already drifted that way (8 slice colours + color.accent.dim)
7-
# and are frozen in KNOWN_SEED_DRIFT; any NEW divergence fails the job
8-
# (--strict). The baseline only shrinks.
4+
# the binary. It used to be kept in sync with resources/themes/default-dark.json
5+
# BY HAND, and drifted (#3184). It is now GENERATED — tools/gen_theme_seed.py
6+
# emits src/core/ThemeSeedGenerated.cpp from the resource — so the two can no
7+
# longer disagree by drifting apart. What CAN still happen is someone editing
8+
# the bundled theme without regenerating, or hand-editing the "DO NOT EDIT"
9+
# file. This job fails on either (--strict).
910
#
10-
# Cheap to run (stdlib-only, no build), so it gates on the two files that can
11-
# cause the drift plus the checker itself.
11+
# Cheap to run (stdlib-only, no build), so it gates on every file that can make
12+
# the committed table wrong: the theme resource, the generator, the generated
13+
# file itself, the checker, and this workflow. Leaving the generator or the
14+
# generated file off this list is how the gate stops protecting the two files
15+
# it exists for.
1216

1317
on:
1418
pull_request:
1519
branches: ["**"]
1620
paths:
1721
- "src/core/ThemeManager.cpp"
22+
- "src/core/ThemeSeedGenerated.cpp"
1823
- "resources/themes/**"
24+
- "tools/gen_theme_seed.py"
1925
- "tools/check_theme_seed.py"
2026
- ".github/workflows/theme-seed-check.yml"
2127

CHANGELOG.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,25 @@ Format follows [Keep a Changelog](https://keepachangelog.com/).
88
99
## [Unreleased]
1010

11+
### Theme fallback colours corrected on themes that predate the token (#3184)
12+
13+
- **Slice colours A–H and `color.accent.dim` now match the bundled themes.**
14+
AetherSDR compiles a copy of the default theme into the binary as a fallback
15+
for tokens a theme file doesn't define. That table was maintained by hand and
16+
had drifted: it said slice A was red (`#ff4040`) while both bundled themes say
17+
cyan (`#00d4ff`), and eight more slice colours disagreed the same way. If your
18+
theme predates the slice tokens you will see the corrected — cyan — palette.
19+
Themes that define their own slice colours are unaffected.
20+
21+
- **24 tokens that had no compiled fallback at all now have one.** The dimmed
22+
slice colours, the highlight and disabled-button colours, and all six
23+
waterfall colormap gradients previously resolved to *transparent* on a theme
24+
that predated them. On such a theme the waterfall could render with no
25+
colormap; it now falls back to the bundled Default Dark gradients.
26+
27+
- The table is generated from `resources/themes/default-dark.json` by
28+
`tools/gen_theme_seed.py`, so the two can no longer disagree.
29+
1130
### Client settings store moved to SQLite (RFC #4603, phase 1)
1231

1332
- **Settings now live in a SQLite database** (`AetherSDR.db` in the config

CMakeLists.txt

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -561,6 +561,7 @@ set(CORE_SOURCES
561561
src/core/OccupiedRegion.cpp
562562
src/core/SettingsHelpers.cpp
563563
src/core/ThemeManager.cpp
564+
src/core/ThemeSeedGenerated.cpp
564565
src/core/BandStackSettings.cpp
565566
src/core/RadioDiscovery.cpp
566567
src/core/RadioConnection.cpp
@@ -2984,6 +2985,7 @@ qt_add_resources(THEME_TEST_RESOURCES resources/resources.qrc)
29842985
add_executable(theme_manager_test
29852986
tests/theme_manager_test.cpp
29862987
src/core/ThemeManager.cpp
2988+
src/core/ThemeSeedGenerated.cpp
29872989
${AETHER_SETTINGS_SOURCES}
29882990
src/core/LogManager.cpp
29892991
src/core/AsyncLogWriter.cpp
@@ -2995,6 +2997,28 @@ add_test(NAME theme_manager_test COMMAND theme_manager_test)
29952997
set_tests_properties(theme_manager_test PROPERTIES
29962998
ENVIRONMENT "QT_QPA_PLATFORM=offscreen")
29972999

3000+
# Compiled-in theme seed (#3184). DELIBERATELY has no ${THEME_TEST_RESOURCES}:
3001+
# with the theme resource linked, the ThemeManager constructor loads Default
3002+
# Dark straight after seeding and the JSON hides whatever the seed actually
3003+
# contains — which is how nine seed tokens drifted unnoticed for months.
3004+
# Without it, the seed IS the rendered palette and every token is assertable
3005+
# through the public API. Adding the resource here would silently turn this
3006+
# into a second, weaker copy of theme_manager_test; the test asserts
3007+
# availableThemes() is empty so that mistake fails loudly instead.
3008+
add_executable(theme_seed_test
3009+
tests/theme_seed_test.cpp
3010+
src/core/ThemeManager.cpp
3011+
src/core/ThemeSeedGenerated.cpp
3012+
${AETHER_SETTINGS_SOURCES}
3013+
src/core/LogManager.cpp
3014+
src/core/AsyncLogWriter.cpp
3015+
)
3016+
target_include_directories(theme_seed_test PRIVATE src)
3017+
target_link_libraries(theme_seed_test PRIVATE Qt6::Core Qt6::Gui Qt6::Widgets Qt6::Test)
3018+
add_test(NAME theme_seed_test COMMAND theme_seed_test)
3019+
set_tests_properties(theme_seed_test PROPERTIES
3020+
ENVIRONMENT "QT_QPA_PLATFORM=offscreen")
3021+
29983022
# Panadapter overlay collapse semantics for owner-managed status cards (#4387):
29993023
# a collapse must shrink the card, survive owner re-assertion, and never make
30003024
# the indicator disappear while its condition still holds.
@@ -3003,6 +3027,7 @@ add_executable(panadapter_message_overlay_test
30033027
tests/panadapter_message_overlay_test.cpp
30043028
src/gui/PanadapterMessageOverlay.cpp
30053029
src/core/ThemeManager.cpp
3030+
src/core/ThemeSeedGenerated.cpp
30063031
${AETHER_SETTINGS_SOURCES}
30073032
src/core/LogManager.cpp
30083033
src/core/AsyncLogWriter.cpp
@@ -3894,6 +3919,7 @@ if (ENABLE_ASR)
38943919
src/gui/FramelessResizer.cpp
38953920
src/gui/FramelessWindowTitleBar.cpp
38963921
src/core/ThemeManager.cpp
3922+
src/core/ThemeSeedGenerated.cpp
38973923
${AETHER_SETTINGS_SOURCES}
38983924
src/core/LogManager.cpp
38993925
src/core/AsyncLogWriter.cpp
@@ -3934,6 +3960,7 @@ add_executable(s_meter_geometry_test
39343960
src/gui/SMeterGeometry.cpp
39353961
src/gui/SMeterWidget.cpp
39363962
src/core/ThemeManager.cpp
3963+
src/core/ThemeSeedGenerated.cpp
39373964
${AETHER_SETTINGS_SOURCES}
39383965
src/core/LogManager.cpp
39393966
src/core/AsyncLogWriter.cpp
@@ -4279,6 +4306,7 @@ add_executable(cwx_panel_test
42794306
# CwxPanel.cpp calls ThemeManager::resolve() post-Phase-2 migration;
42804307
# pull in the manager + its logging deps so the test links.
42814308
src/core/ThemeManager.cpp
4309+
src/core/ThemeSeedGenerated.cpp
42824310
${AETHER_SETTINGS_SOURCES}
42834311
src/core/LogManager.cpp
42844312
src/core/AsyncLogWriter.cpp
@@ -4308,6 +4336,7 @@ add_executable(health_applet_test
43084336
src/gui/HealthApplet.cpp
43094337
src/models/MeterModel.cpp
43104338
src/core/ThemeManager.cpp
4339+
src/core/ThemeSeedGenerated.cpp
43114340
${AETHER_SETTINGS_SOURCES}
43124341
src/core/LogManager.cpp
43134342
src/core/AsyncLogWriter.cpp
@@ -4798,6 +4827,7 @@ add_executable(help_dialog_test
47984827
# HelpDialog.cpp calls ThemeManager::resolve() post-Phase-2 migration;
47994828
# pull in the manager + its logging deps so the test links.
48004829
src/core/ThemeManager.cpp
4830+
src/core/ThemeSeedGenerated.cpp
48014831
${AETHER_SETTINGS_SOURCES}
48024832
src/core/LogManager.cpp
48034833
src/core/AsyncLogWriter.cpp
@@ -4822,6 +4852,7 @@ add_executable(flex_control_dialog_size_test
48224852
src/core/DigitalVoiceModeRegistry.cpp
48234853
src/core/KiwiSdrProtocol.cpp
48244854
src/core/ThemeManager.cpp
4855+
src/core/ThemeSeedGenerated.cpp
48254856
${AETHER_SETTINGS_SOURCES}
48264857
src/core/LogManager.cpp
48274858
src/core/AsyncLogWriter.cpp
@@ -4857,6 +4888,7 @@ add_executable(pan_layout_dialog_size_test
48574888
src/gui/FramelessResizer.cpp
48584889
src/gui/FramelessWindowTitleBar.cpp
48594890
src/core/ThemeManager.cpp
4891+
src/core/ThemeSeedGenerated.cpp
48604892
${AETHER_SETTINGS_SOURCES}
48614893
src/core/LogManager.cpp
48624894
src/core/AsyncLogWriter.cpp
@@ -4963,6 +4995,7 @@ add_executable(container_widget_test
49634995
# logging deps so the test links. ThemeManager's compiled-in
49644996
# seedBuiltinDefaults() means we don't need the theme resource here.
49654997
src/core/ThemeManager.cpp
4998+
src/core/ThemeSeedGenerated.cpp
49664999
src/core/LogManager.cpp
49675000
src/core/AsyncLogWriter.cpp
49685001
)
@@ -4990,6 +5023,7 @@ add_executable(hl2_pc_audio_lock_test
49905023
src/gui/DragValuePopup.cpp
49915024
${AETHER_SETTINGS_SOURCES}
49925025
src/core/ThemeManager.cpp
5026+
src/core/ThemeSeedGenerated.cpp
49935027
src/core/LogManager.cpp
49945028
src/core/AsyncLogWriter.cpp
49955029
)
@@ -5008,6 +5042,7 @@ add_executable(amp_applet_test
50085042
src/gui/DragValuePopup.cpp
50095043
${AETHER_SETTINGS_SOURCES}
50105044
src/core/ThemeManager.cpp
5045+
src/core/ThemeSeedGenerated.cpp
50115046
src/core/LogManager.cpp
50125047
src/core/AsyncLogWriter.cpp
50135048
)
@@ -5045,6 +5080,7 @@ add_executable(container_manager_test
50455080
src/gui/containers/FloatingContainerWindow.cpp
50465081
${AETHER_SETTINGS_SOURCES}
50475082
src/core/ThemeManager.cpp
5083+
src/core/ThemeSeedGenerated.cpp
50485084
src/core/LogManager.cpp
50495085
src/core/AsyncLogWriter.cpp
50505086
)
@@ -5063,6 +5099,7 @@ add_executable(container_nesting_test
50635099
src/gui/containers/FloatingContainerWindow.cpp
50645100
${AETHER_SETTINGS_SOURCES}
50655101
src/core/ThemeManager.cpp
5102+
src/core/ThemeSeedGenerated.cpp
50665103
src/core/LogManager.cpp
50675104
src/core/AsyncLogWriter.cpp
50685105
)
@@ -5165,6 +5202,7 @@ endif()
51655202
set(AETHER_SETTINGS_CONSUMERS
51665203
slice_label_test
51675204
theme_manager_test
5205+
theme_seed_test
51685206
panadapter_message_overlay_test
51695207
app_settings_safety_test
51705208
nr2_settings_model_test

docs/theming/slider-knob-tokens.md

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,13 @@ no slider/knob entries in their on-disk JSON still get the canonical
8787
look + per-applet differentiation. The bundled themes' JSON
8888
re-asserts the same values via primitive aliases (idempotent).
8989

90+
> **Since #3184 the seed is GENERATED, not hand-written.**
91+
> `tools/gen_theme_seed.py` emits `src/core/ThemeSeedGenerated.cpp`
92+
> from `resources/themes/default-dark.json`, resolving `{primitive}`
93+
> aliases to concrete values. Everything above still describes what
94+
> the seed *contains*; it no longer describes how it gets there. Add
95+
> tokens to the JSON and run the generator — never by hand.
96+
9097
## The QSS migration
9198

9299
Two surfaces in `gui/Theme.h`:
@@ -184,11 +191,14 @@ When carving the next control type out (toggles, spinboxes, …):
184191
1. **Define the namespace.** Match the slider/knob shape:
185192
`color.<type>.background`, `.foreground`, `.handle`, each with a
186193
`.disabled` variant.
187-
2. **Seed both layers.**
188-
- Add JSON entries to both bundled themes' root scope, aliased to
189-
primitives.
190-
- Add raw-hex root entries to `seedBuiltinDefaults()` for forked
191-
user themes that pre-date the new namespace.
194+
2. **Add the tokens to both bundled themes' root scope**, aliased to
195+
primitives, then run `python tools/gen_theme_seed.py` and commit
196+
the regenerated `src/core/ThemeSeedGenerated.cpp`. That is what
197+
seeds the compiled fallback for forked user themes that pre-date
198+
the new namespace — do **not** hand-add entries to
199+
`seedBuiltinDefaults()`, which is the dual source of truth #3184
200+
removed. `tools/check_theme_seed.py --strict` fails the PR if you
201+
forget the regeneration step.
192202
3. **Migrate the QSS or paint code.**
193203
- QSS-styled controls (`QToolButton`, `QSpinBox`, …): swap the
194204
borrowed tokens in `Theme.h`'s `appStylesheetTemplate` and any
@@ -199,8 +209,9 @@ When carving the next control type out (toggles, spinboxes, …):
199209
`tm.color(widget, token)` overload.
200210
4. **Optional: per-applet overrides.** Add nested-scope JSON
201211
entries under `scopes.applet.scopes.<name>` for the applets that
202-
should differ, and seed them in `seedBuiltinDefaults()` for
203-
pre-existing user themes.
212+
should differ. The generator picks nested scopes up automatically
213+
and emits them through `seedScopedToken()`, so pre-existing user
214+
themes get them from the same regeneration step as step 2.
204215
5. **Document.** Add a section to this directory referencing the
205216
pattern.
206217

docs/theming/toggle-button-tokens.md

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,13 @@ toggle entries in their on-disk JSON still get the canonical look +
116116
per-applet differentiation. The bundled themes' JSON re-asserts the
117117
same values via primitive aliases (idempotent).
118118

119+
> **Since #3184 the seed is GENERATED, not hand-written.**
120+
> `tools/gen_theme_seed.py` emits `src/core/ThemeSeedGenerated.cpp`
121+
> from `resources/themes/default-dark.json`, resolving `{primitive}`
122+
> aliases to concrete values. Everything above still describes what
123+
> the seed *contains*; it no longer describes how it gets there. Add
124+
> tokens to the JSON and run the generator — never by hand.
125+
119126
## The helper
120127

121128
[`src/gui/Theme.h`](../../src/gui/Theme.h):
@@ -203,8 +210,11 @@ boxes…):
203210
(slider + knob PR).
204211
2. **Match the namespace shape.** Base tokens + state suffixes,
205212
with `<tribe>.<property>.<state>` if tribed.
206-
3. **Seed both layers** (bundled JSON + `seedBuiltinDefaults`) so
207-
user themes forked pre-PR still render correctly.
213+
3. **Add the tokens to the bundled JSON**, then run
214+
`python tools/gen_theme_seed.py` and commit the regenerated
215+
`src/core/ThemeSeedGenerated.cpp`, so user themes forked pre-PR
216+
still render correctly. Never hand-add to `seedBuiltinDefaults()`
217+
— that dual source of truth is what #3184 removed.
208218
4. **Helper in `Theme.h`** with the tribe enum (if applicable),
209219
widget-aware resolution via `applyStyleSheet`.
210220
5. **Document this directory** with the namespace, cascade, helper

0 commit comments

Comments
 (0)