diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_Frame.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_Frame.cs index 20af2e901e42..44e6e10ffba9 100644 --- a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_Frame.cs +++ b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_Frame.cs @@ -1,9 +1,12 @@ using System; +using System.Threading; using System.Threading.Tasks; +using Microsoft.UI.Xaml; using Private.Infrastructure; using Microsoft.UI.Xaml.Controls; -using Uno.UI.RuntimeTests.Tests.Windows_UI_Xaml_Controls; using Microsoft.UI.Xaml.Navigation; +using Uno.Disposables; +using Uno.UI.RuntimeTests.Helpers; namespace Uno.UI.RuntimeTests.Tests.Windows_UI_Xaml_Controls; @@ -489,6 +492,87 @@ public async Task When_Exception_In_OnNavigatedTo() Assert.IsTrue(navigationFailed); } +#if HAS_UNO + [TestMethod] +#if !__ANDROID__ + [Ignore("This test specifically tests Android's NativeFramePresenter")] +#endif + [DataRow(false)] + [DataRow(true)] + public async Task When_Navigating_NativeFrame_Pages_Get_Collected(bool backAndForth) + { + // to clean up residual pages from a previous test run + for (int i = 0; i < 10; i++) + { + GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced); + GC.WaitForPendingFinalizers(); + } + + FinalizeCounterPage.Reset(); + var oldUseWinUIBehavior = FeatureConfiguration.Frame.UseWinUIBehavior; + var oldIsPoolingEnabled = FeatureConfiguration.Page.IsPoolingEnabled; + FeatureConfiguration.Frame.UseWinUIBehavior = false; // WinUI behaviour doesn't cache pages + FeatureConfiguration.Page.IsPoolingEnabled = false; + using var _ = Disposable.Create(() => + { + FeatureConfiguration.Page.IsPoolingEnabled = oldIsPoolingEnabled; + FeatureConfiguration.Frame.UseWinUIBehavior = oldUseWinUIBehavior; + FinalizeCounterPage.Reset(); + }); + + var SUT = new Frame + { + Style = Application.Current.Resources["NativeDefaultFrame"] as Style, + Width = 200, + Height = 200 + }; + + await UITestHelper.Load(SUT); + + // WaitingForIdle doesn't work here, most probably because of native animations. Instead, we use a 3 sec delay + SUT.Navigate(typeof(FinalizeCounterPage)); + await Task.Delay(TimeSpan.FromSeconds(3)); + SUT.Navigate(typeof(FinalizeCounterPage)); + await Task.Delay(TimeSpan.FromSeconds(3)); + SUT.GoBack(); + await Task.Delay(TimeSpan.FromSeconds(3)); + + for (int i = 0; i < 10; i++) + { + if (backAndForth) + { + if (i % 2 == 0) + { + SUT.GoForward(); + } + else + { + SUT.GoBack(); + } + } + else + { + SUT.Navigate(typeof(FinalizeCounterPage)); + await Task.Delay(TimeSpan.FromSeconds(3)); + SUT.BackStack.Clear(); + } + + await Task.Delay(TimeSpan.FromSeconds(3)); + } + + for (int i = 0; i < 10; i++) + { + GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced); + GC.WaitForPendingFinalizers(); + } + + Assert.AreEqual(backAndForth ? 2 : 12, FinalizeCounterPage.ConstructorCalls); + // 10 and not 11 because NativeFramePresenter won't immediately clear its "cache" on + // SUT.BackStack.Clear(), but waits for the next SUT.Navigate() + Assert.AreEqual(backAndForth ? 0 : 10, FinalizeCounterPage.FinalizerCalls); + } +#endif + [TestCleanup] public void Cleanup() { @@ -690,3 +774,28 @@ override void OnNavigatedTo(NavigationEventArgs e) throw new NotSupportedException("Crashed"); } } + +public partial class FinalizeCounterPage : Page +{ + private static int _finalizerCalls; + public static int FinalizerCalls => _finalizerCalls; + + public static int ConstructorCalls { get; private set; } + + public static void Reset() + { + Interlocked.Exchange(ref _finalizerCalls, 0); + ConstructorCalls = 0; + } + + public FinalizeCounterPage() + { + ConstructorCalls++; + Content = new TextBlock { Text = $"FinalizeCounterPage {ConstructorCalls}" }; + } + + ~FinalizeCounterPage() + { + Interlocked.Increment(ref _finalizerCalls); + } +} diff --git a/src/Uno.UI/NativeFramePresenter.Android.cs b/src/Uno.UI/NativeFramePresenter.Android.cs index c031e4abb882..814c7ad9de8e 100644 --- a/src/Uno.UI/NativeFramePresenter.Android.cs +++ b/src/Uno.UI/NativeFramePresenter.Android.cs @@ -3,22 +3,15 @@ using System.Collections.ObjectModel; using System.Collections.Specialized; using System.Linq; -using System.Text; -using System.Threading; using System.Threading.Tasks; -using Windows.UI.Core; using Microsoft.UI.Xaml; using Microsoft.UI.Xaml.Controls; using Microsoft.UI.Xaml.Navigation; using Microsoft.UI.Xaml.Media.Animation; -using Android.App; using Android.Views.Animations; -using Android.Views; -using Uno.Extensions; -using Uno.Extensions.Specialized; -using Uno.Foundation.Logging; using Uno.Disposables; +using Uno.Extensions; using Uno.UI.Extensions; namespace Uno.UI.Controls @@ -27,15 +20,14 @@ public partial class NativeFramePresenter : Grid // Inheriting from Grid is a ha { private static DependencyProperty BackButtonVisibilityProperty = ToolkitHelper.GetProperty("Uno.UI.Toolkit.CommandBarExtensions", "BackButtonVisibility"); - private readonly Grid _pageStack; private Frame _frame; private bool _isUpdatingStack; - private PageStackEntry _currentEntry; - private Queue<(PageStackEntry pageEntry, NavigationEventArgs args)> _stackUpdates = new Queue<(PageStackEntry, NavigationEventArgs)>(); + private (Page page, NavigationTransitionInfo transitionInfo) _currentPage; + private readonly Queue<(Page page, NavigationEventArgs args)> _stackUpdates = new Queue<(Page, NavigationEventArgs)>(); + private CompositeDisposable _subscriptions; public NativeFramePresenter() { - _pageStack = this; } private protected override void OnLoaded() @@ -52,20 +44,33 @@ private void Initialize(Frame frame) return; } + global::System.Diagnostics.Debug.Assert(_subscriptions is null); + _subscriptions = new CompositeDisposable(); + _frame = frame; _frame.Navigated += OnNavigated; + _subscriptions.Add(Disposable.Create(() => _frame.Navigated -= OnNavigated)); + if (_frame.BackStack is ObservableCollection backStack) { backStack.CollectionChanged += OnBackStackChanged; + _subscriptions.Add(Disposable.Create(() => backStack.CollectionChanged -= OnBackStackChanged)); } if (_frame.Content is Page startPage) { - _stackUpdates.Enqueue((_frame.CurrentEntry, new NavigationEventArgs(_frame.Content, NavigationMode.New, null, null, null, null))); + _stackUpdates.Enqueue((_frame.Content as Page, new NavigationEventArgs(_frame.Content, NavigationMode.New, null, null, null, null))); _ = InvalidateStack(); } } + private protected override void OnUnloaded() + { + base.OnUnloaded(); + _subscriptions?.Dispose(); + _subscriptions = null; + } + private void OnBackStackChanged(object sender, NotifyCollectionChangedEventArgs e) { UpdateBackButtonVisibility(); @@ -88,7 +93,7 @@ private void UpdateBackButtonVisibility() private void OnNavigated(object sender, NavigationEventArgs e) { - _stackUpdates.Enqueue((_frame.CurrentEntry, e)); + _stackUpdates.Enqueue((_frame.Content as Page, e)); _ = InvalidateStack(); } @@ -105,99 +110,77 @@ private async Task InvalidateStack() while (_stackUpdates.Any()) { var navigation = _stackUpdates.Dequeue(); - await UpdateStack(navigation.pageEntry, navigation.args); + await UpdateStack(navigation.page, navigation.args.NavigationTransitionInfo); } _isUpdatingStack = false; } - private async Task UpdateStack(PageStackEntry entry, NavigationEventArgs e) + private async Task UpdateStack(Page newPage, NavigationTransitionInfo transitionInfo) { - var oldEntry = _currentEntry; - var newEntry = entry; - - var newPage = newEntry?.Instance; - var oldPage = oldEntry?.Instance; - - if (newPage == null || newPage == oldPage) + // When AndroidUnloadInactivePages is false, we keep the pages that are still a part of the navigation history + // (i.e. in BackStack or ForwardStack) as children and make then invisible instead of removing them. The order + // of these "hidden" children is not necessarily similar to BackStack.Concat(ForwardStack), since the + // back and forward stacks can be manipulated explicitly beside navigating. We could attempt to maintain + // a correspondence between the "hidden" children and the back and forward stacks by listening to their + // CollectionChanged events, but this breaks our optimization attempts since these events fire before + // Navigated events are fired. For example, in a GoBack action, the BackStack is updated first and we would + // remove the element that corresponds to the previously-last element in the BackStack, and then respond to + // the Navigated event by making the newly-navigated-to page visible, except that the element we just removed + // is the one we want. Therefore, we treat the "hidden" children as a list that we have to walk through to + // remove items that are no longer a part of the navigation history. Although costly, navigation is not + // a heavily-automated action and is mostly bottlenecked by human reaction times, so it's fine. + + var oldPage = _currentPage.page; + var oldTransitionInfo = _currentPage.transitionInfo; + _currentPage = (newPage, transitionInfo); + + if (oldPage is not null) { - return; + if (GetIsAnimated(oldTransitionInfo)) + { + await oldPage.AnimateAsync(GetExitAnimation()); + oldPage.ClearAnimation(); + } + if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages) + { + Children.Remove(oldPage); + } + else + { + oldPage.Visibility = Visibility.Collapsed; + } } - switch (e.NavigationMode) + if (newPage is not null) { - case NavigationMode.Forward: - case NavigationMode.New: - case NavigationMode.Refresh: - _pageStack.Children.Add(newPage); - if (GetIsAnimated(newEntry)) - { - await newPage.AnimateAsync(GetEnterAnimation()); - newPage.ClearAnimation(); - } - if (oldPage is not null) - { - if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages) - { - _pageStack.Children.Remove(oldPage); - } - else - { - oldPage.Visibility = Visibility.Collapsed; - } - } - break; - case NavigationMode.Back: - if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages) - { - _pageStack.Children.Insert(0, newPage); - } - else - { - newPage.Visibility = Visibility.Visible; - } - if (GetIsAnimated(oldEntry)) - { - await oldPage.AnimateAsync(GetExitAnimation()); - oldPage.ClearAnimation(); - } - - if (oldPage != null) - { - _pageStack.Children.Remove(oldPage); - } - - if (!FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages) - { - // Remove pages from the grid that may have been removed from the BackStack list - // Those items are not removed on BackStack list changes to avoid interfering with the GoBack method's behavior. - for (var pageIndex = _pageStack.Children.Count - 1; pageIndex >= 0; pageIndex--) - { - var page = _pageStack.Children[pageIndex]; - if (page == newPage) - { - break; - } - - _pageStack.Children.Remove(page); - } - - //In case we cleared the whole stack. This should never happen - if (_pageStack.Children.Count == 0) - { - _pageStack.Children.Insert(0, newPage); - } - } - - break; + if (Children.Contains(newPage)) + { + newPage.Visibility = Visibility.Visible; + } + else + { + Children.Add(newPage); + } + if (GetIsAnimated(transitionInfo)) + { + await newPage.AnimateAsync(GetEnterAnimation()); + newPage.ClearAnimation(); + } } - _currentEntry = newEntry; + if (!FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages) + { + var pagesStillInHistory = _frame.BackStack.Select(entry => entry.Instance).ToHashSet(); + pagesStillInHistory.AddRange(_frame.ForwardStack.Select(entry => entry.Instance)); + pagesStillInHistory.Add(newPage); + Children.Remove(element => !pagesStillInHistory.Contains(element)); + } } - private static bool GetIsAnimated(PageStackEntry entry) + private static bool GetIsAnimated(NavigationTransitionInfo transitionInfo) { - return !(entry.NavigationTransitionInfo is SuppressNavigationTransitionInfo); + return !(transitionInfo is SuppressNavigationTransitionInfo); } private static Animation GetEnterAnimation()