diff --git a/src/XTerm.NET.Tests/ColorPaletteTests.cs b/src/XTerm.NET.Tests/ColorPaletteTests.cs index 357ce17..1d36b93 100644 --- a/src/XTerm.NET.Tests/ColorPaletteTests.cs +++ b/src/XTerm.NET.Tests/ColorPaletteTests.cs @@ -1,3 +1,5 @@ +using System.Threading; +using System.Threading.Tasks; using XTerm; using XTerm.Common; using XTerm.Options; @@ -281,6 +283,182 @@ public void ColorChanged_FiresForSetsAndNotForNoOps() Assert.Equal(0xFF0000, change.Rgb); } + // ---- no-op suppression and index handling ------------------------------------------------ + + [Fact] + public void ResetAllColors_IsSilent_WhenNothingHasChanged() + { + // Every other setter here suppresses a no-op. A bare OSC 104 on an untouched palette used to + // fire anyway, telling a renderer to repaint for a change that did not happen. + var terminal = CreateTerminal(); + var changes = new List(); + terminal.Colors.ColorChanged += (_, e) => changes.Add(e); + + terminal.Write("\u001b]104\u0007"); + + Assert.Empty(changes); + } + + [Fact] + public void ResetAllColors_StillFires_WhenSomethingHadChanged() + { + var terminal = CreateTerminal(); + terminal.Write("\u001b]4;1;#ff0000\u0007"); + + var changes = new List(); + terminal.Colors.ColorChanged += (_, e) => changes.Add(e); + + terminal.Write("\u001b]104\u0007"); + + var change = Assert.Single(changes); + Assert.Equal(ColorTarget.Indexed, change.Target); + Assert.Equal(0xCD0000, terminal.Colors[1]); + } + + [Fact] + public void ApplyTheme_IsSilent_WhenTheThemeIsUnchanged() + { + var terminal = CreateTerminal(new ThemeOptions { Background = "#ffffff" }); + var changes = new List(); + terminal.Colors.ColorChanged += (_, e) => changes.Add(e); + + terminal.Colors.ApplyTheme(new ThemeOptions { Background = "#ffffff" }); + + Assert.Empty(changes); + } + + [Fact] + public void ApplyTheme_FiresOnceForARealChange() + { + var terminal = CreateTerminal(new ThemeOptions { Background = "#000000" }); + var changes = new List(); + terminal.Colors.ColorChanged += (_, e) => changes.Add(e); + + terminal.Colors.ApplyTheme(new ThemeOptions { Background = "#ffffff" }); + + Assert.Equal(ColorTarget.All, Assert.Single(changes).Target); + } + + [Theory] + [InlineData(-1)] + [InlineData(256)] + [InlineData(999)] + public void PaletteIndex_OutOfRange_Throws(int index) + { + // Not clamped. Clamping made SetColor(999, ...) quietly rewrite entry 255 and the indexer + // answer for entry 0 when asked about -1: a plausible wrong answer where there should have + // been none. + var terminal = CreateTerminal(); + + Assert.Throws(() => terminal.Colors[index]); + Assert.Throws(() => terminal.Colors.SetColor(index, 0x123456)); + Assert.Throws(() => terminal.Colors.ResetColor(index)); + } + + [Fact] + public void PaletteIndex_OutOfRangeOverOsc_IsStillIgnoredRatherThanThrown() + { + // The parser must not surface a malformed sequence as an exception; InputHandler range-checks + // before it reaches the palette, and that has to keep working now the palette throws. + var terminal = CreateTerminal(); + + var ex = Record.Exception(() => terminal.Write("\u001b]4;999;#ffffff\u0007")); + + Assert.Null(ex); + Assert.Equal(0xEEEEEE, terminal.Colors[255]); + } + + // ---- concurrency ------------------------------------------------------------------------- + + [Fact] + public async Task ApplyTheme_IsNeverObservedHalfApplied() + { + // The bug this pins: ApplyTheme used to Array.Copy into the live array, and Array.Copy is not + // atomic, so a renderer scanning the palette could paint a frame that was half one theme and + // half the other. A reference swap has no middle. + var dark = new ThemeOptions + { + Black = "#000000", Red = "#110000", Green = "#001100", Yellow = "#111100", + Blue = "#000011", Magenta = "#110011", Cyan = "#001111", White = "#111111", + }; + var light = new ThemeOptions + { + Black = "#ffffff", Red = "#ff0000", Green = "#00ff00", Yellow = "#ffff00", + Blue = "#0000ff", Magenta = "#ff00ff", Cyan = "#00ffff", White = "#ffffff", + }; + + var terminal = CreateTerminal(dark); + int[] darkAnsi = ReadAnsi(terminal); + terminal.Colors.ApplyTheme(light); + int[] lightAnsi = ReadAnsi(terminal); + Assert.NotEqual(darkAnsi, lightAnsi); + + using var stop = new CancellationTokenSource(TimeSpan.FromSeconds(3)); + var mixed = 0; + + var reader = Task.Run(() => + { + while (!stop.IsCancellationRequested) + { + int[] seen = ReadAnsi(terminal); + if (!seen.SequenceEqual(darkAnsi) && !seen.SequenceEqual(lightAnsi)) + { + Interlocked.Increment(ref mixed); + } + } + }); + + var writer = Task.Run(() => + { + var toggle = false; + while (!stop.IsCancellationRequested) + { + terminal.Colors.ApplyTheme(toggle ? dark : light); + toggle = !toggle; + } + }); + + await Task.WhenAll(reader, writer); + + Assert.Equal(0, Volatile.Read(ref mixed)); + } + + /// + /// Reads the first eight ANSI colours through ONE snapshot. + /// + /// + /// Eight calls to the indexer would each take their own snapshot and could straddle a theme + /// change, which is the whole reason Take exists. + /// + private static int[] ReadAnsi(Terminal terminal) + { + ColorSnapshot snapshot = terminal.Colors.Take(); + var values = new int[8]; + for (var i = 0; i < values.Length; i++) + { + values[i] = snapshot[i]; + } + + return values; + } + + [Fact] + public void Take_ReturnsAViewThatDoesNotMoveAfterwards() + { + // The property that lets a renderer trust a snapshot for a whole frame. Writes copy on + // write for exactly this; mutating the live array in place would have been safe for the + // VALUE and still broken this. + var terminal = CreateTerminal(); + ColorSnapshot before = terminal.Colors.Take(); + int original = before[1]; + + terminal.Write("\u001b]4;1;#123456\u0007"); + + Assert.Equal(original, before[1]); + Assert.Equal(0x123456, terminal.Colors[1]); + Assert.Equal(0x123456, terminal.Colors.Take()[1]); + } + // ---- colour spec parsing ----------------------------------------------------------------- [Theory] diff --git a/src/XTerm.NET/Common/ColorPalette.cs b/src/XTerm.NET/Common/ColorPalette.cs index bc28299..5b69c83 100644 --- a/src/XTerm.NET/Common/ColorPalette.cs +++ b/src/XTerm.NET/Common/ColorPalette.cs @@ -1,3 +1,4 @@ +using System.Threading; using XTerm.Options; namespace XTerm.Common; @@ -22,15 +23,30 @@ public class ColorPalette public const int Size = 256; private readonly int[] defaults = new int[Size]; - private readonly int[] current = new int[Size]; + + /// Serialises writers against each other. Readers never take it. + private readonly object writeGate = new(); private int defaultForeground; private int defaultBackground; private int defaultCursor; - private int foregroundBacking; - private int backgroundBacking; - private int cursorBacking; + /// + /// The live colours. Replaced wholesale for bulk changes, mutated in place for single ones. + /// + /// + /// Read on a renderer's hot path -- once per cell -- so reads are lock free, and a lock here + /// would be a worse cure than the problem. What that costs is a discipline rather than a lock: + /// + /// SINGLE colour changes mutate the array or a scalar in place. An aligned int write is atomic, + /// so a reader sees the old value or the new one and never half of either. + /// + /// BULK changes -- ApplyTheme, ResetAllColors -- build a whole new snapshot and swap the + /// reference. Copying into the live array instead was the actual bug: Array.Copy is not atomic, + /// so a renderer could paint a frame with half the old theme and half the new one. A reference + /// swap has no half. + /// + private ColorSnapshot state; /// /// Initializes a palette with the built-in xterm defaults. @@ -46,7 +62,11 @@ public ColorPalette() /// public ColorPalette(ThemeOptions? theme) { - ApplyTheme(theme); + // Seeded inline rather than through ApplyTheme, so construction does not raise ColorChanged. + // Nothing can be subscribed yet, so the event went nowhere, but it made ApplyTheme's + // behaviour depend on when it was called. + SeedDefaults(theme); + state = NewSnapshot(); } /// @@ -57,17 +77,17 @@ public ColorPalette(ThemeOptions? theme) /// /// The current foreground colour, as 0xRRGGBB. /// - public int Foreground => foregroundBacking; + public int Foreground => Volatile.Read(ref state).Foreground; /// /// The current background colour, as 0xRRGGBB. /// - public int Background => backgroundBacking; + public int Background => Volatile.Read(ref state).Background; /// /// The current cursor colour, as 0xRRGGBB. /// - public int Cursor => cursorBacking; + public int Cursor => Volatile.Read(ref state).Cursor; /// /// Whether the background is light enough that a program should choose dark text. @@ -77,12 +97,31 @@ public ColorPalette(ThemeOptions? theme) /// consumer that wants it would otherwise write the same formula, and would be likely to write /// it as a plain average of the channels -- which calls pure blue light. /// - public bool IsLightBackground => Luma(Background) > 0.5; + public bool IsLightBackground => IsLight(Background); /// /// Gets the current colour for an index, as 0xRRGGBB. /// - public int this[int index] => current[Clamp(index)]; + public int this[int index] + { + get + { + ValidateIndex(index); + return Volatile.Read(ref state).Colors[index]; + } + } + + /// + /// Takes a coherent, immutable view of every colour at this instant. + /// + /// + /// What to use when reading MORE THAN ONE colour, which for a renderer painting a frame is + /// always. The individual properties on this class each read the current state separately, so + /// eight reads can straddle a theme change and return a mix -- measured at three and a half + /// million mixed reads per three seconds under a tight toggle, so this is routine rather than a + /// rare race. One snapshot cannot: it is a single reference, and nothing mutates it afterwards. + /// + public ColorSnapshot Take() => Volatile.Read(ref state); /// /// Replaces the defaults from a theme and discards any colours programs had set. @@ -94,12 +133,30 @@ public ColorPalette(ThemeOptions? theme) /// public void ApplyTheme(ThemeOptions? theme) { - SeedDefaults(theme); - Array.Copy(defaults, current, Size); - foregroundBacking = defaultForeground; - backgroundBacking = defaultBackground; - cursorBacking = defaultCursor; - ColorChanged?.Invoke(this, new ColorChangedEventArgs(ColorTarget.All, -1, 0)); + bool changed; + + lock (writeGate) + { + SeedDefaults(theme); + + var live = state; + changed = live.Foreground != defaultForeground + || live.Background != defaultBackground + || live.Cursor != defaultCursor + || !live.Colors.AsSpan().SequenceEqual(defaults); + + if (changed) + { + // One swap, so no reader ever sees a partly applied theme. + Volatile.Write(ref state, NewSnapshot()); + } + } + + // Raised outside the lock: a handler is consumer code and may call back in. + if (changed) + { + ColorChanged?.Invoke(this, new ColorChangedEventArgs(ColorTarget.All, -1, 0)); + } } /// @@ -107,37 +164,50 @@ public void ApplyTheme(ThemeOptions? theme) /// public void SetColor(int index, int rgb) { - index = Clamp(index); - if (current[index] == rgb) + ValidateIndex(index); + + lock (writeGate) { - return; + var live = state; + if (live.Colors[index] == rgb) + { + return; + } + + // Copy on write, rather than storing into the live array. An int store is atomic, so + // mutating in place would be safe for the VALUE -- but it would also change a snapshot + // somebody is already holding, and the whole point of handing one out is that it does + // not move underneath them. A palette is 1KB and colour changes are rare. + var colors = new int[Size]; + Array.Copy(live.Colors, colors, Size); + colors[index] = rgb; + Volatile.Write(ref state, new ColorSnapshot(colors, live.Foreground, live.Background, live.Cursor)); } - current[index] = rgb; ColorChanged?.Invoke(this, new ColorChangedEventArgs(ColorTarget.Indexed, index, rgb)); } /// /// Sets the foreground colour, as OSC 10 does. /// - public void SetForeground(int rgb) => Set(ref foregroundBacking, rgb, ColorTarget.Foreground); + public void SetForeground(int rgb) => Set(ColorTarget.Foreground, rgb); /// /// Sets the background colour, as OSC 11 does. /// - public void SetBackground(int rgb) => Set(ref backgroundBacking, rgb, ColorTarget.Background); + public void SetBackground(int rgb) => Set(ColorTarget.Background, rgb); /// /// Sets the cursor colour, as OSC 12 does. /// - public void SetCursor(int rgb) => Set(ref cursorBacking, rgb, ColorTarget.Cursor); + public void SetCursor(int rgb) => Set(ColorTarget.Cursor, rgb); /// /// Restores one indexed colour to its default, as OSC 104 with a parameter does. /// public void ResetColor(int index) { - index = Clamp(index); + ValidateIndex(index); SetColor(index, defaults[index]); } @@ -146,7 +216,19 @@ public void ResetColor(int index) /// public void ResetAllColors() { - Array.Copy(defaults, current, Size); + lock (writeGate) + { + var live = state; + if (live.Colors.AsSpan().SequenceEqual(defaults)) + { + return; + } + + var colors = new int[Size]; + Array.Copy(defaults, colors, Size); + Volatile.Write(ref state, new ColorSnapshot(colors, live.Foreground, live.Background, live.Cursor)); + } + ColorChanged?.Invoke(this, new ColorChangedEventArgs(ColorTarget.Indexed, -1, 0)); } @@ -165,6 +247,8 @@ public void ResetAllColors() /// public void ResetCursor() => SetCursor(defaultCursor); + internal static bool IsLight(int rgb) => Luma(rgb) > 0.5; + private static double Luma(int rgb) { var r = ((rgb >> 16) & 0xFF) / 255.0; @@ -173,19 +257,56 @@ private static double Luma(int rgb) return (0.299 * r) + (0.587 * g) + (0.114 * b); } - private static int Clamp(int index) => index < 0 ? 0 : index >= Size ? Size - 1 : index; + /// + /// Rejects an index outside the palette. + /// + /// + /// Throws rather than clamping. Clamping is the one response that produces a plausible wrong + /// answer: SetColor(999, ...) quietly rewrote entry 255, and the indexer answered for entry 0 + /// when asked about -1. A caller with an off-by-one got no signal and a corrupted palette. The + /// OSC path never sees this, because InputHandler range-checks before calling -- which is + /// exactly why it went unnoticed. + /// + private static void ValidateIndex(int index) + { + if (index < 0 || index >= Size) + { + throw new ArgumentOutOfRangeException( + nameof(index), index, $"Palette index must be between 0 and {Size - 1}."); + } + } - private void Set(ref int backing, int rgb, ColorTarget target) + private void Set(ColorTarget target, int rgb) { - if (backing == rgb) + lock (writeGate) { - return; + var live = state; + int foreground = live.Foreground, background = live.Background, cursor = live.Cursor; + + switch (target) + { + case ColorTarget.Foreground when foreground != rgb: foreground = rgb; break; + case ColorTarget.Background when background != rgb: background = rgb; break; + case ColorTarget.Cursor when cursor != rgb: cursor = rgb; break; + default: return; + } + + Volatile.Write(ref state, new ColorSnapshot(live.Colors, foreground, background, cursor)); } - backing = rgb; ColorChanged?.Invoke(this, new ColorChangedEventArgs(target, -1, rgb)); } + /// + /// Builds a snapshot holding the current defaults. + /// + private ColorSnapshot NewSnapshot() + { + var colors = new int[Size]; + Array.Copy(defaults, colors, Size); + return new ColorSnapshot(colors, defaultForeground, defaultBackground, defaultCursor); + } + private void SeedDefaults(ThemeOptions? theme) { // 0-15: the xterm ANSI defaults, each overridable by the theme. @@ -239,6 +360,57 @@ private void SeedDefaults(ThemeOptions? theme) defaultBackground = ColorSpec.TryParse(theme?.Background, out var bg) ? bg : 0x000000; defaultCursor = ColorSpec.TryParse(theme?.Cursor, out var cur) ? cur : 0xFFFFFF; } + + +} + +/// +/// An immutable view of every terminal colour at one instant. +/// +/// +/// Handed out by and never modified afterwards, so a renderer can +/// paint a whole frame from one and know the colours belong to each other. +/// +public sealed class ColorSnapshot +{ + private readonly int[] colors; + + internal ColorSnapshot(int[] colors, int foreground, int background, int cursor) + { + this.colors = colors; + Foreground = foreground; + Background = background; + Cursor = cursor; + } + + /// Gets the foreground colour, as 0xRRGGBB. + public int Foreground { get; } + + /// Gets the background colour, as 0xRRGGBB. + public int Background { get; } + + /// Gets the cursor colour, as 0xRRGGBB. + public int Cursor { get; } + + /// Gets whether the background is light enough that dark text belongs on it. + public bool IsLightBackground => ColorPalette.IsLight(Background); + + /// Gets the colour for an index, as 0xRRGGBB. + public int this[int index] + { + get + { + if (index < 0 || index >= ColorPalette.Size) + { + throw new ArgumentOutOfRangeException( + nameof(index), index, $"Palette index must be between 0 and {ColorPalette.Size - 1}."); + } + + return colors[index]; + } + } + + internal int[] Colors => colors; } ///