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))