From 525dbbb2e7e16afb2e86b4f1a64bc209168ed3e6 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Tue, 1 Sep 2020 16:54:17 +0200 Subject: [PATCH 1/2] Addeding failing selection tests. --- .../Primitives/SelectingItemsControlTests.cs | 63 +++++++++++++++++++ .../Selection/SelectionModelTests_Multiple.cs | 32 ++++++++++ .../Selection/SelectionModelTests_Single.cs | 31 +++++++++ 3 files changed, 126 insertions(+) diff --git a/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs b/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs index 33744949c3..00d148093a 100644 --- a/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs +++ b/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(); + } + + public int SelectedIndex + { + get => _selectedIndex; + set + { + _selectedIndex = value; + RaisePropertyChanged(); + } + } + + public ObservableCollection Items { get; } + } + private class RootWithItems : TestRoot { public List Items { get; set; } = new List() { "a", "b", "c", "d", "e" }; diff --git a/tests/Avalonia.Controls.UnitTests/Selection/SelectionModelTests_Multiple.cs b/tests/Avalonia.Controls.UnitTests/Selection/SelectionModelTests_Multiple.cs index 3640faf7cb..3eddd35465 100644 --- a/tests/Avalonia.Controls.UnitTests/Selection/SelectionModelTests_Multiple.cs +++ b/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(); + + 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 diff --git a/tests/Avalonia.Controls.UnitTests/Selection/SelectionModelTests_Single.cs b/tests/Avalonia.Controls.UnitTests/Selection/SelectionModelTests_Single.cs index 345518e729..1b37730797 100644 --- a/tests/Avalonia.Controls.UnitTests/Selection/SelectionModelTests_Single.cs +++ b/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(); + + 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 From 8dfc65d17be00b7f7c96c294dabe7616916951b2 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Tue, 1 Sep 2020 18:16:55 +0200 Subject: [PATCH 2/2] Try to handle selections made in CollectionChanged. There's not much we can do about this except not select invalid indexes. --- .../Selection/SelectionModel.cs | 23 +++++++++++++ .../Selection/SelectionNodeBase.cs | 34 +++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/src/Avalonia.Controls/Selection/SelectionModel.cs b/src/Avalonia.Controls/Selection/SelectionModel.cs index 7ce2624d02..2556bd4c4c 100644 --- a/src/Avalonia.Controls/Selection/SelectionModel.cs +++ b/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) diff --git a/src/Avalonia.Controls/Selection/SelectionNodeBase.cs b/src/Avalonia.Controls/Selection/SelectionNodeBase.cs index ff3b8f43a8..230575074a 100644 --- a/src/Avalonia.Controls/Selection/SelectionNodeBase.cs +++ b/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? 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;