Handle "groups but no actual cells" cases for reloading UICollectionView (#9911) Fixes #9580 Fixes #9686

* Handle "groups but no actual cells" case for reloading UICollectionView
Fixes #9580

* Add check for reload when adding/removing groups
Fixes #9686

* Prevent prevent reloads during batch updates
Fixes #9686

* Removed obsolete TODO comment

* Fix test (missing success condition)

* Fix possible tap speed issue in UI test

* Switch await loop to SemaphoreSlim

* Fix semaphore deadlocks

* Fix incorrect check for semaphore release

* If this fixes it, I'm gonna be soooooo mad...

* When in doubt, just use a button

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue9580.cs

@@ -0,0 +1,88 @@
+using System.Collections.ObjectModel;
+using Xamarin.Forms.CustomAttributes;
+
+#if UITEST
+using Xamarin.UITest;
+using NUnit.Framework;
+using Xamarin.Forms.Core.UITests;
+#endif
+
+namespace Xamarin.Forms.Controls.Issues
+{
+	[Issue(IssueTracker.Github, 9580, "[Bug] CollectionView - iOS - Crash when adding first item to empty item group", 
+		PlatformAffected.iOS)]
+	public class Issue9580 : TestContentPage
+	{
+		const string Success = "Success";
+		const string Test9580 = "9580";
+
+		protected override void Init()
+		{
+			var layout = new StackLayout();
+
+			var cv = new CollectionView
+			{
+				IsGrouped = true
+			};
+
+			var groups = new ObservableCollection<_9580Group>()
+			{
+				new _9580Group() { Name = "One" }, new _9580Group(){ Name = "Two" }, new _9580Group(){ Name = "Three" },
+				new _9580Group() { Name = "Four" }, new _9580Group(){ Name = "Five" }, new _9580Group(){ Name = "Six" }
+			};
+
+			cv.ItemTemplate = new DataTemplate(() => {
+				var label = new Label() { Margin = new Thickness(5, 0, 0, 0) };
+				label.SetBinding(Label.TextProperty, new Binding("Text"));
+				return label;
+			});
+			
+			cv.GroupHeaderTemplate = new DataTemplate(() => {
+				var label = new Label();
+				label.SetBinding(Label.TextProperty, new Binding("Name"));
+				return label;
+			});
+
+			cv.ItemsSource = groups;
+
+			var instructions = new Label { Text = $"Tap the '{Test9580}' button. The application doesn't crash, this test has passed." };
+
+			var result = new Label { };
+
+			var button = new Button { Text = Test9580 };
+			button.Clicked += (sender, args) =>
+			{
+				groups[0].Add(new _9580Item { Text = "An Item" });
+				result.Text = Success;
+			};
+
+			layout.Children.Add(instructions);
+			layout.Children.Add(result);
+			layout.Children.Add(button);
+			layout.Children.Add(cv);
+
+			Content = layout;
+		}
+
+		class _9580Item
+		{ 
+			public string Text { get; set; }
+		}
+
+		class _9580Group : ObservableCollection<_9580Item> 
+		{
+			public string Name { get; set; }
+		}
+
+#if UITEST
+		[Category(UITestCategories.CollectionView)]
+		[Test]
+		public void AllEmptyGroupsShouldNotCrashOnItemInsert()
+		{
+			RunningApp.WaitForElement(Test9580);
+			RunningApp.Tap(Test9580);
+			RunningApp.WaitForElement(Success);
+		}
+#endif
+	}
+}

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue9686.cs

@@ -0,0 +1,191 @@
+using System.Collections.Generic;
+using System.Collections.ObjectModel;
+using System.Linq;
+using System.Windows.Input;
+using Xamarin.Forms.CustomAttributes;
+
+#if UITEST
+using Xamarin.UITest;
+using NUnit.Framework;
+using Xamarin.Forms.Core.UITests;
+#endif
+
+namespace Xamarin.Forms.Controls.Issues
+{
+	[Issue(IssueTracker.Github, 9686, "[Bug, CollectionView,iOS] Foundation.Monotouch Exception in Grouped CollectionView",
+			PlatformAffected.iOS)]
+	public class Issue9686 : TestContentPage
+	{
+		const string Success = "Success";
+		const string Run = "Run";
+
+		protected override void Init()
+		{
+			var layout = new StackLayout();
+
+			var cv = new CollectionView
+			{
+				IsGrouped = true
+			};
+
+			BindingContext = new _9686ViewModel();
+
+			cv.ItemTemplate = new DataTemplate(() => {
+				var label = new Label() { Margin = new Thickness(5, 0, 0, 0) };
+				label.SetBinding(Label.TextProperty, new Binding("Name"));
+				return label;
+			});
+
+			cv.GroupHeaderTemplate = new DataTemplate(() => {
+				var label = new Label();
+				label.SetBinding(Label.TextProperty, new Binding("GroupName"));
+				return label;
+			});
+
+			cv.SetBinding(ItemsView.ItemsSourceProperty, new Binding(nameof(_9686ViewModel.Groups)));
+
+			var instructions = new Label { Text = $"Tap the button once, then again. The application doesn't crash, this test has passed." };
+
+			var result = new Label { };
+
+			var button = new Button { Text  = Run, AutomationId = Run };
+			button.Command = ((_9686ViewModel)BindingContext).ShowOrHideCommand;
+			button.CommandParameter = ((_9686ViewModel)BindingContext).Groups[0];
+
+			layout.Children.Add(instructions);
+			layout.Children.Add(button);
+			layout.Children.Add(result);
+			layout.Children.Add(cv);
+
+			Content = layout;
+		}
+
+		public class _9686Item
+		{
+			public string Name { get; set; }
+		}
+
+		public class _9686Group : List<_9686Item>
+		{
+			public string GroupName { get; set; }
+
+			public _9686Group(string groupName, ObservableCollection<_9686Item> items) : base(items)
+			{
+				GroupName = groupName;
+			}
+		}
+
+		public class _9686ViewModel
+		{
+			public ICommand ShowOrHideCommand { get; set; }
+			public ObservableCollection<_9686Group> Groups { get; set; }
+
+			public _9686Group PreviousGroup { get; set; }
+
+			public _9686ViewModel()
+			{
+				ShowOrHideCommand = new Command<_9686Group>((group) => ShowOrHideItems(group));
+
+				Groups = new ObservableCollection<_9686Group>
+				{
+					new _9686Group("Group 1", new ObservableCollection<_9686Item>()),
+					new _9686Group("Group 2", new ObservableCollection<_9686Item>()),
+					new _9686Group("Group 3", new ObservableCollection<_9686Item>()),
+					new _9686Group("Group 4", new ObservableCollection<_9686Item>()),
+					new _9686Group("Group 5", new ObservableCollection<_9686Item>()),
+					new _9686Group("Group 6", new ObservableCollection<_9686Item>())
+				};
+			}
+
+			void ShowOrHideItems(_9686Group group)
+			{
+				if (PreviousGroup == group)
+				{
+					if (PreviousGroup.Any())
+					{
+						PreviousGroup.Clear();
+					}
+					else
+					{
+						PreviousGroup.AddRange(new List<_9686Item>
+						{
+							new _9686Item
+							{
+								Name = "Item 1"
+							},
+							new _9686Item
+							{
+								Name = "Item 2"
+							},
+							new _9686Item
+							{
+								Name = "Item 3"
+							},
+							new _9686Item
+							{
+								Name = "Item 4"
+							},
+						});
+					}
+
+					UpdateCollection(PreviousGroup);
+				}
+				else
+				{
+					if (PreviousGroup != null)
+					{
+						PreviousGroup.Clear();
+						UpdateCollection(PreviousGroup);
+					}
+
+					group.AddRange(new List<_9686Item>
+					{
+						new _9686Item
+						{
+							Name = "Item 1"
+						},
+						new _9686Item
+						{
+							Name = "Item 2"
+						},
+						new _9686Item
+						{
+							Name = "Item 3"
+						},
+						new _9686Item
+						{
+							Name = "Item 4"
+						},
+					});
+
+					UpdateCollection(group);
+					PreviousGroup = group;
+				}
+			}
+
+			void UpdateCollection(_9686Group group)
+			{
+				var index = Groups.IndexOf(group);
+				Groups.Remove(group);
+				if (group.Count == 0)
+				{
+					group.GroupName = Success;
+				}
+				Groups.Insert(index, group);
+			}
+		}
+
+#if UITEST
+		[Category(UITestCategories.CollectionView)]
+		[Test]
+		public void AddRemoveEmptyGroupsShouldNotCrashOnInsert()
+		{
+			RunningApp.WaitForElement(Run);
+			RunningApp.Tap(Run);
+			RunningApp.WaitForElement("Item 1");
+			RunningApp.Tap(Run);
+			RunningApp.WaitForElement(Success);
+		}
+#endif
+	}
+}

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems

@@ -37,6 +37,7 @@
       <DependentUpon>Issue8902.xaml</DependentUpon>
       <SubType>Code</SubType>
     </Compile>
+    <Compile Include="$(MSBuildThisFileDirectory)Issue9580.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Issue9682.xaml.cs">
       <DependentUpon>Issue9682.xaml</DependentUpon>
       <SubType>Code</SubType>
@@ -216,6 +217,7 @@
       <DependentUpon>Issue9783.xaml</DependentUpon>
       <SubType>Code</SubType>
     </Compile>
+    <Compile Include="$(MSBuildThisFileDirectory)Issue9686.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Issue9694.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Issue9771.xaml.cs">
       <SubType>Code</SubType>

Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs

@@ -2,6 +2,8 @@
 using System.Collections;
 using System.Collections.Generic;
 using System.Collections.Specialized;
+using System.Threading;
+using System.Threading.Tasks;
 using Foundation;
 using UIKit;
 
@@ -13,6 +15,7 @@ namespace Xamarin.Forms.Platform.iOS
 		readonly UICollectionViewController _collectionViewController;
 		readonly IList _groupSource;
 		bool _disposed;
+		SemaphoreSlim _batchUpdating = new SemaphoreSlim(1, 1);
 		List<ObservableItemsSource> _groups = new List<ObservableItemsSource>();
 
 		public ObservableGroupedSource(IEnumerable groupSource, UICollectionViewController collectionViewController)
@@ -39,7 +42,7 @@ namespace Xamarin.Forms.Platform.iOS
 
 		public int GroupCount => _groupSource.Count;
 
-		int IItemsViewSource.ItemCount
+		public int ItemCount
 		{
 			get
 			{
@@ -127,47 +130,53 @@ namespace Xamarin.Forms.Platform.iOS
 			}
 		}
 
-		void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args)
+		async void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args)
 		{
 			if (Device.IsInvokeRequired)
 			{
-				Device.BeginInvokeOnMainThread(() => CollectionChanged(args));
+				await Device.InvokeOnMainThreadAsync(async () => await CollectionChanged(args));
 			}
 			else
 			{
-				CollectionChanged(args);
+				await CollectionChanged(args);
 			}
 		}
 
-		void CollectionChanged(NotifyCollectionChangedEventArgs args)
+		async Task CollectionChanged(NotifyCollectionChangedEventArgs args)
 		{
 			switch (args.Action)
+
 			{
 				case NotifyCollectionChangedAction.Add:
-					Add(args);
+					await Add(args);
 					break;
 				case NotifyCollectionChangedAction.Remove:
-					Remove(args);
+					await Remove(args);
 					break;
 				case NotifyCollectionChangedAction.Replace:
-					Replace(args);
+					await Replace(args);
 					break;
 				case NotifyCollectionChangedAction.Move:
 					Move(args);
 					break;
 				case NotifyCollectionChangedAction.Reset:
-					Reload();
+					await Reload();
 					break;
 				default:
 					throw new ArgumentOutOfRangeException();
 			}
 		}
 
-		void Reload()
+		async Task Reload()
 		{
 			ResetGroupTracking();
+
+			await _batchUpdating.WaitAsync();
+
 			_collectionView.ReloadData();
 			_collectionView.CollectionViewLayout.InvalidateLayout();
+
+			_batchUpdating.Release();
 		}
 
 		NSIndexSet CreateIndexSetFrom(int startIndex, int count)
@@ -182,8 +191,14 @@ namespace Xamarin.Forms.Platform.iOS
 			return !_collectionViewController.IsViewLoaded || _collectionViewController.View.Window == null;
 		}
 
-		void Add(NotifyCollectionChangedEventArgs args)
+		async Task Add(NotifyCollectionChangedEventArgs args)
 		{
+			if (ReloadRequired())
+			{
+				await Reload();
+				return;
+			}
+
 			var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : _groupSource.IndexOf(args.NewItems[0]);
 			var count = args.NewItems.Count;
 
@@ -191,53 +206,40 @@ namespace Xamarin.Forms.Platform.iOS
 			// is to reset all the group tracking to get it up-to-date
 			ResetGroupTracking();
 
-			if (NotLoadedYet())
+			// Queue up the updates to the UICollectionView
-			{
+			BatchUpdate(() => _collectionView.InsertSections(CreateIndexSetFrom(startIndex, count)));
-				_collectionView.ReloadData();
-				return;
-			}
-
-			if (_collectionView.NumberOfSections() == 0)
-			{
-				// Okay, we're going from completely empty to more than 0 items; there's an iOS bug which apparently
-				// will just crash if we call InsertItems here, so we have to do ReloadData.
-				_collectionView.ReloadData();
-				return;
-			}
-
-			_collectionView.InsertSections(CreateIndexSetFrom(startIndex, count));
 		}
 
-		void Remove(NotifyCollectionChangedEventArgs args)
+		async Task Remove(NotifyCollectionChangedEventArgs args)
 		{
 			var startIndex = args.OldStartingIndex;
 
 			if (startIndex < 0)
 			{
 				// INCC implementation isn't giving us enough information to know where the removed items were in the
-				// collection. So the best we can do is a ReloadData()
+				// collection. So the best we can do is a complete reload
-				Reload();
+				await Reload();
 				return;
 			}
 
-			// If we have a start index, we can be more clever about removing the item(s) (and get the nifty animations)
+			if (ReloadRequired())
-			var count = args.OldItems.Count;
+			{
+				await Reload();
+				return;
+			}
 
 			// Removing a group will change the section index for all subsequent groups, so the easiest thing to do
 			// is to reset all the group tracking to get it up-to-date
 			ResetGroupTracking();
 
-			if (NotLoadedYet())
+			// Since we have a start index, we can be more clever about removing the item(s) (and get the nifty animations)
-			{
+			var count = args.OldItems.Count;
-				_collectionView.ReloadData();
+
-			}
+			// Queue up the updates to the UICollectionView
-			else
+			BatchUpdate(() => _collectionView.DeleteSections(CreateIndexSetFrom(startIndex, count)));
-			{
-				_collectionView.DeleteSections(CreateIndexSetFrom(startIndex, count));
-			}
 		}
 
-		void Replace(NotifyCollectionChangedEventArgs args)
+		async Task Replace(NotifyCollectionChangedEventArgs args)
 		{
 			var newCount = args.NewItems.Count;
 
@@ -254,7 +256,7 @@ namespace Xamarin.Forms.Platform.iOS
 
 			// The original and replacement sets are of unequal size; this means that everything currently in view will 
 			// have to be updated. So we just have to use ReloadData and let the UICollectionView update everything
-			Reload();
+			await Reload();
 		}
 
 		void Move(NotifyCollectionChangedEventArgs args)
@@ -287,7 +289,7 @@ namespace Xamarin.Forms.Platform.iOS
 					var enumerator = enumerable.GetEnumerator();
 					while (enumerator.MoveNext())
 					{
-						count += 1; 
+						count += 1;
 					}
 					return count;
 			}
@@ -339,6 +341,36 @@ namespace Xamarin.Forms.Platform.iOS
 
 			return -1;
 		}
-	}
 
+		bool ReloadRequired()
+		{
+			// If the UICollectionView has never been loaded, or doesn't yet have any sections, or has no actual
+			// cells (just supplementary views like Header/Footer), any insert/delete operations are gonna crash
+			// hard. We'll need to reload the data instead.
+
+			return NotLoadedYet()
+				|| _collectionView.NumberOfSections() == 0
+				|| _collectionView.VisibleCells.Length == 0;
+		}
+
+		void BatchUpdate(Action update)
+		{
+			_collectionView.PerformBatchUpdates(() =>
+			{
+				if (_batchUpdating.CurrentCount > 0)
+				{
+					_batchUpdating.Wait();
+				}
+
+				update();
+			},
+					(_) =>
+					{
+						if (_batchUpdating.CurrentCount == 0)
+						{
+							_batchUpdating.Release();
+						}
+					});
+		}
+	}
 }

Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs

@@ -1,6 +1,8 @@
 using System;
 using System.Collections;
 using System.Collections.Specialized;
+using System.Threading;
+using System.Threading.Tasks;
 using Foundation;
 using UIKit;
 
@@ -14,6 +16,7 @@ namespace Xamarin.Forms.Platform.iOS
 		readonly int _section;
 		readonly IEnumerable _itemsSource;
 		bool _disposed;
+		SemaphoreSlim _batchUpdating = new SemaphoreSlim(1, 1);
 
 		public ObservableItemsSource(IEnumerable itemSource, UICollectionViewController collectionViewController, int group = -1)
 		{
@@ -94,36 +97,36 @@ namespace Xamarin.Forms.Platform.iOS
 			}
 		}
 
-		void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args)
+		async void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args)
 		{
 			if (Device.IsInvokeRequired)
 			{
-				Device.BeginInvokeOnMainThread(() => CollectionChanged(args));
+				await Device.InvokeOnMainThreadAsync(async () => await CollectionChanged(args));
 			}
 			else
 			{
-				CollectionChanged(args);
+				await CollectionChanged(args);
 			}
 		}
 
-		void CollectionChanged(NotifyCollectionChangedEventArgs args)
+		async Task CollectionChanged(NotifyCollectionChangedEventArgs args)
 		{
 			switch (args.Action)
 			{
 				case NotifyCollectionChangedAction.Add:
-					Add(args);
+					await Add(args);
 					break;
 				case NotifyCollectionChangedAction.Remove:
-					Remove(args);
+					await Remove(args);
 					break;
 				case NotifyCollectionChangedAction.Replace:
-					Replace(args);
+					await Replace(args);
 					break;
 				case NotifyCollectionChangedAction.Move:
 					Move(args);
 					break;
 				case NotifyCollectionChangedAction.Reset:
-					Reload();
+					await Reload();
 					break;
 				default:
 					throw new ArgumentOutOfRangeException();
@@ -132,11 +135,15 @@ namespace Xamarin.Forms.Platform.iOS
 			CollectionItemsSourceChanged?.Invoke(this, args);
 		}
 
-		void Reload()
+		async Task Reload()
 		{
+			await _batchUpdating.WaitAsync();
+
 			_collectionView.ReloadData();
 			_collectionView.CollectionViewLayout.InvalidateLayout();
 			Count = ItemsCount();
+
+			_batchUpdating.Release();
 		}
 
 		NSIndexPath[] CreateIndexesFrom(int startIndex, int count)
@@ -151,70 +158,49 @@ namespace Xamarin.Forms.Platform.iOS
 			return result;
 		}
 
-		bool NotLoadedYet()
+		async Task Add(NotifyCollectionChangedEventArgs args)
 		{
-			// If the UICollectionView hasn't actually been loaded, then calling InsertItems or DeleteItems is 
+			if (ReloadRequired())
-			// going to crash or get in an unusable state; instead, ReloadData should be used
-			return !_collectionViewController.IsViewLoaded || _collectionViewController.View.Window == null;
-		}
-
-		void Add(NotifyCollectionChangedEventArgs args)
-		{
-			if (NotLoadedYet())
 			{
-				_collectionView.ReloadData();
+				await Reload();
 				return;
 			}
 
-			var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : IndexOf(args.NewItems[0]);
 			var count = args.NewItems.Count;
+			Count += count;
+			var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : IndexOf(args.NewItems[0]);
 
-			if (!_grouped && _collectionView.NumberOfItemsInSection(_section) == 0)
+			// Queue up the updates to the UICollectionView
-			{
+			BatchUpdate(() => _collectionView.InsertItems(CreateIndexesFrom(startIndex, count)));
-				// Okay, we're going from completely empty to more than 0 items; there's an iOS bug which apparently
-				// will just crash if we call InsertItems here, so we have to do ReloadData.
-				_collectionView.ReloadData();
-				Count += count;
-				return;
-			}
-
-			_collectionView.PerformBatchUpdates(() =>
-				{
-					var indexes = CreateIndexesFrom(startIndex, count);
-					_collectionView.InsertItems(indexes);
-					Count += count;
-				}, null);
 		}
 
-		void Remove(NotifyCollectionChangedEventArgs args)
+		async Task Remove(NotifyCollectionChangedEventArgs args)
 		{
-			if (NotLoadedYet())
-			{
-				_collectionView.ReloadData();
-				return;
-			}
-
 			var startIndex = args.OldStartingIndex;
 
 			if (startIndex < 0)
 			{
 				// INCC implementation isn't giving us enough information to know where the removed items were in the
 				// collection. So the best we can do is a ReloadData()
-				Reload();
+				await Reload();
+				return;
+			}
+
+			if (ReloadRequired())
+			{
+				await Reload();
 				return;
 			}
 
 			// If we have a start index, we can be more clever about removing the item(s) (and get the nifty animations)
 			var count = args.OldItems.Count;
-			
+			Count -= count;
-			_collectionView.PerformBatchUpdates(() =>
+
-			{
+			// Queue up the updates to the UICollectionView
-				_collectionView.DeleteItems(CreateIndexesFrom(startIndex, count));
+			BatchUpdate(() => _collectionView.DeleteItems(CreateIndexesFrom(startIndex, count)));
-				Count -= count;
-			}, null);
 		}
 
-		void Replace(NotifyCollectionChangedEventArgs args)
+		async Task Replace(NotifyCollectionChangedEventArgs args)
 		{
 			var newCount = args.NewItems.Count;
 
@@ -229,7 +215,7 @@ namespace Xamarin.Forms.Platform.iOS
 
 			// The original and replacement sets are of unequal size; this means that everything currently in view will 
 			// have to be updated. So we just have to use ReloadData and let the UICollectionView update everything
-			Reload();
+			await Reload();
 		}
 
 		void Move(NotifyCollectionChangedEventArgs args)
@@ -293,5 +279,52 @@ namespace Xamarin.Forms.Platform.iOS
 
 			return -1;
 		}
+
+		bool NotLoadedYet()
+		{
+			// If the UICollectionView hasn't actually been loaded, then calling InsertItems or DeleteItems is 
+			// going to crash or get in an unusable state; instead, ReloadData should be used
+			return !_collectionViewController.IsViewLoaded || _collectionViewController.View.Window == null;
+		}
+
+		bool ReloadRequired()
+		{
+			if (NotLoadedYet())
+			{
+				return true;
+			}
+
+			// UICollectionView doesn't like when we insert items into a completely empty un-grouped CV,
+			// and it doesn't like when we insert items into a grouped CV with no actual cells (just empty groups)
+			// In those circumstances, we just need to ask it to reload the data so it can get its internal
+			// accounting in order
+
+			if (!_grouped && _collectionView.NumberOfItemsInSection(_section) == 0)
+			{
+				return true;
+			}
+
+			return _collectionView.VisibleCells.Length == 0;
+		}
+
+		void BatchUpdate(Action update)
+		{
+			_collectionView.PerformBatchUpdates(() =>
+			{
+				if (_batchUpdating.CurrentCount > 0)
+				{
+					_batchUpdating.Wait();
+				}
+
+				update();
+			},
+					(_) =>
+					{
+						if (_batchUpdating.CurrentCount > 0)
+						{
+							_batchUpdating.Release();
+						}
+					});
+		}
 	}
 }