From e16d987945cc717bf0ffbf80902e052dd84b3665 Mon Sep 17 00:00:00 2001 From: SKProCH Date: Mon, 18 Mar 2024 23:21:11 +0300 Subject: [PATCH 1/7] Add Points binding observability support for Polygon and Polyline --- src/Avalonia.Controls/Shapes/Polygon.cs | 1 + src/Avalonia.Controls/Shapes/Polyline.cs | 1 + src/Avalonia.Controls/Shapes/Shape.cs | 35 ++++++++++++++++++++++++ 3 files changed, 37 insertions(+) diff --git a/src/Avalonia.Controls/Shapes/Polygon.cs b/src/Avalonia.Controls/Shapes/Polygon.cs index e51e117e330..f8728354b72 100644 --- a/src/Avalonia.Controls/Shapes/Polygon.cs +++ b/src/Avalonia.Controls/Shapes/Polygon.cs @@ -12,6 +12,7 @@ public class Polygon : Shape static Polygon() { AffectsGeometry(PointsProperty); + CollectionAffectsGeometry(PointsProperty); } public Polygon() diff --git a/src/Avalonia.Controls/Shapes/Polyline.cs b/src/Avalonia.Controls/Shapes/Polyline.cs index 84fccb66f11..ec933e391a3 100644 --- a/src/Avalonia.Controls/Shapes/Polyline.cs +++ b/src/Avalonia.Controls/Shapes/Polyline.cs @@ -13,6 +13,7 @@ static Polyline() { StrokeThicknessProperty.OverrideDefaultValue(1); AffectsGeometry(PointsProperty); + CollectionAffectsGeometry(PointsProperty); } public Polyline() diff --git a/src/Avalonia.Controls/Shapes/Shape.cs b/src/Avalonia.Controls/Shapes/Shape.cs index e358656fae9..345e8108d11 100644 --- a/src/Avalonia.Controls/Shapes/Shape.cs +++ b/src/Avalonia.Controls/Shapes/Shape.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Collections.Specialized; using Avalonia.Collections; using Avalonia.Media; using Avalonia.Media.Immutable; @@ -219,6 +220,40 @@ protected static void AffectsGeometry(params AvaloniaProperty[] properti }); } } + + /// + /// Marks a property as a collection whose elements affecting the shape's geometry. + /// + /// The properties. + /// + /// After a call to this method in a control's static constructor, any change to the + /// property will cause to be called on the element. + /// + protected static void CollectionAffectsGeometry(params AvaloniaProperty[] properties) + where TShape : Shape + { + foreach (var property in properties) + { + property.Changed.Subscribe(e => + { + if (e.Sender is not TShape shape) return; + if (e.OldValue is INotifyCollectionChanged oldCollection) + { + oldCollection.CollectionChanged -= shape.OnCollectionThatAffectsGeometryChanged; + } + + if (e.NewValue is INotifyCollectionChanged newCollection) + { + newCollection.CollectionChanged += shape.OnCollectionThatAffectsGeometryChanged; + } + }); + } + } + + private void OnCollectionThatAffectsGeometryChanged(object? sender, NotifyCollectionChangedEventArgs e) + { + InvalidateGeometry(); + } /// /// Creates the shape's defining geometry. From 7f303607a555ddfb324265950779f0a758ef929c Mon Sep 17 00:00:00 2001 From: SKProCH Date: Tue, 19 Mar 2024 00:33:04 +0300 Subject: [PATCH 2/7] Add simple tests for Polygon and Polyline Points updating --- .../Shapes/PolygonTests.cs | 25 +++++++++++++++++++ .../Shapes/PolylineTests.cs | 25 +++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 tests/Avalonia.Controls.UnitTests/Shapes/PolygonTests.cs create mode 100644 tests/Avalonia.Controls.UnitTests/Shapes/PolylineTests.cs diff --git a/tests/Avalonia.Controls.UnitTests/Shapes/PolygonTests.cs b/tests/Avalonia.Controls.UnitTests/Shapes/PolygonTests.cs new file mode 100644 index 00000000000..d56dd49d122 --- /dev/null +++ b/tests/Avalonia.Controls.UnitTests/Shapes/PolygonTests.cs @@ -0,0 +1,25 @@ +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(); + + var target = new Polygon() { Points = points }; + target.Measure(new Size()); + Assert.True(target.IsMeasureValid); + + + points.Add(new Point()); + + Assert.False(target.IsMeasureValid); + } +} diff --git a/tests/Avalonia.Controls.UnitTests/Shapes/PolylineTests.cs b/tests/Avalonia.Controls.UnitTests/Shapes/PolylineTests.cs new file mode 100644 index 00000000000..a099deac161 --- /dev/null +++ b/tests/Avalonia.Controls.UnitTests/Shapes/PolylineTests.cs @@ -0,0 +1,25 @@ +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(); + + var target = new Polyline { Points = points }; + target.Measure(new Size()); + Assert.True(target.IsMeasureValid); + + + points.Add(new Point()); + + Assert.False(target.IsMeasureValid); + } +} From 9a4130279aaf1c9a79c530275417cc09d49b2604 Mon Sep 17 00:00:00 2001 From: SKProCH Date: Tue, 19 Mar 2024 00:55:36 +0300 Subject: [PATCH 3/7] Revert "Add Points binding observability support for Polygon and Polyline" This reverts commit e16d987945cc717bf0ffbf80902e052dd84b3665. --- src/Avalonia.Controls/Shapes/Polygon.cs | 1 - src/Avalonia.Controls/Shapes/Polyline.cs | 1 - src/Avalonia.Controls/Shapes/Shape.cs | 35 ------------------------ 3 files changed, 37 deletions(-) diff --git a/src/Avalonia.Controls/Shapes/Polygon.cs b/src/Avalonia.Controls/Shapes/Polygon.cs index f8728354b72..e51e117e330 100644 --- a/src/Avalonia.Controls/Shapes/Polygon.cs +++ b/src/Avalonia.Controls/Shapes/Polygon.cs @@ -12,7 +12,6 @@ public class Polygon : Shape static Polygon() { AffectsGeometry(PointsProperty); - CollectionAffectsGeometry(PointsProperty); } public Polygon() diff --git a/src/Avalonia.Controls/Shapes/Polyline.cs b/src/Avalonia.Controls/Shapes/Polyline.cs index ec933e391a3..84fccb66f11 100644 --- a/src/Avalonia.Controls/Shapes/Polyline.cs +++ b/src/Avalonia.Controls/Shapes/Polyline.cs @@ -13,7 +13,6 @@ static Polyline() { StrokeThicknessProperty.OverrideDefaultValue(1); AffectsGeometry(PointsProperty); - CollectionAffectsGeometry(PointsProperty); } public Polyline() diff --git a/src/Avalonia.Controls/Shapes/Shape.cs b/src/Avalonia.Controls/Shapes/Shape.cs index 345e8108d11..e358656fae9 100644 --- a/src/Avalonia.Controls/Shapes/Shape.cs +++ b/src/Avalonia.Controls/Shapes/Shape.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Generic; -using System.Collections.Specialized; using Avalonia.Collections; using Avalonia.Media; using Avalonia.Media.Immutable; @@ -220,40 +219,6 @@ protected static void AffectsGeometry(params AvaloniaProperty[] properti }); } } - - /// - /// Marks a property as a collection whose elements affecting the shape's geometry. - /// - /// The properties. - /// - /// After a call to this method in a control's static constructor, any change to the - /// property will cause to be called on the element. - /// - protected static void CollectionAffectsGeometry(params AvaloniaProperty[] properties) - where TShape : Shape - { - foreach (var property in properties) - { - property.Changed.Subscribe(e => - { - if (e.Sender is not TShape shape) return; - if (e.OldValue is INotifyCollectionChanged oldCollection) - { - oldCollection.CollectionChanged -= shape.OnCollectionThatAffectsGeometryChanged; - } - - if (e.NewValue is INotifyCollectionChanged newCollection) - { - newCollection.CollectionChanged += shape.OnCollectionThatAffectsGeometryChanged; - } - }); - } - } - - private void OnCollectionThatAffectsGeometryChanged(object? sender, NotifyCollectionChangedEventArgs e) - { - InvalidateGeometry(); - } /// /// Creates the shape's defining geometry. From 5a47b6b5b46bbded776548c92f542bb914ab032b Mon Sep 17 00:00:00 2001 From: SKProCH Date: Wed, 20 Mar 2024 00:53:56 +0300 Subject: [PATCH 4/7] Move Geometry.Changed handler to Shape class to make it available to all inheritors --- src/Avalonia.Base/Media/PolylineGeometry.cs | 8 ++-- src/Avalonia.Controls/Shapes/Path.cs | 51 --------------------- src/Avalonia.Controls/Shapes/Polygon.cs | 2 +- src/Avalonia.Controls/Shapes/Polyline.cs | 3 +- src/Avalonia.Controls/Shapes/Shape.cs | 44 ++++++++++++++++++ 5 files changed, 51 insertions(+), 57 deletions(-) diff --git a/src/Avalonia.Base/Media/PolylineGeometry.cs b/src/Avalonia.Base/Media/PolylineGeometry.cs index 47cf2f48a48..13b11d84445 100644 --- a/src/Avalonia.Base/Media/PolylineGeometry.cs +++ b/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? newValue) { _pointsObserver?.Dispose(); - _pointsObserver = (newValue as IAvaloniaList)?.ForEachItem( - _ => InvalidateGeometry(), - _ => InvalidateGeometry(), - InvalidateGeometry); + _pointsObserver = (newValue as INotifyCollectionChanged)?.GetWeakCollectionChangedObservable() + .Subscribe(_ => InvalidateGeometry()); } } } diff --git a/src/Avalonia.Controls/Shapes/Path.cs b/src/Avalonia.Controls/Shapes/Path.cs index 9cbe6f943f3..e743527a2c2 100644 --- a/src/Avalonia.Controls/Shapes/Path.cs +++ b/src/Avalonia.Controls/Shapes/Path.cs @@ -8,12 +8,9 @@ public class Path : Shape public static readonly StyledProperty DataProperty = AvaloniaProperty.Register(nameof(Data)); - private EventHandler? _geometryChangedHandler; - static Path() { AffectsGeometry(DataProperty); - DataProperty.Changed.AddClassHandler((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(); - } } } diff --git a/src/Avalonia.Controls/Shapes/Polygon.cs b/src/Avalonia.Controls/Shapes/Polygon.cs index e51e117e330..48530432f02 100644 --- a/src/Avalonia.Controls/Shapes/Polygon.cs +++ b/src/Avalonia.Controls/Shapes/Polygon.cs @@ -27,7 +27,7 @@ public IList Points protected override Geometry CreateDefiningGeometry() { - return new PolylineGeometry(Points, true); + return new PolylineGeometry { Points = Points, IsFilled = true }; } } } diff --git a/src/Avalonia.Controls/Shapes/Polyline.cs b/src/Avalonia.Controls/Shapes/Polyline.cs index 84fccb66f11..628fc70e1d9 100644 --- a/src/Avalonia.Controls/Shapes/Polyline.cs +++ b/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 Points protected override Geometry CreateDefiningGeometry() { - return new PolylineGeometry(Points, false); + return new PolylineGeometry { Points = Points, IsFilled = false }; } } } diff --git a/src/Avalonia.Controls/Shapes/Shape.cs b/src/Avalonia.Controls/Shapes/Shape.cs index e358656fae9..dae9149c6ee 100644 --- a/src/Avalonia.Controls/Shapes/Shape.cs +++ b/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; /// /// Gets a value that represents the of the shape. @@ -75,6 +76,10 @@ public Geometry? DefiningGeometry if (_definingGeometry == null) { _definingGeometry = CreateDefiningGeometry(); + if (_definingGeometry 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(params AvaloniaProperty[] properti /// /// Defining of the shape. protected abstract Geometry? CreateDefiningGeometry(); + + /// + /// Called when the underlying changed + /// + /// + /// + protected virtual void OnGeometryChanged(object? sender, EventArgs e) + { + _renderedGeometry = null; + + InvalidateMeasure(); + } /// /// Invalidates the geometry of this shape. /// 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) { From 606834193d761ef7081bc8df00a738ad8e4b0b1d Mon Sep 17 00:00:00 2001 From: SKProCH Date: Wed, 20 Mar 2024 18:54:11 +0300 Subject: [PATCH 5/7] Fixes the event subscriptions --- src/Avalonia.Controls/Shapes/Shape.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Avalonia.Controls/Shapes/Shape.cs b/src/Avalonia.Controls/Shapes/Shape.cs index dae9149c6ee..8457f4789c9 100644 --- a/src/Avalonia.Controls/Shapes/Shape.cs +++ b/src/Avalonia.Controls/Shapes/Shape.cs @@ -76,7 +76,7 @@ public Geometry? DefiningGeometry if (_definingGeometry == null) { _definingGeometry = CreateDefiningGeometry(); - if (_definingGeometry is not null) + if (_definingGeometry is not null && VisualRoot is not null) { _definingGeometry.Changed += GeometryChangedHandler; } @@ -252,7 +252,7 @@ protected void InvalidateGeometry() { if (_definingGeometry is not null) { - _definingGeometry.Changed += GeometryChangedHandler; + _definingGeometry.Changed -= GeometryChangedHandler; } _renderedGeometry = null; From 15321a32990d6ed0755d9f8d635903c081a5a100 Mon Sep 17 00:00:00 2001 From: SKProCH Date: Wed, 20 Mar 2024 20:50:00 +0300 Subject: [PATCH 6/7] Fix tests --- tests/Avalonia.Controls.UnitTests/Shapes/PolygonTests.cs | 3 +++ tests/Avalonia.Controls.UnitTests/Shapes/PolylineTests.cs | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/Avalonia.Controls.UnitTests/Shapes/PolygonTests.cs b/tests/Avalonia.Controls.UnitTests/Shapes/PolygonTests.cs index d56dd49d122..1584e836e80 100644 --- a/tests/Avalonia.Controls.UnitTests/Shapes/PolygonTests.cs +++ b/tests/Avalonia.Controls.UnitTests/Shapes/PolygonTests.cs @@ -17,9 +17,12 @@ public void Polygon_Will_Update_Geometry_On_Shapes_Collection_Content_Change() target.Measure(new Size()); Assert.True(target.IsMeasureValid); + var root = new TestRoot(target); points.Add(new Point()); Assert.False(target.IsMeasureValid); + + root.Child = null; } } diff --git a/tests/Avalonia.Controls.UnitTests/Shapes/PolylineTests.cs b/tests/Avalonia.Controls.UnitTests/Shapes/PolylineTests.cs index a099deac161..a68dc9d4b4c 100644 --- a/tests/Avalonia.Controls.UnitTests/Shapes/PolylineTests.cs +++ b/tests/Avalonia.Controls.UnitTests/Shapes/PolylineTests.cs @@ -16,10 +16,13 @@ public void Polyline_Will_Update_Geometry_On_Shapes_Collection_Content_Change() 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; } } From a152b776445516c9a357d6ed5cc6df6ee0830337 Mon Sep 17 00:00:00 2001 From: SKProCH Date: Fri, 22 Mar 2024 23:35:34 +0300 Subject: [PATCH 7/7] Add memory leak tests --- tests/Avalonia.LeakTests/ControlTests.cs | 42 ++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/Avalonia.LeakTests/ControlTests.cs b/tests/Avalonia.LeakTests/ControlTests.cs index 8c393348206..36f08186381 100644 --- a/tests/Avalonia.LeakTests/ControlTests.cs +++ b/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(){new()}; + + Func run = () => + { + var window = new Window + { + Content = new Polyline() + { + Points = observableCollection + } + }; + + window.Show(); + + window.LayoutManager.ExecuteInitialLayoutPass(); + Assert.IsType(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()).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()