diff --git a/src/Controls/src/Core/Shapes/TransformGroup.cs b/src/Controls/src/Core/Shapes/TransformGroup.cs index 992da66f05a4..1d348ece6eaf 100644 --- a/src/Controls/src/Core/Shapes/TransformGroup.cs +++ b/src/Controls/src/Core/Shapes/TransformGroup.cs @@ -1,4 +1,5 @@ #nullable disable +using System.Collections.Generic; using System.Collections.Specialized; using System.ComponentModel; @@ -10,10 +11,11 @@ namespace Microsoft.Maui.Controls.Shapes [ContentProperty("Children")] public sealed class TransformGroup : Transform { + readonly Dictionary _subscribedTransforms = new(); + /// Bindable property for . public static readonly BindableProperty ChildrenProperty = - BindableProperty.Create(nameof(Children), typeof(TransformCollection), typeof(TransformGroup), null, - propertyChanged: OnTransformGroupChanged); + BindableProperty.Create(nameof(Children), typeof(TransformCollection), typeof(TransformGroup), null, propertyChanged: OnChildrenChanged); /// /// Initializes a new instance of the class. @@ -32,38 +34,125 @@ public TransformCollection Children get { return (TransformCollection)GetValue(ChildrenProperty); } } - static void OnTransformGroupChanged(BindableObject bindable, object oldValue, object newValue) + static void OnChildrenChanged(BindableObject bindable, object oldValue, object newValue) + { + var transformGroup = (TransformGroup)bindable; + transformGroup.UpdateChildren( + oldValue as TransformCollection, + newValue as TransformCollection); + } + + void UpdateChildren(TransformCollection oldCollection, TransformCollection newCollection) { - if (oldValue != null) + DetachCollection(oldCollection); + AttachCollection(newCollection); + + UpdateTransformMatrix(); + } + + void AttachCollection(TransformCollection collection) + { + if (collection is null) + { + return; + } + + collection.CollectionChanged += OnChildrenCollectionChanged; + + foreach (var transform in collection) { - (oldValue as TransformCollection).CollectionChanged -= (bindable as TransformGroup).OnChildrenCollectionChanged; + SubscribeToTransformPropertyChanged(transform); } + } - if (newValue != null) + void DetachCollection(TransformCollection collection) + { + if (collection is null) { - (newValue as TransformCollection).CollectionChanged += (bindable as TransformGroup).OnChildrenCollectionChanged; + return; } - (bindable as TransformGroup).UpdateTransformMatrix(); + collection.CollectionChanged -= OnChildrenCollectionChanged; + + ClearAllTransformSubscriptions(); } void OnChildrenCollectionChanged(object sender, NotifyCollectionChangedEventArgs args) { - if (args.NewItems != null) - foreach (INotifyPropertyChanged item in args.NewItems) + if (args.Action == NotifyCollectionChangedAction.Reset) + { + ClearAllTransformSubscriptions(); + + if (sender is TransformCollection collection) + { + foreach (INotifyPropertyChanged item in collection) + { + SubscribeToTransformPropertyChanged(item); + } + } + } + else + { + if (args.OldItems is not null) { - item.PropertyChanged += OnTransformPropertyChanged; + foreach (INotifyPropertyChanged item in args.OldItems) + { + UnsubscribeFromTransformPropertyChanged(item); + } } - if (args.OldItems != null) - foreach (INotifyPropertyChanged item in args.OldItems) + if (args.NewItems is not null) { - item.PropertyChanged -= OnTransformPropertyChanged; + foreach (INotifyPropertyChanged item in args.NewItems) + { + SubscribeToTransformPropertyChanged(item); + } } + } UpdateTransformMatrix(); } + void SubscribeToTransformPropertyChanged(INotifyPropertyChanged item) + { + if (_subscribedTransforms.TryGetValue(item, out int count)) + { + _subscribedTransforms[item] = count + 1; + return; + } + + item.PropertyChanged += OnTransformPropertyChanged; + _subscribedTransforms[item] = 1; + } + + void UnsubscribeFromTransformPropertyChanged(INotifyPropertyChanged item) + { + if (!_subscribedTransforms.TryGetValue(item, out int count)) + { + return; + } + + if (count > 1) + { + _subscribedTransforms[item] = count - 1; + return; + } + + item.PropertyChanged -= OnTransformPropertyChanged; + _subscribedTransforms.Remove(item); + } + + // Unsubscribes all tracked transforms from PropertyChanged and clears the dictionary. + void ClearAllTransformSubscriptions() + { + foreach (var item in _subscribedTransforms) + { + item.Key.PropertyChanged -= OnTransformPropertyChanged; + } + + _subscribedTransforms.Clear(); + } + void OnTransformPropertyChanged(object sender, PropertyChangedEventArgs args) { UpdateTransformMatrix(); diff --git a/src/Controls/tests/Core.UnitTests/Shapes/TransformGroupTests.cs b/src/Controls/tests/Core.UnitTests/Shapes/TransformGroupTests.cs new file mode 100644 index 000000000000..1454597ff147 --- /dev/null +++ b/src/Controls/tests/Core.UnitTests/Shapes/TransformGroupTests.cs @@ -0,0 +1,87 @@ +using System; +using System.ComponentModel; +using System.Threading.Tasks; +using Microsoft.Maui.Controls.Shapes; +using Xunit; + +namespace Microsoft.Maui.Controls.Core.UnitTests.Shapes +{ + public class TransformGroupTests : BaseTestFixture + { + [Fact] + public async Task ReplacingChildrenUnsubscribesFromOldChildPropertyChanged() + { + var sharedTransform = new ScaleTransform { ScaleX = 1.0, ScaleY = 1.0 }; + WeakReference weakGroup; + + { + var group = new TransformGroup(); + group.Children.Add(sharedTransform); + group.Children = new TransformCollection(); + weakGroup = new WeakReference(group); + } + + Assert.False(await weakGroup.WaitForCollect(), + "TransformGroup should be collected after Children replacement. " + + "Shared child transform is keeping it alive via stale PropertyChanged subscription."); + } + + [Fact] + public void ReplacingChildrenSubscribesToNewChildPropertyChanged() + { + var group = new TransformGroup(); + var newCollection = new TransformCollection(); + var childTransform = new ScaleTransform { ScaleX = 1.0, ScaleY = 1.0 }; + newCollection.Add(childTransform); + + group.Children = newCollection; + + var matrixBefore = group.Value; + childTransform.ScaleX = 2.0; + var matrixAfter = group.Value; + + Assert.NotEqual(matrixBefore, matrixAfter); + } + + [Fact] + public async Task SharedTransformDoesNotRetainMultipleGroups() + { + var sharedTransform = new ScaleTransform { ScaleX = 1.0, ScaleY = 1.0 }; + var weakRefs = new WeakReference[10]; + + { + for (int i = 0; i < 10; i++) + { + var group = new TransformGroup(); + group.Children.Add(sharedTransform); + group.Children = new TransformCollection(); + weakRefs[i] = new WeakReference(group); + } + } + + for (int i = 0; i < weakRefs.Length; i++) + { + Assert.False(await weakRefs[i].WaitForCollect(), + $"TransformGroup #{i} should be collected. Shared transform is retaining it."); + } + } + + [Fact] + public async Task ClearingChildrenUnsubscribesAllTransforms() + { + var sharedTransform = new ScaleTransform { ScaleX = 1.0, ScaleY = 1.0 }; + WeakReference weakGroup; + + { + var group = new TransformGroup(); + group.Children.Add(sharedTransform); + group.Children.Clear(); + weakGroup = new WeakReference(group); + } + + Assert.False(await weakGroup.WaitForCollect(), + "TransformGroup should be collected after Children.Clear(). " + + "Shared child transform is keeping it alive via stale PropertyChanged subscription."); + } + } +} \ No newline at end of file