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.

Sign up for GitHub

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

Jump to bottom

[ios/catalyst] fix memory leaks in *Cell #22067

Merged
merged 8 commits into from
May 17, 2024
Merged

[ios/catalyst] fix memory leaks in *Cell #22067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f349108
[ios/catalyst] fix memory leaks in `*Cell`
jonathanpeppers Apr 25, 2024
6c1cb88
Fixed all public API changes
jonathanpeppers Apr 26, 2024
bff1848
Better null checking
jonathanpeppers May 8, 2024
ee996a6
- add null check for ImageCellRenderer
PureWeen May 16, 2024
6afe10b
- wire everything up through handler paths
PureWeen May 16, 2024
d9e0f64
- cleanup
PureWeen May 16, 2024
12c1161
Update CellTableViewCell.cs
PureWeen May 16, 2024
d518c87
- add additional unshipped
PureWeen May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Controls/src/Core/Cells/Cell.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ async void OnForceUpdateSizeRequested()
// don't run more than once per 16 milliseconds
await Task.Delay(TimeSpan.FromMilliseconds(16));
ForceUpdateSizeRequested?.Invoke(this, null);
Handler.Invoke("ForceUpdateSizeRequested", null);

_nextCallToForceUpdateSizeQueued = false;
}
Expand Down
76 changes: 35 additions & 41 deletions src/Controls/src/Core/Compatibility/Handlers/ListView/iOS/CellRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ public class CellRenderer : ElementHandler<Cell, UITableViewCell>, IRegisterable
{
static readonly BindableProperty RealCellProperty = BindableProperty.CreateAttached("RealCell", typeof(UITableViewCell), typeof(Cell), null);

EventHandler? _onForceUpdateSizeRequested;
PropertyChangedEventHandler? _onPropertyChangedEventHandler;
readonly UIColor _defaultCellBgColor = (OperatingSystem.IsIOSVersionAtLeast(13) || OperatingSystem.IsTvOSVersionAtLeast(13)) ? UIColor.Clear : UIColor.White;

public static PropertyMapper<Cell, CellRenderer> Mapper =
Expand All @@ -22,6 +20,8 @@ public class CellRenderer : ElementHandler<Cell, UITableViewCell>, IRegisterable
new CommandMapper<Cell, CellRenderer>(ElementHandler.ElementCommandMapper);
UITableView? _tableView;

private protected event PropertyChangedEventHandler? CellPropertyChanged;

public CellRenderer() : base(Mapper, CommandMapper)
{
}
Expand All @@ -44,8 +44,6 @@ public virtual UITableViewCell GetCell(Cell item, UITableViewCell reusableCell,

tvc.Cell = item;

WireUpForceUpdateSizeRequested(item, tvc, tv);

if (OperatingSystem.IsIOSVersionAtLeast(14))
{
var content = tvc.DefaultContentConfiguration;
Expand Down Expand Up @@ -135,44 +133,9 @@ protected void UpdateBackground(UITableViewCell tableViewCell, Cell cell)
SetBackgroundColor(tableViewCell, cell, uiBgColor);
}

[Obsolete("The ForceUpdateSizeRequested event is now managed by the command mapper, so it's not necessary to invoke this event manually.")]
Copy link
Member

Choose a reason for hiding this comment

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

This method was just a really complicated way of wiring up to _onForceUpdateSizeRequested and making sure to unwire from it.

We can just achieve this same thing through the commandmapper now

protected void WireUpForceUpdateSizeRequested(ICellController cell, UITableViewCell platformCell, UITableView tableView)
{
var inpc = cell as INotifyPropertyChanged;
cell.ForceUpdateSizeRequested -= _onForceUpdateSizeRequested;

if (inpc != null)
inpc.PropertyChanged -= _onPropertyChangedEventHandler;

_onForceUpdateSizeRequested = (sender, e) =>
{
var index = tableView?.IndexPathForCell(platformCell);
if (index == null && sender is Cell c)
{
index = Controls.Compatibility.Platform.iOS.CellExtensions.GetIndexPath(c);
}

if (index != null)
tableView?.ReloadRows(new[] { index }, UITableViewRowAnimation.None);
};

_onPropertyChangedEventHandler = (sender, e) =>
{
if (e.PropertyName == "RealCell" && sender is BindableObject bo && GetRealCell(bo) == null)
{
if (sender is ICellController icc)
icc.ForceUpdateSizeRequested -= _onForceUpdateSizeRequested;

if (sender is INotifyPropertyChanged notifyPropertyChanged)
notifyPropertyChanged.PropertyChanged -= _onPropertyChangedEventHandler;

_onForceUpdateSizeRequested = null;
_onPropertyChangedEventHandler = null;
}
};

cell.ForceUpdateSizeRequested += _onForceUpdateSizeRequested;
if (inpc != null)
inpc.PropertyChanged += _onPropertyChangedEventHandler;

}

Expand All @@ -190,5 +153,36 @@ internal static void SetRealCell(BindableObject cell, UITableViewCell renderer)
{
cell.SetValue(RealCellProperty, renderer);
}

public override void UpdateValue(string property)
{
base.UpdateValue(property);
var args = new PropertyChangedEventArgs(property);
if (VirtualView is BindableObject bindableObject &&
GetRealCell(bindableObject) is CellTableViewCell ctv )
{
ctv.HandlePropertyChanged(bindableObject, args);
}

CellPropertyChanged?.Invoke(VirtualView, args);
}

public override void Invoke(string command, object? args)
{
base.Invoke(command, args);

if (command == "ForceUpdateSizeRequested" &&
VirtualView is BindableObject bindableObject &&
GetRealCell(bindableObject) is UITableViewCell ctv)
{
var index = _tableView?.IndexPathForCell(ctv);
if (index == null && VirtualView is Cell c)
{
index = Controls.Compatibility.Platform.iOS.CellExtensions.GetIndexPath(c);
}
if (index != null)
_tableView?.ReloadRows(new[] { index }, UITableViewRowAnimation.None);
}
}
}
}
}
38 changes: 20 additions & 18 deletions src/Controls/src/Core/Compatibility/Handlers/ListView/iOS/CellTableViewCell.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace Microsoft.Maui.Controls.Handlers.Compatibility
{
public class CellTableViewCell : UITableViewCell, INativeElementView
{
Cell _cell;
WeakReference<Cell> _cell;

public Action<object, PropertyChangedEventArgs> PropertyChanged;

Expand All @@ -21,23 +21,28 @@ public CellTableViewCell(UITableViewCellStyle style, string key) : base(style, k

public Cell Cell
{
get { return _cell; }
get => _cell?.GetTargetOrDefault();
set
{
if (_cell == value)
return;

if (_cell != null)
if (_cell is not null)
{
_cell.PropertyChanged -= HandlePropertyChanged;
BeginInvokeOnMainThread(_cell.SendDisappearing);
if (_cell.TryGetTarget(out var cell) && cell == value)
return;

if (cell is not null)
{
BeginInvokeOnMainThread(cell.SendDisappearing);
}
}
_cell = value;

if (_cell != null)
if (value is not null)
{
_cell = new(value);
BeginInvokeOnMainThread(value.SendAppearing);
}
else
{
_cell.PropertyChanged += HandlePropertyChanged;
BeginInvokeOnMainThread(_cell.SendAppearing);
_cell = null;
}
}
}
Expand Down Expand Up @@ -109,12 +114,9 @@ protected override void Dispose(bool disposing)

if (disposing)
{
PropertyChanged = null;

if (_cell != null)
if (Cell is Cell cell)
{
_cell.PropertyChanged -= HandlePropertyChanged;
CellRenderer.SetRealCell(_cell, null);
CellRenderer.SetRealCell(cell, null);
}
_cell = null;
}
Expand All @@ -124,4 +126,4 @@ protected override void Dispose(bool disposing)
base.Dispose(disposing);
}
}
}
}
8 changes: 4 additions & 4 deletions src/Controls/src/Core/Compatibility/Handlers/ListView/iOS/EntryCellRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,23 @@ public override UITableViewCell GetCell(Cell item, UITableViewCell reusableCell,

var tvc = reusableCell as EntryCellTableViewCell;
if (tvc == null)
{
tvc = new EntryCellTableViewCell(item.GetType().FullName);
}
else
{
tvc.PropertyChanged -= HandlePropertyChanged;
CellPropertyChanged -= HandlePropertyChanged;
Copy link
Member

@PureWeen PureWeen May 16, 2024 •
edited

Choose a reason for hiding this comment

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

Because this event now is just local to this class this isn't going to cause a circular reference. I opted to keep this event path because this maintains the timing of when HandlePropertyChanged will start and stop firing

tvc.TextFieldTextChanged -= OnTextFieldTextChanged;
tvc.KeyboardDoneButtonPressed -= OnKeyBoardDoneButtonPressed;
}

SetRealCell(item, tvc);

tvc.Cell = item;
tvc.PropertyChanged += HandlePropertyChanged;
CellPropertyChanged += HandlePropertyChanged;
tvc.TextFieldTextChanged += OnTextFieldTextChanged;
tvc.KeyboardDoneButtonPressed += OnKeyBoardDoneButtonPressed;

WireUpForceUpdateSizeRequested(item, tvc, tv);

UpdateBackground(tvc, entryCell);
UpdateLabel(tvc, entryCell);
UpdateText(tvc, entryCell);
Expand Down
8 changes: 3 additions & 5 deletions src/Controls/src/Core/Compatibility/Handlers/ListView/iOS/ImageCellRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ public override UITableViewCell GetCell(Cell item, UITableViewCell reusableCell,

var imageCell = (ImageCell)item;

WireUpForceUpdateSizeRequested(item, result, tv);

SetImage(imageCell, result);

return result;
Expand All @@ -47,12 +45,12 @@ void SetImage(ImageCell cell, CellTableViewCell target)

source.LoadImage(cell.FindMauiContext(), (result) =>
{
var uiimage = result.Value;
if (uiimage != null)
var uiimage = result?.Value;
if (uiimage is not null)
{
NSRunLoop.Main.BeginInvokeOnMainThread(() =>
{
if (target.Cell != null)
if (target.Cell is not null)
{
target.ImageView.Image = uiimage;
target.SetNeedsLayout();
Expand Down
6 changes: 2 additions & 4 deletions src/Controls/src/Core/Compatibility/Handlers/ListView/iOS/SwitchCellRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public override UITableViewCell GetCell(Cell item, UITableViewCell reusableCell,
else
{
uiSwitch = tvc.AccessoryView as UISwitch;
tvc.PropertyChanged -= HandlePropertyChanged;
CellPropertyChanged -= HandlePropertyChanged;
}

SetRealCell(item, tvc);
Expand All @@ -45,16 +45,14 @@ public override UITableViewCell GetCell(Cell item, UITableViewCell reusableCell,
_defaultOnColor = UISwitch.Appearance.OnTintColor;

tvc.Cell = item;
tvc.PropertyChanged += HandlePropertyChanged;
CellPropertyChanged += HandlePropertyChanged;
tvc.AccessoryView = uiSwitch;
#pragma warning disable CA1416, CA1422 // TODO: 'UITableViewCell.TextLabel' is unsupported on: 'ios' 14.0 and later
tvc.TextLabel.Text = boolCell.Text;
#pragma warning restore CA1416, CA1422

uiSwitch.On = boolCell.On;

WireUpForceUpdateSizeRequested(item, tvc, tv);

UpdateBackground(tvc, item);
UpdateIsEnabled(tvc, boolCell);
UpdateFlowDirection(tvc, boolCell);
Expand Down
6 changes: 2 additions & 4 deletions src/Controls/src/Core/Compatibility/Handlers/ListView/iOS/TextCellRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,19 @@ public override UITableViewCell GetCell(Cell item, UITableViewCell reusableCell,
if (!(reusableCell is CellTableViewCell tvc))
tvc = new CellTableViewCell(UITableViewCellStyle.Subtitle, item.GetType().FullName);
else
tvc.PropertyChanged -= HandleCellPropertyChanged;
CellPropertyChanged -= HandleCellPropertyChanged;

SetRealCell(item, tvc);

tvc.Cell = textCell;
tvc.PropertyChanged = HandleCellPropertyChanged;
CellPropertyChanged += HandleCellPropertyChanged;

#pragma warning disable CA1416, CA1422 // TODO: 'UITableViewCell.TextLabel', DetailTextLabel is unsupported on: 'ios' 14.0 and later
tvc.TextLabel.Text = textCell.Text;
tvc.DetailTextLabel.Text = textCell.Detail;
tvc.TextLabel.TextColor = (textCell.TextColor ?? DefaultTextColor).ToPlatform();
tvc.DetailTextLabel.TextColor = (textCell.DetailColor ?? DefaultDetailColor).ToPlatform();

WireUpForceUpdateSizeRequested(item, tvc, tv);

UpdateIsEnabled(tvc, textCell);
#pragma warning restore CA1416, CA1422

Expand Down