From ad2e73e35a77f8116fdfe81092e7065444ba02eb Mon Sep 17 00:00:00 2001 From: Christoph Wille Date: Wed, 12 Aug 2026 20:20:41 +0200 Subject: [PATCH] Snapshot original highlighting colours per colour instance Review of #3989 pointed out that guarding registration on the theme-aware marker conflates "XSHD opts out", "already themed" and "already registered", and leans on two non-contractual AvaloniaEdit details (the delay-load wrapper's Properties forwarding and its materialize-on-touch behaviour). Keying the pristine-colour snapshots by colour instance (ConditionalWeakTable) instead of by definition makes the in-place theming idempotent no matter how many definition identities expose the colours, so correctness no longer depends on registration order or the marker; the guard remains only to honour the XSHD opt-out and to skip redundant list entries. ApplyHighlightingColors is now private so nothing can set the marker outside a registration. Also from review: the new tests move to a uniquely named fixture (the old name collided with Themes/ThemeAwareHighlightingColorizerTests), gain coverage of the Light/Dark switch path after a dark startup, and the cache characterization asserts the reconverted content instead of relying on inert theme switches. Assisted-by: Claude:claude-fable-5:Claude Code --- .../DoubleDarkConversionTests.cs} | 69 ++++++++++++------- .../ThemeAwareHighlightingColorizer.cs | 4 +- ILSpy/Themes/ThemeManager.cs | 51 +++++++------- 3 files changed, 75 insertions(+), 49 deletions(-) rename ILSpy.Tests/{ThemeAwareHighlightingColorizerTests.cs => Themes/DoubleDarkConversionTests.cs} (77%) diff --git a/ILSpy.Tests/ThemeAwareHighlightingColorizerTests.cs b/ILSpy.Tests/Themes/DoubleDarkConversionTests.cs similarity index 77% rename from ILSpy.Tests/ThemeAwareHighlightingColorizerTests.cs rename to ILSpy.Tests/Themes/DoubleDarkConversionTests.cs index c83f744d8..d852218a6 100644 --- a/ILSpy.Tests/ThemeAwareHighlightingColorizerTests.cs +++ b/ILSpy.Tests/Themes/DoubleDarkConversionTests.cs @@ -22,28 +22,25 @@ using Avalonia.Headless.NUnit; using Avalonia.Media; using AvaloniaEdit.Highlighting; -using AvaloniaEdit.Rendering; using AwesomeAssertions; -using ICSharpCode.ILSpy; using ICSharpCode.ILSpy.TextView; using ICSharpCode.ILSpy.Themes; using NUnit.Framework; -namespace ICSharpCode.ILSpy.Tests; +namespace ICSharpCode.ILSpy.Tests.Themes; /// /// Guards the ThemeManager / ThemeAwareHighlightingColorizer contract against double dark /// conversion. ThemeManager darkens a REGISTERED definition's named colours in place; the -/// colorizer per-paint-remaps colours of UNREGISTERED definitions. Both mechanisms active at -/// once on the same definition converts colours twice (washed-out output), so the colorizer -/// must observe a registration that happens after it was constructed, and must not serve -/// cached conversions computed from colour values that a theme switch has since replaced. +/// colorizer per-paint-remaps colours of UNREGISTERED definitions. Both mechanisms active +/// at once on the same definition -- or the in-place rewrite running twice over one set of +/// colours -- converts colours twice and washes the palette out. /// [TestFixture] -public class ThemeAwareHighlightingColorizerTests +public class DoubleDarkConversionTests { // Minimal definition stub: real Properties (ThemeManager marks theme-awareness there) // and real named colours (registration snapshots and rewrites them in place). @@ -120,7 +117,7 @@ public class ThemeAwareHighlightingColorizerTests settings.Theme = "Dark"; var color = MakeColor("String", Colors.Red); - var pristine = (HighlightingColor)color.Clone(); + var pristine = color.Clone(); var wrapper = new LazyLoadedDefinition(new StubHighlightingDefinition(color)); // HighlightingService.GetByExtension registers what the HighlightingManager @@ -140,6 +137,36 @@ public class ThemeAwareHighlightingColorizerTests } } + [AvaloniaTest] + public void ThemeSwitchRestoresPristineColorsAfterDarkStartup() + { + var settings = AttachThemeSettings(); + try + { + settings.Theme = "Dark"; + + var color = MakeColor("String", Colors.Red); + var pristine = color.Clone(); + var wrapper = new LazyLoadedDefinition(new StubHighlightingDefinition(color)); + ThemeManager.Current.RegisterThemableDefinition(wrapper); + + // Every later switch must be computed from the pristine snapshot, not from + // whatever the colour held after the previous rewrite: Light restores the + // .xshd values exactly, and Dark again yields the single conversion. + settings.Theme = "Light"; + color.Should().Be(pristine, + "switching to Light must restore the original .xshd colours exactly"); + + settings.Theme = "Dark"; + color.Should().Be(ThemeManager.GetColorForDarkTheme(pristine), + "switching back to Dark must convert the pristine colours exactly once"); + } + finally + { + settings.Theme = "Light"; + } + } + [AvaloniaTest] public void RegistrationAfterConstructionDisablesPerPaintRemap() { @@ -170,7 +197,7 @@ public class ThemeAwareHighlightingColorizerTests } [AvaloniaTest] - public void DarkCacheDoesNotSurviveThemeSwitch() + public void DarkConversionCacheMissesAfterInPlaceRecolor() { var settings = AttachThemeSettings(); try @@ -180,21 +207,17 @@ public class ThemeAwareHighlightingColorizerTests var colorizer = new ThemeAwareHighlightingColorizer(definition); settings.Theme = "Dark"; - var beforeSwitch = colorizer.GetEffectiveColor(color); - - // The colour's content changes in place (as ThemeManager does for managed - // definitions) with no paint in between. The colorizer's conversion cache is - // keyed by HighlightingColor's content-based equality, so the changed content - // must miss the cache and reconvert -- serving the conversion of the old values - // here would paint stale colours. If HighlightingColor ever moved to reference - // equality, this test goes red and the cache needs an explicit flush instead. + colorizer.GetEffectiveColor(color); // primes the cache with the conversion of Red + + // The colour's content changes in place with no paint in between. The colorizer's + // conversion cache is keyed by HighlightingColor's content-based equality, so the + // changed content must miss the cache and reconvert -- serving the conversion of + // the old values would paint stale colours. If HighlightingColor ever moved to + // reference equality, this goes red and the cache needs an explicit flush instead. color.Foreground = new SimpleHighlightingBrush(Colors.Lime); - settings.Theme = "Light"; - settings.Theme = "Dark"; - var afterSwitch = colorizer.GetEffectiveColor(color); - afterSwitch.Should().NotBeSameAs(beforeSwitch, - "conversions cached from superseded colour values must not be served"); + colorizer.GetEffectiveColor(color).Should().Be(ThemeManager.GetColorForDarkTheme(color), + "the conversion served after an in-place recolour must be computed from the new colour values"); } finally { diff --git a/ILSpy/TextView/ThemeAwareHighlightingColorizer.cs b/ILSpy/TextView/ThemeAwareHighlightingColorizer.cs index a34c13d61..03c67f827 100644 --- a/ILSpy/TextView/ThemeAwareHighlightingColorizer.cs +++ b/ILSpy/TextView/ThemeAwareHighlightingColorizer.cs @@ -37,7 +37,9 @@ namespace ICSharpCode.ILSpy.TextView /// The remapped colours are cached per source ; the /// cache key is content-based (HighlightingColor overrides Equals/GetHashCode), so an /// in-place recolour of a source colour misses the cache and reconverts instead of - /// serving a conversion of the old values. + /// serving a conversion of the old values. The entry keyed by the old content is + /// stranded rather than replaced -- bounded by the definition's colour count per + /// recolour, and it dies with the colorizer. /// public sealed class ThemeAwareHighlightingColorizer : HighlightingColorizer { diff --git a/ILSpy/Themes/ThemeManager.cs b/ILSpy/Themes/ThemeManager.cs index 357bd130f..57f7485e5 100644 --- a/ILSpy/Themes/ThemeManager.cs +++ b/ILSpy/Themes/ThemeManager.cs @@ -19,6 +19,7 @@ using System; using System.Collections.Generic; using System.ComponentModel; +using System.Runtime.CompilerServices; using Avalonia; using Avalonia.Media; @@ -40,11 +41,21 @@ namespace ICSharpCode.ILSpy.Themes // declares its own dark palette in two variants). const string IsThemeAwareKey = "ILSpy.IsThemeAware"; - // Highlighting definitions whose named colours we re-theme on every theme switch, plus a - // snapshot of each definition's ORIGINAL (light, .xshd-default) colours so switching back - // to Light restores them exactly. Keyed by colour name within each definition. + // Highlighting definitions whose named colours we re-theme on every theme switch. readonly List themableDefinitions = new(); - readonly Dictionary> originalColors = new(); + + // The ORIGINAL (light, .xshd-default) values of every colour ever themed, so switching + // back to Light restores them exactly. Keyed by colour INSTANCE: ConditionalWeakTable + // compares by reference (unaffected by HighlightingColor's content-based Equals) and + // lets snapshots die with their definition. Keying by instance rather than by definition + // makes theming idempotent across definition identities -- AvaloniaEdit's + // HighlightingManager hands out a delay-loaded wrapper that forwards to the inner + // definition ILSpy's load callback registers, so the same colours can arrive here under + // two definition objects. A per-definition snapshot taken via the second identity would + // capture already-dark values as "originals" and dark-convert them again (washed-out + // colours whenever dark is active at first touch); the per-instance snapshot is taken + // exactly once, before the first rewrite, no matter which identity triggers it. + readonly ConditionalWeakTable originalColors = new(); public static ThemeManager Current { get; } = new(); @@ -108,15 +119,13 @@ namespace ICSharpCode.ILSpy.Themes public void RegisterThemableDefinition(IHighlightingDefinition definition) { ArgumentNullException.ThrowIfNull(definition); - // An already-theme-aware definition must not be registered again under a second - // identity. HighlightingManager hands out a delay-loaded WRAPPER whose members - // forward to the inner definition our Load() callback registers, so registering - // the wrapper too would snapshot the shared colours AFTER the inner registration - // already themed them -- and, when dark is active before the first touch (dark - // theme preselected at startup), dark-convert the already-dark values a second - // time, washing the palette out. Reading IsThemeAware here also forces the - // wrapper to materialize, so the check observes the inner registration's marker. - // This equally respects definitions whose XSHD opts out via ILSpy.IsThemeAware. + // Skip definitions that are already theme-aware: either their XSHD declares + // ILSpy.IsThemeAware (they ship theme-correct colours we must not rewrite), or a + // previous registration themed them -- typically the inner definition behind the + // delay-loaded wrapper HighlightingManager returns, which materializes (and + // registers itself) when the marker is read here. Re-theming would be harmless + // either way (the per-instance colour snapshots make it idempotent); there is + // simply nothing to do, and no reason to track a second identity in the list. if (IsThemeAware(definition)) return; if (!themableDefinitions.Contains(definition)) @@ -132,22 +141,14 @@ namespace ICSharpCode.ILSpy.Themes /// algorithmic conversion elsewhere. Marks the definition theme-aware so the per-paint /// colorizer doesn't additionally remap it. /// - public void ApplyHighlightingColors(IHighlightingDefinition definition) + void ApplyHighlightingColors(IHighlightingDefinition definition) { - ArgumentNullException.ThrowIfNull(definition); - if (!originalColors.TryGetValue(definition, out var snapshot)) - { - snapshot = new Dictionary(); - foreach (var color in definition.NamedHighlightingColors) - snapshot[color.Name] = color.Clone(); - originalColors[definition] = snapshot; - } - var darkPalette = definition.Name == "C#" ? SyntaxColorPalettes.CSharpDark : null; foreach (var color in definition.NamedHighlightingColors) { - if (!snapshot.TryGetValue(color.Name, out var original)) - continue; + // Snapshot before the first rewrite; every later visit gets the stored + // pristine values back, never the colour's current (possibly dark) content. + var original = originalColors.GetValue(color, static c => c.Clone()); if (IsDarkTheme) { if (darkPalette is not null && darkPalette.TryGetValue(color.Name, out var syntaxColor))