Merge pull request #4593 from AvaloniaUI/fixes/selectionmodel-collectionchanged

Try to handle selection being changed in CollectionChanged handler.

src/Avalonia.Controls/Selection/SelectionModel.cs

@@ -436,6 +436,29 @@ namespace Avalonia.Controls.Selection
             }
         }
 
+        private protected override bool IsValidCollectionChange(NotifyCollectionChangedEventArgs e)
+        {
+            if (!base.IsValidCollectionChange(e))
+            {
+                return false;
+            }
+
+            if (ItemsView is object && e.Action == NotifyCollectionChangedAction.Add)
+            {
+                if (e.NewStartingIndex <= _selectedIndex)
+                {
+                    return _selectedIndex + e.NewItems.Count < ItemsView.Count;
+                }
+
+                if (e.NewStartingIndex <= _anchorIndex)
+                {
+                    return _anchorIndex + e.NewItems.Count < ItemsView.Count;
+                }
+            }
+
+            return true;
+        }
+
         protected override void OnSourceCollectionChangeFinished()
         {
             if (_operation is object)

src/Avalonia.Controls/Selection/SelectionNodeBase.cs

@@ -2,6 +2,7 @@
 using System.Collections;
 using System.Collections.Generic;
 using System.Collections.Specialized;
+using System.Linq;
 using Avalonia.Controls.Utils;
 
 #nullable enable
@@ -234,6 +235,11 @@ namespace Avalonia.Controls.Selection
             var shiftIndex = -1;
             List<T>? removed = null;
 
+            if (!IsValidCollectionChange(e))
+            {
+                return;
+            }
+
             switch (e.Action)
             {
                 case NotifyCollectionChangedAction.Add:
@@ -276,6 +282,34 @@ namespace Avalonia.Controls.Selection
             }
         }
 
+        private protected virtual bool IsValidCollectionChange(NotifyCollectionChangedEventArgs e)
+        {
+            // If the selection is modified in a CollectionChanged handler before the selection
+            // model's CollectionChanged handler has had chance to run then we can end up with
+            // a selected index that refers to the *new* state of the Source intermixed with
+            // indexes that reference an old state of the source.
+            //
+            // There's not much we can do in this situation, so detect whether shifting the
+            // current selected indexes would result in an invalid index in the source, and if
+            // so bail.
+            //
+            // See unit test Handles_Selection_Made_In_CollectionChanged for more details.
+            if (ItemsView is object &&
+                RangesEnabled &&
+                Ranges.Count > 0 &&
+                e.Action == NotifyCollectionChangedAction.Add)
+            {
+                var lastIndex = Ranges.Last().End;
+
+                if (e.NewStartingIndex <= lastIndex)
+                {
+                    return lastIndex + e.NewItems.Count < ItemsView.Count;
+                }
+            }
+
+            return true;
+        }
+
         private protected struct CollectionChangeState
         {
             public int ShiftIndex;

tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs

@@ -1335,6 +1335,47 @@ namespace Avalonia.Controls.UnitTests.Primitives
             Assert.False(model.SingleSelect);
         }
 
+        [Fact]
+        public void Does_The_Best_It_Can_With_AutoSelecting_ViewModel()
+        {
+            // Tests the following scenario:
+            //
+            // - Items changes from empty to having 1 item
+            // - ViewModel auto-selects item 0 in CollectionChanged
+            // - SelectionModel receives CollectionChanged
+            // - And so adjusts the selected item from 0 to 1, which is past the end of the items.
+            //
+            // There's not much we can do about this situation because the order in which
+            // CollectionChanged handlers are called can't be known (the problem also exists with
+            // WPF). The best we can do is not select an invalid index.
+            var vm = new SelectionViewModel();
+
+            vm.Items.CollectionChanged += (s, e) =>
+            {
+                if (vm.SelectedIndex == -1 && vm.Items.Count > 0)
+                {
+                    vm.SelectedIndex = 0;
+                }
+            };
+
+            var target = new ListBox
+            {
+                [!ListBox.ItemsProperty] = new Binding("Items"),
+                [!ListBox.SelectedIndexProperty] = new Binding("SelectedIndex"),
+                DataContext = vm,
+            };
+
+            Prepare(target);
+
+            vm.Items.Add("foo");
+            vm.Items.Add("bar");
+
+            Assert.Equal(0, target.SelectedIndex);
+            Assert.Equal(new[] { 0 }, target.Selection.SelectedIndexes);
+            Assert.Equal("foo", target.SelectedItem);
+            Assert.Equal(new[] { "foo" }, target.SelectedItems);
+        }
+
         private static void Prepare(SelectingItemsControl target)
         {
             var root = new TestRoot
@@ -1397,6 +1438,28 @@ namespace Avalonia.Controls.UnitTests.Primitives
             public int SelectedIndex { get; set; }
         }
 
+        private class SelectionViewModel : NotifyingBase
+        {
+            private int _selectedIndex = -1;
+
+            public SelectionViewModel()
+            {
+                Items = new ObservableCollection<string>();
+            }
+
+            public int SelectedIndex
+            {
+                get => _selectedIndex;
+                set
+                {
+                    _selectedIndex = value;
+                    RaisePropertyChanged();
+                }
+            }
+
+            public ObservableCollection<string> Items { get; }
+        }
+
         private class RootWithItems : TestRoot
         {
             public List<string> Items { get; set; } = new List<string>() { "a", "b", "c", "d", "e" };

tests/Avalonia.Controls.UnitTests/Selection/SelectionModelTests_Multiple.cs

@@ -1230,6 +1230,38 @@ namespace Avalonia.Controls.UnitTests.Selection
                 Assert.Equal(1, resetRaised);
                 Assert.Equal(1, selectedIndexRaised);
             }
+
+            [Fact]
+            public void Handles_Selection_Made_In_CollectionChanged()
+            {
+                // Tests the following scenario:
+                //
+                // - Items changes from empty to having 2 items
+                // - ViewModel auto-selects range 0..1 in CollectionChanged
+                // - SelectionModel receives CollectionChanged
+                // - And so adjusts the selected item from 0..1 to 2..4, which is past the end of
+                //   the items.
+                //
+                // There's not much we can do about this situation because the order in which
+                // CollectionChanged handlers are called can't be known (the problem also exists with
+                // WPF). The best we can do is not select an invalid index.
+                var target = CreateTarget(createData: false);
+                var data = new AvaloniaList<string>();
+
+                data.CollectionChanged += (s, e) =>
+                {
+                    target.SelectRange(0, 1);
+                };
+
+                target.Source = data;
+                data.AddRange(new[] { "foo", "bar" });
+
+                Assert.Equal(0, target.SelectedIndex);
+                Assert.Equal(new[] { 0, 1 }, target.SelectedIndexes);
+                Assert.Equal("foo", target.SelectedItem);
+                Assert.Equal(new[] { "foo", "bar" }, target.SelectedItems);
+                Assert.Equal(0, target.AnchorIndex);
+            }
         }
 
         public class BatchUpdate

tests/Avalonia.Controls.UnitTests/Selection/SelectionModelTests_Single.cs

@@ -1052,6 +1052,37 @@ namespace Avalonia.Controls.UnitTests.Selection
                 Assert.Equal(1, resetRaised);
                 Assert.Equal(1, selectedIndexRaised);
             }
+
+            [Fact]
+            public void Handles_Selection_Made_In_CollectionChanged()
+            {
+                // Tests the following scenario:
+                //
+                // - Items changes from empty to having 1 item
+                // - ViewModel auto-selects item 0 in CollectionChanged
+                // - SelectionModel receives CollectionChanged
+                // - And so adjusts the selected item from 0 to 1, which is past the end of the items.
+                //
+                // There's not much we can do about this situation because the order in which
+                // CollectionChanged handlers are called can't be known (the problem also exists with
+                // WPF). The best we can do is not select an invalid index.
+                var target = CreateTarget(createData: false);
+                var data = new AvaloniaList<string>();
+
+                data.CollectionChanged += (s, e) =>
+                {
+                    target.Select(0);
+                };
+
+                target.Source = data;
+                data.Add("foo");
+
+                Assert.Equal(0, target.SelectedIndex);
+                Assert.Equal(new[] { 0 }, target.SelectedIndexes);
+                Assert.Equal("foo", target.SelectedItem);
+                Assert.Equal(new[] { "foo" }, target.SelectedItems);
+                Assert.Equal(0, target.AnchorIndex);
+            }
         }
 
         public class BatchUpdate