Skip to content

Commit

Permalink
Fixing regression in Picker behavior in 8.0.60 (#23369)
Browse files Browse the repository at this point in the history
* Fixing regression in Picker behavior when inserting at the beginning and incrementing SelectedIndex.

* Fixing off by one error in Picker item removing logic

* Working on picker unit tests for adding and removing items and how it interacts with SelectedItem and SelectedIndex.

* 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.
  • Loading branch information
BurningLights authored Jul 5, 2024
1 parent ea35a6f commit 84ae20f
Show file tree
Hide file tree
Showing 2 changed files with 144 additions and 4 deletions.
25 changes: 21 additions & 4 deletions src/Controls/src/Core/Picker/Picker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -302,19 +302,36 @@ 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;
// 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 (index <= SelectedIndex)
if (removeStart <= SelectedIndex)
{
ClampSelectedIndex();
}
Expand Down
123 changes: 123 additions & 0 deletions src/Controls/tests/Core.UnitTests/PickerTests.cs
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -57,6 +61,42 @@ public object ConvertBack(object value, Type targetType, object parameter, Cultu
}
}

class ObservableRangeCollection<T> : ObservableCollection<T>
{
static readonly PropertyChangedEventArgs CountChangedArgs = new(nameof(Count));
static readonly PropertyChangedEventArgs IndexerChangedArgs = new("Item[]");

public void InsertRange(int index, IEnumerable<T> 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()
{
Expand Down Expand Up @@ -338,6 +378,89 @@ public void TestItemsSourceCollectionChangedRemove()
Assert.Equal("Ringo", picker.Items[1]);
}

[Theory]
[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<object>
{
new { Name = "John" },
new { Name = "Paul" },
new { Name = "Ringo" }
};
var picker = new Picker
{
ItemDisplayBinding = new Binding("Name"),
ItemsSource = items,
SelectedIndex = 1
};
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(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 ObservableRangeCollection<object>
{
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.RemoveRange(removeIndex, removeCount);

Assert.Equal(4 - removeCount, picker.Items.Count);
Assert.Equal(1, picker.SelectedIndex);
Assert.Equal(items[1], picker.SelectedItem);
}

[Theory]
[InlineData(1)]
[InlineData(2)]
public void TestItemsSourceCollectionChangedRemoveAtEndSelected(int removeCount)
{
var items = new ObservableRangeCollection<object>
{
new { Name = "John" },
new { Name = "Paul" },
new { Name = "Ringo" },
new { Name = "George" }
};
var picker = new Picker
{
ItemDisplayBinding = new Binding("Name"),
ItemsSource = items,
SelectedIndex = 4 - removeCount
};
items.RemoveRange(4 - removeCount, removeCount);

Assert.Equal(4 - removeCount, picker.Items.Count);
Assert.Equal(items.Count - 1, picker.SelectedIndex);
Assert.Equal(items[^1], picker.SelectedItem);
}

[Fact]
public void TestSelectedIndexAssignedItemsSourceCollectionChangedAppend()
{
Expand Down

0 comments on commit 84ae20f

Please sign in to comment.