From fe075766561a515b1af7bba034d7baae2a7f0a7b Mon Sep 17 00:00:00 2001 From: jbe2277 Date: Thu, 11 Apr 2024 20:14:50 +0200 Subject: [PATCH] WAF: ObservableList fix adding same item multiple times; improve Clear performance --- .../Foundation/ObservableListTest.cs | 52 ++++++++++++++++++- .../Foundation/ObservableList.cs | 40 ++++++++++---- 2 files changed, 79 insertions(+), 13 deletions(-) diff --git a/src/System.Waf/System.Waf/System.Waf.Core.Test/Foundation/ObservableListTest.cs b/src/System.Waf/System.Waf/System.Waf.Core.Test/Foundation/ObservableListTest.cs index 97653139..7c9fc9ff 100644 --- a/src/System.Waf/System.Waf/System.Waf.Core.Test/Foundation/ObservableListTest.cs +++ b/src/System.Waf/System.Waf/System.Waf.Core.Test/Foundation/ObservableListTest.cs @@ -22,6 +22,54 @@ internal class CollectionEventsTestModel : Model [TestClass] public class ObservableListTest { + [TestMethod] + public void AddSameObjectTwiceTest() + { + var collectionItemChangedList = new List<(object? item, string? propertyName)>(); + + var item = new CollectionEventsTestModel { Name = "1" }; + var list = new ObservableList(new[] { item, item }); + list.CollectionItemChanged += CollectionItemChangedHandler; + list.Add(item); + list.Add(null); + list[list.Count - 1] = item; + item.Name = "2"; + Assert.AreEqual(nameof(item.Name), collectionItemChangedList.Single().propertyName); + collectionItemChangedList.Clear(); + + list[list.Count - 1] = null; + list.Remove(item); + item.Name = "3"; + Assert.AreEqual(nameof(item.Name), collectionItemChangedList.Single().propertyName); + collectionItemChangedList.Clear(); + + list.Clear(); + item.Name = "4"; + Assert.AreEqual(0, collectionItemChangedList.Count); + + void CollectionItemChangedHandler(object? item, PropertyChangedEventArgs e) => collectionItemChangedList.Add((item, e.PropertyName)); + } + + [TestMethod] + public void UseNullAsItemTest() + { + var list = new ObservableList(new CollectionEventsTestModel?[] { null }); + AssertHelper.SequenceEqual(new CollectionEventsTestModel?[] { null }, list); + list.Remove(null); + Assert.AreEqual(0, list.Count); + + list.Add(null); + AssertHelper.SequenceEqual(new CollectionEventsTestModel?[] { null }, list); + list.RemoveAt(0); + Assert.AreEqual(0, list.Count); + + list.Add(new()); + list[0] = null; + AssertHelper.SequenceEqual(new CollectionEventsTestModel?[] { null }, list); + list.Clear(); + Assert.AreEqual(0, list.Count); + } + [TestMethod] public void PropertyChangedEventTest() { @@ -50,7 +98,7 @@ public void CollectionEventsTest() var list = new ObservableList(); CollectionEventsTestCore(list, list); } - + internal static void CollectionEventsTestCore(ObservableCollection source, object observable) { var collectionChangingArgs = new List(); @@ -119,7 +167,7 @@ public void CollectionItemChangedTest() internal static void CollectionItemChangedTestCore(ObservableCollection source, object observable) { var collectionItemChangedList = new List<(object? item, string? propertyName)>(); - + var item1 = source[0]; item1.Name = "Empty"; diff --git a/src/System.Waf/System.Waf/System.Waf.Core/Foundation/ObservableList.cs b/src/System.Waf/System.Waf/System.Waf.Core/Foundation/ObservableList.cs index b0f9948b..3ea57167 100644 --- a/src/System.Waf/System.Waf/System.Waf.Core/Foundation/ObservableList.cs +++ b/src/System.Waf/System.Waf/System.Waf.Core/Foundation/ObservableList.cs @@ -10,7 +10,7 @@ namespace System.Waf.Foundation /// The type of elements in the collection. public class ObservableList : ObservableCollection, IReadOnlyObservableList { - private readonly Dictionary weakEventProxies = new(); + private readonly Dictionary weakEventProxies = new(); /// Initializes a new instance of the class. public ObservableList() { } @@ -18,7 +18,7 @@ public ObservableList() { } /// Initializes a new instance of the class that contains elements copied from the specified collection. /// The collection from which the elements are copied. /// The collection parameter cannot be null. - public ObservableList(IEnumerable collection) : base(collection) + public ObservableList(IEnumerable collection) : base(collection) { foreach (var x in this) TryAddItemPropertyChanged(x); } @@ -39,7 +39,7 @@ public ObservableList(IEnumerable collection) : base(collection) /// protected override void ClearItems() { - foreach (var x in this) TryRemoveItemPropertyChanged(x); + ClearItemPropertyChanged(); OnCollectionChanging(EventArgsCache.ResetCollectionChanged); base.ClearItems(); } @@ -90,19 +90,37 @@ protected override void MoveItem(int oldIndex, int newIndex) private void ItemPropertyChanged(object? sender, PropertyChangedEventArgs e) => OnCollectionItemChanged(sender, e); - private void TryAddItemPropertyChanged(T? item) - { - if (item is INotifyPropertyChanged x) weakEventProxies.Add(x, WeakEvent.PropertyChanged.Add(x, ItemPropertyChanged)); + private void TryAddItemPropertyChanged(T? item) + { + if (item is not INotifyPropertyChanged observable) return; + if (weakEventProxies.TryGetValue(observable, out var x)) + { + weakEventProxies[observable] = (x.count + 1, x.proxy); + } + else + { + weakEventProxies.Add(observable, (1, WeakEvent.PropertyChanged.Add(observable, ItemPropertyChanged))); + } } - private void TryRemoveItemPropertyChanged(T? item) + private void TryRemoveItemPropertyChanged(T? item) { - if (item is null) return; - if (weakEventProxies.TryGetValue(item, out var proxy)) + if (item is not INotifyPropertyChanged observable) return; + if (weakEventProxies.TryGetValue(observable, out var x)) { - proxy.Remove(); - weakEventProxies.Remove(item); + if (x.count <= 1) + { + x.proxy.Remove(); + weakEventProxies.Remove(observable); + } + else weakEventProxies[observable] = (x.count - 1, x.proxy); } } + + private void ClearItemPropertyChanged() + { + foreach (var x in weakEventProxies.Values) x.proxy.Remove(); + weakEventProxies.Clear(); + } } }