Picker ItemsSource fixes

src/Controls/src/Core/Picker/Picker.cs

@@ -243,6 +243,11 @@ void OnItemDisplayBindingChanged(BindingBase oldValue, BindingBase newValue)
 
 		void OnItemsCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
 		{
+			// Do not execute when Items is locked because updates to ItemsSource will
+			// take care of it
+			if (((LockableObservableListWrapper)Items).IsLocked)
+				return;
+
 			var oldIndex = SelectedIndex;
 			var newIndex = SelectedIndex.Clamp(-1, Items.Count - 1);
 			//FIXME use the specificity of the caller
@@ -278,8 +283,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 +312,27 @@ 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)
+			{
+				var oldIndex = SelectedIndex;
+				var newIndex = SelectedIndex.Clamp(-1, Items.Count - 1);
+
+				SetValue(SelectedIndexProperty, newIndex, SetterSpecificity.FromHandler);
+				// Update SelectedItem separately if index hasn't changed
+				if (newIndex == oldIndex)
+				{
+					UpdateSelectedItem(newIndex);
+				}
+			}
 		}
 
 		void ResetItems()
@@ -323,7 +343,12 @@ void ResetItems()
 			foreach (object item in ItemsSource)
 				((LockableObservableListWrapper)Items).InternalAdd(GetDisplayMember(item));
 			Handler?.UpdateValue(nameof(IPicker.Items));
-			UpdateSelectedItem(SelectedIndex);
+
+			var oldIndex = SelectedIndex;
+			var newIndex = SelectedIndex.Clamp(-1, Items.Count - 1);
+			SetValue(SelectedIndexProperty, newIndex, SetterSpecificity.FromHandler);
+			if (oldIndex == newIndex)
+				UpdateSelectedItem(SelectedIndex);
 		}
 
 		static void OnSelectedIndexChanged(object bindable, object oldValue, object newValue)

src/Controls/tests/Core.UnitTests/DeviceUnitTests.cs

@@ -108,7 +108,7 @@ async Task<bool> boom()
 			Assert.False(invoked, "Action invoked early.");
 
 			async Task MethodThatThrows() => await task;
-			Assert.ThrowsAsync<ApplicationException>(MethodThatThrows);
+			await Assert.ThrowsAsync<ApplicationException>(MethodThatThrows);
 		});
 
 		[Fact]
@@ -144,7 +144,7 @@ async Task boom()
 			Assert.False(invoked, "Action invoked early.");
 
 			async Task MethodThatThrows() => await task;
-			Assert.ThrowsAsync<ApplicationException>(MethodThatThrows);
+			await Assert.ThrowsAsync<ApplicationException>(MethodThatThrows);
 		});
 
 		private void MockPlatformServices(Action onInvokeOnMainThread, Action<Action> invokeOnMainThread = null)

src/Controls/tests/Core.UnitTests/NavigationUnitTest.cs

@@ -836,7 +836,7 @@ public async Task PopModalWithEmptyStackThrows()
 			var window = new TestWindow(new ContentPage());
 			var contentPage1 = new ContentPage();
 			var navigationPage = new TestNavigationPage(true, contentPage1);
-			Assert.ThrowsAsync<InvalidOperationException>(() => window.Navigation.PopModalAsync());
+			await Assert.ThrowsAsync<InvalidOperationException>(() => window.Navigation.PopModalAsync());
 		}
 
 		[Fact]

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()
 		{

src/Controls/tests/Core.UnitTests/ScrollViewUnitTests.cs

@@ -267,12 +267,12 @@ public void TestScrollToElementNotAnimated()
 		}
 
 		[Fact]
-		public void TestScrollToInvalid()
+		public async void TestScrollToInvalid()
 		{
 			var scrollView = new ScrollView();
 
-			Assert.ThrowsAsync<ArgumentException>(() => scrollView.ScrollToAsync(new VisualElement(), ScrollToPosition.Center, true));
-			Assert.ThrowsAsync<ArgumentException>(() => scrollView.ScrollToAsync(null, (ScrollToPosition)500, true));
+			await Assert.ThrowsAsync<ArgumentException>(() => scrollView.ScrollToAsync(new VisualElement(), ScrollToPosition.Center, true));
+			await Assert.ThrowsAsync<ArgumentException>(() => scrollView.ScrollToAsync(null, (ScrollToPosition)500, true));
 		}
 
 		[Fact]

src/Controls/tests/Core.UnitTests/ShellNavigatingTests.cs

@@ -682,7 +682,7 @@ public async Task AbsoluteRoutingToPage()
 
 			Routing.RegisterRoute("catdetails", typeof(ContentPage));
 
-			Assert.ThrowsAnyAsync<Exception>(async () => await shell.GoToAsync($"//catdetails"));
+			await Assert.ThrowsAnyAsync<Exception>(async () => await shell.GoToAsync($"//catdetails"));
 		}
 
 		[Fact]

src/Controls/tests/Core.UnitTests/ShellUriHandlerTests.cs

@@ -250,7 +250,7 @@ public async Task RelativeNavigationToShellElementThrows()
 			shell.Items.Add(item1);
 			shell.Items.Add(item2);
 
-			Assert.ThrowsAnyAsync<Exception>(async () => await shell.GoToAsync($"domestic"));
+			await Assert.ThrowsAnyAsync<Exception>(async () => await shell.GoToAsync($"domestic"));
 		}
 
 
@@ -266,7 +266,7 @@ public async Task RelativeNavigationWithRoute()
 			shell.Items.Add(item2);
 
 			Routing.RegisterRoute("catdetails", typeof(ContentPage));
-			Assert.ThrowsAnyAsync<Exception>(async () => await shell.GoToAsync($"cats/catdetails?name=domestic"));
+			await Assert.ThrowsAnyAsync<Exception>(async () => await shell.GoToAsync($"cats/catdetails?name=domestic"));
 
 			// once relative routing with a stack is fixed then we can remove the above exception check and add below back in
 			// await shell.GoToAsync($"cats/catdetails?name=domestic")