From bc1a7d47e16fcabc83beee219c7b9792cd71b5ae Mon Sep 17 00:00:00 2001 From: Zeljko Predjeskovic Date: Wed, 22 Jul 2026 09:28:15 +0200 Subject: [PATCH] cover cache-reuse + first-change paths; fix pin color under select tool - RenderCacheColorTests: add SetAttributeChangeToken unit tests (unchanged token reuses brush; first/subsequent change invalidates); drop redundant ScreenWidth/Height assignments. - ColorTool: color a selected pin's shape by element identity, not the active tool, so recoloring works under the Move/Selection tool. Adds PinColorSelectionTests. --- .../Svg.Editor.Avalon.Forms.csproj | 1 + .../Svg.Editor.Avalon.Views.csproj | 1 + .../ColorTargetExtensibilityTests.cs | 79 +++++++++++++++++ .../PinColorSelectionTests.cs | 86 +++++++++++++++++++ .../RenderCacheColorTests.cs | 66 ++++++++++++-- Svg.Editor.Core/Svg.Editor.Core.csproj | 1 + Svg.Editor.Core/Tools/ColorTool.cs | 42 ++++++--- Svg.Editor.Core/Tools/PinTool.cs | 15 +++- Svg/Svg.csproj | 1 + 9 files changed, 275 insertions(+), 17 deletions(-) create mode 100644 Svg.Editor.Core.Tests/ColorTargetExtensibilityTests.cs create mode 100644 Svg.Editor.Core.Tests/PinColorSelectionTests.cs diff --git a/Svg.Editor.Avalonia.Forms/Svg.Editor.Avalon.Forms.csproj b/Svg.Editor.Avalonia.Forms/Svg.Editor.Avalon.Forms.csproj index 9799c90c8..55d058441 100644 --- a/Svg.Editor.Avalonia.Forms/Svg.Editor.Avalon.Forms.csproj +++ b/Svg.Editor.Avalonia.Forms/Svg.Editor.Avalon.Forms.csproj @@ -7,6 +7,7 @@ #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 diff --git a/Svg.Editor.Avalonia.Views/Svg.Editor.Avalon.Views.csproj b/Svg.Editor.Avalonia.Views/Svg.Editor.Avalon.Views.csproj index b22b72c3d..292ceaf72 100644 --- a/Svg.Editor.Avalonia.Views/Svg.Editor.Avalon.Views.csproj +++ b/Svg.Editor.Avalonia.Views/Svg.Editor.Avalon.Views.csproj @@ -7,6 +7,7 @@ #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 diff --git a/Svg.Editor.Core.Tests/ColorTargetExtensibilityTests.cs b/Svg.Editor.Core.Tests/ColorTargetExtensibilityTests.cs new file mode 100644 index 000000000..0f68375f9 --- /dev/null +++ b/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(), SvgEngine.Resolve())); + + _colorMock = new MockColorInputService(); + SvgEngine.Register(() => _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().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"); + } + + /// A stand-in for "some future shape's tool" the ColorTool has never heard of. + private class WidgetTool : ToolBase, IColorTargetProvider + { + public const string WidgetMarker = "data-widget"; + + public WidgetTool() : base("Widget") { } + + public IEnumerable 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 GetHexaColorFromUserInput(string title) => Task.FromResult(Hex); + } + } +} diff --git a/Svg.Editor.Core.Tests/PinColorSelectionTests.cs b/Svg.Editor.Core.Tests/PinColorSelectionTests.cs new file mode 100644 index 000000000..3f0d65268 --- /dev/null +++ b/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 _pinInputServiceMock; + private MockColorInputService _colorMock; + + protected override void SetupOverride() + { + _pinInputServiceMock = new Mock(); + + var pinToolProperties = new Dictionary + { + {"pinsizenames", new[] {"Small", "Medium", "Large", "ExtraLarge" } } + }; + + Canvas.LoadTools( + () => new MoveTool(SvgEngine.Resolve()), + () => new PinTool(pinToolProperties, SvgEngine.Resolve()), + () => new ColorTool(new Dictionary(), SvgEngine.Resolve())); + + _colorMock = new MockColorInputService(); + SvgEngine.Register(() => _colorMock); + SvgEngine.Register(() => new MockTextInputService()); + SvgEngine.Register(() => _pinInputServiceMock.Object); + } + + [Test] + public async Task WhenPinColoredWhileSelectToolActive_ShapeGetsChosenColor() + { + // Arrange - create a pin (with the PinTool active) + await Canvas.EnsureInitialized(); + Canvas.ActiveTool = Canvas.Tools.OfType().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().Single(); + _colorMock.Hex = "#FF0000"; + Canvas.SelectedElements.Clear(); + Canvas.SelectedElements.Add(pin); + var changeColorCommand = Canvas.Tools.OfType().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 GetHexaColorFromUserInput(string title) => Task.FromResult(Hex); + } + } +} diff --git a/Svg.Editor.Core.Tests/RenderCacheColorTests.cs b/Svg.Editor.Core.Tests/RenderCacheColorTests.cs index f99f62820..fffb48953 100644 --- a/Svg.Editor.Core.Tests/RenderCacheColorTests.cs +++ b/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(); diff --git a/Svg.Editor.Core/Svg.Editor.Core.csproj b/Svg.Editor.Core/Svg.Editor.Core.csproj index ac013948a..aa169c999 100644 --- a/Svg.Editor.Core/Svg.Editor.Core.csproj +++ b/Svg.Editor.Core/Svg.Editor.Core.csproj @@ -9,6 +9,7 @@ #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 diff --git a/Svg.Editor.Core/Tools/ColorTool.cs b/Svg.Editor.Core/Tools/ColorTool.cs index c9526129a..993d27950 100644 --- a/Svg.Editor.Core/Tools/ColorTool.cs +++ b/Svg.Editor.Core/Tools/ColorTool.cs @@ -20,6 +20,18 @@ public interface ISupportTextColor string GetDefaultTextColorIndex(string color); } + /// + /// Implemented by a tool that owns a composite element (e.g. a group whose visible shape is a + /// child) to tell the which element(s) actually carry the color. + /// This keeps the 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 null for elements this tool does not own. + /// + public interface IColorTargetProvider + { + IEnumerable 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}"; } + /// + /// 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 ; otherwise the element itself is used. + /// + private IEnumerable ResolveColorTargets(SvgElement element) + { + var targets = Canvas.Tools + .OfType() + .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 diff --git a/Svg.Editor.Core/Tools/PinTool.cs b/Svg.Editor.Core/Tools/PinTool.cs index 0fdf5861b..97373f173 100644 --- a/Svg.Editor.Core/Tools/PinTool.cs +++ b/Svg.Editor.Core/Tools/PinTool.cs @@ -16,7 +16,7 @@ public interface IPinInputService IEnumerable 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; } + /// + /// 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. + /// + public IEnumerable GetColorTargets(SvgElement element) + { + if (element.CustomAttributes.ContainsKey(PinSizeAttributeKey) && element.Children.Count >= 2) + return new[] { element.Children[0] }; + + return null; + } + #endregion #region Overrides diff --git a/Svg/Svg.csproj b/Svg/Svg.csproj index b82602cc9..ef8ecaf67 100644 --- a/Svg/Svg.csproj +++ b/Svg/Svg.csproj @@ -9,6 +9,7 @@ #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