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

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;
 
 /// <summary>
 /// 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.
 /// </summary>
 [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
 		{

ILSpy/TextView/ThemeAwareHighlightingColorizer.cs

@@ -37,7 +37,9 @@ namespace ICSharpCode.ILSpy.TextView
 	/// The remapped colours are cached per source <see cref="HighlightingColor"/>; 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.
 	/// </summary>
 	public sealed class ThemeAwareHighlightingColorizer : HighlightingColorizer
 	{

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<IHighlightingDefinition> themableDefinitions = new();
-		readonly Dictionary<IHighlightingDefinition, Dictionary<string, HighlightingColor>> 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<HighlightingColor, HighlightingColor> 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.
 		/// </summary>
-		public void ApplyHighlightingColors(IHighlightingDefinition definition)
+		void ApplyHighlightingColors(IHighlightingDefinition definition)
 		{
-			ArgumentNullException.ThrowIfNull(definition);
-			if (!originalColors.TryGetValue(definition, out var snapshot))
-			{
-				snapshot = new Dictionary<string, HighlightingColor>();
-				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))