From 158ebe7710f9cdc8ec69a113200a6b8a66566695 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 26 Aug 2021 22:52:49 +0200 Subject: [PATCH 1/5] Lazy subscribe to collection changed. Subscribing to the inner collection's `CollectionChanged` event when an `ItemsSourceView` was created means the only way to unsubscribe is to dispose the `ItemsSourceView` meaning that the instance can't be easily shared. --- src/Avalonia.Controls/ItemsSourceView.cs | 48 ++++++++++++++++-------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/src/Avalonia.Controls/ItemsSourceView.cs b/src/Avalonia.Controls/ItemsSourceView.cs index b2663f3213..2884295386 100644 --- a/src/Avalonia.Controls/ItemsSourceView.cs +++ b/src/Avalonia.Controls/ItemsSourceView.cs @@ -33,7 +33,7 @@ namespace Avalonia.Controls public static ItemsSourceView Empty { get; } = new ItemsSourceView(Array.Empty()); private protected readonly IList _inner; - private INotifyCollectionChanged? _notifyCollectionChanged; + private NotifyCollectionChangedEventHandler? _collectionChanged; /// /// Initializes a new instance of the ItemsSourceView class for the specified data source. @@ -55,8 +55,6 @@ namespace Avalonia.Controls { _inner = new List(source.Cast()); } - - ListenToCollectionChanges(); } /// @@ -82,14 +80,41 @@ namespace Avalonia.Controls /// /// Occurs when the collection has changed to indicate the reason for the change and which items changed. /// - public event NotifyCollectionChangedEventHandler? CollectionChanged; + public event NotifyCollectionChangedEventHandler? CollectionChanged + { + add + { + if (_collectionChanged is null) + { + if (_inner is INotifyCollectionChanged incc) + { + incc.CollectionChanged += OnCollectionChanged; + } + } + + _collectionChanged += value; + } + + remove + { + _collectionChanged -= value; + + if (_collectionChanged is null) + { + if (_inner is INotifyCollectionChanged incc) + { + incc.CollectionChanged -= OnCollectionChanged; + } + } + } + } /// public void Dispose() { - if (_notifyCollectionChanged != null) + if (_inner is INotifyCollectionChanged incc) { - _notifyCollectionChanged.CollectionChanged -= OnCollectionChanged; + incc.CollectionChanged -= OnCollectionChanged; } } @@ -162,16 +187,7 @@ namespace Avalonia.Controls protected void OnItemsSourceChanged(NotifyCollectionChangedEventArgs args) { - CollectionChanged?.Invoke(this, args); - } - - private void ListenToCollectionChanges() - { - if (_inner is INotifyCollectionChanged incc) - { - incc.CollectionChanged += OnCollectionChanged; - _notifyCollectionChanged = incc; - } + _collectionChanged?.Invoke(this, args); } private void OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e) From 5ae9a2d60c902a1b5fb0067b65df9ef5ea0c398c Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 26 Aug 2021 23:15:07 +0200 Subject: [PATCH 2/5] Validate source collection, and add tests. --- src/Avalonia.Controls/ItemsSourceView.cs | 21 +++---- .../ItemsSourceViewTests.cs | 63 +++++++++++++++++++ 2 files changed, 72 insertions(+), 12 deletions(-) create mode 100644 tests/Avalonia.Controls.UnitTests/ItemsSourceViewTests.cs diff --git a/src/Avalonia.Controls/ItemsSourceView.cs b/src/Avalonia.Controls/ItemsSourceView.cs index 2884295386..d306939e4b 100644 --- a/src/Avalonia.Controls/ItemsSourceView.cs +++ b/src/Avalonia.Controls/ItemsSourceView.cs @@ -42,19 +42,16 @@ namespace Avalonia.Controls public ItemsSourceView(IEnumerable source) { source = source ?? throw new ArgumentNullException(nameof(source)); - - if (source is IList list) - { - _inner = list; - } - else if (source is IEnumerable objectEnumerable) + _inner = source switch { - _inner = new List(objectEnumerable); - } - else - { - _inner = new List(source.Cast()); - } + ItemsSourceView => throw new ArgumentException("Cannot wrap an existing ItemsSourceView.", nameof(source)), + IList list => list, + INotifyCollectionChanged => throw new ArgumentException( + "Collection implements INotifyCollectionChanged by not IList.", + nameof(source)), + IEnumerable iObj => new List(iObj), + _ => new List(source.Cast()) + }; } /// diff --git a/tests/Avalonia.Controls.UnitTests/ItemsSourceViewTests.cs b/tests/Avalonia.Controls.UnitTests/ItemsSourceViewTests.cs new file mode 100644 index 0000000000..529b3b1aa8 --- /dev/null +++ b/tests/Avalonia.Controls.UnitTests/ItemsSourceViewTests.cs @@ -0,0 +1,63 @@ +using System; +using System.Collections; +using System.Collections.Generic; +using System.Collections.Specialized; +using System.Text; +using Avalonia.Collections; +using Avalonia.Diagnostics; +using Xunit; + +namespace Avalonia.Controls.UnitTests +{ + public class ItemsSourceViewTests + { + [Fact] + public void Only_Subscribes_To_Source_CollectionChanged_When_CollectionChanged_Subscribed() + { + var source = new AvaloniaList(); + var target = new ItemsSourceView(source); + var debug = (INotifyCollectionChangedDebug)source; + + Assert.Null(debug.GetCollectionChangedSubscribers()); + + void Handler(object sender, NotifyCollectionChangedEventArgs e) { } + target.CollectionChanged += Handler; + + Assert.NotNull(debug.GetCollectionChangedSubscribers()); + Assert.Equal(1, debug.GetCollectionChangedSubscribers().Length); + + target.CollectionChanged -= Handler; + + Assert.Null(debug.GetCollectionChangedSubscribers()); + } + + [Fact] + public void Cannot_Wrap_An_ItemsSourceView_In_Another() + { + var source = new ItemsSourceView(new string[0]); + Assert.Throws(() => new ItemsSourceView(source)); + } + + [Fact] + public void Cannot_Create_ItemsSourceView_With_Collection_That_Implements_INCC_But_Not_List() + { + var source = new InvalidCollection(); + Assert.Throws(() => new ItemsSourceView(source)); + } + + private class InvalidCollection : INotifyCollectionChanged, IEnumerable + { + public event NotifyCollectionChangedEventHandler CollectionChanged; + + public IEnumerator GetEnumerator() + { + yield break; + } + + IEnumerator IEnumerable.GetEnumerator() + { + yield break; + } + } + } +} From 2517a70994f6630d80c96e90b324e39262416db9 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 26 Aug 2021 23:25:54 +0200 Subject: [PATCH 3/5] Expose inner list, and throw if disposed. --- src/Avalonia.Controls/ItemsSourceView.cs | 41 ++++++++++++++++++------ 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/src/Avalonia.Controls/ItemsSourceView.cs b/src/Avalonia.Controls/ItemsSourceView.cs index d306939e4b..e8869d6d0c 100644 --- a/src/Avalonia.Controls/ItemsSourceView.cs +++ b/src/Avalonia.Controls/ItemsSourceView.cs @@ -32,7 +32,7 @@ namespace Avalonia.Controls /// public static ItemsSourceView Empty { get; } = new ItemsSourceView(Array.Empty()); - private protected readonly IList _inner; + private IList? _inner; private NotifyCollectionChangedEventHandler? _collectionChanged; /// @@ -57,7 +57,7 @@ namespace Avalonia.Controls /// /// Gets the number of items in the collection. /// - public int Count => _inner.Count; + public int Count => Inner.Count; /// /// Gets a value that indicates whether the items source can provide a unique key for each item. @@ -67,6 +67,19 @@ namespace Avalonia.Controls /// public bool HasKeyIndexMapping => false; + /// + /// Gets the inner collection. + /// + public IList Inner + { + get + { + if (_inner is null) + ThrowDisposed(); + return _inner!; + } + } + /// /// Retrieves the item at the specified index. /// @@ -81,6 +94,9 @@ namespace Avalonia.Controls { add { + if (_inner is null) + ThrowDisposed(); + if (_collectionChanged is null) { if (_inner is INotifyCollectionChanged incc) @@ -94,6 +110,9 @@ namespace Avalonia.Controls remove { + if (_inner is null) + ThrowDisposed(); + _collectionChanged -= value; if (_collectionChanged is null) @@ -113,6 +132,8 @@ namespace Avalonia.Controls { incc.CollectionChanged -= OnCollectionChanged; } + + _inner = null; } /// @@ -120,9 +141,9 @@ namespace Avalonia.Controls /// /// The index. /// The item. - public object? GetAt(int index) => _inner[index]; + public object? GetAt(int index) => Inner[index]; - public int IndexOf(object? item) => _inner.IndexOf(item); + public int IndexOf(object? item) => Inner.IndexOf(item); public static ItemsSourceView GetOrCreate(IEnumerable? items) { @@ -168,7 +189,7 @@ namespace Avalonia.Controls internal void AddListener(ICollectionChangedListener listener) { - if (_inner is INotifyCollectionChanged incc) + if (Inner is INotifyCollectionChanged incc) { CollectionChangedEventManager.Instance.AddListener(incc, listener); } @@ -176,7 +197,7 @@ namespace Avalonia.Controls internal void RemoveListener(ICollectionChangedListener listener) { - if (_inner is INotifyCollectionChanged incc) + if (Inner is INotifyCollectionChanged incc) { CollectionChangedEventManager.Instance.RemoveListener(incc, listener); } @@ -191,6 +212,8 @@ namespace Avalonia.Controls { OnItemsSourceChanged(e); } + + private void ThrowDisposed() => throw new ObjectDisposedException(nameof(ItemsSourceView)); } public class ItemsSourceView : ItemsSourceView, IReadOnlyList @@ -229,10 +252,10 @@ namespace Avalonia.Controls /// The index. /// The item. [return: MaybeNull] - public new T GetAt(int index) => (T)_inner[index]; + public new T GetAt(int index) => (T)Inner[index]; - public IEnumerator GetEnumerator() => _inner.Cast().GetEnumerator(); - IEnumerator IEnumerable.GetEnumerator() => _inner.GetEnumerator(); + public IEnumerator GetEnumerator() => Inner.Cast().GetEnumerator(); + IEnumerator IEnumerable.GetEnumerator() => Inner.GetEnumerator(); public static new ItemsSourceView GetOrCreate(IEnumerable? items) { From be4614e4236977a93833c6091a1a52ea47c9e469 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 9 Sep 2021 15:19:48 +0200 Subject: [PATCH 4/5] Fix errors with old SDK. --- src/Avalonia.Controls/ItemsSourceView.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Avalonia.Controls/ItemsSourceView.cs b/src/Avalonia.Controls/ItemsSourceView.cs index e8869d6d0c..ca95b7acc8 100644 --- a/src/Avalonia.Controls/ItemsSourceView.cs +++ b/src/Avalonia.Controls/ItemsSourceView.cs @@ -44,9 +44,9 @@ namespace Avalonia.Controls source = source ?? throw new ArgumentNullException(nameof(source)); _inner = source switch { - ItemsSourceView => throw new ArgumentException("Cannot wrap an existing ItemsSourceView.", nameof(source)), + ItemsSourceView _ => throw new ArgumentException("Cannot wrap an existing ItemsSourceView.", nameof(source)), IList list => list, - INotifyCollectionChanged => throw new ArgumentException( + INotifyCollectionChanged _ => throw new ArgumentException( "Collection implements INotifyCollectionChanged by not IList.", nameof(source)), IEnumerable iObj => new List(iObj), From 1e6155fea377fe88ca07acef1eb8a8995992575b Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 30 Sep 2021 13:15:17 +0200 Subject: [PATCH 5/5] Simplify code a bit. --- src/Avalonia.Controls/ItemsSourceView.cs | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/src/Avalonia.Controls/ItemsSourceView.cs b/src/Avalonia.Controls/ItemsSourceView.cs index ca95b7acc8..c2d20495ef 100644 --- a/src/Avalonia.Controls/ItemsSourceView.cs +++ b/src/Avalonia.Controls/ItemsSourceView.cs @@ -94,15 +94,9 @@ namespace Avalonia.Controls { add { - if (_inner is null) - ThrowDisposed(); - - if (_collectionChanged is null) + if (_collectionChanged is null && Inner is INotifyCollectionChanged incc) { - if (_inner is INotifyCollectionChanged incc) - { - incc.CollectionChanged += OnCollectionChanged; - } + incc.CollectionChanged += OnCollectionChanged; } _collectionChanged += value; @@ -110,17 +104,11 @@ namespace Avalonia.Controls remove { - if (_inner is null) - ThrowDisposed(); - _collectionChanged -= value; - if (_collectionChanged is null) + if (_collectionChanged is null && Inner is INotifyCollectionChanged incc) { - if (_inner is INotifyCollectionChanged incc) - { - incc.CollectionChanged -= OnCollectionChanged; - } + incc.CollectionChanged -= OnCollectionChanged; } } }