cover cache-reuse + first-change paths; fix pin color under select tool

Svg.Editor.Avalonia.Forms/Svg.Editor.Avalon.Forms.csproj

@@ -7,6 +7,7 @@
 		<PackageReleaseNotes>
 			#3.2.0-optiq09
 			SvgVisualElement always rebuild the cached pens/brushes when the token differs
+			fix pin color under select tool
 			#3.2.0-optiq08
 			circle/ellipse should only select on their outline, not their bounding box
 			#3.2.0-optiq07

Svg.Editor.Avalonia.Views/Svg.Editor.Avalon.Views.csproj

@@ -7,6 +7,7 @@
 		<PackageReleaseNotes>
 			#3.2.0-optiq09
 			SvgVisualElement always rebuild the cached pens/brushes when the token differs
+			fix pin color under select tool
 			#3.2.0-optiq08
 			circle/ellipse should only select on their outline, not their bounding box
 			#3.2.0-optiq07

Svg.Editor.Core.Tests/ColorTargetExtensibilityTests.cs

@@ -0,0 +1,79 @@
+using NUnit.Framework;
+using Svg;
+using Svg.Editor.Core.Test;
+using Svg.Editor.Interfaces;
+using Svg.Editor.Tools;
+using Svg.Interfaces;
+using System.Collections.Generic;
+using System.Linq;
+using System.Threading.Tasks;
+
+namespace Svg.Editor.Core.Tests
+{
+    // Proves the Open/Closed design: a brand-new composite shape can redirect coloring to a child
+    // purely by having its tool implement IColorTargetProvider. The ColorTool is not modified and
+    // has no knowledge of this shape.
+    [TestFixture]
+    public class ColorTargetExtensibilityTests : SvgDrawingCanvasTestBase
+    {
+        private MockColorInputService _colorMock;
+
+        protected override void SetupOverride()
+        {
+            Canvas.LoadTools(
+                () => new WidgetTool(),
+                () => new ColorTool(new Dictionary<string, object>(), SvgEngine.Resolve<IUndoRedoService>()));
+
+            _colorMock = new MockColorInputService();
+            SvgEngine.Register<IColorInputService>(() => _colorMock);
+        }
+
+        [Test]
+        public async Task ColorTool_ColorsChildTarget_ForAToolItHasNeverHeardOf()
+        {
+            // Arrange - a composite element whose visible shape is a child that carries its own fill
+            await Canvas.EnsureInitialized();
+            var shape = new SvgRectangle
+            {
+                X = 10, Y = 10, Width = 40, Height = 40,
+                Fill = new SvgColourServer(Color.Create("#000000"))
+            };
+            var widget = new SvgGroup();
+            widget.Children.Add(shape);
+            widget.CustomAttributes.Add(WidgetTool.WidgetMarker, "");
+            Canvas.Document.Children.Add(widget);
+
+            // Act - colorize the selected widget red
+            _colorMock.Hex = "#FF0000";
+            Canvas.SelectedElements.Add(widget);
+            var changeColorCommand = Canvas.Tools.OfType<ColorTool>().Single()
+                .Commands.Single(c => c.Name == "Change color");
+            changeColorCommand.Execute(null);
+            await Task.Delay(50); // Execute is async void; let it settle
+
+            // Assert - the child shape (the provider's target) took the color, not the group
+            var fill = shape.Fill as SvgColourServer;
+            Assert.AreEqual(Color.Create("#FF0000").ToString(), fill?.Colour.ToString(),
+                "ColorTool should color the target reported by IColorTargetProvider");
+        }
+
+        /// <summary>A stand-in for "some future shape's tool" the ColorTool has never heard of.</summary>
+        private class WidgetTool : ToolBase, IColorTargetProvider
+        {
+            public const string WidgetMarker = "data-widget";
+
+            public WidgetTool() : base("Widget") { }
+
+            public IEnumerable<SvgElement> GetColorTargets(SvgElement element)
+                => element.CustomAttributes.ContainsKey(WidgetMarker) && element.Children.Count > 0
+                    ? new[] { element.Children[0] }
+                    : null;
+        }
+
+        private class MockColorInputService : IColorInputService
+        {
+            public string Hex { get; set; } = "#000000";
+            public Task<string> GetHexaColorFromUserInput(string title) => Task.FromResult(Hex);
+        }
+    }
+}

Svg.Editor.Core.Tests/PinColorSelectionTests.cs

@@ -0,0 +1,86 @@
+using Moq;
+using NUnit.Framework;
+using Svg.Editor.Core.Test;
+using Svg.Editor.Core.Test.Mocks;
+using Svg.Editor.Events;
+using Svg.Editor.Interfaces;
+using Svg.Editor.Tools;
+using Svg.Interfaces;
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Threading.Tasks;
+
+namespace Svg.Editor.Core.Tests
+{
+    // Regression: colorizing a selected pin used to branch on Canvas.ActiveTool being
+    // ISupportTextColor (only the PinTool is). Because pins are normally selected with the
+    // Move/Selection tool, the color tool then colorized the group instead of the shape child -
+    // and since the shape carries its own fill, the pin visibly kept its old color. Coloring must
+    // work regardless of which tool is active.
+    [TestFixture]
+    public class PinColorSelectionTests : SvgDrawingCanvasTestBase
+    {
+        private Mock<IPinInputService> _pinInputServiceMock;
+        private MockColorInputService _colorMock;
+
+        protected override void SetupOverride()
+        {
+            _pinInputServiceMock = new Mock<IPinInputService>();
+
+            var pinToolProperties = new Dictionary<string, object>
+            {
+                {"pinsizenames", new[] {"Small", "Medium", "Large", "ExtraLarge" } }
+            };
+
+            Canvas.LoadTools(
+                () => new MoveTool(SvgEngine.Resolve<IUndoRedoService>()),
+                () => new PinTool(pinToolProperties, SvgEngine.Resolve<IUndoRedoService>()),
+                () => new ColorTool(new Dictionary<string, object>(), SvgEngine.Resolve<IUndoRedoService>()));
+
+            _colorMock = new MockColorInputService();
+            SvgEngine.Register<IColorInputService>(() => _colorMock);
+            SvgEngine.Register<ITextInputService>(() => new MockTextInputService());
+            SvgEngine.Register<IPinInputService>(() => _pinInputServiceMock.Object);
+        }
+
+        [Test]
+        public async Task WhenPinColoredWhileSelectToolActive_ShapeGetsChosenColor()
+        {
+            // Arrange - create a pin (with the PinTool active)
+            await Canvas.EnsureInitialized();
+            Canvas.ActiveTool = Canvas.Tools.OfType<PinTool>().Single();
+            await LongPress(PointF.Create(50, 50));
+            var pin = (SvgVisualElement)Canvas.Document.Children[Canvas.Document.Children.Count - 1];
+            var shape = pin.Children[0];
+
+            // Act - select the pin with a NON-pin tool active and colorize it red
+            Canvas.ActiveTool = Canvas.Tools.OfType<MoveTool>().Single();
+            _colorMock.Hex = "#FF0000";
+            Canvas.SelectedElements.Clear();
+            Canvas.SelectedElements.Add(pin);
+            var changeColorCommand = Canvas.Tools.OfType<ColorTool>().Single()
+                .Commands.Single(c => c.Name == "Change color");
+            changeColorCommand.Execute(null);
+            await Task.Delay(50); // Execute is async void; let it settle
+
+            // Assert - the pin's shape (not just the group) must take the chosen color
+            var fill = shape.Fill as SvgColourServer;
+            Assert.AreEqual(Color.Create("#FF0000").ToString(), fill?.Colour.ToString(),
+                "Pin shape was not colored when selected with a non-pin tool active");
+        }
+
+        private async Task LongPress(PointF position)
+        {
+            await Canvas.OnEvent(new PointerEvent(EventType.PointerDown, position, position, position, 1));
+            await Task.Delay(TimeSpan.FromMilliseconds(900));
+            await Canvas.OnEvent(new PointerEvent(EventType.PointerUp, position, position, position, 1));
+        }
+
+        private class MockColorInputService : IColorInputService
+        {
+            public string Hex { get; set; } = "#000000";
+            public Task<string> GetHexaColorFromUserInput(string title) => Task.FromResult(Hex);
+        }
+    }
+}

Svg.Editor.Core.Tests/RenderCacheColorTests.cs

@@ -1,8 +1,10 @@
 using NUnit.Framework;
 using SkiaSharp;
+using Svg;
 using Svg.Editor.Core.Test;
 using Svg.Editor.Services;
 using Svg.Interfaces;
+using System;
 using System.Threading.Tasks;
 
 namespace Svg.Editor.Core.Tests
@@ -19,11 +21,10 @@ public class RenderCacheColorTests : SvgDrawingCanvasTestBase
         [Test]
         public async Task WhenElementFillChangesForTheFirstTime_ItIsRenderedInTheNewColor()
         {
-            // Arrange - a shape that is initially black, sized to fill the view
+            // Arrange - a shape that is initially black, sized to fill the view.
+            // (OnDraw sets ScreenWidth/Height from the renderer, so we don't set them here.)
             const int w = 200, h = 200;
             await Canvas.EnsureInitialized();
-            Canvas.ScreenWidth = w;
-            Canvas.ScreenHeight = h;
 
             var doc = new SvgDocument { ViewBox = new SvgViewBox(0, 0, w, h) };
             var rect = new SvgRectangle
@@ -58,11 +59,10 @@ public async Task WhenElementFillChangesForTheFirstTime_ItIsRenderedInTheNewColo
         [Test]
         public async Task WhenElementStrokeChangesForTheFirstTime_ItIsRenderedInTheNewColor()
         {
-            // Arrange - an unfilled shape with a thick black stroke, sized to fill the view
+            // Arrange - an unfilled shape with a thick black stroke, sized to fill the view.
+            // (OnDraw sets ScreenWidth/Height from the renderer, so we don't set them here.)
             const int w = 200, h = 200;
             await Canvas.EnsureInitialized();
-            Canvas.ScreenWidth = w;
-            Canvas.ScreenHeight = h;
 
             var doc = new SvgDocument { ViewBox = new SvgViewBox(0, 0, w, h) };
             var rect = new SvgRectangle
@@ -95,6 +95,60 @@ public async Task WhenElementStrokeChangesForTheFirstTime_ItIsRenderedInTheNewCo
                 "The first stroke color change was not rendered - the stale cached brush was reused");
         }
 
+        // --- Cache-invalidation contract --------------------------------------
+        // These exercise RenderCacheEntryBase.SetAttributeChangeToken directly. That single method
+        // is inherited (not overridden) by RenderCacheEntry, TextRenderCacheEntry and the image
+        // cache entry, so locking its behavior here covers the fix's whole blast radius.
+
+        [Test]
+        public void SetAttributeChangeToken_FirstChangeFromUntrackedInitialState_InvalidatesCache()
+        {
+            // Reproduces the bug at unit level: the element carries the initial Empty token into
+            // its first render (attribute tracking only starts afterwards), the brush gets cached,
+            // then the first *tracked* change flips the token Empty -> real. The old code treated
+            // Empty as "initial, don't dispose" and kept the stale brush.
+            var entry = new SvgVisualElement.RenderCacheEntry();
+
+            entry.SetAttributeChangeToken(Guid.Empty);         // first render with untracked (Empty) token
+            var brush = SvgEngine.Factory.CreateSolidBrush(SvgEngine.Factory.Colors.Black);
+            entry.FillBrush = brush;                           // brush cached during that render
+
+            entry.SetAttributeChangeToken(Guid.NewGuid());     // first tracked attribute change
+
+            Assert.IsNull(entry.FillBrush, "the first attribute change must invalidate the cached brush");
+        }
+
+        [Test]
+        public void SetAttributeChangeToken_UnchangedToken_KeepsCachedBrush()
+        {
+            // Performance contract: when nothing changed the cached brush must be reused, not rebuilt.
+            var entry = new SvgVisualElement.RenderCacheEntry();
+            var token = Guid.NewGuid();
+
+            entry.SetAttributeChangeToken(token);
+            var brush = SvgEngine.Factory.CreateSolidBrush(SvgEngine.Factory.Colors.Black);
+            entry.FillBrush = brush;
+
+            entry.SetAttributeChangeToken(token);              // same token, e.g. a plain repaint
+            entry.SetAttributeChangeToken(token);
+
+            Assert.AreSame(brush, entry.FillBrush, "an unchanged token must reuse the cached brush");
+        }
+
+        [Test]
+        public void SetAttributeChangeToken_SubsequentChange_InvalidatesCache()
+        {
+            // 2nd/3rd change: once tracking a real token, each further change rebuilds the cache.
+            var entry = new SvgVisualElement.RenderCacheEntry();
+
+            entry.SetAttributeChangeToken(Guid.NewGuid());
+            entry.FillBrush = SvgEngine.Factory.CreateSolidBrush(SvgEngine.Factory.Colors.Black);
+
+            entry.SetAttributeChangeToken(Guid.NewGuid());     // another change
+
+            Assert.IsNull(entry.FillBrush, "a subsequent attribute change must invalidate the cached brush");
+        }
+
         private static int CountPixels(SKSurface surface, bool isBlack)
         {
             using var image = surface.Snapshot();

Svg.Editor.Core/Svg.Editor.Core.csproj

@@ -9,6 +9,7 @@
 	  <PackageReleaseNotes>
 		  #3.2.0-optiq09
 		  SvgVisualElement always rebuild the cached pens/brushes when the token differs
+		  fix pin color under select tool
 		  #3.2.0-optiq08
 		  circle/ellipse should only select on their outline, not their bounding box
 		  #3.2.0-optiq07

Svg.Editor.Core/Tools/ColorTool.cs

@@ -20,6 +20,18 @@ public interface ISupportTextColor
 		string GetDefaultTextColorIndex(string color);
 	}
 
+	/// <summary>
+	/// Implemented by a tool that owns a composite element (e.g. a group whose visible shape is a
+	/// child) to tell the <see cref="ColorTool"/> which element(s) actually carry the color.
+	/// This keeps the <see cref="ColorTool"/> closed for modification: it never needs to know about
+	/// any concrete shape - a new shape's tool just implements this and returns its color targets.
+	/// Return <c>null</c> for elements this tool does not own.
+	/// </summary>
+	public interface IColorTargetProvider
+	{
+		IEnumerable<SvgElement> GetColorTargets(SvgElement element);
+	}
+
 	public class ColorTool : UndoableToolBase
 	{
 		#region Private fields and properties
@@ -106,6 +118,21 @@ private static string StringifyColor(Color color)
 			return $"{color.R}_{color.G}_{color.B}";
 		}
 
+		/// <summary>
+		/// Resolves which element(s) should actually receive the color for a selected element.
+		/// Tools that own composite shapes can redirect coloring to a child (e.g. a pin's shape)
+		/// by implementing <see cref="IColorTargetProvider"/>; otherwise the element itself is used.
+		/// </summary>
+		private IEnumerable<SvgElement> ResolveColorTargets(SvgElement element)
+		{
+			var targets = Canvas.Tools
+				.OfType<IColorTargetProvider>()
+				.Select(p => p.GetColorTargets(element))
+				.FirstOrDefault(t => t != null);
+
+			return targets ?? new[] { element };
+		}
+
 		private void ColorizeElement(SvgElement element, string hxColor)
 		{
 			var noFill = element.Fill == null ||element.Fill == SvgPaintServer.None || element.Fill == SvgColourServer.NotSet || element.HasConstraints(NoFillConstraint);
@@ -217,19 +244,14 @@ public override async void Execute(object parameter)
 				if (_canvas.SelectedElements.Any())
 				{
 					t.UndoRedoService.ExecuteCommand(new UndoableActionCommand("Colorize selected elements", o => { }));
-					// change the color of all selected items
+					// change the color of all selected items. Which element actually carries the
+					// color (the element itself, or a child for composite shapes) is resolved via
+					// IColorTargetProvider, so the ColorTool needs no knowledge of concrete shapes.
 					foreach (var selectedElement in _canvas.SelectedElements)
 					{
-						if (t.Canvas.ActiveTool is ISupportTextColor)
-						{
-							// in this case the selected element is a group
-							// the first child of the group is the shape itself
-							// and the second child is the text contained by the shape
-							t.ColorizeElement(selectedElement.Children[0], hxColor);
-						}
-						else
+						foreach (var target in t.ResolveColorTargets(selectedElement))
 						{
-							t.ColorizeElement(selectedElement, hxColor);
+							t.ColorizeElement(target, hxColor);
 						}
 					}
 					// don't change the global color when items are selected

Svg.Editor.Core/Tools/PinTool.cs

@@ -16,7 +16,7 @@ public interface IPinInputService
             IEnumerable<string> pinSizeOptions, int oldSizeIndex = 1);
     }
 
-    public class PinTool : UndoableToolBase, ISupportTextColor, ISupportMoving
+    public class PinTool : UndoableToolBase, ISupportTextColor, ISupportMoving, IColorTargetProvider
     {
         #region Private fields
 
@@ -69,6 +69,19 @@ public string GetDefaultTextColorIndex(string color)
             return color;
         }
 
+        /// <summary>
+        /// A pin is a group whose first child is the visible shape (carrying its own fill/stroke)
+        /// and whose second child is the text. Coloring the group would have no visible effect, so
+        /// the ColorTool is told to color the shape child instead. Returns null for non-pin elements.
+        /// </summary>
+        public IEnumerable<SvgElement> GetColorTargets(SvgElement element)
+        {
+            if (element.CustomAttributes.ContainsKey(PinSizeAttributeKey) && element.Children.Count >= 2)
+                return new[] { element.Children[0] };
+
+            return null;
+        }
+
         #endregion
 
         #region Overrides

Svg/Svg.csproj

@@ -9,6 +9,7 @@
 		<PackageReleaseNotes>
 			#3.2.0-optiq09
 			SvgVisualElement always rebuild the cached pens/brushes when the token differs
+			fix pin color under select tool
 			#3.2.0-optiq08
 			circle/ellipse should only select on their outline, not their bounding box
 			#3.2.0-optiq07