WAF: ObservableList fix adding same item multiple times; improve Clea…

…r performance
jbe2277 · Apr 11, 2024 · fe07576 · fe07576
1 parent e9a65c6
commit fe07576
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 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
@@ -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<CollectionEventsTestModel?>(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<CollectionEventsTestModel?>(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<CollectionEventsTestModel>();
             CollectionEventsTestCore(list, list);
         }
-        
+
         internal static void CollectionEventsTestCore(ObservableCollection<CollectionEventsTestModel> source, object observable)
         {
             var collectionChangingArgs = new List<NotifyCollectionChangedEventArgs>();
@@ -119,7 +167,7 @@ public void CollectionItemChangedTest()
         internal static void CollectionItemChangedTestCore(ObservableCollection<CollectionEventsTestModel> 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
@@ -10,15 +10,15 @@ namespace System.Waf.Foundation
     /// <typeparam name="T">The type of elements in the collection.</typeparam>
     public class ObservableList<T> : ObservableCollection<T>, IReadOnlyObservableList<T>
     {
-        private readonly Dictionary<object, IWeakEventProxy> weakEventProxies = new();
+        private readonly Dictionary<INotifyPropertyChanged, (int count, IWeakEventProxy proxy)> weakEventProxies = new();
 
         /// <summary>Initializes a new instance of the <see cref="ObservableCollection{T}"/> class.</summary>
         public ObservableList() { }
 
         /// <summary>Initializes a new instance of the <see cref="ObservableCollection{T}"/> class that contains elements copied from the specified collection.</summary>
         /// <param name="collection">The collection from which the elements are copied.</param>
         /// <exception cref="ArgumentNullException">The collection parameter cannot be null.</exception>
-        public ObservableList(IEnumerable<T> collection) : base(collection) 
+        public ObservableList(IEnumerable<T> collection) : base(collection)
         {
             foreach (var x in this) TryAddItemPropertyChanged(x);
         }
@@ -39,7 +39,7 @@ public ObservableList(IEnumerable<T> collection) : base(collection)
         /// <inheritdoc />
         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();
+        }
     }
 }