From a3577ad257e790ad2902372dad3487170c532190 Mon Sep 17 00:00:00 2001 From: Ben Nace Date: Sun, 30 Jun 2024 19:49:49 -0400 Subject: [PATCH 1/4] Fixing regression in Picker behavior when inserting at the beginning and incrementing SelectedIndex. --- src/Controls/src/Core/Picker/Picker.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/Controls/src/Core/Picker/Picker.cs b/src/Controls/src/Core/Picker/Picker.cs index 7858babaa320..a8a1edb1d366 100644 --- a/src/Controls/src/Core/Picker/Picker.cs +++ b/src/Controls/src/Core/Picker/Picker.cs @@ -302,19 +302,21 @@ void CollectionChanged(object sender, NotifyCollectionChangedEventArgs e) void AddItems(NotifyCollectionChangedEventArgs e) { - int index = e.NewStartingIndex < 0 ? Items.Count : e.NewStartingIndex; + int insertIndex = e.NewStartingIndex < 0 ? Items.Count : e.NewStartingIndex; + int index = insertIndex; foreach (object newItem in e.NewItems) ((LockableObservableListWrapper)Items).InternalInsert(index++, GetDisplayMember(newItem)); - if (index <= SelectedIndex) + if (insertIndex <= SelectedIndex) UpdateSelectedItem(SelectedIndex); } void RemoveItems(NotifyCollectionChangedEventArgs e) { - int index = e.OldStartingIndex < Items.Count ? e.OldStartingIndex : Items.Count; + int removeStart = e.OldStartingIndex < Items.Count ? e.OldStartingIndex : Items.Count - e.OldItems.Count; + int index = removeStart + e.OldItems.Count; foreach (object _ in e.OldItems) ((LockableObservableListWrapper)Items).InternalRemoveAt(index--); - if (index <= SelectedIndex) + if (removeStart <= SelectedIndex) { ClampSelectedIndex(); } From 2b087e33b0bbac15e06ffceaac1cce946865c3ea Mon Sep 17 00:00:00 2001 From: Ben Nace Date: Mon, 1 Jul 2024 05:53:46 -0400 Subject: [PATCH 2/4] Fixing off by one error in Picker item removing logic --- src/Controls/src/Core/Picker/Picker.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Controls/src/Core/Picker/Picker.cs b/src/Controls/src/Core/Picker/Picker.cs index a8a1edb1d366..2b1f8cb9f86c 100644 --- a/src/Controls/src/Core/Picker/Picker.cs +++ b/src/Controls/src/Core/Picker/Picker.cs @@ -313,7 +313,7 @@ void AddItems(NotifyCollectionChangedEventArgs e) void RemoveItems(NotifyCollectionChangedEventArgs e) { int removeStart = e.OldStartingIndex < Items.Count ? e.OldStartingIndex : Items.Count - e.OldItems.Count; - int index = removeStart + e.OldItems.Count; + int index = removeStart + e.OldItems.Count - 1; foreach (object _ in e.OldItems) ((LockableObservableListWrapper)Items).InternalRemoveAt(index--); if (removeStart <= SelectedIndex) From cb3a34800a2a91b20cdbd727c86208fc761ce5e0 Mon Sep 17 00:00:00 2001 From: Ben Nace Date: Tue, 2 Jul 2024 20:39:36 -0400 Subject: [PATCH 3/4] Working on picker unit tests for adding and removing items and how it interacts with SelectedItem and SelectedIndex. --- .../tests/Core.UnitTests/PickerTests.cs | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/src/Controls/tests/Core.UnitTests/PickerTests.cs b/src/Controls/tests/Core.UnitTests/PickerTests.cs index d92abf6b28bb..c88d4a8261e0 100644 --- a/src/Controls/tests/Core.UnitTests/PickerTests.cs +++ b/src/Controls/tests/Core.UnitTests/PickerTests.cs @@ -338,6 +338,118 @@ public void TestItemsSourceCollectionChangedRemove() Assert.Equal("Ringo", picker.Items[1]); } + [Theory] + [InlineData(0)] + [InlineData(1)] + public void TestItemsSourceCollectionChangedInsertBeforeSelected(int insertionIndex) + { + var items = new ObservableCollection + { + new { Name = "John" }, + new { Name = "Paul" }, + new { Name = "Ringo" } + }; + var picker = new Picker + { + ItemDisplayBinding = new Binding("Name"), + ItemsSource = items, + SelectedIndex = 1 + }; + items.Insert(insertionIndex, new { Name = "George" }); + Assert.Equal(1, picker.SelectedIndex); + Assert.Equal(items[1], picker.SelectedItem); + } + + [Theory] + [InlineData(2)] + [InlineData(3)] + public void TestItemsSourceCollectionChangedInsertAfterSelected(int insertionIndex) + { + var items = new ObservableCollection + { + new { Name = "John" }, + new { Name = "Paul" }, + new { Name = "Ringo" } + }; + var picker = new Picker + { + ItemDisplayBinding = new Binding("Name"), + ItemsSource = items, + SelectedIndex = 1 + }; + items.Insert(insertionIndex, new { Name = "George" }); + Assert.Equal(1, picker.SelectedIndex); + Assert.Equal(items[1], picker.SelectedItem); + } + + [Theory] + [InlineData(0)] + [InlineData(1)] + public void TestItemsSourceCollectionChangedRemoveBeforeSelected(int removeIndex) + { + var items = new ObservableCollection + { + new { Name = "John" }, + new { Name = "Paul" }, + new { Name = "Ringo" }, + new { Name = "George" } + }; + var picker = new Picker + { + ItemDisplayBinding = new Binding("Name"), + ItemsSource = items, + SelectedIndex = 1 + }; + items.RemoveAt(removeIndex); + Assert.Equal(1, picker.SelectedIndex); + Assert.Equal(items[1], picker.SelectedItem); + } + + [Fact] + public void TestItemsSourceCollectionChangedRemoveAtEndSelected() + { + var items = new ObservableCollection + { + new { Name = "John" }, + new { Name = "Paul" }, + new { Name = "Ringo" }, + new { Name = "George" } + }; + var picker = new Picker + { + ItemDisplayBinding = new Binding("Name"), + ItemsSource = items, + SelectedIndex = 3 + }; + items.RemoveAt(3); + Assert.Equal(2, picker.SelectedIndex); + Assert.Equal(items[2], picker.SelectedItem); + } + + [Fact] + public void TestItemsSourceCollectionChangedRemoveAfterSelected() + { + var items = new ObservableCollection + { + new { Name = "John" }, + new { Name = "Paul" }, + new { Name = "Ringo" }, + new { Name = "George" } + }; + var picker = new Picker + { + ItemDisplayBinding = new Binding("Name"), + ItemsSource = items, + SelectedIndex = 1 + }; + items.RemoveAt(2); + Assert.Equal(1, picker.SelectedIndex); + Assert.Equal(items[1], picker.SelectedItem); + } + + + // TODO: Multi-item add and remove + [Fact] public void TestSelectedIndexAssignedItemsSourceCollectionChangedAppend() { From 5ad8da5444f46cc6d35489ca32377a18e27bb041 Mon Sep 17 00:00:00 2001 From: Ben Nace Date: Wed, 3 Jul 2024 22:03:35 -0400 Subject: [PATCH 4/4] Updating Picker remove index calculation logic to make it easier to read. Added unit tests for Picker to check SelectedIndex and SelectedItem when adding/removing single and multiple items. --- src/Controls/src/Core/Picker/Picker.cs | 19 ++- .../tests/Core.UnitTests/PickerTests.cs | 137 ++++++++++-------- 2 files changed, 91 insertions(+), 65 deletions(-) diff --git a/src/Controls/src/Core/Picker/Picker.cs b/src/Controls/src/Core/Picker/Picker.cs index 2b1f8cb9f86c..85f282d717c5 100644 --- a/src/Controls/src/Core/Picker/Picker.cs +++ b/src/Controls/src/Core/Picker/Picker.cs @@ -312,8 +312,23 @@ void AddItems(NotifyCollectionChangedEventArgs e) void RemoveItems(NotifyCollectionChangedEventArgs e) { - int removeStart = e.OldStartingIndex < Items.Count ? e.OldStartingIndex : Items.Count - e.OldItems.Count; - int index = removeStart + e.OldItems.Count - 1; + int removeStart; + // Items are removed in reverse order, so index starts at the index of the last item to remove + int index; + + if (e.OldStartingIndex < Items.Count) + { + // Remove e.OldItems.Count items starting at e.OldStartingIndex + removeStart = e.OldStartingIndex; + index = e.OldStartingIndex + e.OldItems.Count - 1; + } + else + { + // Remove e.OldItems.Count items at the end when e.OldStartingIndex is past the end of the Items collection + removeStart = Items.Count - e.OldItems.Count; + index = Items.Count - 1; + } + foreach (object _ in e.OldItems) ((LockableObservableListWrapper)Items).InternalRemoveAt(index--); if (removeStart <= SelectedIndex) diff --git a/src/Controls/tests/Core.UnitTests/PickerTests.cs b/src/Controls/tests/Core.UnitTests/PickerTests.cs index c88d4a8261e0..b3b8dffd061c 100644 --- a/src/Controls/tests/Core.UnitTests/PickerTests.cs +++ b/src/Controls/tests/Core.UnitTests/PickerTests.cs @@ -1,6 +1,10 @@ using System; +using System.Collections.Generic; using System.Collections.ObjectModel; +using System.Collections.Specialized; +using System.ComponentModel; using System.Globalization; +using System.Linq; using Microsoft.Maui.Graphics; using Xunit; @@ -57,6 +61,42 @@ public object ConvertBack(object value, Type targetType, object parameter, Cultu } } + class ObservableRangeCollection : ObservableCollection + { + static readonly PropertyChangedEventArgs CountChangedArgs = new(nameof(Count)); + static readonly PropertyChangedEventArgs IndexerChangedArgs = new("Item[]"); + + public void InsertRange(int index, IEnumerable items) + { + CheckReentrancy(); + int currIndex = index; + foreach (T item in items) + { + Items.Insert(currIndex++, item); + } + + OnPropertyChanged(CountChangedArgs); + OnPropertyChanged(IndexerChangedArgs); + OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, items.ToList(), index)); + } + + public void RemoveRange(int index, int count) + { + CheckReentrancy(); + T[] removeItems = new T[count]; + for (int i = 0; i < count; i++) + { + // Always remove at index, since removing each item at index shifts the next item to that spot + removeItems[i] = Items[index]; + Items.RemoveAt(index); + } + + OnPropertyChanged(CountChangedArgs); + OnPropertyChanged(IndexerChangedArgs); + OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, removeItems, index)); + } + } + [Fact] public void TestSetSelectedIndexOnNullRows() { @@ -339,11 +379,17 @@ public void TestItemsSourceCollectionChangedRemove() } [Theory] - [InlineData(0)] - [InlineData(1)] - public void TestItemsSourceCollectionChangedInsertBeforeSelected(int insertionIndex) - { - var items = new ObservableCollection + [InlineData(0, new string[] { "George" })] + [InlineData(1, new string[] { "George" })] + [InlineData(2, new string[] { "George" })] + [InlineData(3, new string[] { "George" })] + [InlineData(0, new string[] { "George", "Pete" })] + [InlineData(1, new string[] { "George", "Pete" })] + [InlineData(2, new string[] { "George", "Pete" })] + [InlineData(3, new string[] { "George", "Pete" })] + public void TestItemsSourceCollectionChangedInsertBeforeSelected(int insertionIndex, string[] insertNames) + { + var items = new ObservableRangeCollection { new { Name = "John" }, new { Name = "Paul" }, @@ -355,39 +401,22 @@ public void TestItemsSourceCollectionChangedInsertBeforeSelected(int insertionIn ItemsSource = items, SelectedIndex = 1 }; - items.Insert(insertionIndex, new { Name = "George" }); + items.InsertRange(insertionIndex, insertNames.Select(name => new { Name = name })); + Assert.Equal(3 + insertNames.Length, picker.Items.Count); Assert.Equal(1, picker.SelectedIndex); Assert.Equal(items[1], picker.SelectedItem); } [Theory] - [InlineData(2)] - [InlineData(3)] - public void TestItemsSourceCollectionChangedInsertAfterSelected(int insertionIndex) + [InlineData(0, 1)] + [InlineData(1, 1)] + [InlineData(2, 1)] + [InlineData(0, 2)] + [InlineData(1, 2)] + [InlineData(2, 2)] + public void TestItemsSourceCollectionChangedRemoveBeforeSelected(int removeIndex, int removeCount) { - var items = new ObservableCollection - { - new { Name = "John" }, - new { Name = "Paul" }, - new { Name = "Ringo" } - }; - var picker = new Picker - { - ItemDisplayBinding = new Binding("Name"), - ItemsSource = items, - SelectedIndex = 1 - }; - items.Insert(insertionIndex, new { Name = "George" }); - Assert.Equal(1, picker.SelectedIndex); - Assert.Equal(items[1], picker.SelectedItem); - } - - [Theory] - [InlineData(0)] - [InlineData(1)] - public void TestItemsSourceCollectionChangedRemoveBeforeSelected(int removeIndex) - { - var items = new ObservableCollection + var items = new ObservableRangeCollection { new { Name = "John" }, new { Name = "Paul" }, @@ -400,15 +429,19 @@ public void TestItemsSourceCollectionChangedRemoveBeforeSelected(int removeIndex ItemsSource = items, SelectedIndex = 1 }; - items.RemoveAt(removeIndex); + items.RemoveRange(removeIndex, removeCount); + + Assert.Equal(4 - removeCount, picker.Items.Count); Assert.Equal(1, picker.SelectedIndex); Assert.Equal(items[1], picker.SelectedItem); } - [Fact] - public void TestItemsSourceCollectionChangedRemoveAtEndSelected() + [Theory] + [InlineData(1)] + [InlineData(2)] + public void TestItemsSourceCollectionChangedRemoveAtEndSelected(int removeCount) { - var items = new ObservableCollection + var items = new ObservableRangeCollection { new { Name = "John" }, new { Name = "Paul" }, @@ -419,37 +452,15 @@ public void TestItemsSourceCollectionChangedRemoveAtEndSelected() { ItemDisplayBinding = new Binding("Name"), ItemsSource = items, - SelectedIndex = 3 + SelectedIndex = 4 - removeCount }; - items.RemoveAt(3); - Assert.Equal(2, picker.SelectedIndex); - Assert.Equal(items[2], picker.SelectedItem); - } + items.RemoveRange(4 - removeCount, removeCount); - [Fact] - public void TestItemsSourceCollectionChangedRemoveAfterSelected() - { - var items = new ObservableCollection - { - new { Name = "John" }, - new { Name = "Paul" }, - new { Name = "Ringo" }, - new { Name = "George" } - }; - var picker = new Picker - { - ItemDisplayBinding = new Binding("Name"), - ItemsSource = items, - SelectedIndex = 1 - }; - items.RemoveAt(2); - Assert.Equal(1, picker.SelectedIndex); - Assert.Equal(items[1], picker.SelectedItem); + Assert.Equal(4 - removeCount, picker.Items.Count); + Assert.Equal(items.Count - 1, picker.SelectedIndex); + Assert.Equal(items[^1], picker.SelectedItem); } - - // TODO: Multi-item add and remove - [Fact] public void TestSelectedIndexAssignedItemsSourceCollectionChangedAppend() {