From 5f5b0871672f9baf5e7005841812401ae4739f86 Mon Sep 17 00:00:00 2001 From: Christoph Wille Date: Wed, 12 Aug 2026 17:36:45 +0200 Subject: [PATCH] Check highlighting theme-awareness per paint, not at colorizer creation ThemeManager and ThemeAwareHighlightingColorizer split dark mode between them: the manager darkens a registered definition's named colours in place, the colorizer per-paint-remaps colours of unregistered definitions. Running both on one definition converts every colour twice and washes the palette out. The colorizer captured IsThemeAware once in its constructor, so a definition registered after the colorizer was created would be double-converted from then on. Today every colorizer is created after registration (HighlightingService registers inside GetByExtension/Load before returning), but that is a calling convention, not an invariant; reading the flag per paint removes the ordering dependency. The colorizer's dark-conversion cache needs no matching flush: its keys use HighlightingColor's content-based equality, so recolouring a source colour in place changes its hash and the lookup misses instead of serving a conversion of the old values. A characterization test pins that, so an equality-semantics change in AvaloniaEdit shows up as a red test rather than as stale colours. Assisted-by: Claude:claude-fable-5:Claude Code --- .../ThemeAwareHighlightingColorizerTests.cs | 140 ++++++++++++++++++ .../ThemeAwareHighlightingColorizer.cs | 31 ++-- 2 files changed, 162 insertions(+), 9 deletions(-) create mode 100644 ILSpy.Tests/ThemeAwareHighlightingColorizerTests.cs diff --git a/ILSpy.Tests/ThemeAwareHighlightingColorizerTests.cs b/ILSpy.Tests/ThemeAwareHighlightingColorizerTests.cs new file mode 100644 index 000000000..05db23704 --- /dev/null +++ b/ILSpy.Tests/ThemeAwareHighlightingColorizerTests.cs @@ -0,0 +1,140 @@ +// Copyright (c) 2026 Christoph Wille +// +// Permission is hereby granted, free of charge, to any person obtaining a copy of this +// software and associated documentation files (the "Software"), to deal in the Software +// without restriction, including without limitation the rights to use, copy, modify, merge, +// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons +// to whom the Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all copies or +// substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, +// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR +// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE +// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + +using System.Collections.Generic; + +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; + +/// +/// 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. +/// +[TestFixture] +public class ThemeAwareHighlightingColorizerTests +{ + // Minimal definition stub: real Properties (ThemeManager marks theme-awareness there) + // and real named colours (registration snapshots and rewrites them in place). + sealed class StubHighlightingDefinition : IHighlightingDefinition + { + readonly List colors; + + public StubHighlightingDefinition(params HighlightingColor[] colors) + { + this.colors = new List(colors); + } + + public string Name => "ColorizerTestStub"; + public HighlightingRuleSet MainRuleSet { get; } = new(); + public IEnumerable NamedHighlightingColors => colors; + public IDictionary Properties { get; } = new Dictionary(); + public HighlightingRuleSet GetNamedRuleSet(string name) => MainRuleSet; + public HighlightingColor GetNamedColor(string name) => colors.Find(c => c.Name == name)!; + } + + static HighlightingColor MakeColor(string name, Color foreground) + => new() { Name = name, Foreground = new SimpleHighlightingBrush(foreground) }; + + // Drives ThemeManager through its public surface; restores Light afterwards so the + // process-lived singleton doesn't leak dark mode into other tests. + static SessionSettings AttachThemeSettings() + { + var settings = new SessionSettings(); + ThemeManager.Current.Attach(settings); + return settings; + } + + [AvaloniaTest] + public void RegistrationAfterConstructionDisablesPerPaintRemap() + { + var settings = AttachThemeSettings(); + try + { + var color = MakeColor("Keyword", Colors.Blue); + var definition = new StubHighlightingDefinition(color); + var colorizer = new ThemeAwareHighlightingColorizer(definition); + + settings.Theme = "Dark"; + + // Unregistered definition: the colorizer owns the dark conversion. + colorizer.GetEffectiveColor(color).Should().NotBeSameAs(color, + "an unregistered definition's colours must be remapped per paint in dark mode"); + + // Late registration: ThemeManager now darkens the live colours in place. If the + // colorizer kept remapping on top of that, every colour would be converted twice. + ThemeManager.Current.RegisterThemableDefinition(definition); + + colorizer.GetEffectiveColor(color).Should().BeSameAs(color, + "once the definition is theme-managed, a second per-paint conversion would double-darken"); + } + finally + { + settings.Theme = "Light"; + } + } + + [AvaloniaTest] + public void DarkCacheDoesNotSurviveThemeSwitch() + { + var settings = AttachThemeSettings(); + try + { + var color = MakeColor("String", Colors.Red); + var definition = new StubHighlightingDefinition(color); + 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. + 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"); + } + finally + { + settings.Theme = "Light"; + } + } +} diff --git a/ILSpy/TextView/ThemeAwareHighlightingColorizer.cs b/ILSpy/TextView/ThemeAwareHighlightingColorizer.cs index 34d13db51..a34c13d61 100644 --- a/ILSpy/TextView/ThemeAwareHighlightingColorizer.cs +++ b/ILSpy/TextView/ThemeAwareHighlightingColorizer.cs @@ -27,27 +27,40 @@ namespace ICSharpCode.ILSpy.TextView { /// /// Wraps AvaloniaEdit's default colorizer so colours flip when the active theme is - /// Dark. The decision is per-paint and reads - /// directly, so a theme switch followed by an editor redraw renders in the new - /// palette without needing a second colorizer instance. The remapped colours are - /// cached per source instance. + /// Dark. The whole decision is per-paint: it reads + /// and directly, so a theme switch -- or a + /// definition getting registered with the theme manager after this colorizer was + /// created -- takes effect on the next redraw without needing a second colorizer + /// instance. Checking theme-awareness per paint matters for correctness, not just + /// freshness: darkens a registered definition's named + /// colours in place, so remapping them here a second time would wash the palette out. + /// 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. /// public sealed class ThemeAwareHighlightingColorizer : HighlightingColorizer { readonly Dictionary darkColors = new(); - readonly bool definitionIsThemeAware; + readonly IHighlightingDefinition definition; public ThemeAwareHighlightingColorizer(IHighlightingDefinition highlightingDefinition) : base(highlightingDefinition) { - definitionIsThemeAware = ThemeManager.Current.IsThemeAware(highlightingDefinition); + definition = highlightingDefinition; } protected override void ApplyColorToElement(VisualLineElement element, HighlightingColor color) { - if (!definitionIsThemeAware && ThemeManager.Current.IsDarkTheme) - color = GetCachedDarkColor(color); - base.ApplyColorToElement(element, color); + base.ApplyColorToElement(element, GetEffectiveColor(color)); + } + + // The per-paint colour decision, split out so tests can drive it without a TextView. + internal HighlightingColor GetEffectiveColor(HighlightingColor color) + { + if (ThemeManager.Current.IsDarkTheme && !ThemeManager.Current.IsThemeAware(definition)) + return GetCachedDarkColor(color); + return color; } HighlightingColor GetCachedDarkColor(HighlightingColor lightColor)