Merge pull request #6485 from AvaloniaUI/fixes/itemssourceview-fixes

Tweaks to ItemsSourceView

Author: grokys

src/Avalonia.Controls/ItemsSourceView.cs

@@ -32,8 +32,8 @@ public class ItemsSourceView : INotifyCollectionChanged, IDisposable
         /// </summary>
         public static ItemsSourceView Empty { get; } = new ItemsSourceView(Array.Empty<object>());
 
-        private protected readonly IList _inner;
-        private INotifyCollectionChanged? _notifyCollectionChanged;
+        private IList? _inner;
+        private NotifyCollectionChangedEventHandler? _collectionChanged;
 
         /// <summary>
         /// Initializes a new instance of the ItemsSourceView class for the specified data source.
@@ -42,27 +42,22 @@ public class ItemsSourceView : INotifyCollectionChanged, IDisposable
         public ItemsSourceView(IEnumerable source)
         {
             source = source ?? throw new ArgumentNullException(nameof(source));
-
-            if (source is IList list)
-            {
-                _inner = list;
-            }
-            else if (source is IEnumerable<object> objectEnumerable)
+            _inner = source switch
             {
-                _inner = new List<object>(objectEnumerable);
-            }
-            else
-            {
-                _inner = new List<object>(source.Cast<object>());
-            }
-
-            ListenToCollectionChanges();
+                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<object> iObj => new List<object>(iObj),
+                _ => new List<object>(source.Cast<object>())
+            };
         }
 
         /// <summary>
         /// Gets the number of items in the collection.
         /// </summary>
-        public int Count => _inner.Count;
+        public int Count => Inner.Count;
 
         /// <summary>
         /// Gets a value that indicates whether the items source can provide a unique key for each item.
@@ -72,6 +67,19 @@ public ItemsSourceView(IEnumerable source)
         /// </remarks>
         public bool HasKeyIndexMapping => false;
 
+        /// <summary>
+        /// Gets the inner collection.
+        /// </summary>
+        public IList Inner
+        {
+            get
+            {
+                if (_inner is null)
+                    ThrowDisposed();
+                return _inner!;
+            }
+        }
+
         /// <summary>
         /// Retrieves the item at the specified index.
         /// </summary>
@@ -82,25 +90,48 @@ public ItemsSourceView(IEnumerable source)
         /// <summary>
         /// Occurs when the collection has changed to indicate the reason for the change and which items changed.
         /// </summary>
-        public event NotifyCollectionChangedEventHandler? CollectionChanged;
+        public event NotifyCollectionChangedEventHandler? CollectionChanged
+        {
+            add
+            {
+                if (_collectionChanged is null && Inner is INotifyCollectionChanged incc)
+                {
+                    incc.CollectionChanged += OnCollectionChanged;
+                }
+
+                _collectionChanged += value;
+            }
+
+            remove
+            {
+                _collectionChanged -= value;
+
+                if (_collectionChanged is null && Inner is INotifyCollectionChanged incc)
+                {
+                    incc.CollectionChanged -= OnCollectionChanged;
+                }
+            }
+        }
 
         /// <inheritdoc/>
         public void Dispose()
         {
-            if (_notifyCollectionChanged != null)
+            if (_inner is INotifyCollectionChanged incc)
             {
-                _notifyCollectionChanged.CollectionChanged -= OnCollectionChanged;
+                incc.CollectionChanged -= OnCollectionChanged;
             }
+
+            _inner = null;
         }
 
         /// <summary>
         /// Retrieves the item at the specified index.
         /// </summary>
         /// <param name="index">The index.</param>
         /// <returns>The item.</returns>
-        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)
         {
@@ -146,38 +177,31 @@ public int IndexFromKey(string key)
 
         internal void AddListener(ICollectionChangedListener listener)
         {
-            if (_inner is INotifyCollectionChanged incc)
+            if (Inner is INotifyCollectionChanged incc)
             {
                 CollectionChangedEventManager.Instance.AddListener(incc, listener);
             }
         }
 
         internal void RemoveListener(ICollectionChangedListener listener)
         {
-            if (_inner is INotifyCollectionChanged incc)
+            if (Inner is INotifyCollectionChanged incc)
             {
                 CollectionChangedEventManager.Instance.RemoveListener(incc, listener);
             }
         }
 
         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)
         {
             OnItemsSourceChanged(e);
         }
+
+        private void ThrowDisposed() => throw new ObjectDisposedException(nameof(ItemsSourceView));
     }
 
     public class ItemsSourceView<T> : ItemsSourceView, IReadOnlyList<T>
@@ -216,10 +240,10 @@ private ItemsSourceView(IEnumerable source)
         /// <param name="index">The index.</param>
         /// <returns>The item.</returns>
         [return: MaybeNull]
-        public new T GetAt(int index) => (T)_inner[index];
+        public new T GetAt(int index) => (T)Inner[index];
 
-        public IEnumerator<T> GetEnumerator() => _inner.Cast<T>().GetEnumerator();
-        IEnumerator IEnumerable.GetEnumerator() => _inner.GetEnumerator();
+        public IEnumerator<T> GetEnumerator() => Inner.Cast<T>().GetEnumerator();
+        IEnumerator IEnumerable.GetEnumerator() => Inner.GetEnumerator();
 
         public static new ItemsSourceView<T> GetOrCreate(IEnumerable? items)
         {

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<string>();
+            var target = new ItemsSourceView<string>(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<string>(new string[0]);
+            Assert.Throws<ArgumentException>(() => new ItemsSourceView<string>(source));
+        }
+
+        [Fact]
+        public void Cannot_Create_ItemsSourceView_With_Collection_That_Implements_INCC_But_Not_List()
+        {
+            var source = new InvalidCollection();
+            Assert.Throws<ArgumentException>(() => new ItemsSourceView<string>(source));
+        }
+
+        private class InvalidCollection : INotifyCollectionChanged, IEnumerable<string>
+        {
+            public event NotifyCollectionChangedEventHandler CollectionChanged;
+
+            public IEnumerator<string> GetEnumerator()
+            {
+                yield break;
+            }
+
+            IEnumerator IEnumerable.GetEnumerator()
+            {
+                yield break;
+            }
+        }
+    }
+}