From 10b3d774c7650d43b17726bc87590ea60240e680 Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Wed, 16 Oct 2019 14:54:16 -0600 Subject: [PATCH] [iOS] Prevent crash when adding item to source of CollectionView in tab (#7940) * Prevent crash when adding item to source of CollectionView that's never been displayed; fixes #7700 * Fix renaming error in UI test * Add test and fix for adding groups offscreen * Fix rebase error * Remove flags * Add instructions * Better instructions --- .../Issue7700.cs | 165 ++++++++++++++++++ .../Issue7993.xaml.cs | 2 - ...rin.Forms.Controls.Issues.Shared.projitems | 1 + .../GroupableItemsViewController.cs | 2 +- .../CollectionView/ItemsSourceFactory.cs | 8 +- .../CollectionView/ItemsViewController.cs | 2 +- .../CollectionView/ListSource.cs | 2 +- .../CollectionView/ObservableGroupedSource.cs | 31 +++- .../CollectionView/ObservableItemsSource.cs | 50 ++++-- 9 files changed, 231 insertions(+), 32 deletions(-) create mode 100644 Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7700.cs diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7700.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7700.cs new file mode 100644 index 000000000..b400aafe7 --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7700.cs @@ -0,0 +1,165 @@ +using System; +using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.Text; +using Xamarin.Forms.CustomAttributes; +using Xamarin.Forms.Internals; + +#if UITEST +using Xamarin.Forms.Core.UITests; +using Xamarin.UITest; +using NUnit.Framework; +#endif + +namespace Xamarin.Forms.Controls.Issues +{ +#if UITEST + [Category(UITestCategories.CollectionView)] +#endif + [Preserve(AllMembers = true)] + [Issue(IssueTracker.Github, 7700, "[Bug][iOS] If CollectionView in other Tab gets changed before it's displayed, it stays invisible", + PlatformAffected.iOS)] + public class Issue7700 : TestTabbedPage + { + readonly ObservableCollection _source = new ObservableCollection(){ "one", "two", "three" }; + readonly ObservableCollection _groupedSource = new ObservableCollection(); + + [Preserve(AllMembers = true)] + class Group : List + { + public string Text { get; set; } + + public Group() + { + Add("Uno"); + Add("Dos"); + Add("Tres"); + } + } + + const string Add1 = "Add1"; + const string Add2 = "Add2"; + const string Success = "Success"; + const string Tab2 = "Tab2"; + const string Tab3 = "Tab3"; + const string Add1Label = "Add to List"; + const string Add2Label = "Add to Grouped List"; + + protected override void Init() + { +#if APP + Children.Add(FirstPage()); + Children.Add(CollectionViewPage()); + Children.Add(GroupedCollectionViewPage()); +#endif + } + + ContentPage FirstPage() + { + var page = new ContentPage() { Title = "7700 First Page", Padding = 40 }; + + var instructions = new Label { Text = $"Tap the button marked '{Add1Label}'. Then tap the button marked '{Add2Label}'. If the application does not crash, the test has passed." }; + + var button1 = new Button() { Text = Add1Label, AutomationId = Add1 }; + button1.Clicked += Button1Clicked; + + var button2 = new Button() { Text = Add2Label, AutomationId = Add2 }; + button2.Clicked += Button2Clicked; + + var layout = new StackLayout { Children = { instructions, button1, button2 } }; + + page.Content = layout; + + return page; + } + + void Button1Clicked(object sender, EventArgs e) + { + _source.Insert(0, Success); + } + + void Button2Clicked(object sender, EventArgs e) + { + _groupedSource.Insert(0, new Group() { Text = Success }); + } + + ContentPage CollectionViewPage() + { + var cv = new CollectionView + { + ItemTemplate = new DataTemplate(() => + { + var label = new Label(); + label.SetBinding(Label.TextProperty, new Binding(".")); + return label; + }), + + ItemsSource = _source + }; + + var page = new ContentPage() { Title = Tab2, Padding = 40 }; + + page.Content = cv; + + return page; + } + + ContentPage GroupedCollectionViewPage() + { + var cv = new CollectionView + { + ItemTemplate = new DataTemplate(() => + { + var label = new Label(); + label.SetBinding(Label.TextProperty, new Binding(".")); + return label; + }), + + GroupHeaderTemplate = new DataTemplate(() => + { + var label = new Label(); + label.SetBinding(Label.TextProperty, new Binding("Text")); + return label; + }), + + GroupFooterTemplate = new DataTemplate(() => + { + var label = new Label(); + label.SetBinding(Label.TextProperty, new Binding("Text")); + return label; + }), + + ItemsSource = _groupedSource, + IsGrouped = true + }; + + var page = new ContentPage() { Title = Tab3, Padding = 40 }; + + page.Content = cv; + + return page; + } + +#if UITEST + [Test] + public void AddingItemToUnviewedCollectionViewShouldNotCrash() + { + RunningApp.WaitForElement(Add1); + RunningApp.Tap(Add1); + RunningApp.Tap(Tab2); + + RunningApp.WaitForElement(Success); + } + + [Test] + public void AddingGroupToUnviewedGroupedCollectionViewShouldNotCrash() + { + RunningApp.WaitForElement(Add2); + RunningApp.Tap(Add2); + RunningApp.Tap(Tab3); + + RunningApp.WaitForElement(Success); + } +#endif + } +} diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7993.xaml.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7993.xaml.cs index a250d0ab7..1441ce94d 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7993.xaml.cs +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7993.xaml.cs @@ -28,8 +28,6 @@ namespace Xamarin.Forms.Controls.Issues #if APP public Issue7993() { - Device.SetFlags(new List { CollectionView.CollectionViewExperimental }); - InitializeComponent(); BindingContext = new ViewModel7993(); 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 2bf01e6be..9de27c1ab 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 @@ -59,6 +59,7 @@ Code + Code diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/GroupableItemsViewController.cs b/Xamarin.Forms.Platform.iOS/CollectionView/GroupableItemsViewController.cs index c134a2cd6..ae416e5bf 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/GroupableItemsViewController.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/GroupableItemsViewController.cs @@ -30,7 +30,7 @@ namespace Xamarin.Forms.Platform.iOS // Use the BindableProperty here (instead of _isGroupingEnabled) because the cached value might not be set yet if (ItemsView.IsGrouped) { - return ItemsSourceFactory.CreateGrouped(ItemsView.ItemsSource, CollectionView); + return ItemsSourceFactory.CreateGrouped(ItemsView.ItemsSource, this); } return base.CreateItemsViewSource(); diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsSourceFactory.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsSourceFactory.cs index 7ec7df28b..b38261bc1 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsSourceFactory.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsSourceFactory.cs @@ -7,7 +7,7 @@ namespace Xamarin.Forms.Platform.iOS { internal static class ItemsSourceFactory { - public static IItemsViewSource Create(IEnumerable itemsSource, UICollectionView collectionView) + public static IItemsViewSource Create(IEnumerable itemsSource, UICollectionViewController collectionViewController) { if (itemsSource == null) { @@ -17,21 +17,21 @@ namespace Xamarin.Forms.Platform.iOS switch (itemsSource) { case INotifyCollectionChanged _: - return new ObservableItemsSource(itemsSource as IList, collectionView); + return new ObservableItemsSource(itemsSource as IList, collectionViewController); case IEnumerable _: default: return new ListSource(itemsSource); } } - public static IItemsViewSource CreateGrouped(IEnumerable itemsSource, UICollectionView collectionView) + public static IItemsViewSource CreateGrouped(IEnumerable itemsSource, UICollectionViewController collectionViewController) { if (itemsSource == null) { return new EmptySource(); } - return new ObservableGroupedSource(itemsSource, collectionView); + return new ObservableGroupedSource(itemsSource, collectionViewController); } } } \ No newline at end of file diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewController.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewController.cs index 8102155a3..4e059082d 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewController.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewController.cs @@ -166,7 +166,7 @@ namespace Xamarin.Forms.Platform.iOS protected virtual IItemsViewSource CreateItemsViewSource() { - return ItemsSourceFactory.Create(ItemsView.ItemsSource, CollectionView); + return ItemsSourceFactory.Create(ItemsView.ItemsSource, this); } public virtual void UpdateItemsSource() diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ListSource.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ListSource.cs index c789ce52b..c2673d378 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ListSource.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ListSource.cs @@ -42,7 +42,7 @@ namespace Xamarin.Forms.Platform.iOS } } - public int GroupCount => Count == 0 ? 0 : 1; + public int GroupCount => 1; public int ItemCount => Count; diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs index 68ef02436..fe2f1667f 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs @@ -4,19 +4,22 @@ using System.Collections.Generic; using System.Collections.Specialized; using Foundation; using UIKit; +using Xamarin.Forms.Internals; namespace Xamarin.Forms.Platform.iOS { internal class ObservableGroupedSource : IItemsViewSource { readonly UICollectionView _collectionView; + UICollectionViewController _collectionViewController; readonly IList _groupSource; bool _disposed; List _groups = new List(); - public ObservableGroupedSource(IEnumerable groupSource, UICollectionView collectionView) + public ObservableGroupedSource(IEnumerable groupSource, UICollectionViewController collectionViewController) { - _collectionView = collectionView; + _collectionViewController = collectionViewController; + _collectionView = _collectionViewController.CollectionView; _groupSource = groupSource as IList ?? new ListSource(groupSource); if (_groupSource is INotifyCollectionChanged incc) @@ -120,7 +123,7 @@ namespace Xamarin.Forms.Platform.iOS { if (_groupSource[n] is INotifyCollectionChanged && _groupSource[n] is IList list) { - _groups.Add(new ObservableItemsSource(list, _collectionView, n)); + _groups.Add(new ObservableItemsSource(list, _collectionViewController, n)); } } } @@ -161,6 +164,13 @@ namespace Xamarin.Forms.Platform.iOS return NSIndexSet.FromNSRange(new NSRange(startIndex, count)); } + bool NotLoadedYet() + { + // If the UICollectionView hasn't actually been loaded, then calling InsertSections or DeleteSections 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) { var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : _groupSource.IndexOf(args.NewItems[0]); @@ -170,6 +180,12 @@ namespace Xamarin.Forms.Platform.iOS // is to reset all the group tracking to get it up-to-date ResetGroupTracking(); + if (NotLoadedYet()) + { + _collectionView.ReloadData(); + return; + } + _collectionView.InsertSections(CreateIndexSetFrom(startIndex, count)); } @@ -192,7 +208,14 @@ namespace Xamarin.Forms.Platform.iOS // is to reset all the group tracking to get it up-to-date ResetGroupTracking(); - _collectionView.DeleteSections(CreateIndexSetFrom(startIndex, count)); + if (NotLoadedYet()) + { + _collectionView.ReloadData(); + } + else + { + _collectionView.DeleteSections(CreateIndexSetFrom(startIndex, count)); + } } void Replace(NotifyCollectionChangedEventArgs args) diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs index 6b881cdc1..1d9d749c2 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs @@ -8,21 +8,23 @@ namespace Xamarin.Forms.Platform.iOS { internal class ObservableItemsSource : IItemsViewSource { + readonly UICollectionViewController _collectionViewController; readonly UICollectionView _collectionView; readonly bool _grouped; readonly int _section; readonly IList _itemsSource; bool _disposed; - public ObservableItemsSource(IList itemSource, UICollectionView collectionView, int group = -1) + public ObservableItemsSource(IList itemSource, UICollectionViewController collectionViewController, int group = -1) { - _collectionView = collectionView; + _collectionViewController = collectionViewController; + _collectionView = _collectionViewController.CollectionView; _section = group < 0 ? 0 : group; _grouped = group >= 0; _itemsSource = itemSource; - + ((INotifyCollectionChanged)itemSource).CollectionChanged += CollectionChanged; } @@ -71,7 +73,7 @@ namespace Xamarin.Forms.Platform.iOS return NSIndexPath.Create(-1, -1); } - public int GroupCount => _itemsSource.Count == 0 ? 0 : 1; + public int GroupCount => 1; public int ItemCount => _itemsSource.Count; @@ -130,32 +132,49 @@ namespace Xamarin.Forms.Platform.iOS return result; } + 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; + } + void Add(NotifyCollectionChangedEventArgs args) { var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : _itemsSource.IndexOf(args.NewItems[0]); var count = args.NewItems.Count; - if (!_grouped && _collectionView.NumberOfSections() != GroupCount && count > 0) + if (NotLoadedYet()) { - // Okay, we're going from completely empty to more than 0 items; this means we don't even - // have a section 0 yet. Inserting a section 0 manually results in an unexplained crash, so instead - // we'll just reload the data so the UICollectionView can get its internal state sorted out. _collectionView.ReloadData(); + return; } - else + + if (!_grouped && _collectionView.NumberOfItemsInSection(_section) == 0) { - _collectionView.PerformBatchUpdates(() => + // 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.PerformBatchUpdates(() => { var indexes = CreateIndexesFrom(startIndex, count); _collectionView.InsertItems(indexes); }, null); - } } void Remove(NotifyCollectionChangedEventArgs args) { var startIndex = args.OldStartingIndex; + if (NotLoadedYet()) + { + _collectionView.ReloadData(); + return; + } + if (startIndex < 0) { // INCC implementation isn't giving us enough information to know where the removed items were in the @@ -163,20 +182,13 @@ namespace Xamarin.Forms.Platform.iOS 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)); - - if (!_grouped && _collectionView.NumberOfSections() != GroupCount) - { - // We had a non-grouped list with items, and we're removing the last one; - // we also need to remove the group it was in - _collectionView.DeleteSections(new NSIndexSet(0)); - } }, null); }