From 4f81cd1f24e65924251f2927c7c9ecb6a41d07af Mon Sep 17 00:00:00 2001 From: Jon Kazama Date: Tue, 26 May 2026 13:03:30 +0200 Subject: [PATCH] feat(themes): add editing buffer with begin/update/save/discard --- HellionChat/Themes/ThemeRegistry.cs | 291 ++++++++++++++++++++++++++++ 1 file changed, 291 insertions(+) diff --git a/HellionChat/Themes/ThemeRegistry.cs b/HellionChat/Themes/ThemeRegistry.cs index c706e34..f734102 100644 --- a/HellionChat/Themes/ThemeRegistry.cs +++ b/HellionChat/Themes/ThemeRegistry.cs @@ -1,3 +1,4 @@ +using System.Text.Json; using HellionChat.Themes.Builtin; using Microsoft.Extensions.Logging; @@ -42,6 +43,37 @@ public sealed class ThemeRegistry private long _crossfadeStartTickMs = long.MinValue; private const int CrossfadeDurationMs = 300; + private Theme? _editingThemeBuffer; + public Theme? EditingThemeBuffer => _editingThemeBuffer; + public event Action? OnEditingBufferChanged; + + // Shared slug guard for any code path that turns a slug into a filename. + // Both SaveEditingBuffer (F1) and ImportFromPath (M6) call this so the + // path-traversal/invalid-char rules live in exactly one place. + // + // Whitespace rejection is intentional: Path.GetInvalidFileNameChars on + // POSIX only flags NUL and '/', so a slug like "foo bar" would pass the + // platform check yet break URL-safety and cross-platform portability. + // Slugs are user-visible identifiers that may end up in filenames on + // Windows + Linux, in config keys, and in JSON — keeping them whitespace- + // free dodges the whole class of "did the user mean this or that" bugs. + internal static bool IsSafeThemeSlug(string? slug) + { + if (string.IsNullOrWhiteSpace(slug)) + return false; + + foreach (var c in slug) + { + if (char.IsWhiteSpace(c)) + return false; + } + + return !slug.Contains("..", StringComparison.Ordinal) + && !slug.Contains('/') + && !slug.Contains('\\') + && slug.IndexOfAny(Path.GetInvalidFileNameChars()) < 0; + } + public ThemeRegistry(string? customThemesDir = null, ILogger? logger = null) { _logger = logger; @@ -73,6 +105,49 @@ public sealed class ThemeRegistry public Theme Active => _active; + // Read-only exposure of the configured custom themes directory. + // M6 ThemeImportExportRow opens this path via Process.Start. + public string? CustomThemesDir => _customThemesDir; + + // Read-only enumeration of all built-in theme slugs. T2 ThemePickerCategoryStep + // diffs this set against ThemePicker.CategoryMapSlugs to enforce coverage. + public IEnumerable BuiltinSlugs => _builtIns.Keys; + + // True try-pattern lookup: returns false when neither built-in nor custom + // cache holds the slug, no fallback to default. M3 ThemePicker uses this + // for card-rendering, M6 ThemeImportExportRow for fork-slug collisions. + // Cold-cache fallback: LoadCustomBySlug only reverse-iterates the + // pre-populated _customCache (see ThemeRegistry.cs:263-280). If a freshly + // imported file has not been enumerated yet (or no warm-up ran), the first + // lookup would miss silently. Drain RefreshCustomCache once on miss so the + // custom file gets picked up before the second lookup. + public bool TryGet(string slug, out Theme theme) + { + if (_builtIns.TryGetValue(slug, out var b)) + { + theme = b; + return true; + } + + var custom = LoadCustomBySlug(slug, out _); + if (custom is null) + { + // Force-enumerate the yield-iterator so _customCache picks up any + // file that landed in the themes dir since the last warm-up. + foreach (var _ in RefreshCustomCache()) { } + custom = LoadCustomBySlug(slug, out _); + } + + if (custom is not null) + { + theme = custom; + return true; + } + + theme = null!; + return false; + } + public Theme Get(string slug) { if (_builtIns.TryGetValue(slug, out var b)) @@ -102,6 +177,12 @@ public sealed class ThemeRegistry if (string.Equals(_active.Slug, slug, StringComparison.OrdinalIgnoreCase)) return; + if (_editingThemeBuffer is not null) + { + DiscardEditingBuffer(); + _logger?.LogWarning("Theme switch to {Slug} discarded unsaved edits", slug); + } + ArmCrossfade(); if (_builtIns.TryGetValue(slug, out var builtin)) @@ -142,6 +223,11 @@ public sealed class ThemeRegistry if (string.Equals(_active.Slug, slug, StringComparison.OrdinalIgnoreCase)) return; + if (_editingThemeBuffer is not null) + { + DiscardEditingBuffer(); + } + if (_builtIns.TryGetValue(slug, out var builtin)) { _active = builtin; @@ -165,6 +251,211 @@ public sealed class ThemeRegistry _activeCustomPath = null; } + public void BeginEditing(Theme source) + { + // Shallow record-with-clone: Theme.Colors gets an explicit second-level + // with-copy so ColorPicker edits never mutate the source record. Layout + // and Typography are value-record-clean (only primitive fields). Chat- + // Colors stays a reference share — fine for v1.7.0 because the editor + // never touches ChatColors. If a future cycle adds a ChatColors editor, + // BeginEditing must also clone the channel dictionary + // (ThemeChatColors holds IReadOnlyDictionary). + _editingThemeBuffer = source with + { + Colors = source.Colors with { }, + }; + } + + public void UpdateEditingBuffer(ThemeColors newColors) + { + if (_editingThemeBuffer is null) + { + return; + } + + _editingThemeBuffer = _editingThemeBuffer with { Colors = newColors }; + OnEditingBufferChanged?.Invoke(); + } + + // CALLER CONTRACT: the buffer slug must NOT collide with a built-in slug. + // Switch() prefers built-ins over custom themes with the same slug + // (ThemeRegistry.cs:107-112), so saving a custom file under a built-in + // slug persists the file but leaves the built-in active — looks green, + // behaves broken. M4 ColorPicker DrawIdleState forks built-in themes + // into a custom slug before BeginEditing, M6 ImportFromPath renames + // built-in-colliding imports to _imported. New call-sites must + // either fork first or rename to a non-built-in slug. + public bool SaveEditingBuffer(out string targetPath) + { + targetPath = string.Empty; + if (_editingThemeBuffer is null || _customThemesDir is null) + { + return false; + } + + // Slug ends up as a filename below — refuse anything that contains path + // separators, parent-directory tokens, or platform-invalid filename chars. + // Without this guard an imported theme with Slug "../../../etc/passwd" + // would let Path.Combine escape _customThemesDir entirely. Shared helper + // so M6 ImportFromPath uses the exact same rule set. + var safeSlug = _editingThemeBuffer.Slug; + if (!IsSafeThemeSlug(safeSlug)) + { + _logger?.LogWarning( + "Refusing to save editing buffer with unsafe slug {Slug}", + safeSlug + ); + return false; + } + + // Safe-by-construction: refuse any slug that collides with a built-in + // BEFORE we touch the disk. Switch() prefers built-ins over custom files + // with the same slug (ThemeRegistry.cs:107-112). Without this reject a + // mis-routed caller (or a future bug in ImportFromPath) could persist a + // custom file under a built-in slug — the file lands on disk, Switch + // keeps the built-in active, and the post-save active-slug check below + // returns false. The caller then sees "save failed" while a garbage file + // accumulates in the themes dir on every retry. M4 ColorPicker forks + // built-in themes into a custom slug before BeginEditing, M6 ImportFromPath + // renames built-in-colliding imports to _imported, so production + // paths already steer clear; this guard catches everything else. + if (_builtIns.ContainsKey(safeSlug)) + { + _logger?.LogWarning( + "Refusing to save editing buffer under built-in slug {Slug}", + safeSlug + ); + return false; + } + + try + { + targetPath = Path.Combine(_customThemesDir, $"{safeSlug}.json"); + + // Defence in depth: even after the character-level scrub above, make + // sure the resolved full path is still rooted in _customThemesDir. + // Catches edge cases like alternate data streams or symlink-style + // tricks the loader could otherwise follow. + var fullDir = Path.GetFullPath(_customThemesDir); + var fullTarget = Path.GetFullPath(targetPath); + if ( + !fullTarget.StartsWith( + fullDir + Path.DirectorySeparatorChar, + StringComparison.OrdinalIgnoreCase + ) + ) + { + _logger?.LogWarning( + "Theme save target {Target} escapes themes dir {Dir}", + fullTarget, + fullDir + ); + return false; + } + + var json = ThemeJsonWriter.Serialize(_editingThemeBuffer); + + // Atomic-replace: write to a sibling .tmp file first, then File.Move + // with overwrite=true. POSIX rename() and Windows MoveFileEx with + // MOVEFILE_REPLACE_EXISTING are both atomic on the same volume — a + // mid-write crash (power loss, Wine kill, OOM) leaves either the + // previous content or the new content on disk, never a partial JSON + // that would silently disappear at next Plugin-Start through the + // ThemeJsonLoader catch-and-continue path (ThemeRegistry.cs:319-322). + var tmpPath = targetPath + ".tmp"; + File.WriteAllText(tmpPath, json); + File.Move(tmpPath, targetPath, overwrite: true); + + // Note: the redundant `_lastActiveStamp = DateTime.MinValue` reset from + // the earlier plan-draft was removed — Switch() itself already resets + // _lastActiveStamp on the custom-theme path (ThemeRegistry.cs:124) as + // part of the active-switch, so a pre-Switch reset is overwritten anyway. + + // RefreshCustomCache is a yield-iterator (ThemeRegistry.cs:282) — a bare + // call would build the iterator but never enumerate it, so the cache + // side-effect (_customCache[key] = (theme, stamp)) would never run. + // Force-enumerate so the subsequent Switch() finds the freshly saved file. + foreach (var _ in RefreshCustomCache()) { } + + // Use the sanitised slug for Switch() too — the buffer's raw Slug + // already passed the guard, but staying on safeSlug keeps the lookup + // value consistent with the on-disk filename we just wrote. + var targetSlug = safeSlug; + + // CRITICAL: null the buffer BEFORE Switch() so the Switch-Guard + // (step 3d) does not fire on our own save-internal Switch call. + // Without this pre-nullify the guard would log a misleading + // "discarded unsaved edits" warning on every save and run + // DiscardEditingBuffer twice (once in the guard, once at method end). + _editingThemeBuffer = null; + + Switch(targetSlug); + + // Same-slug in-place edit: Switch() hits the Same-Slug-Noop-Return + // (ThemeRegistry.cs:102-103) and leaves _active pointing at the + // PRE-edit Theme reference. The newly saved colours would only + // surface on the next RefreshActiveIfStale tick (1Hz-throttled, + // up to ~1s lag). Force-pull the freshly-cached Theme directly so + // the post-Save UI sees the edit in the next frame. + if (string.Equals(_active.Slug, targetSlug, StringComparison.OrdinalIgnoreCase)) + { + var reloaded = LoadCustomBySlug(targetSlug, out _); + if (reloaded is not null) + { + reloaded.RecomputeAbgrCache(); + _active = reloaded; + } + } + + // Switch() falls back to DefaultSlug when neither built-in nor custom + // matches (ThemeRegistry.cs:128-132). Verify we actually landed on the + // intended theme before reporting success — a silent fallback to the + // default would otherwise mask a save that did persist the file but + // failed to become active (e.g. cache race on slow disks). + if (!string.Equals(_active.Slug, targetSlug, StringComparison.OrdinalIgnoreCase)) + { + _logger?.LogWarning( + "SaveEditingBuffer persisted {Path} but Switch landed on {Active} instead of {Target}", + targetPath, + _active.Slug, + targetSlug + ); + return false; + } + + return true; + } + catch (IOException ex) + { + _logger?.LogWarning(ex, "I/O error saving editing buffer to {Path}", targetPath); + return false; + } + catch (UnauthorizedAccessException ex) + { + _logger?.LogWarning(ex, "Access denied saving editing buffer to {Path}", targetPath); + return false; + } + catch (JsonException ex) + { + // ThemeJsonWriter.Serialize could in principle throw on malformed + // theme graphs; keep this granular so transient I/O and serialisation + // failures don't get lumped together with future structural bugs. + // Requires `using System.Text.Json;` at the top of ThemeRegistry.cs + // — verify before saving and add the import if it's not yet present. + _logger?.LogWarning( + ex, + "JSON serialisation failed for editing buffer at {Path}", + targetPath + ); + return false; + } + } + + public void DiscardEditingBuffer() + { + _editingThemeBuffer = null; + } + // Captures the AbgrCache snapshot that PushGlobal should fade FROM. // If a crossfade is already mid-flight (second Switch within 300ms), // the current lerped state replaces the snapshot -- the next fade