From 4330451d2c53b561a87cf8fa0aef71792ee65efb Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 28 Aug 2020 15:43:04 +0200 Subject: [PATCH 1/2] Added tests for CollectionChangedEventManager. Including a failing test: `Receives_Events_From_Wrapped_Collection`. --- .../CollectionChangedEventManagerTests.cs | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 tests/Avalonia.Controls.UnitTests/Utils/CollectionChangedEventManagerTests.cs diff --git a/tests/Avalonia.Controls.UnitTests/Utils/CollectionChangedEventManagerTests.cs b/tests/Avalonia.Controls.UnitTests/Utils/CollectionChangedEventManagerTests.cs new file mode 100644 index 0000000000..8bbb5e456f --- /dev/null +++ b/tests/Avalonia.Controls.UnitTests/Utils/CollectionChangedEventManagerTests.cs @@ -0,0 +1,89 @@ +using System; +using System.Collections.Generic; +using System.Collections.Specialized; +using System.Text; +using Avalonia.Collections; +using Avalonia.Controls.Utils; +using Xunit; +using CollectionChangedEventManager = Avalonia.Controls.Utils.CollectionChangedEventManager; + +namespace Avalonia.Controls.UnitTests.Utils +{ + public class CollectionChangedEventManagerTests + { + [Fact] + public void AddListener_Listens_To_Events() + { + var source = new AvaloniaList(); + var listener = new Listener(); + + CollectionChangedEventManager.Instance.AddListener(source, listener); + + Assert.Empty(listener.Received); + + source.Add("foo"); + + Assert.Equal(1, listener.Received.Count); + } + + [Fact] + public void RemoveListener_Stops_Listening_To_Events() + { + var source = new AvaloniaList(); + var listener = new Listener(); + + CollectionChangedEventManager.Instance.AddListener(source, listener); + CollectionChangedEventManager.Instance.RemoveListener(source, listener); + + source.Add("foo"); + + Assert.Empty(listener.Received); + } + + [Fact] + public void Receives_Events_From_Wrapped_Collection() + { + var source = new WrappingCollection(); + var listener = new Listener(); + + CollectionChangedEventManager.Instance.AddListener(source, listener); + + Assert.Empty(listener.Received); + + source.Add("foo"); + + Assert.Equal(1, listener.Received.Count); + } + + private class Listener : ICollectionChangedListener + { + public List Received { get; } = new List(); + + public void Changed(INotifyCollectionChanged sender, NotifyCollectionChangedEventArgs e) + { + Received.Add(e); + } + + public void PostChanged(INotifyCollectionChanged sender, NotifyCollectionChangedEventArgs e) + { + } + + public void PreChanged(INotifyCollectionChanged sender, NotifyCollectionChangedEventArgs e) + { + } + } + + private class WrappingCollection : INotifyCollectionChanged + { + private AvaloniaList _inner = new AvaloniaList(); + + public void Add(string s) => _inner.Add(s); + + public event NotifyCollectionChangedEventHandler CollectionChanged + { + add => _inner.CollectionChanged += value; + remove => _inner.CollectionChanged -= value; + } + } + } +} From d5fae441df1f5e580296ec9c2451e8230de6499b Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 28 Aug 2020 23:15:24 +0200 Subject: [PATCH 2/2] Don't use sender as the lookup key. #4577 was caused because the `sender` of the `CollectionChanged` event didn't correspond to the collection that the event listener was registered on. Instead create an `Entry` object for each collection and put the event handler on that. --- .../Utils/CollectionChangedEventManager.cs | 100 ++++++++++-------- 1 file changed, 58 insertions(+), 42 deletions(-) diff --git a/src/Avalonia.Controls/Utils/CollectionChangedEventManager.cs b/src/Avalonia.Controls/Utils/CollectionChangedEventManager.cs index 1e5ada8409..c26c5e0513 100644 --- a/src/Avalonia.Controls/Utils/CollectionChangedEventManager.cs +++ b/src/Avalonia.Controls/Utils/CollectionChangedEventManager.cs @@ -17,12 +17,12 @@ namespace Avalonia.Controls.Utils void PostChanged(INotifyCollectionChanged sender, NotifyCollectionChangedEventArgs e); } - internal class CollectionChangedEventManager : IWeakSubscriber + internal class CollectionChangedEventManager { public static CollectionChangedEventManager Instance { get; } = new CollectionChangedEventManager(); - private ConditionalWeakTable>> _entries = - new ConditionalWeakTable>>(); + private ConditionalWeakTable _entries = + new ConditionalWeakTable(); private CollectionChangedEventManager() { @@ -34,17 +34,13 @@ namespace Avalonia.Controls.Utils listener = listener ?? throw new ArgumentNullException(nameof(listener)); Dispatcher.UIThread.VerifyAccess(); - if (!_entries.TryGetValue(collection, out var listeners)) + if (!_entries.TryGetValue(collection, out var entry)) { - listeners = new List>(); - _entries.Add(collection, listeners); - WeakSubscriptionManager.Subscribe( - collection, - nameof(INotifyCollectionChanged.CollectionChanged), - this); + entry = new Entry(collection); + _entries.Add(collection, entry); } - foreach (var l in listeners) + foreach (var l in entry.Listeners) { if (l.TryGetTarget(out var target) && target == listener) { @@ -53,7 +49,7 @@ namespace Avalonia.Controls.Utils } } - listeners.Add(new WeakReference(listener)); + entry.Listeners.Add(new WeakReference(listener)); } public void RemoveListener(INotifyCollectionChanged collection, ICollectionChangedListener listener) @@ -62,8 +58,10 @@ namespace Avalonia.Controls.Utils listener = listener ?? throw new ArgumentNullException(nameof(listener)); Dispatcher.UIThread.VerifyAccess(); - if (_entries.TryGetValue(collection, out var listeners)) + if (_entries.TryGetValue(collection, out var entry)) { + var listeners = entry.Listeners; + for (var i = 0; i < listeners.Count; ++i) { if (listeners[i].TryGetTarget(out var target) && target == listener) @@ -72,10 +70,7 @@ namespace Avalonia.Controls.Utils if (listeners.Count == 0) { - WeakSubscriptionManager.Unsubscribe( - collection, - nameof(INotifyCollectionChanged.CollectionChanged), - this); + entry.Dispose(); _entries.Remove(collection); } @@ -88,51 +83,72 @@ namespace Avalonia.Controls.Utils "Collection listener not registered for this collection/listener combination."); } - void IWeakSubscriber.OnEvent(object sender, NotifyCollectionChangedEventArgs e) + private class Entry : IWeakSubscriber, IDisposable { - static void Notify( - INotifyCollectionChanged incc, - NotifyCollectionChangedEventArgs args, - List> listeners) + private INotifyCollectionChanged _collection; + + public Entry(INotifyCollectionChanged collection) { - foreach (var l in listeners) + _collection = collection; + Listeners = new List>(); + WeakSubscriptionManager.Subscribe( + _collection, + nameof(INotifyCollectionChanged.CollectionChanged), + this); + } + + public List> Listeners { get; } + + public void Dispose() + { + WeakSubscriptionManager.Unsubscribe( + _collection, + nameof(INotifyCollectionChanged.CollectionChanged), + this); + } + + void IWeakSubscriber.OnEvent(object sender, NotifyCollectionChangedEventArgs e) + { + static void Notify( + INotifyCollectionChanged incc, + NotifyCollectionChangedEventArgs args, + List> listeners) { - if (l.TryGetTarget(out var target)) + foreach (var l in listeners) { - target.PreChanged(incc, args); + if (l.TryGetTarget(out var target)) + { + target.PreChanged(incc, args); + } } - } - foreach (var l in listeners) - { - if (l.TryGetTarget(out var target)) + foreach (var l in listeners) { - target.Changed(incc, args); + if (l.TryGetTarget(out var target)) + { + target.Changed(incc, args); + } } - } - foreach (var l in listeners) - { - if (l.TryGetTarget(out var target)) + foreach (var l in listeners) { - target.PostChanged(incc, args); + if (l.TryGetTarget(out var target)) + { + target.PostChanged(incc, args); + } } } - } - if (sender is INotifyCollectionChanged incc && _entries.TryGetValue(incc, out var listeners)) - { - var l = listeners.ToList(); + var l = Listeners.ToList(); if (Dispatcher.UIThread.CheckAccess()) { - Notify(incc, e, l); + Notify(_collection, e, l); } else { - var inccCapture = incc; var eCapture = e; - Dispatcher.UIThread.Post(() => Notify(inccCapture, eCapture, l)); + Dispatcher.UIThread.Post(() => Notify(_collection, eCapture, l)); } } }