diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue9580.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue9580.cs new file mode 100644 index 000000000..f6ba912b5 --- /dev/null +++ b/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 + } +} diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue9686.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue9686.cs new file mode 100644 index 000000000..856146378 --- /dev/null +++ b/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 + } +} diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems index c708a9956..233646876 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems @@ -37,6 +37,7 @@ Issue8902.xaml Code + Issue9682.xaml Code @@ -216,6 +217,7 @@ Issue9783.xaml Code + Code diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs index 7958483a1..4ebb79566 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs +++ b/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 _groups = new List(); 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()) - { - _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)); + // Queue up the updates to the UICollectionView + BatchUpdate(() => _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() - Reload(); + // collection. So the best we can do is a complete 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) - var count = args.OldItems.Count; + if (ReloadRequired()) + { + 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()) - { - _collectionView.ReloadData(); - } - else - { - _collectionView.DeleteSections(CreateIndexSetFrom(startIndex, count)); - } + // 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; + + // Queue up the updates to the UICollectionView + BatchUpdate(() => _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(); + } + }); + } + } } diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs index e5ae0839c..c735e8f5f 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs +++ b/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 - // 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()) + if (ReloadRequired()) { - _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) - { - // 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); + // Queue up the updates to the UICollectionView + BatchUpdate(() => _collectionView.InsertItems(CreateIndexesFrom(startIndex, count))); } - 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; - - _collectionView.PerformBatchUpdates(() => - { - _collectionView.DeleteItems(CreateIndexesFrom(startIndex, count)); - Count -= count; - }, null); + Count -= count; + + // Queue up the updates to the UICollectionView + BatchUpdate(() => _collectionView.DeleteItems(CreateIndexesFrom(startIndex, count))); } - 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(); + } + }); + } } }