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/catalyst] fix more cycles in NavigationPage #23164

Merged
merged 3 commits into from
Jun 21, 2024
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 @@ -48,7 +48,8 @@ public class NavigationRenderer : UINavigationController, INavigationViewHandler
public static CommandMapper<NavigationPage, NavigationRenderer> CommandMapper = new CommandMapper<NavigationPage, NavigationRenderer>(ViewHandler.ViewCommandMapper);
ViewHandlerDelegator<NavigationPage> _viewHandlerWrapper;
bool _navigating = false;
VisualElement _element;
WeakReference<VisualElement> _element;
WeakReference<Page> _current;
bool _uiRequestedPop; // User tapped the back button or swiped to navigate back
MauiNavigationDelegate NavigationDelegate => Delegate as MauiNavigationDelegate;

Expand All @@ -60,14 +61,18 @@ public NavigationRenderer() : base(typeof(MauiControlsNavigationBar), null)
Delegate = new MauiNavigationDelegate(this);
}

Page Current { get; set; }
Page Current
{
get => _current?.GetTargetOrDefault();
set => _current = value is null ? null : new(value);
}

IPageController PageController => Element as IPageController;

NavigationPage NavPage => Element as NavigationPage;
INavigationPageController NavPageController => NavPage;

public VisualElement Element { get => _viewHandlerWrapper.Element ?? _element; }
public VisualElement Element { get => _viewHandlerWrapper.Element ?? _element?.GetTargetOrDefault(); }

public event EventHandler<VisualElementChangedEventArgs> ElementChanged;

Expand All @@ -85,7 +90,7 @@ public UIView NativeView
public void SetElement(VisualElement element)
{
(this as IElementHandler).SetVirtualView(element);
_element = element;
_element = element is null ? null : new(element);
}

public UIViewController ViewController
Expand Down Expand Up @@ -171,7 +176,8 @@ public override void ViewDidDisappear(bool animated)
public override void ViewWillLayoutSubviews()
{
base.ViewWillLayoutSubviews();
if (Current == null)

if (Current is not Page current)
return;

UpdateToolBarVisible();
Expand All @@ -180,7 +186,7 @@ public override void ViewWillLayoutSubviews()
var toolbar = _secondaryToolbar;

//save the state of the Current page we are calculating, this will fire before Current is updated
_hasNavigationBar = NavigationPage.GetHasNavigationBar(Current);
_hasNavigationBar = NavigationPage.GetHasNavigationBar(current);

// Use 0 if the NavBar is hidden or will be hidden
var toolbarY = NavigationBarHidden || NavigationBar.Translucent || !_hasNavigationBar ? 0 : navBarFrameBottom;
Expand Down Expand Up @@ -475,8 +481,8 @@ void HandlePropertyChanged(object sender, PropertyChangedEventArgs e)
}
else if (e.PropertyName == NavigationPage.CurrentPageProperty.PropertyName)
{
Current = NavPage?.CurrentPage;
ValidateNavbarExists(Current);
var current = Current = NavPage?.CurrentPage;
ValidateNavbarExists(current);
}
else if (e.PropertyName == IsNavigationBarTranslucentProperty.PropertyName)
{
Expand Down Expand Up @@ -806,7 +812,7 @@ void UpdateBarTextColor()
}

// set Tint color (i. e. Back Button arrow and Text)
var iconColor = Current != null ? NavigationPage.GetIconColor(Current) : null;
var iconColor = Current is Page current ? NavigationPage.GetIconColor(current) : null;
if (iconColor == null)
iconColor = barTextColor;

Expand Down Expand Up @@ -860,8 +866,8 @@ void UpdateToolBarVisible()

if (currentHidden != _secondaryToolbar.Hidden)
{
if (Current?.Handler != null)
Current.ToPlatform().InvalidateMeasure(Current);
if (Current is Page current && current.Handler is not null)
current.ToPlatform().InvalidateMeasure(current);

if (VisibleViewController is ParentingViewController pvc)
pvc.UpdateFrames();
Expand Down Expand Up @@ -1716,7 +1722,7 @@ void IElementHandler.SetMauiContext(IMauiContext mauiContext)
void IElementHandler.SetVirtualView(Maui.IElement view)
{
_viewHandlerWrapper.SetVirtualView(view, ElementChanged, false);
_element = view as VisualElement;
_element = view is VisualElement v ? new(v) : null;

void ElementChanged(ElementChangedEventArgs<NavigationPage> e)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,14 @@ class ViewHandlerDelegator<TElement>
internal IPropertyMapper _mapper;
internal readonly CommandMapper _commandMapper;
IPlatformViewHandler _viewHandler;
#if IOS || MACCATALYST
TElement? _tempElement;
WeakReference<TElement>? _element;
public TElement? Element => _tempElement ?? _element?.GetTargetOrDefault();
#else
TElement? _element;
public TElement? Element => _element;
#endif
bool _disposed;

public ViewHandlerDelegator(
Expand All @@ -48,11 +54,11 @@ public void Invoke(string command, object? args)

public void DisconnectHandler()
{
if (_element == null)
if (Element is not TElement element)
return;

if (_element.Handler == _viewHandler)
_element.Handler = null;
if (element.Handler == _viewHandler)
element.Handler = null;

_element = null;

Expand Down Expand Up @@ -80,6 +86,12 @@ public void SetVirtualView(
{
#if WINDOWS
VisualElementRenderer<TElement, TNativeElement>.SetVirtualView(view, _viewHandler, onElementChanged, ref _element, ref _mapper, _defaultMapper, autoPackage);
#elif IOS || MACCATALYST
// _tempElement is used here, because the Element property is accessed before SetVirtualView() returns
VisualElementRenderer<TElement>.SetVirtualView(view, _viewHandler, onElementChanged, ref _tempElement, ref _mapper, _defaultMapper, autoPackage);
// We use _element as a WeakReference, and clear _tempElement
_element = _tempElement is null ? null : new(_tempElement);
_tempElement = null;
#else
VisualElementRenderer<TElement>.SetVirtualView(view, _viewHandler, onElementChanged, ref _element, ref _mapper, _defaultMapper, autoPackage);
#endif
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,24 +311,25 @@ await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async
public async Task DoesNotLeak()
{
SetupBuilder();
WeakReference pageReference = null;
WeakReference handlerReference = null;
var references = new List<WeakReference>();

{
var navPage = new NavigationPage(new ContentPage());
var window = new Window(navPage);

await CreateHandlerAndAddToWindow<WindowHandlerStub>(window, (handler) =>
{
pageReference = new WeakReference(navPage);
handlerReference = new WeakReference(handler);
references.Add(new(navPage));
references.Add(new(navPage.Handler));
references.Add(new(navPage.Handler.PlatformView));
references.Add(new(handler));

// Just replace the page with a new one
window.Page = new ContentPage();
});
}

await AssertionExtensions.WaitForGC(pageReference, handlerReference);
await AssertionExtensions.WaitForGC(references.ToArray());
}

[Fact(DisplayName = "Child Pages Do Not Leak")]
Expand Down
16 changes: 13 additions & 3 deletions src/Controls/tests/DeviceTests/Memory/MemoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,10 @@ void SetupBuilder()
});
}

[Fact("Page Does Not Leak")]
public async Task PageDoesNotLeak()
[Theory("Pages Do Not Leak")]
[InlineData(typeof(ContentPage))]
[InlineData(typeof(NavigationPage))]
public async Task PagesDoNotLeak(Type type)
{
SetupBuilder();

Expand All @@ -80,7 +82,15 @@ public async Task PageDoesNotLeak()

await CreateHandlerAndAddToWindow(new Window(navPage), async () =>
{
var page = new ContentPage { Content = new Label() };
var page = (Page)Activator.CreateInstance(type);
if (page is ContentPage contentPage)
{
contentPage.Content = new Label();
}
else if (page is NavigationPage navigationPage)
{
await navigationPage.PushAsync(new ContentPage { Content = new Label() });
}

await navPage.Navigation.PushModalAsync(page);

Expand Down
Loading