Palette: reject bad indexes, suppress no-op events, make reads coherent

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<ColorChangedEventArgs>();
+        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<ColorChangedEventArgs>();
+        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<ColorChangedEventArgs>();
+        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<ColorChangedEventArgs>();
+        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<ArgumentOutOfRangeException>(() => terminal.Colors[index]);
+        Assert.Throws<ArgumentOutOfRangeException>(() => terminal.Colors.SetColor(index, 0x123456));
+        Assert.Throws<ArgumentOutOfRangeException>(() => 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));
+    }
+
+    /// <summary>
+    /// Reads the first eight ANSI colours through ONE snapshot.
+    /// </summary>
+    /// <remarks>
+    /// Eight calls to the indexer would each take their own snapshot and could straddle a theme
+    /// change, which is the whole reason Take exists.
+    /// </remarks>
+    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]