Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent iOS CollectionView size shifts from clearing the cell size cache #18464

Merged
merged 2 commits into from
Nov 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 7 additions & 16 deletions src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
using Foundation;
using Microsoft.Maui.Controls.Internals;
using Microsoft.Maui.Graphics;
using ObjCRuntime;
using UIKit;

namespace Microsoft.Maui.Controls.Handlers.Items
Expand Down Expand Up @@ -178,12 +177,12 @@ public override void ViewDidLoad()
public override void ViewWillAppear(bool animated)
{
base.ViewWillAppear(animated);
ConstrainToItemsView();
ConstrainItemsToBounds();
}

public override void ViewWillLayoutSubviews()
{
ConstrainToItemsView();
ConstrainItemsToBounds();
base.ViewWillLayoutSubviews();
InvalidateMeasureIfContentSizeChanged();
LayoutEmptyView();
Expand Down Expand Up @@ -243,26 +242,19 @@ void InvalidateMeasureIfContentSizeChanged()

internal Size? GetSize()
{
if (_emptyViewDisplayed)
if (_emptyViewDisplayed)
{
return _emptyUIView.Frame.Size.ToSize();
}

return CollectionView.CollectionViewLayout.CollectionViewContentSize.ToSize();
}

void ConstrainToItemsView()
void ConstrainItemsToBounds()
{
var itemsViewWidth = ItemsView.Width;
var itemsViewHeight = ItemsView.Height;

if (itemsViewHeight < 0 || itemsViewWidth < 0)
{
ItemsViewLayout.UpdateConstraints(CollectionView.Bounds.Size);
return;
}

ItemsViewLayout.UpdateConstraints(new CGSize(itemsViewWidth, itemsViewHeight));
var contentBounds = CollectionView.AdjustedContentInset.InsetRect(CollectionView.Bounds);
var constrainedSize = contentBounds.Size;
ItemsViewLayout.UpdateConstraints(constrainedSize);
}

void EnsureLayoutInitialized()
Expand Down Expand Up @@ -321,7 +313,6 @@ public virtual void UpdateFlowDirection()
Layout.InvalidateLayout();
}


public override nint NumberOfSections(UICollectionView collectionView)
{
CheckForEmptySource();
Expand Down
38 changes: 4 additions & 34 deletions src/Controls/src/Core/Handlers/Items/iOS/ItemsViewLayout.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
using CoreGraphics;
using Foundation;
using Microsoft.Extensions.Logging;
using Microsoft.Maui.Controls.Internals;
using ObjCRuntime;
using UIKit;

namespace Microsoft.Maui.Controls.Handlers.Items
Expand All @@ -21,9 +19,7 @@ public abstract class ItemsViewLayout : UICollectionViewFlowLayout
CGSize _currentSize;
WeakReference<Func<UICollectionViewCell>> _getPrototype;

const double ConstraintSizeTolerance = 0.00001;

Dictionary<object, CGSize> _cellSizeCache = new Dictionary<object, CGSize>();
readonly Dictionary<object, CGSize> _cellSizeCache = new();

public ItemsUpdatingScrollMode ItemsUpdatingScrollMode { get; set; }

Expand Down Expand Up @@ -102,7 +98,7 @@ protected virtual void HandlePropertyChanged(PropertyChangedEventArgs propertyCh

internal virtual void UpdateConstraints(CGSize size)
{
if (!RequiresConstraintUpdate(size, _currentSize))
if (size.IsCloseTo(_currentSize))
{
return;
}
Expand Down Expand Up @@ -568,23 +564,12 @@ static void ForceScrollToLastItem(UICollectionView collectionView, ItemsLayout i

public override bool ShouldInvalidateLayoutForBoundsChange(CGRect newBounds)
{
if (newBounds.Size == _currentSize)
if(newBounds.Size == _currentSize)
{
return base.ShouldInvalidateLayoutForBoundsChange(newBounds);
}

if (OperatingSystem.IsIOSVersionAtLeast(11) || OperatingSystem.IsMacCatalystVersionAtLeast(11)
#if TVOS
|| OperatingSystem.IsTvOSVersionAtLeast(11)
#endif
)
{
UpdateConstraints(CollectionView.AdjustedContentInset.InsetRect(newBounds).Size);
}
else
{
UpdateConstraints(CollectionView.Bounds.Size);
}
UpdateConstraints(CollectionView.AdjustedContentInset.InsetRect(newBounds).Size);

return true;
}
Expand All @@ -610,20 +595,5 @@ internal void ClearCellSizeCache()
{
_cellSizeCache.Clear();
}

bool RequiresConstraintUpdate(CGSize newSize, CGSize current)
{
if (Math.Abs(newSize.Width - current.Width) > ConstraintSizeTolerance)
{
return true;
}

if (Math.Abs(newSize.Height - current.Height) > ConstraintSizeTolerance)
{
return true;
}

return false;
}
}
}
42 changes: 42 additions & 0 deletions src/Controls/src/Core/Handlers/Items/iOS/SizeExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#nullable disable
using System;
using CoreGraphics;
using Microsoft.Maui.Graphics;

namespace Microsoft.Maui.Controls.Handlers.Items
{
internal static class SizeExtensions
{
const double Tolerance = 0.001;

public static bool IsCloseTo(this CGSize sizeA, CGSize sizeB)
{
if (Math.Abs(sizeA.Height - sizeB.Height) > Tolerance)
{
return false;
}

if (Math.Abs(sizeA.Width - sizeB.Width) > Tolerance)
{
return false;
}

return true;
}

public static bool IsCloseTo(this CGSize sizeA, Size sizeB)
{
if (Math.Abs(sizeA.Height - sizeB.Height) > Tolerance)
{
return false;
}

if (Math.Abs(sizeA.Width - sizeB.Width) > Tolerance)
{
return false;
}

return true;
}
}
}
22 changes: 1 addition & 21 deletions src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using Foundation;
using Microsoft.Maui.Controls.Internals;
using Microsoft.Maui.Graphics;
using ObjCRuntime;
using UIKit;

namespace Microsoft.Maui.Controls.Handlers.Items
Expand Down Expand Up @@ -66,7 +65,7 @@ public override UICollectionViewLayoutAttributes PreferredLayoutAttributesFittin

var preferredSize = preferredAttributes.Frame.Size;

if (SizesAreSame(preferredSize, _size)
if (preferredSize.IsCloseTo(_size)
&& AttributesConsistentWithConstrainedDimension(preferredAttributes))
{
return preferredAttributes;
Expand All @@ -79,8 +78,6 @@ public override UICollectionViewLayoutAttributes PreferredLayoutAttributesFittin

OnLayoutAttributesChanged(preferredAttributes);

//_isMeasured = true;

return preferredAttributes;
}

Expand Down Expand Up @@ -290,23 +287,6 @@ protected void OnLayoutAttributesChanged(UICollectionViewLayoutAttributes newAtt

protected abstract bool AttributesConsistentWithConstrainedDimension(UICollectionViewLayoutAttributes attributes);

bool SizesAreSame(CGSize preferredSize, Size elementSize)
{
const double tolerance = 0.000001;

if (Math.Abs(preferredSize.Height - elementSize.Height) > tolerance)
{
return false;
}

if (Math.Abs(preferredSize.Width - elementSize.Width) > tolerance)
{
return false;
}

return true;
}

void UpdateVisualStates()
{
if (PlatformHandler?.VirtualView is VisualElement element)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
using System.Collections.Generic;
using System.Threading.Tasks;
using CoreGraphics;
using Foundation;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Controls.Handlers.Items;
using UIKit;
using Xunit;

namespace Microsoft.Maui.DeviceTests
{
#if !MACCATALYST
public partial class CollectionViewTests
{
public class CacheTestCollectionView : CollectionView { }

class CacheTestCollectionViewHandler : ReorderableItemsViewHandler<CacheTestCollectionView>
{
protected override ItemsViewController<CacheTestCollectionView> CreateController(CacheTestCollectionView itemsView, ItemsViewLayout layout)
{
return new CacheTestItemsViewController(itemsView, layout);
}
}

class CacheTestItemsViewController : ItemsViewController<CacheTestCollectionView>
{
protected override bool IsHorizontal { get; }

public UICollectionViewDelegateFlowLayout DelegateFlowLayout { get; private set; }

public CacheTestItemsViewController(CacheTestCollectionView reorderableItemsView, ItemsViewLayout layout) : base(reorderableItemsView, layout)
{
}

protected override UICollectionViewDelegateFlowLayout CreateDelegator()
{
DelegateFlowLayout = new CacheMissCountingDelegate(ItemsViewLayout, this);
return DelegateFlowLayout;
}
}

internal class CacheMissCountingDelegate : ItemsViewDelegator<CacheTestCollectionView, ItemsViewController<CacheTestCollectionView>>
{
public int CacheMissCount { get; set; }

public CacheMissCountingDelegate(ItemsViewLayout itemsViewLayout, ItemsViewController<CacheTestCollectionView> itemsViewController) : base(itemsViewLayout, itemsViewController)
{
}

public override CGSize GetSizeForItem(UICollectionView collectionView, UICollectionViewLayout layout, NSIndexPath indexPath)
{
var itemSize = base.GetSizeForItem(collectionView, layout, indexPath);

if (ViewController.Layout is UICollectionViewFlowLayout flowLayout)
{
// This is totally a cheat from a unit-testing perspective; we know how the item size cache
// functions internally (that it will return the estimated size if the item size is not found in the cache),
// and we're relying on that for this test. Normally we wouldn't do this, but we need to ensure that further
// code changes don't break the cache again, and the only observable outside consequence is that the scrolling
// for the CollectionView becomes "janky". We don't want to rely entirely on human perception of "jankiness" to
// catch this problem.

// If the size comes back as EstimatedItemSize, we'll know that the actual value was not found in the cache.

if (itemSize == flowLayout.EstimatedItemSize)
{
CacheMissCount += 1;
}
}

return itemSize;
}
}

internal class ItemModel
{
public int Index { get; set; }
public double HeightRequest => 40 * (Index + 1);
}

[Fact]
public async Task EnsureCellSizesAreCached()
{
SetupBuilder();

var collectionView = new CacheTestCollectionView()
{
// Deliberately choosing a height which will cause rounding issues which can caused pathological
// sizing/layout loops if handled wrong
HeightRequest = 654.66666
};

var template = new DataTemplate(() => {
var content = new Label() { Text = "Howdy" };
content.SetBinding(VisualElement.HeightRequestProperty, new Binding(nameof(VisualElement.HeightRequest)));
return content;
});

// Build up a view model that's got enough items to ensure scrolling and template reuse
int itemCount = 15;
var source = new List<ItemModel>();
for (int n = 0; n < itemCount; n++)
{
source.Add(new ItemModel() { Index = n });
}

collectionView.ItemTemplate = template;
collectionView.ItemsSource = source;

var frame = collectionView.Frame;

await CreateHandlerAndAddToWindow<CacheTestCollectionViewHandler>(collectionView, async handler =>
{
// Wait until the CollectionView is actually rendering
await WaitForUIUpdate(frame, collectionView);

// Tell it to scroll to the bottom (and give it time to do so)
collectionView.ScrollTo(itemCount - 1);
await Task.Delay(1000);

// Now back to the top
collectionView.ScrollTo(0);
await Task.Delay(1000);

if (handler.Controller is CacheTestItemsViewController controller
&& controller.DelegateFlowLayout is CacheMissCountingDelegate cacheMissCounter)
{
// Different screen sizes and timings mean this isn't 100% predictable. But we can work out some conditions
// which will tell us that the cache is completely broken and test for those.

// With 15 items in the list, we can assume a minimum of 15 size cache misses until the cache is populated.
// Plus at least one for the initial proxy item measurement.

// The bugs we are trying to avoid clear the cache prematurely and cause _every_ GetSizeForItem call
// to miss the cache, so scrolling top to bottom and back with 15 items we would see at _least_ 30
// cache misses, likely more (since item sizes will be retrieved more than once as items scroll).

// If we have fewer than 20 cache misses, we at least know that the cache isn't being wiped as we scroll.
int missCountThreshold = 20;
int actualMissCount = cacheMissCounter.CacheMissCount;
Assert.True(actualMissCount < missCountThreshold, $"Cache miss count {actualMissCount} was higher than the threshold value of {missCountThreshold}");
}
else
{
// Something went wrong with this test
Assert.Fail("Wrong controller type in the test; is the handler registration broken?");
}
});
}
}
#endif
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ void SetupBuilder()
handlers.AddHandler<VerticalStackLayout, LayoutHandler>();
handlers.AddHandler<Grid, LayoutHandler>();
handlers.AddHandler<Label, LabelHandler>();

#if IOS && !MACCATALYST
handlers.AddHandler<CacheTestCollectionView, CacheTestCollectionViewHandler>();
#endif
});
});
}
Expand Down
Loading
Loading