Themes: dark/light/system colour themes - #6
Merged
Merged
Conversation
Add pure ThemeResolver.Resolve(ThemeChoice, OsTheme) mapping to a ThemePreset, with a light-friendly DiffPalette authored alongside it. Wire :theme dark|light|system through PaletteCommandParser and ShellViewModel (mirrors :scope's report-current-vs-set pattern), and document the command in the palette help text.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
In TOML a `theme = ...` line written after a `[contexts.*]` header binds to that context table. The context parser only read organization/project/pr_scope, so a `theme` appended to the end of a config (the natural place to add it) was silently ignored — leaving the app on the default theme with no error. Fail loudly with a message pointing to the top-level fix. Adversarial-review finding on the theming feature. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRcdzgES1cBLLxEEW4jw1d
Adversarial-review low-severity nits on the theming feature: - ThemeResolver.Resolve now maps ThemeChoice exhaustively and throws on an unknown value instead of silently defaulting to dark (catches drift if a fourth choice is ever added). - `:theme system` re-issued while already on system now re-resolves against the live OS and repaints (a real refresh, e.g. to recover if the OS-follow watcher stopped); re-issuing a fixed theme stays a no-op. - The config `theme` value is trimmed, so `theme = " light "` parses instead of hard-failing startup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRcdzgES1cBLLxEEW4jw1d
Adversarial-review findings C1/C2 (+ C3/C5) on the Windows OS-follow seam. The watch loop's control flow is extracted into ThemeWatchLoop behind an IThemeWatchOps port, so the ordering and retry policy are now deterministically unit-testable on any OS; only the thin RegNotifyChangeKeyValue glue stays a native seam. - C1 (arm-before-read): the change notification is armed *before* the value is sampled, so a registry write racing the read fires the already-armed notification and is picked up next iteration instead of being lost. - C2 (retry, don't park): arm failures back off (capped exponential) and retry instead of parking the watcher forever, so a transient failure or a momentarily-absent key self-heals. Kept silent by design — a blip that auto-recovers shouldn't nag the user. - C3: Start() is a no-op after Dispose (guards Start-after-Dispose). - C5: onChanged fires only on a real theme change, so a spurious wake (any write under the watched key, e.g. an accent-colour change) no longer forces a redundant repaint. Behaviour is verifiable end-to-end only on a real Windows terminal (the native seam is un-unit-tested by design); the extracted policy has full unit coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRcdzgES1cBLLxEEW4jw1d
…atchLoop CS1674: `using var ops = new RegistryWatchOps(...)` requires the type to implement IDisposable; a Dispose method alone isn't enough for a class. The port owns native handles, so make IThemeWatchOps : IDisposable and give the test fake a no-op Dispose. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRcdzgES1cBLLxEEW4jw1d
Avoid a collection-expression first argument, which can be ambiguous between xUnit's Assert.Equal(T,T) and Assert.Equal(IEnumerable<T>,IEnumerable<T>). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRcdzgES1cBLLxEEW4jw1d
Add the tests the deep-review found missing on introduced theming behaviour: - DiffPaletteTests pins DiffPalette.Dark to its literal pre-theming colours (the existing self-comparisons were tautological and couldn't catch an edit). - ShellThemeFollowTests covers the `:theme` command's shell-side apply path (OnThemeChangeRequested) end-to-end; previously only the VM event-raise and the OS-follow path were tested. - ShellViewModelTests now asserts the "re-synced to OS" message text on a repeat `:theme system`. No production code change.
The first version drove `:theme light` (a fixed theme), so ThemeResolver ignored the OS value and the test passed even if OnThemeChangeRequested's `_themeMonitor?.Current ?? OsTheme.Unknown` read regressed. Switch to `:theme system` with a light monitor: resolution now depends on the monitor read, so a broken fallback drops to the dark palette and fails the test (verified RED by forcing os = Unknown).
Address the low-severity findings from the multi-model review: - WindowsOsThemeMonitor.RegistryWatchOps.Dispose: close the registry key before the notification event, so a still-pending one-shot RegNotify flushes before its event handle is freed (never signals a reused handle on teardown). - DiffPalette.Light: move the light diff palette here next to DiffPalette.Dark (verbatim values); ThemeResolver now references it and no longer owns colour data. Drops ThemeResolver's now-unused Terminal.Gui.Drawing import. - CobaltShell.ApplyPreset: extract the Apply+LayoutAndDraw pair shared by the :theme command and the OS-follow path so the two can't drift. - ShellViewModelTests: assert the "already <theme>" no-op and "unknown theme" error message text (previously only level/event-count were checked). No behaviour change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes cobalt's colours themeable. Stacked on PR #4 (base
feature/diff-view-enhancements); retarget tomainonce #4 merges.What
theme = "dark" | "light" | "system"inconfig.toml(defaultdark= the original look), switchable live with:theme dark|light|system.DiffPalettecolours the diff tints, so both recolour together.dark→TGDefault(byte-identical to today),light→TGLight.systemfollows the OS light/dark setting live on Windows (registryAppsUseLightTheme+RegNotifyChangeKeyValuewatch); no-op fallback on macOS/Linux (use:thememanually).How it was built
Contract-lock → 4 parallel tracks (config parse · resolver+presets+command · Windows OS monitor · TG integration) → review-fix-loop. 708 tests, 0 warnings.
Notes
ConfigurationManageris[Obsolete]but is the only runtime-theming API (MEC replacement can't own theme data yet — TG #5416); the suppression is confined toThemeService.cs.Enableis scoped toLibraryResourcesonly, so user/app TG config is never read; the library's non-theme settings were verified to equal TG's hardcoded defaults (no keybinding/behaviour change).Needs manual/PTY UAT (ADR 0010 — colours aren't unit-testable)
theme = dark/ empty config looks byte-identical to before (chrome + keybindings).:theme dark|light|systemrepaint (with the diff review open) and OS light/dark follow in a real terminal.