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

[iOS] Clear BindingContext when cell is queued for reuse #14619

Merged
merged 13 commits into from
Feb 12, 2024
Merged
16 changes: 16 additions & 0 deletions src/Controls/src/Core/Handlers/Items/iOS/ItemsViewDelegator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,22 @@ public override nfloat GetMinimumLineSpacingForSection(UICollectionView collecti

public override void CellDisplayingEnded(UICollectionView collectionView, UICollectionViewCell cell, NSIndexPath indexPath)
{
if (cell is TemplatedCell templatedCell &&
(templatedCell.PlatformHandler?.VirtualView as View)?.BindingContext is object bindingContext)
{
// We want to unbind a cell that is no longer present in the items source. Unfortunately
// it's too expensive to check directly, so let's check that the current binding context
// matches the item at a given position.

var itemsSource = ViewController?.ItemsSource;
if (itemsSource is null ||
!itemsSource.IsIndexPathValid(indexPath) ||
!Equals(itemsSource[indexPath], bindingContext))
{
templatedCell.Unbind();
}
}

if (ItemsViewLayout.ScrollDirection == UICollectionViewScrollDirection.Horizontal)
{
var actualWidth = collectionView.ContentSize.Width - collectionView.Bounds.Size.Width;
Expand Down
29 changes: 18 additions & 11 deletions src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@ protected void ClearConstraints()
ConstrainedDimension = default;
}

internal void Unbind()
{
if (PlatformHandler?.VirtualView is View view)
{
view.MeasureInvalidated -= MeasureInvalidated;
view.BindingContext = null;
}
}

public override UICollectionViewLayoutAttributes PreferredLayoutAttributesFittingAttributes(
UICollectionViewLayoutAttributes layoutAttributes)
{
Expand Down Expand Up @@ -116,6 +125,12 @@ protected void Layout(CGSize constraints)
_size = rectangle.Size;
}

public override void PrepareForReuse()
{
Unbind();
base.PrepareForReuse();
}

public void Bind(DataTemplate template, object bindingContext, ItemsView itemsView)
{
var oldElement = PlatformHandler?.VirtualView as View;
Expand Down Expand Up @@ -157,18 +172,10 @@ public void Bind(DataTemplate template, object bindingContext, ItemsView itemsVi
// Same template
if (oldElement != null)
{
if (oldElement.BindingContext == null || !(oldElement.BindingContext.Equals(bindingContext)))
mattleibow marked this conversation as resolved.
Show resolved Hide resolved
{
// If the data is different, update it
oldElement.BindingContext = bindingContext;
oldElement.MeasureInvalidated += MeasureInvalidated;

// Unhook the MeasureInvalidated handler, otherwise it'll fire for every invalidation during the
// BindingContext change
oldElement.MeasureInvalidated -= MeasureInvalidated;
oldElement.BindingContext = bindingContext;
oldElement.MeasureInvalidated += MeasureInvalidated;

UpdateCellSize();
}
UpdateCellSize();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ Microsoft.Maui.Controls.Region.Equals(Microsoft.Maui.Controls.Region other) -> b
Microsoft.Maui.Controls.Shapes.Matrix.Equals(Microsoft.Maui.Controls.Shapes.Matrix other) -> bool
Microsoft.Maui.Controls.Shapes.Shape.~Shape() -> void
Microsoft.Maui.Controls.VisualElement.~VisualElement() -> void
override Microsoft.Maui.Controls.Handlers.Items.TemplatedCell.PrepareForReuse() -> void
override Microsoft.Maui.Controls.Handlers.Compatibility.PhoneFlyoutPageRenderer.ViewWillLayoutSubviews() -> void
override Microsoft.Maui.Controls.LayoutOptions.GetHashCode() -> int
override Microsoft.Maui.Controls.Platform.Compatibility.ShellPageRendererTracker.TitleViewContainer.LayoutSubviews() -> void
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ Microsoft.Maui.Controls.Region.Equals(Microsoft.Maui.Controls.Region other) -> b
Microsoft.Maui.Controls.Shapes.Matrix.Equals(Microsoft.Maui.Controls.Shapes.Matrix other) -> bool
Microsoft.Maui.Controls.Shapes.Shape.~Shape() -> void
Microsoft.Maui.Controls.VisualElement.~VisualElement() -> void
override Microsoft.Maui.Controls.Handlers.Items.TemplatedCell.PrepareForReuse() -> void
override Microsoft.Maui.Controls.Handlers.Compatibility.PhoneFlyoutPageRenderer.ViewWillLayoutSubviews() -> void
override Microsoft.Maui.Controls.LayoutOptions.GetHashCode() -> int
override Microsoft.Maui.Controls.Platform.Compatibility.ShellPageRendererTracker.TitleViewContainer.LayoutSubviews() -> void
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -397,5 +397,68 @@ static async Task WaitForUIUpdate(Rect frame, CollectionView collectionView, int
timeout -= interval;
}
}

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

IReadOnlyList<Element> logicalChildren = null;
var collectionView = new CollectionView
{
ItemTemplate = new DataTemplate(() => new Label() { HeightRequest = 30, WidthRequest = 200 }),
WidthRequest = 200,
HeightRequest = 200,
};

await CreateHandlerAndAddToWindow<CollectionViewHandler>(collectionView, async handler =>
{
var data = new ObservableCollection<MyRecord>()
{
new MyRecord("Item 1"),
new MyRecord("Item 2"),
new MyRecord("Item 3"),
};
collectionView.ItemsSource = data;
await Task.Delay(100);

logicalChildren = collectionView.LogicalChildrenInternal;
Assert.NotNull(logicalChildren);
Assert.True(logicalChildren.Count == 3);

// Clear collection
var savedItems = data.ToArray();
data.Clear();

await Task.Delay(100);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No GC's need to occur on these Task.Delay() calls? Is it just giving a chance for something to run on the UI thread?

I'm just wondering how we know 100ms is enough, or will it be a flaky test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just needs to run layout on UI thread. Likely Task.Yield() would have been enough but I followed what the other tests used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other tests are probably also wrong, lol. Maybe we should make a helper method for something like:

await WaitForMainThread();
// Which does something like this underneath?
// await InvokeOnMainThreadAsync(() => { });

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incidentally, that's what Task.Yield() does (as long as we are running in the correct synchronization context, which is the case here).

Copy link
Member

@rmarinho rmarinho Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PureWeen didn't we add a helper for this?


// Check that all logical children have no binding context
foreach (var logicalChild in logicalChildren)
{
Assert.Null(logicalChild.BindingContext);
}

// Re-add the old children
foreach (var savedItem in savedItems)
{
data.Add(savedItem);
}

await Task.Delay(100);

// Check that the right number of logical children have binding context again
int boundChildren = 0;
foreach (var logicalChild in logicalChildren)
{
if (logicalChild.BindingContext is not null)
{
boundChildren++;
}
}
Assert.Equal(3, boundChildren);
});
}

record MyRecord(string Name);
}
}
Loading