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

[controls] fix memory leak in CollectionView #14329

Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ public class MauiRecyclerView<TItemsView, TAdapter, TItemsViewSource> : Recycler

ItemTouchHelper _itemTouchHelper;
SimpleItemTouchHelperCallback _itemTouchHelperCallback;
WeakNotifyPropertyChangedProxy _layoutPropertyChangedProxy;
PropertyChangedEventHandler _layoutPropertyChanged;

~MauiRecyclerView() => _layoutPropertyChangedProxy?.Unsubscribe();
rmarinho marked this conversation as resolved.
Show resolved Hide resolved

public MauiRecyclerView(Context context, Func<IItemsLayout> getItemsLayout, Func<TAdapter> getAdapter) : base(context)
{
Expand All @@ -58,9 +62,10 @@ public MauiRecyclerView(Context context, Func<IItemsLayout> getItemsLayout, Func
public virtual void TearDownOldElement(TItemsView oldElement)
{
// Stop listening for layout property changes
if (ItemsLayout != null)
if (_layoutPropertyChangedProxy is not null)
{
ItemsLayout.PropertyChanged -= LayoutPropertyChanged;
_layoutPropertyChangedProxy.Unsubscribe();
_layoutPropertyChanged = null;
}

// Stop listening for ScrollTo requests
Expand Down Expand Up @@ -283,14 +288,16 @@ public virtual void UpdateCanReorderItems()

public virtual void UpdateLayoutManager()
{
if (ItemsLayout != null)
ItemsLayout.PropertyChanged -= LayoutPropertyChanged;
_layoutPropertyChangedProxy?.Unsubscribe();

ItemsLayout = _getItemsLayout();

// Keep track of the ItemsLayout's property changes
if (ItemsLayout != null)
ItemsLayout.PropertyChanged += LayoutPropertyChanged;
{
_layoutPropertyChanged ??= LayoutPropertyChanged;
_layoutPropertyChangedProxy = new WeakNotifyPropertyChangedProxy(ItemsLayout, _layoutPropertyChanged);
}

SetLayoutManager(SelectLayoutManager(ItemsLayout));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ public partial class StructuredItemsViewHandler<TItemsView> : ItemsViewHandler<T
{
View _currentHeader;
View _currentFooter;
WeakNotifyPropertyChangedProxy _layoutPropertyChangedProxy;
PropertyChangedEventHandler _layoutPropertyChanged;

~StructuredItemsViewHandler() => _layoutPropertyChangedProxy?.Unsubscribe();

protected override IItemsLayout Layout { get => ItemsView?.ItemsLayout; }

Expand All @@ -24,15 +28,26 @@ protected override void ConnectHandler(ListViewBase platformView)
base.ConnectHandler(platformView);

if (Layout is not null)
Layout.PropertyChanged += LayoutPropertyChanged;
{
_layoutPropertyChanged ??= LayoutPropertyChanged;
_layoutPropertyChangedProxy = new WeakNotifyPropertyChangedProxy(Layout, _layoutPropertyChanged);
}
else if (_layoutPropertyChangedProxy is not null)
{
_layoutPropertyChangedProxy.Unsubscribe();
_layoutPropertyChangedProxy = null;
}
}

protected override void DisconnectHandler(ListViewBase platformView)
{
base.DisconnectHandler(platformView);

if (Layout is not null)
Layout.PropertyChanged -= LayoutPropertyChanged;
if (_layoutPropertyChangedProxy is not null)
{
_layoutPropertyChangedProxy.Unsubscribe();
_layoutPropertyChangedProxy = null;
}
}

void LayoutPropertyChanged(object sender, PropertyChangedEventArgs e)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ protected override (bool VisibleItems, int First, int Center, int Last) GetVisib
{
var (VisibleItems, First, Center, Last) = GetVisibleItemsIndexPath();
int firstVisibleItemIndex = -1, centerItemIndex = -1, lastVisibleItemIndex = -1;
if (VisibleItems)
if (VisibleItems && ViewController is CarouselViewController vc)
{
firstVisibleItemIndex = ViewController.GetIndexFromIndexPath(First);
centerItemIndex = ViewController.GetIndexFromIndexPath(Center);
lastVisibleItemIndex = ViewController.GetIndexFromIndexPath(Last);
firstVisibleItemIndex = vc.GetIndexFromIndexPath(First);
centerItemIndex = vc.GetIndexFromIndexPath(Center);
lastVisibleItemIndex = vc.GetIndexFromIndexPath(Last);
}
return (VisibleItems, firstVisibleItemIndex, centerItemIndex, lastVisibleItemIndex);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ public GroupableItemsViewDelegator(ItemsViewLayout itemsViewLayout, TViewControl

public override CGSize GetReferenceSizeForHeader(UICollectionView collectionView, UICollectionViewLayout layout, nint section)
{
return ViewController.GetReferenceSizeForHeader(collectionView, layout, section);
return ViewController?.GetReferenceSizeForHeader(collectionView, layout, section) ?? CGSize.Empty;
}

public override CGSize GetReferenceSizeForFooter(UICollectionView collectionView, UICollectionViewLayout layout, nint section)
{
return ViewController.GetReferenceSizeForFooter(collectionView, layout, section);
return ViewController?.GetReferenceSizeForFooter(collectionView, layout, section) ?? CGSize.Empty;
}

public override void ScrollAnimationEnded(UIScrollView scrollView)
Expand All @@ -37,7 +37,7 @@ public override UIEdgeInsets GetInsetForSection(UICollectionView collectionView,
return default;
}

return ViewController.GetInsetForSection(ItemsViewLayout, collectionView, section);
return ViewController?.GetInsetForSection(ItemsViewLayout, collectionView, section) ?? UIEdgeInsets.Zero;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public abstract class ItemsViewController<TItemsView> : UICollectionViewControll
bool _emptyViewDisplayed;
bool _disposed;

Func<UICollectionViewCell> _getPrototype;
UIView _emptyUIView;
VisualElement _emptyViewFormsElement;
Dictionary<object, TemplatedCell> _measurementCells = new Dictionary<object, TemplatedCell>();
Expand Down Expand Up @@ -200,7 +201,8 @@ void EnsureLayoutInitialized()

_initialized = true;

ItemsViewLayout.GetPrototype = GetPrototype;
_getPrototype ??= GetPrototype;
ItemsViewLayout.GetPrototype = _getPrototype;
rmarinho marked this conversation as resolved.
Show resolved Hide resolved

Delegator = CreateDelegator();
CollectionView.Delegate = Delegator;
Expand Down
24 changes: 17 additions & 7 deletions src/Controls/src/Core/Handlers/Items/iOS/ItemsViewDelegator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,17 @@ public class ItemsViewDelegator<TItemsView, TViewController> : UICollectionViewD
where TItemsView : ItemsView
where TViewController : ItemsViewController<TItemsView>
{
readonly WeakReference<TViewController> _viewController;

public ItemsViewLayout ItemsViewLayout { get; }
public TViewController ViewController { get; }
public TViewController ViewController => _viewController.TryGetTarget(out var vc) ? vc : null;

protected float PreviousHorizontalOffset, PreviousVerticalOffset;

public ItemsViewDelegator(ItemsViewLayout itemsViewLayout, TViewController itemsViewController)
{
ItemsViewLayout = itemsViewLayout;
ViewController = itemsViewController;
_viewController = new(itemsViewController);
}

public override void Scrolled(UIScrollView scrollView)
Expand All @@ -45,8 +47,12 @@ public override void Scrolled(UIScrollView scrollView)
LastVisibleItemIndex = lastVisibleItemIndex
};

var itemsView = ViewController.ItemsView;
var source = ViewController.ItemsSource;
var viewController = ViewController;
if (viewController is null)
return;

var itemsView = viewController.ItemsView;
var source = viewController.ItemsSource;
itemsView.SendScrolled(itemsViewScrolledEventArgs);

PreviousHorizontalOffset = (float)contentOffsetX;
Expand Down Expand Up @@ -119,15 +125,19 @@ public override void CellDisplayingEnded(UICollectionView collectionView, UIColl

protected virtual (bool VisibleItems, NSIndexPath First, NSIndexPath Center, NSIndexPath Last) GetVisibleItemsIndexPath()
{
var indexPathsForVisibleItems = ViewController.CollectionView.IndexPathsForVisibleItems.OrderBy(x => x.Row).ToList();
var collectionView = ViewController?.CollectionView;
if (collectionView is null)
return default;

var indexPathsForVisibleItems = collectionView.IndexPathsForVisibleItems.OrderBy(x => x.Row).ToList();

var visibleItems = indexPathsForVisibleItems.Count > 0;
NSIndexPath firstVisibleItemIndex = null, centerItemIndex = null, lastVisibleItemIndex = null;

if (visibleItems)
{
firstVisibleItemIndex = indexPathsForVisibleItems.First();
centerItemIndex = GetCenteredIndexPath(ViewController.CollectionView);
centerItemIndex = GetCenteredIndexPath(collectionView);
lastVisibleItemIndex = indexPathsForVisibleItems.Last();
}

Expand Down Expand Up @@ -166,7 +176,7 @@ static NSIndexPath GetCenteredIndexPath(UICollectionView collectionView)

public override CGSize GetSizeForItem(UICollectionView collectionView, UICollectionViewLayout layout, NSIndexPath indexPath)
{
return ViewController.GetSizeForItem(indexPath);
return ViewController?.GetSizeForItem(indexPath) ?? CGSize.Empty;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ public abstract class ItemsViewLayout : UICollectionViewFlowLayout
CGSize _adjustmentSize0;
CGSize _adjustmentSize1;
CGSize _currentSize;
WeakReference<Func<UICollectionViewCell>> _getPrototype;

const double ConstraintSizeTolerance = 0.00001;

Expand All @@ -28,7 +29,11 @@ public abstract class ItemsViewLayout : UICollectionViewFlowLayout

public nfloat ConstrainedDimension { get; set; }

public Func<UICollectionViewCell> GetPrototype { get; set; }
public Func<UICollectionViewCell> GetPrototype
{
get => _getPrototype is not null && _getPrototype.TryGetTarget(out var func) ? func : null;
set => _getPrototype = new(value);
}

internal ItemSizingStrategy ItemSizingStrategy { get; private set; }

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#nullable disable
using System;
using System.Collections;
using Foundation;
using ObjCRuntime;
Expand Down Expand Up @@ -26,8 +27,12 @@ protected override NSIndexPath[] CreateIndexesFrom(int startIndex, int count)
return base.CreateIndexesFrom(startIndex, count);
}

var collectionView = CollectionView;
if (collectionView is null)
return Array.Empty<NSIndexPath>();

return IndexPathHelpers.GenerateLoopedIndexPathRange(Section,
(int)CollectionView.NumberOfItemsInSection(Section), LoopBy, startIndex, count);
(int)collectionView.NumberOfItemsInSection(Section), LoopBy, startIndex, count);
}
}
}
44 changes: 28 additions & 16 deletions src/Controls/src/Core/Handlers/Items/iOS/ObservableItemsSource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,15 @@ namespace Microsoft.Maui.Controls.Handlers.Items
{
internal class ObservableItemsSource : IObservableItemsViewSource
{
readonly UICollectionViewController _collectionViewController;
protected readonly UICollectionView CollectionView;
readonly WeakReference<UICollectionViewController> _collectionViewController;
readonly bool _grouped;
readonly int _section;
readonly IEnumerable _itemsSource;
bool _disposed;

public ObservableItemsSource(IEnumerable itemSource, UICollectionViewController collectionViewController, int group = -1)
{
_collectionViewController = collectionViewController;
CollectionView = _collectionViewController.CollectionView;
_collectionViewController = new(collectionViewController);

_section = group < 0 ? 0 : group;
_grouped = group >= 0;
Expand All @@ -35,6 +33,8 @@ public ObservableItemsSource(IEnumerable itemSource, UICollectionViewController
internal event NotifyCollectionChangedEventHandler CollectionViewUpdating;
internal event NotifyCollectionChangedEventHandler CollectionViewUpdated;

internal UICollectionView CollectionView => _collectionViewController.TryGetTarget(out var controller) ? controller.CollectionView : null;

public int Count { get; private set; }

public int Section => _section;
Expand Down Expand Up @@ -125,9 +125,13 @@ void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args)

void CollectionChanged(NotifyCollectionChangedEventArgs args)
{
if (!_collectionViewController.TryGetTarget(out var controller))
return;

// Force UICollectionView to get the internal accounting straight
if (!CollectionView.Hidden)
CollectionView.NumberOfItemsInSection(_section);
var collectionView = controller.CollectionView;
if (!collectionView.Hidden)
collectionView.NumberOfItemsInSection(_section);

switch (args.Action)
{
Expand All @@ -153,14 +157,18 @@ void CollectionChanged(NotifyCollectionChangedEventArgs args)

void Reload()
{
if (!_collectionViewController.TryGetTarget(out var controller))
return;

var args = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset);

Count = ItemsCount();

OnCollectionViewUpdating(args);

CollectionView.ReloadData();
CollectionView.CollectionViewLayout.InvalidateLayout();
var collectionView = controller.CollectionView;
collectionView.ReloadData();
collectionView.CollectionViewLayout.InvalidateLayout();

OnCollectionViewUpdated(args);
}
Expand All @@ -177,7 +185,7 @@ void Add(NotifyCollectionChangedEventArgs args)
var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : IndexOf(args.NewItems[0]);

// Queue up the updates to the UICollectionView
Update(() => CollectionView.InsertItems(CreateIndexesFrom(startIndex, count)), args);
Update(c => c.InsertItems(CreateIndexesFrom(startIndex, count)), args);
}

void Remove(NotifyCollectionChangedEventArgs args)
Expand All @@ -196,7 +204,7 @@ void Remove(NotifyCollectionChangedEventArgs args)
var count = args.OldItems.Count;
Count -= count;

Update(() => CollectionView.DeleteItems(CreateIndexesFrom(startIndex, count)), args);
Update(c => c.DeleteItems(CreateIndexesFrom(startIndex, count)), args);
}

void Replace(NotifyCollectionChangedEventArgs args)
Expand All @@ -209,7 +217,7 @@ void Replace(NotifyCollectionChangedEventArgs args)

// We are replacing one set of items with a set of equal size; we can do a simple item range update

Update(() => CollectionView.ReloadItems(CreateIndexesFrom(startIndex, newCount)), args);
Update(c => c.ReloadItems(CreateIndexesFrom(startIndex, newCount)), args);
return;
}

Expand All @@ -228,14 +236,14 @@ void Move(NotifyCollectionChangedEventArgs args)
var oldPath = NSIndexPath.Create(_section, args.OldStartingIndex);
var newPath = NSIndexPath.Create(_section, args.NewStartingIndex);

Update(() => CollectionView.MoveItem(oldPath, newPath), args);
Update(c => c.MoveItem(oldPath, newPath), args);
return;
}

var start = Math.Min(args.OldStartingIndex, args.NewStartingIndex);
var end = Math.Max(args.OldStartingIndex, args.NewStartingIndex) + count;

Update(() => CollectionView.ReloadItems(CreateIndexesFrom(start, end)), args);
Update(c => c.ReloadItems(CreateIndexesFrom(start, end)), args);
}

internal int ItemsCount()
Expand Down Expand Up @@ -281,15 +289,19 @@ internal int IndexOf(object item)
return -1;
}

void Update(Action update, NotifyCollectionChangedEventArgs args)
void Update(Action<UICollectionView> update, NotifyCollectionChangedEventArgs args)
{
if (CollectionView.Hidden)
if (!_collectionViewController.TryGetTarget(out var controller))
return;

var collectionView = controller.CollectionView;
if (collectionView.Hidden)
{
return;
}

OnCollectionViewUpdating(args);
update();
update(collectionView);
OnCollectionViewUpdated(args);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public override NSIndexPath GetTargetIndexPathForMove(UICollectionView collectio
{
NSIndexPath targetIndexPath;

var itemsView = ViewController.ItemsView;
var itemsView = ViewController?.ItemsView;
if (itemsView?.IsGrouped == true)
{
if (originalIndexPath.Section == proposedIndexPath.Section || itemsView.CanMixGroups)
Expand Down
Loading