Fix theme extra colors lost after user customization#33021
Fix theme extra colors lost after user customization#33021luapmartin wants to merge 2 commits intomusescore:masterfrom
Conversation
📝 WalkthroughWalkthroughThe pull request modifies theme handling logic in two files. The 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/ui/internal/uiconfiguration.cpp`:
- Around line 219-223: Add a regression unit test covering the persisted theme
merge path: write a test that uses ThemeConverter::fromMap() and
UiConfiguration::updateThemes() to round-trip a theme where you modify a
standard value (e.g., change a named color in ThemeInfo::values) while the
original cfg-backed ThemeInfo contains an extra color in ThemeInfo::extra,
persist and reload the themes (simulate saving/loading settings via the same
map/json path used by ThemeConverter::fromMap()), call
UiConfiguration::updateThemes() to merge, and assert that the reloaded/merged
theme preserves the extra color and the customized standard value; target the
test to exercise the code paths that build theme.values and merge into existing
cfg-backed ThemeInfo so regressions in merging extra are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80c94351-ae85-41c8-8d8c-7c09a350e5ee
📒 Files selected for processing (2)
src/framework/ui/internal/themeconverter.cppsrc/framework/ui/internal/uiconfiguration.cpp
| for (auto key : it->values.keys()) { | ||
| if (key != UNKNOWN) { | ||
| theme.values[key] = it->values[key]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a regression test for the persisted theme merge path.
This fix now depends on ThemeConverter::fromMap() restoring only ThemeInfo::values and UiConfiguration::updateThemes() merging those values back into a cfg-backed theme that still owns ThemeInfo::extra. Please lock that in with a round-trip test that customizes a standard value, reloads themes from settings, and verifies an extra color is still present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/ui/internal/uiconfiguration.cpp` around lines 219 - 223, Add a
regression unit test covering the persisted theme merge path: write a test that
uses ThemeConverter::fromMap() and UiConfiguration::updateThemes() to round-trip
a theme where you modify a standard value (e.g., change a named color in
ThemeInfo::values) while the original cfg-backed ThemeInfo contains an extra
color in ThemeInfo::extra, persist and reload the themes (simulate
saving/loading settings via the same map/json path used by
ThemeConverter::fromMap()), call UiConfiguration::updateThemes() to merge, and
assert that the reloaded/merged theme preserves the extra color and the
customized standard value; target the test to exercise the code paths that build
theme.values and merge into existing cfg-backed ThemeInfo so regressions in
merging extra are caught.
| theme = *it; | ||
| for (auto key : it->values.keys()) { | ||
| if (key != UNKNOWN) { | ||
| theme.values[key] = it->values[key]; |
2 participants
Resolves: Audacity issue where theme extra colors (clip colors, waveform ruler colors, etc.) are lost when a user customizes any theme property (e.g. accent color). The saved theme only contains standard values, but on reload it fully replaces the theme loaded from
.cfgfiles, wiping the extra color map. Allui.theme.extra["..."]lookups return undefined rendering affected colors as black.instead of