Picker ItemsSource fixes (#19728)

* Fixing Picker getting in infinite loop when updating ItemsSource during update of SelectedIndex * Fixing use of brackets * Changing back dependency versions * - move code to reusable method --------- Co-authored-by: Shane Neuville <[email protected]>
dotnet · Jun 2, 2024 · cfbb5ae · cfbb5ae
1 parent fcabff1
commit cfbb5ae
Show file tree

Hide file tree

Showing 2 changed files with 188 additions and 9 deletions.
diff --git a/src/Controls/src/Core/Picker/Picker.cs b/src/Controls/src/Core/Picker/Picker.cs
@@ -243,14 +243,12 @@ void OnItemDisplayBindingChanged(BindingBase oldValue, BindingBase newValue)
 
 		void OnItemsCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
 		{
-			var oldIndex = SelectedIndex;
-			var newIndex = SelectedIndex.Clamp(-1, Items.Count - 1);
-			//FIXME use the specificity of the caller
-			SetValue(SelectedIndexProperty, newIndex, SetterSpecificity.FromHandler);
-			// If the index has not changed, still need to change the selected item
-			if (newIndex == oldIndex)
-				UpdateSelectedItem(newIndex);
+			// Do not execute when Items is locked because updates to ItemsSource will
+			// take care of it
+			if (((LockableObservableListWrapper)Items).IsLocked)
+				return;
 
+			ClampSelectedIndex();
 			Handler?.UpdateValue(nameof(IPicker.Items));
 		}
 
@@ -278,8 +276,9 @@ void OnItemsSourceChanged(IList oldValue, IList newValue)
 			}
 			else
 			{
-				((LockableObservableListWrapper)Items).InternalClear();
+				// Unlock, then clear, so OnItemsCollectionChanged executes
 				((LockableObservableListWrapper)Items).IsLocked = false;
+				((LockableObservableListWrapper)Items).InternalClear();
 			}
 		}
 
@@ -306,13 +305,19 @@ void AddItems(NotifyCollectionChangedEventArgs e)
 			int index = e.NewStartingIndex < 0 ? Items.Count : e.NewStartingIndex;
 			foreach (object newItem in e.NewItems)
 				((LockableObservableListWrapper)Items).InternalInsert(index++, GetDisplayMember(newItem));
+			if (index <= SelectedIndex)
+				UpdateSelectedItem(SelectedIndex);
 		}
 
 		void RemoveItems(NotifyCollectionChangedEventArgs e)
 		{
 			int index = e.OldStartingIndex < Items.Count ? e.OldStartingIndex : Items.Count;
 			foreach (object _ in e.OldItems)
 				((LockableObservableListWrapper)Items).InternalRemoveAt(index--);
+			if (index <= SelectedIndex)
+			{
+				ClampSelectedIndex();
+			}
 		}
 
 		void ResetItems()
@@ -323,7 +328,8 @@ void ResetItems()
 			foreach (object item in ItemsSource)
 				((LockableObservableListWrapper)Items).InternalAdd(GetDisplayMember(item));
 			Handler?.UpdateValue(nameof(IPicker.Items));
-			UpdateSelectedItem(SelectedIndex);
+
+			ClampSelectedIndex();
 		}
 
 		static void OnSelectedIndexChanged(object bindable, object oldValue, object newValue)
@@ -339,6 +345,17 @@ static void OnSelectedItemChanged(BindableObject bindable, object oldValue, obje
 			picker.UpdateSelectedIndex(newValue);
 		}
 
+		void ClampSelectedIndex()
+		{
+			var oldIndex = SelectedIndex;
+			var newIndex = SelectedIndex.Clamp(-1, Items.Count - 1);
+			//FIXME use the specificity of the caller
+			SetValue(SelectedIndexProperty, newIndex, SetterSpecificity.FromHandler);
+			// If the index has not changed, still need to change the selected item
+			if (newIndex == oldIndex)
+				UpdateSelectedItem(newIndex);
+		}
+
 		void UpdateSelectedIndex(object selectedItem)
 		{
 			//FIXME use the specificity of the caller

diff --git a/src/Controls/tests/Core.UnitTests/PickerTests.cs b/src/Controls/tests/Core.UnitTests/PickerTests.cs
@@ -338,6 +338,168 @@ public void TestItemsSourceCollectionChangedRemove()
 			Assert.Equal("Ringo", picker.Items[1]);
 		}
 
+		[Fact]
+		public void TestSelectedIndexAssignedItemsSourceCollectionChangedAppend()
+		{
+			var items = new ObservableCollection<object>
+			{
+				new { Name = "John" },
+				new { Name = "Paul" },
+				"Ringo"
+			};
+			var picker = new Picker
+			{
+				ItemDisplayBinding = new Binding("Name"),
+				ItemsSource = items,
+			};
+
+			picker.PropertyChanged += (sender, e) =>
+			{
+				if (e.PropertyName == nameof(Picker.SelectedIndex))
+				{
+					items.Add(new { Name = "George" });
+				}
+			};
+
+			picker.SelectedIndex = 1;
+
+			Assert.Equal(4, picker.Items.Count);
+			Assert.Equal("George", picker.Items[picker.Items.Count - 1]);
+			Assert.Equal(1, picker.SelectedIndex);
+			Assert.Equal(items[1], picker.SelectedItem);
+		}
+
+		[Fact]
+		public void TestSelectedIndexAssignedItemsSourceCollectionChangedClear()
+		{
+			var items = new ObservableCollection<object>
+			{
+				new { Name = "John" },
+				"Paul",
+				"Ringo"
+			};
+			var picker = new Picker
+			{
+				ItemDisplayBinding = new Binding("Name"),
+				ItemsSource = items,
+			};
+
+			picker.PropertyChanged += (sender, e) =>
+			{
+				if (e.PropertyName == nameof(Picker.SelectedIndex))
+				{
+					items.Clear();
+				}
+			};
+
+			picker.SelectedIndex = 1;
+
+			Assert.Empty(picker.Items);
+			Assert.Equal(-1, picker.SelectedIndex);
+			Assert.Null(picker.SelectedItem);
+		}
+
+		[Fact]
+		public void TestSelectedIndexAssignedItemsSourceCollectionChangedInsert()
+		{
+			var items = new ObservableCollection<object>
+			{
+				new { Name = "John" },
+				"Paul",
+				"Ringo"
+			};
+			var picker = new Picker
+			{
+				ItemDisplayBinding = new Binding("Name"),
+				ItemsSource = items,
+			};
+
+			picker.PropertyChanged += (sender, e) =>
+			{
+				if (e.PropertyName == nameof(Picker.SelectedIndex))
+				{
+					items.Insert(1, new { Name = "George" });
+				}
+			};
+
+			picker.SelectedIndex = 2;
+
+			Assert.Equal(4, picker.Items.Count);
+			Assert.Equal("George", picker.Items[1]);
+			Assert.Equal(2, picker.SelectedIndex);
+			Assert.Equal(items[2], picker.SelectedItem);
+		}
+
+		[Fact]
+		public void TestSelectedIndexAssignedItemsSourceCollectionChangedReAssign()
+		{
+			var items = new ObservableCollection<object>
+			{
+				new { Name = "John" },
+				"Paul",
+				"Ringo"
+			};
+			var bindingContext = new { Items = items };
+			var picker = new Picker
+			{
+				ItemDisplayBinding = new Binding("Name"),
+				BindingContext = bindingContext
+			};
+			picker.SetBinding(Picker.ItemsSourceProperty, "Items");
+
+			picker.PropertyChanged += (sender, e) =>
+			{
+				if (e.PropertyName == nameof(Picker.SelectedIndex))
+				{
+					items = new ObservableCollection<object>
+					{
+						"Peach",
+						"Orange"
+					};
+					picker.BindingContext = new { Items = items };
+					picker.ItemDisplayBinding = null;
+				}
+			};
+
+			picker.SelectedIndex = 1;
+
+			Assert.Equal(2, picker.Items.Count);
+			Assert.Equal("Peach", picker.Items[0]);
+			Assert.Equal(1, picker.SelectedIndex);
+			Assert.Equal("Orange", picker.SelectedItem);
+		}
+
+		[Fact]
+		public void TestSelectedItemAssignedItemsSourceCollectionChangedRemove()
+		{
+			var items = new ObservableCollection<object>
+			{
+				new { Name = "John" },
+				new { Name = "Paul" },
+				new { Name = "Ringo"},
+			};
+			var picker = new Picker
+			{
+				ItemDisplayBinding = new Binding("Name"),
+				ItemsSource = items,
+			};
+
+			picker.PropertyChanged += (sender, e) =>
+			{
+				if (e.PropertyName == nameof(Picker.SelectedIndex))
+				{
+					items.RemoveAt(1);
+				}
+			};
+
+			picker.SelectedIndex = 1;
+
+			Assert.Equal(2, picker.Items.Count);
+			Assert.Equal("Ringo", picker.Items[1]);
+			Assert.Equal(1, picker.SelectedIndex);
+			Assert.Equal(items[1], picker.SelectedItem);
+		}
+
 		[Fact]
 		public void SettingSelectedIndexUpdatesSelectedItem()
 		{