Add Points collection observability support for Polygon and Polyline (#…

…15030)

* Add Points binding observability support for Polygon and Polyline

* Add simple tests for Polygon and Polyline Points updating

* Revert "Add Points binding observability support for Polygon and Polyline"

This reverts commit e16d987.

* Move Geometry.Changed handler to Shape class to make it available to all inheritors

* Fixes the event subscriptions

* Fix tests

* Add memory leak tests

src/Avalonia.Base/Media/PolylineGeometry.cs

@@ -1,8 +1,10 @@
 using System;
 using System.Collections.Generic;
+using System.Collections.Specialized;
 using Avalonia.Collections;
 using Avalonia.Metadata;
 using Avalonia.Platform;
+using Avalonia.Reactive;
 
 namespace Avalonia.Media
 {
@@ -100,10 +102,8 @@ public override Geometry Clone()
         private void OnPointsChanged(IList<Point>? newValue)
         {
             _pointsObserver?.Dispose();
-            _pointsObserver = (newValue as IAvaloniaList<Point>)?.ForEachItem(
-                _ => InvalidateGeometry(),
-                _ => InvalidateGeometry(),
-                InvalidateGeometry);
+            _pointsObserver = (newValue as INotifyCollectionChanged)?.GetWeakCollectionChangedObservable()
+                .Subscribe(_ => InvalidateGeometry());
         }
     }
 }

src/Avalonia.Controls/Shapes/Path.cs

@@ -8,12 +8,9 @@ public class Path : Shape
         public static readonly StyledProperty<Geometry?> DataProperty =
             AvaloniaProperty.Register<Path, Geometry?>(nameof(Data));
 
-        private EventHandler? _geometryChangedHandler;
-
         static Path()
         {
             AffectsGeometry<Path>(DataProperty);
-            DataProperty.Changed.AddClassHandler<Path>((o, e) => o.DataChanged(e));
         }
 
         public Geometry? Data
@@ -22,54 +19,6 @@ public Geometry? Data
             set => SetValue(DataProperty, value);
         }
 
-        private EventHandler GeometryChangedHandler => _geometryChangedHandler ??= GeometryChanged;
-
         protected override Geometry? CreateDefiningGeometry() => Data;
-
-        protected override void OnAttachedToVisualTree(VisualTreeAttachmentEventArgs e)
-        {
-            base.OnAttachedToVisualTree(e);
-
-            if (Data is object)
-            {
-                Data.Changed += GeometryChangedHandler;
-            }
-        }
-
-        protected override void OnDetachedFromVisualTree(VisualTreeAttachmentEventArgs e)
-        {
-            base.OnDetachedFromVisualTree(e);
-
-            if (Data is object)
-            {
-                Data.Changed -= GeometryChangedHandler;
-            }
-        }
-
-        private void DataChanged(AvaloniaPropertyChangedEventArgs e)
-        {
-            if (VisualRoot is null)
-            {
-                return;
-            }
-
-            var oldGeometry = (Geometry?)e.OldValue;
-            var newGeometry = (Geometry?)e.NewValue;
-
-            if (oldGeometry is object)
-            {
-                oldGeometry.Changed -= GeometryChangedHandler;
-            }
-
-            if (newGeometry is object)
-            {
-                newGeometry.Changed += GeometryChangedHandler;
-            }
-        }
-
-        private void GeometryChanged(object? sender, EventArgs e)
-        {
-            InvalidateGeometry();
-        }
     }
 }

src/Avalonia.Controls/Shapes/Polygon.cs

@@ -27,7 +27,7 @@ public IList<Point> Points
 
         protected override Geometry CreateDefiningGeometry()
         {
-            return new PolylineGeometry(Points, true);
+            return new PolylineGeometry { Points = Points, IsFilled = true };
         }
     }
 }

src/Avalonia.Controls/Shapes/Polyline.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Collections.Generic;
 using Avalonia.Media;
 using Avalonia.Data;
@@ -28,7 +29,7 @@ public IList<Point> Points
 
         protected override Geometry CreateDefiningGeometry()
         {
-            return new PolylineGeometry(Points, false);
+            return new PolylineGeometry { Points = Points, IsFilled = false };
         }
     }
 }

src/Avalonia.Controls/Shapes/Shape.cs

@@ -64,6 +64,7 @@ public abstract class Shape : Control
         private Geometry? _definingGeometry;
         private Geometry? _renderedGeometry;
         private IPen? _strokePen;
+        private EventHandler? _geometryChangedHandler;
 
         /// <summary>
         /// Gets a value that represents the <see cref="Geometry"/> of the shape.
@@ -75,6 +76,10 @@ public Geometry? DefiningGeometry
                 if (_definingGeometry == null)
                 {
                     _definingGeometry = CreateDefiningGeometry();
+                    if (_definingGeometry is not null && VisualRoot is not null)
+                    {
+                        _definingGeometry.Changed += GeometryChangedHandler;
+                    }
                 }
 
                 return _definingGeometry;
@@ -186,6 +191,8 @@ public PenLineJoin StrokeJoin
             get => GetValue(StrokeJoinProperty);
             set => SetValue(StrokeJoinProperty, value);
         }
+
+        private EventHandler GeometryChangedHandler => _geometryChangedHandler ??= OnGeometryChanged;
 
         public sealed override void Render(DrawingContext context)
         {
@@ -225,12 +232,29 @@ protected static void AffectsGeometry<TShape>(params AvaloniaProperty[] properti
         /// </summary>
         /// <returns>Defining <see cref="Geometry"/> of the shape.</returns>
         protected abstract Geometry? CreateDefiningGeometry();
+
+        /// <summary>
+        /// Called when the underlying <see cref="Geometry"/> changed
+        /// </summary>
+        /// <param name="sender"></param>
+        /// <param name="e"></param>
+        protected virtual void OnGeometryChanged(object? sender, EventArgs e)
+        {
+            _renderedGeometry = null;
+
+            InvalidateMeasure();
+        }
 
         /// <summary>
         /// Invalidates the geometry of this shape.
         /// </summary>
         protected void InvalidateGeometry()
         {
+            if (_definingGeometry is not null)
+            {
+                _definingGeometry.Changed -= GeometryChangedHandler;
+            }
+
             _renderedGeometry = null;
             _definingGeometry = null;
 
@@ -294,6 +318,26 @@ protected override Size ArrangeOverride(Size finalSize)
 
             return default;
         }
+
+        protected override void OnAttachedToVisualTree(VisualTreeAttachmentEventArgs e)
+        {
+            base.OnAttachedToVisualTree(e);
+
+            if (_definingGeometry is not null)
+            {
+                _definingGeometry.Changed += GeometryChangedHandler;
+            }
+        }
+
+        protected override void OnDetachedFromVisualTree(VisualTreeAttachmentEventArgs e)
+        {
+            base.OnDetachedFromVisualTree(e);
+
+            if (_definingGeometry is not null)
+            {
+                _definingGeometry.Changed -= GeometryChangedHandler;
+            }
+        }
 
         internal static (Size size, Matrix transform) CalculateSizeAndTransform(Size availableSize, Rect shapeBounds, Stretch Stretch)
         {

tests/Avalonia.Controls.UnitTests/Shapes/PolygonTests.cs

@@ -0,0 +1,28 @@
+using System.Collections.ObjectModel;
+using Avalonia.Controls.Shapes;
+using Avalonia.UnitTests;
+using Xunit;
+
+namespace Avalonia.Controls.UnitTests.Shapes;
+
+public class PolygonTests
+{
+    [Fact]
+    public void Polygon_Will_Update_Geometry_On_Shapes_Collection_Content_Change()
+    {
+        using var app = UnitTestApplication.Start(TestServices.MockPlatformRenderInterface);
+        var points = new ObservableCollection<Point>();
+
+        var target = new Polygon() { Points = points };
+        target.Measure(new Size());
+        Assert.True(target.IsMeasureValid);
+
+        var root = new TestRoot(target);
+
+        points.Add(new Point());
+
+        Assert.False(target.IsMeasureValid);
+
+        root.Child = null;
+    }
+}

tests/Avalonia.Controls.UnitTests/Shapes/PolylineTests.cs

@@ -0,0 +1,28 @@
+using System.Collections.ObjectModel;
+using Avalonia.Controls.Shapes;
+using Avalonia.UnitTests;
+using Xunit;
+
+namespace Avalonia.Controls.UnitTests.Shapes;
+
+public class PolylineTests
+{
+    [Fact]
+    public void Polyline_Will_Update_Geometry_On_Shapes_Collection_Content_Change()
+    {
+        using var app = UnitTestApplication.Start(TestServices.MockPlatformRenderInterface);
+        var points = new ObservableCollection<Point>();
+
+        var target = new Polyline { Points = points };
+        target.Measure(new Size());
+        Assert.True(target.IsMeasureValid);
+
+        var root = new TestRoot(target);
+
+        points.Add(new Point());
+
+        Assert.False(target.IsMeasureValid);
+
+        root.Child = null;
+    }
+}

tests/Avalonia.LeakTests/ControlTests.cs

@@ -713,6 +713,48 @@ public void Path_Is_Freed()
                 GC.KeepAlive(geometry);
             }
         }
+
+        [Fact]
+        public void Polyline_WithObservableCollectionPointsBinding_Is_Freed()
+        {
+            using (Start())
+            {
+                var observableCollection = new ObservableCollection<Point>(){new()};
+
+                Func<Window> run = () =>
+                {
+                    var window = new Window
+                    {
+                        Content = new Polyline()
+                        {
+                            Points = observableCollection
+                        }
+                    };
+
+                    window.Show();
+
+                    window.LayoutManager.ExecuteInitialLayoutPass();
+                    Assert.IsType<Polyline>(window.Presenter.Child);
+
+                    window.Content = null;
+                    window.LayoutManager.ExecuteLayoutPass();
+                    Assert.Null(window.Presenter.Child);
+
+                    return window;
+                };
+
+                var result = run();
+
+                // Process all Loaded events to free control reference(s)
+                Dispatcher.UIThread.RunJobs(DispatcherPriority.Loaded);
+
+                dotMemory.Check(memory =>
+                    Assert.Equal(0, memory.GetObjects(where => where.Type.Is<Polyline>()).ObjectsCount));
+
+                // We are keeping collection alive to simulate a resource that outlives the control.
+                GC.KeepAlive(observableCollection);
+            }
+        }
 
         [Fact]
         public void ElementName_Binding_In_DataTemplate_Is_Freed()