diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue13126.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue13126.cs new file mode 100644 index 00000000000..efb0cbaab8b --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue13126.cs @@ -0,0 +1,181 @@ +using System; +using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.Collections.Specialized; +using System.ComponentModel; +using System.Globalization; +using System.Text; +using System.Threading.Tasks; +using Xamarin.Forms.CustomAttributes; +using Xamarin.Forms.Internals; + +#if UITEST +using Xamarin.UITest; +using NUnit.Framework; +using Xamarin.Forms.Core.UITests; +#endif + +namespace Xamarin.Forms.Controls.Issues +{ + [Issue(IssueTracker.Github, 13126, "[Bug] Regression: 5.0.0-pre5 often fails to draw dynamically loaded collection view content", PlatformAffected.iOS)] +#if UITEST + [NUnit.Framework.Category(UITestCategories.CollectionView)] +#endif + public class Issue13126 : TestContentPage + { + _13126VM _vm; + const string Success = "Success"; + + protected override void Init() + { + var collectionView = BindingWithConverter(); + + var grid = new Grid + { + RowDefinitions = new RowDefinitionCollection + { + new RowDefinition() { Height = GridLength.Star }, + } + }; + + grid.Children.Add(collectionView); + + Content = grid; + + _vm = new _13126VM(); + BindingContext = _vm; + } + + protected async override void OnParentSet() + { + base.OnParentSet(); + _vm.IsBusy = true; + + await Task.Delay(1000); + + _vm.Data.Add(Success); + + _vm.IsBusy = false; + } + + internal static CollectionView BindingWithConverter() + { + var cv = new CollectionView + { + IsVisible = true, + + ItemTemplate = new DataTemplate(() => + { + var label = new Label(); + label.SetBinding(Label.TextProperty, new Binding(".")); + return label; + }) + }; + + cv.EmptyView = new Label { Text = "Should not see me" }; + + cv.SetBinding(CollectionView.ItemsSourceProperty, new Binding("Data")); + cv.SetBinding(VisualElement.IsVisibleProperty, new Binding("IsBusy", converter: new BoolInverter())); + + return cv; + } + + class BoolInverter : IValueConverter + { + public object Convert(object value, Type targetType, object parameter, CultureInfo culture) + { + return !((bool)value); + } + + public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture) + { + throw new NotImplementedException(); + } + } + + internal class _13126VM : INotifyPropertyChanged + { + private bool _isBusy; + + public bool IsBusy + { + get + { + return _isBusy; + } + + set + { + _isBusy = value; + OnPropertyChanged(nameof(IsBusy)); + } + } + + public OptimizedObservableCollection Data { get; } = new OptimizedObservableCollection(); + + public event PropertyChangedEventHandler PropertyChanged; + + void OnPropertyChanged(string name) + { + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(name)); + } + } + + internal class OptimizedObservableCollection : ObservableCollection + { + bool _shouldRaiseNotifications = true; + + public OptimizedObservableCollection() + { + } + + public OptimizedObservableCollection(IEnumerable collection) + : base(collection) + { + } + + public IDisposable BeginMassUpdate() + { + return new MassUpdater(this); + } + + protected override void OnCollectionChanged(NotifyCollectionChangedEventArgs e) + { + if (_shouldRaiseNotifications) + base.OnCollectionChanged(e); + } + + protected override void OnPropertyChanged(PropertyChangedEventArgs e) + { + if (_shouldRaiseNotifications) + base.OnPropertyChanged(e); + } + + class MassUpdater : IDisposable + { + readonly OptimizedObservableCollection parent; + public MassUpdater(OptimizedObservableCollection parent) + { + this.parent = parent; + parent._shouldRaiseNotifications = false; + } + + public void Dispose() + { + parent._shouldRaiseNotifications = true; + parent.OnPropertyChanged(new PropertyChangedEventArgs("Count")); + parent.OnPropertyChanged(new PropertyChangedEventArgs("Item[]")); + parent.OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset)); + } + } + } + +#if UITEST + [Test] + public void CollectionViewShouldSourceShouldUpdateWhileInvisible() + { + RunningApp.WaitForElement(Success); + } +#endif + } +} diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue13126_2.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue13126_2.cs new file mode 100644 index 00000000000..263d40cdbad --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue13126_2.cs @@ -0,0 +1,74 @@ +using System; +using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.Collections.Specialized; +using System.ComponentModel; +using System.Globalization; +using System.Text; +using System.Threading.Tasks; +using Xamarin.Forms.CustomAttributes; +using Xamarin.Forms.Internals; +using static Xamarin.Forms.Controls.Issues.Issue13126; + +#if UITEST +using Xamarin.UITest; +using NUnit.Framework; +using Xamarin.Forms.Core.UITests; +#endif + +namespace Xamarin.Forms.Controls.Issues +{ + [Issue(IssueTracker.Github, 13126, "[Bug] Regression: 5.0.0-pre5 often fails to draw dynamically loaded collection view content", + PlatformAffected.iOS, issueTestNumber:1)] +#if UITEST + [NUnit.Framework.Category(UITestCategories.CollectionView)] +#endif + public class Issue13126_2 : TestContentPage + { + _13126VM _vm; + const string Success = "Success"; + + protected override void Init() + { + var collectionView = BindingWithConverter(); + + var grid = new Grid + { + RowDefinitions = new RowDefinitionCollection + { + new RowDefinition() { Height = GridLength.Star }, + } + }; + + grid.Children.Add(collectionView); + + Content = grid; + + _vm = new _13126VM(); + BindingContext = _vm; + } + + protected async override void OnParentSet() + { + base.OnParentSet(); + _vm.IsBusy = true; + + await Task.Delay(1000); + + using (_vm.Data.BeginMassUpdate()) + { + _vm.Data.Add(Success); + } + + _vm.IsBusy = false; + } + +#if UITEST + [Test] + public void CollectionViewShouldSourceShouldResetWhileInvisible() + { + 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 84a65ef8817..c3239ce5963 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 @@ -12,6 +12,8 @@ + + diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/CarouselViewController.cs b/Xamarin.Forms.Platform.iOS/CollectionView/CarouselViewController.cs index df300a111b0..a8b34c3b68d 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/CarouselViewController.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/CarouselViewController.cs @@ -469,6 +469,18 @@ void UpdateVisualStates() _oldViews = newViews; } + + protected internal override void UpdateVisibility() + { + if (ItemsView.IsVisible) + { + CollectionView.Hidden = false; + } + else + { + CollectionView.Hidden = true; + } + } } class CarouselViewLoopManager : IDisposable diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewController.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewController.cs index 995957b72bd..076b29176b7 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewController.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewController.cs @@ -22,8 +22,7 @@ public abstract class ItemsViewController : UICollectionViewControll UIView _emptyUIView; VisualElement _emptyViewFormsElement; - Dictionary _measurementCells = new Dictionary(); - Dictionary _cellSizeCache = new Dictionary(); + Dictionary _measurementCells = new Dictionary(); protected UICollectionViewDelegateFlowLayout Delegator { get; set; } @@ -31,8 +30,6 @@ protected ItemsViewController(TItemsView itemsView, ItemsViewLayout layout) : ba { ItemsView = itemsView; ItemsViewLayout = layout; - - ItemsView.PropertyChanged += ItemsViewPropertyChanged; } public void UpdateLayout(ItemsViewLayout newLayout) @@ -62,8 +59,6 @@ protected override void Dispose(bool disposing) if (disposing) { - ItemsView.PropertyChanged -= ItemsViewPropertyChanged; - ItemsSource?.Dispose(); CollectionView.Delegate = null; @@ -100,11 +95,6 @@ public override UICollectionViewCell GetCell(UICollectionView collectionView, NS public override nint GetItemsCount(UICollectionView collectionView, nint section) { - if (!_initialized) - { - return 0; - } - CheckForEmptySource(); return ItemsSource.ItemCountInGroup(section); @@ -119,7 +109,7 @@ void CheckForEmptySource() if (_isEmpty) { _measurementCells.Clear(); - _cellSizeCache.Clear(); + ItemsViewLayout?.ClearCellSizeCache(); } if (wasEmpty != _isEmpty) @@ -154,28 +144,41 @@ public override void ViewDidLoad() } RegisterViewTypes(); + + EnsureLayoutInitialized(); + } + + public override void ViewWillAppear(bool animated) + { + base.ViewWillAppear(animated); + ConstrainToItemsView(); } public override void ViewWillLayoutSubviews() { + ConstrainToItemsView(); base.ViewWillLayoutSubviews(); - - EnsureLayoutInitialized(); - LayoutEmptyView(); } - void EnsureLayoutInitialized() + void ConstrainToItemsView() { - if (_initialized) + var itemsViewWidth = ItemsView.Width; + var itemsViewHeight = ItemsView.Height; + + if (itemsViewHeight < 0 || itemsViewWidth < 0) { + ItemsViewLayout.UpdateConstraints(CollectionView.Bounds.Size); return; } - if (!ItemsView.IsVisible) + ItemsViewLayout.UpdateConstraints(new CGSize(itemsViewWidth, itemsViewHeight)); + } + + void EnsureLayoutInitialized() + { + if (_initialized) { - // If the CollectionView starts out invisible, we'll get a layout pass with a size of 1,1 and everything will - // go pear-shaped. So until the first time this CollectionView is visible, we do nothing. return; } @@ -205,7 +208,7 @@ protected virtual IItemsViewSource CreateItemsViewSource() public virtual void UpdateItemsSource() { _measurementCells.Clear(); - _cellSizeCache.Clear(); + ItemsViewLayout?.ClearCellSizeCache(); ItemsSource = CreateItemsViewSource(); CollectionView.ReloadData(); CollectionView.CollectionViewLayout.InvalidateLayout(); @@ -225,11 +228,6 @@ public virtual void UpdateFlowDirection() public override nint NumberOfSections(UICollectionView collectionView) { - if(!_initialized) - { - return 0; - } - CheckForEmptySource(); return ItemsSource.GroupCount; } @@ -249,10 +247,12 @@ protected virtual void UpdateTemplatedCell(TemplatedCell cell, NSIndexPath index cell.ContentSizeChanged -= CellContentSizeChanged; cell.LayoutAttributesChanged -= CellLayoutAttributesChanged; + var bindingContext = ItemsSource[indexPath]; + // If we've already created a cell for this index path (for measurement), re-use the content - if (_measurementCells.TryGetValue(indexPath, out TemplatedCell measurementCell)) + if (_measurementCells.TryGetValue(bindingContext, out TemplatedCell measurementCell)) { - _measurementCells.Remove(indexPath); + _measurementCells.Remove(bindingContext); measurementCell.ContentSizeChanged -= CellContentSizeChanged; measurementCell.LayoutAttributesChanged -= CellLayoutAttributesChanged; cell.UseContent(measurementCell); @@ -316,7 +316,7 @@ protected virtual void CacheCellAttributes(NSIndexPath indexPath, CGSize size) var item = ItemsSource[indexPath]; if (item != null) { - _cellSizeCache[item] = size; + ItemsViewLayout.CacheCellSize(item, size); } } @@ -568,14 +568,16 @@ public UICollectionViewCell CreateMeasurementCell(NSIndexPath indexPath) { var frame = new CGRect(0, 0, ItemsViewLayout.EstimatedItemSize.Width, ItemsViewLayout.EstimatedItemSize.Height); + DefaultCell cell; if (ItemsViewLayout.ScrollDirection == UICollectionViewScrollDirection.Horizontal) { - var cell1 = new HorizontalDefaultCell(frame); - UpdateDefaultCell(cell1, indexPath); - return cell1; + cell = new HorizontalDefaultCell(frame); } - - var cell = new VerticalDefaultCell(frame); + else + { + cell = new VerticalDefaultCell(frame); + } + UpdateDefaultCell(cell, indexPath); return cell; } @@ -585,7 +587,7 @@ public UICollectionViewCell CreateMeasurementCell(NSIndexPath indexPath) UpdateTemplatedCell(templatedCell, indexPath); // Keep this cell around, we can transfer the contents to the actual cell when the UICollectionView creates it - _measurementCells[indexPath] = templatedCell; + _measurementCells[ItemsSource[indexPath]] = templatedCell; return templatedCell; } @@ -601,7 +603,7 @@ internal CGSize GetSizeForItem(NSIndexPath indexPath) { var item = ItemsSource[indexPath]; - if (item != null && _cellSizeCache.TryGetValue(item, out CGSize size)) + if (item != null && ItemsViewLayout.TryGetCachedCellSize(item, out CGSize size)) { return size; } @@ -610,16 +612,21 @@ internal CGSize GetSizeForItem(NSIndexPath indexPath) return ItemsViewLayout.EstimatedItemSize; } - void ItemsViewPropertyChanged(object sender, PropertyChangedEventArgs changedProperty) + internal protected virtual void UpdateVisibility() { - if (changedProperty.Is(VisualElement.IsVisibleProperty)) + if (ItemsView.IsVisible) { - if (ItemsView.IsVisible) + if (CollectionView.Hidden) { + CollectionView.Hidden = false; Layout.InvalidateLayout(); CollectionView.LayoutIfNeeded(); } } + else + { + CollectionView.Hidden = true; + } } } } diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewLayout.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewLayout.cs index 9dd62c5bcbd..ecac7073591 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewLayout.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewLayout.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.ComponentModel; using CoreGraphics; using Foundation; @@ -16,6 +17,8 @@ public abstract class ItemsViewLayout : UICollectionViewFlowLayout CGSize _adjustmentSize1; CGSize _currentSize; + Dictionary _cellSizeCache = new Dictionary(); + public ItemsUpdatingScrollMode ItemsUpdatingScrollMode { get; set; } public nfloat ConstrainedDimension { get; set; } @@ -90,6 +93,8 @@ internal virtual void UpdateConstraints(CGSize size) return; } + ClearCellSizeCache(); + _currentSize = size; var newSize = new CGSize(Math.Floor(size.Width), Math.Floor(size.Height)); @@ -561,5 +566,27 @@ public override bool ShouldInvalidateLayoutForBoundsChange(CGRect newBounds) return true; } + + internal bool TryGetCachedCellSize(object item, out CGSize size) + { + if (_cellSizeCache.TryGetValue(item, out CGSize internalSize)) + { + size = internalSize; + return true; + } + + size = CGSize.Empty; + return false; + } + + internal void CacheCellSize(object item, CGSize size) + { + _cellSizeCache[item] = size; + } + + internal void ClearCellSizeCache() + { + _cellSizeCache.Clear(); + } } } diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewRenderer.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewRenderer.cs index fd740c03e6c..d91585bd871 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewRenderer.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewRenderer.cs @@ -71,6 +71,10 @@ protected override void OnElementPropertyChanged(object sender, PropertyChangedE { UpdateFlowDirection(); } + else if (changedProperty.Is(VisualElement.IsVisibleProperty)) + { + UpdateVisibility(); + } } protected abstract ItemsViewLayout SelectLayout(); @@ -101,6 +105,7 @@ protected virtual void SetUpNewElement(TItemsView newElement) UpdateVerticalScrollBarVisibility(); UpdateItemsUpdatingScrollMode(); UpdateFlowDirection(); + UpdateVisibility(); // Listen for ScrollTo requests newElement.ScrollToRequested += ScrollToRequested; @@ -142,6 +147,11 @@ protected virtual void UpdateItemsSource() Controller.UpdateItemsSource(); } + protected virtual void UpdateVisibility() + { + Controller?.UpdateVisibility(); + } + protected abstract TViewController CreateController(TItemsView newElement, ItemsViewLayout layout); NSIndexPath DetermineIndex(ScrollToRequestEventArgs args) diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs index 5ab2bdc6563..0358e6b2671 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs @@ -142,7 +142,6 @@ void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args) void CollectionChanged(NotifyCollectionChangedEventArgs args) { switch (args.Action) - { case NotifyCollectionChangedAction.Add: Add(args); @@ -200,7 +199,7 @@ void Add(NotifyCollectionChangedEventArgs args) ResetGroupTracking(); // Queue up the updates to the UICollectionView - _collectionView.InsertSections(CreateIndexSetFrom(startIndex, count)); + Update(() => _collectionView.InsertSections(CreateIndexSetFrom(startIndex, count))); } void Remove(NotifyCollectionChangedEventArgs args) @@ -229,7 +228,7 @@ void Remove(NotifyCollectionChangedEventArgs args) var count = args.OldItems.Count; // Queue up the updates to the UICollectionView - _collectionView.DeleteSections(CreateIndexSetFrom(startIndex, count)); + Update(() => _collectionView.DeleteSections(CreateIndexSetFrom(startIndex, count))); } void Replace(NotifyCollectionChangedEventArgs args) @@ -243,7 +242,7 @@ void Replace(NotifyCollectionChangedEventArgs args) var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : _groupSource.IndexOf(args.NewItems[0]); // We are replacing one set of items with a set of equal size; we can do a simple item range update - _collectionView.ReloadSections(CreateIndexSetFrom(startIndex, newCount)); + Update(() => _collectionView.ReloadSections(CreateIndexSetFrom(startIndex, newCount))); return; } @@ -261,14 +260,14 @@ void Move(NotifyCollectionChangedEventArgs args) if (count == 1) { // For a single item, we can use MoveSection and get the animation - _collectionView.MoveSection(args.OldStartingIndex, args.NewStartingIndex); + Update(() => _collectionView.MoveSection(args.OldStartingIndex, args.NewStartingIndex)); return; } var start = Math.Min(args.OldStartingIndex, args.NewStartingIndex); var end = Math.Max(args.OldStartingIndex, args.NewStartingIndex) + count; - _collectionView.ReloadSections(CreateIndexSetFrom(start, end)); + Update(() => _collectionView.ReloadSections(CreateIndexSetFrom(start, end))); } int GetGroupCount(int groupIndex) @@ -343,5 +342,15 @@ bool ReloadRequired() return NotLoadedYet() || _collectionView.NumberOfSections() == 0; } + + void Update(Action update) + { + if (_collectionView.Hidden) + { + return; + } + + update(); + } } } diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs index 596d93a4ea6..bcf85a9a912 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs @@ -111,12 +111,6 @@ void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args) void CollectionChanged(NotifyCollectionChangedEventArgs args) { - if (CollectionView.NumberOfSections() == 0) - { - // The CollectionView isn't fully initialized yet - return; - } - // Force UICollectionView to get the internal accounting straight CollectionView.NumberOfItemsInSection(_section); @@ -199,9 +193,8 @@ void Replace(NotifyCollectionChangedEventArgs args) var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : IndexOf(args.NewItems[0]); // We are replacing one set of items with a set of equal size; we can do a simple item range update - OnCollectionViewUpdating(args); - CollectionView.ReloadItems(CreateIndexesFrom(startIndex, newCount)); - OnCollectionViewUpdated(args); + + Update(() => CollectionView.ReloadItems(CreateIndexesFrom(startIndex, newCount)), args); return; } @@ -220,18 +213,14 @@ void Move(NotifyCollectionChangedEventArgs args) var oldPath = NSIndexPath.Create(_section, args.OldStartingIndex); var newPath = NSIndexPath.Create(_section, args.NewStartingIndex); - OnCollectionViewUpdating(args); - CollectionView.MoveItem(oldPath, newPath); - OnCollectionViewUpdated(args); + Update(() => CollectionView.MoveItem(oldPath, newPath), args); return; } var start = Math.Min(args.OldStartingIndex, args.NewStartingIndex); var end = Math.Max(args.OldStartingIndex, args.NewStartingIndex) + count; - OnCollectionViewUpdating(args); - CollectionView.ReloadItems(CreateIndexesFrom(start, end)); - OnCollectionViewUpdated(args); + Update(() => CollectionView.ReloadItems(CreateIndexesFrom(start, end)), args); } internal int ItemsCount() @@ -276,8 +265,14 @@ internal int IndexOf(object item) return -1; } + void Update(Action update, NotifyCollectionChangedEventArgs args) { + if (CollectionView.Hidden) + { + return; + } + OnCollectionViewUpdating(args); update(); OnCollectionViewUpdated(args);