From 9d1ddff5eeaa114de383daf958e3c12282a238e2 Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Tue, 17 Dec 2024 01:47:21 +0200 Subject: [PATCH 1/5] fix(nativeframepresenter): work with WinUI implementation and fix instances where exit animations were not run --- src/Uno.UI/NativeFramePresenter.Android.cs | 121 ++++++++++----------- 1 file changed, 59 insertions(+), 62 deletions(-) diff --git a/src/Uno.UI/NativeFramePresenter.Android.cs b/src/Uno.UI/NativeFramePresenter.Android.cs index c031e4abb882..47419b422479 100644 --- a/src/Uno.UI/NativeFramePresenter.Android.cs +++ b/src/Uno.UI/NativeFramePresenter.Android.cs @@ -30,8 +30,8 @@ public partial class NativeFramePresenter : Grid // Inheriting from Grid is a ha 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)>(); public NativeFramePresenter() { @@ -61,7 +61,7 @@ private void Initialize(Frame frame) 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(); } } @@ -88,7 +88,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 +105,96 @@ private async Task InvalidateStack() while (_stackUpdates.Any()) { var navigation = _stackUpdates.Dequeue(); - await UpdateStack(navigation.pageEntry, navigation.args); + await UpdateStack(navigation.page, navigation.args); } _isUpdatingStack = false; } - private async Task UpdateStack(PageStackEntry entry, NavigationEventArgs e) + private async Task UpdateStack(Page newPage, NavigationEventArgs e) { - var oldEntry = _currentEntry; - var newEntry = entry; - - var newPage = newEntry?.Instance; - var oldPage = oldEntry?.Instance; - - if (newPage == null || newPage == oldPage) - { - return; - } - switch (e.NavigationMode) { case NavigationMode.Forward: case NavigationMode.New: case NavigationMode.Refresh: - _pageStack.Children.Add(newPage); - if (GetIsAnimated(newEntry)) { - await newPage.AnimateAsync(GetEnterAnimation()); - newPage.ClearAnimation(); + if (_currentPage.page is { } oldPage) + { + if (GetIsAnimated(_currentPage.transitionInfo)) + { + await _currentPage.page.AnimateAsync(GetExitAnimation()); + _currentPage.page.ClearAnimation(); + } + if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages) + { + _pageStack.Children.Remove(oldPage); + } + else + { + oldPage.Visibility = Visibility.Collapsed; + } + } + _pageStack.Children.Add(newPage); + if (GetIsAnimated(e.NavigationTransitionInfo)) + { + await newPage.AnimateAsync(GetEnterAnimation()); + newPage.ClearAnimation(); + } } - if (oldPage is not null) + break; + case NavigationMode.Back: { if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages) { - _pageStack.Children.Remove(oldPage); + _pageStack.Children.Insert(0, newPage); } else { - oldPage.Visibility = Visibility.Collapsed; + newPage.Visibility = Visibility.Visible; } - } - 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--) + if (_currentPage.page is { } oldPage) { - var page = _pageStack.Children[pageIndex]; - if (page == newPage) + if (GetIsAnimated(_currentPage.transitionInfo)) { - break; + await _currentPage.page.AnimateAsync(GetExitAnimation()); + _currentPage.page.ClearAnimation(); } - - _pageStack.Children.Remove(page); + _pageStack.Children.Remove(oldPage); } - //In case we cleared the whole stack. This should never happen - if (_pageStack.Children.Count == 0) + if (!FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages) { - _pageStack.Children.Insert(0, newPage); + // 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; } - _currentEntry = newEntry; + _currentPage = (newPage, e.NavigationTransitionInfo); } - 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() From 2747f1718db214819a332527bcff8d68892e091e Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Tue, 17 Dec 2024 03:04:16 +0200 Subject: [PATCH 2/5] fix(nativeframepresenter): fix !AndroidUnloadInactivePages optimizations --- src/Uno.UI/NativeFramePresenter.Android.cs | 144 +++++++++------------ 1 file changed, 58 insertions(+), 86 deletions(-) diff --git a/src/Uno.UI/NativeFramePresenter.Android.cs b/src/Uno.UI/NativeFramePresenter.Android.cs index 47419b422479..47d7474ee343 100644 --- a/src/Uno.UI/NativeFramePresenter.Android.cs +++ b/src/Uno.UI/NativeFramePresenter.Android.cs @@ -1,24 +1,15 @@ -using System; -using System.Collections.Generic; +using System.Collections.Generic; 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.UI.Extensions; namespace Uno.UI.Controls @@ -105,91 +96,72 @@ private async Task InvalidateStack() while (_stackUpdates.Any()) { var navigation = _stackUpdates.Dequeue(); - await UpdateStack(navigation.page, navigation.args); + await UpdateStack(navigation.page, navigation.args.NavigationTransitionInfo); } _isUpdatingStack = false; } - private async Task UpdateStack(Page newPage, NavigationEventArgs e) + private async Task UpdateStack(Page newPage, NavigationTransitionInfo transitionInfo) { - switch (e.NavigationMode) + // 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) { - case NavigationMode.Forward: - case NavigationMode.New: - case NavigationMode.Refresh: - { - if (_currentPage.page is { } oldPage) - { - if (GetIsAnimated(_currentPage.transitionInfo)) - { - await _currentPage.page.AnimateAsync(GetExitAnimation()); - _currentPage.page.ClearAnimation(); - } - if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages) - { - _pageStack.Children.Remove(oldPage); - } - else - { - oldPage.Visibility = Visibility.Collapsed; - } - } - _pageStack.Children.Add(newPage); - if (GetIsAnimated(e.NavigationTransitionInfo)) - { - await newPage.AnimateAsync(GetEnterAnimation()); - newPage.ClearAnimation(); - } - } - break; - case NavigationMode.Back: - { - if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages) - { - _pageStack.Children.Insert(0, newPage); - } - else - { - newPage.Visibility = Visibility.Visible; - } - - if (_currentPage.page is { } oldPage) - { - if (GetIsAnimated(_currentPage.transitionInfo)) - { - await _currentPage.page.AnimateAsync(GetExitAnimation()); - _currentPage.page.ClearAnimation(); - } - _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 (GetIsAnimated(oldTransitionInfo)) + { + await oldPage.AnimateAsync(GetExitAnimation()); + oldPage.ClearAnimation(); + } + if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages) + { + _pageStack.Children.Remove(oldPage); + } + else + { + oldPage.Visibility = Visibility.Collapsed; + } } - _currentPage = (newPage, e.NavigationTransitionInfo); + if (newPage is not null) + { + if (_pageStack.Children.Contains(newPage)) + { + newPage.Visibility = Visibility.Visible; + } + else + { + _pageStack.Children.Add(newPage); + } + if (GetIsAnimated(transitionInfo)) + { + await newPage.AnimateAsync(GetEnterAnimation()); + newPage.ClearAnimation(); + } + } + + if (!FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages) + { + var pagesStillInHistory = _frame.BackStack.Select(entry => entry.Instance).ToHashSet(); + pagesStillInHistory.AddRange(_frame.ForwardStack.Select(entry => entry.Instance)); + pagesStillInHistory.Add(newPage); + _pageStack.Children.Remove(element => !pagesStillInHistory.Contains(element)); + } } private static bool GetIsAnimated(NavigationTransitionInfo transitionInfo) From a128f70bdebc2e147526c5872204c26832b822e0 Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Tue, 17 Dec 2024 03:46:52 +0200 Subject: [PATCH 3/5] chore: add unsubscriptions --- src/Uno.UI/NativeFramePresenter.Android.cs | 28 ++++++++++++++++------ 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/Uno.UI/NativeFramePresenter.Android.cs b/src/Uno.UI/NativeFramePresenter.Android.cs index 47d7474ee343..814c7ad9de8e 100644 --- a/src/Uno.UI/NativeFramePresenter.Android.cs +++ b/src/Uno.UI/NativeFramePresenter.Android.cs @@ -1,4 +1,5 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using System.Collections.ObjectModel; using System.Collections.Specialized; using System.Linq; @@ -9,6 +10,7 @@ using Microsoft.UI.Xaml.Navigation; using Microsoft.UI.Xaml.Media.Animation; using Android.Views.Animations; +using Uno.Disposables; using Uno.Extensions; using Uno.UI.Extensions; @@ -18,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 (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() @@ -43,11 +44,17 @@ 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) @@ -57,6 +64,13 @@ private void Initialize(Frame frame) } } + private protected override void OnUnloaded() + { + base.OnUnloaded(); + _subscriptions?.Dispose(); + _subscriptions = null; + } + private void OnBackStackChanged(object sender, NotifyCollectionChangedEventArgs e) { UpdateBackButtonVisibility(); @@ -130,7 +144,7 @@ private async Task UpdateStack(Page newPage, NavigationTransitionInfo transition } if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages) { - _pageStack.Children.Remove(oldPage); + Children.Remove(oldPage); } else { @@ -140,13 +154,13 @@ private async Task UpdateStack(Page newPage, NavigationTransitionInfo transition if (newPage is not null) { - if (_pageStack.Children.Contains(newPage)) + if (Children.Contains(newPage)) { newPage.Visibility = Visibility.Visible; } else { - _pageStack.Children.Add(newPage); + Children.Add(newPage); } if (GetIsAnimated(transitionInfo)) { @@ -160,7 +174,7 @@ private async Task UpdateStack(Page newPage, NavigationTransitionInfo transition var pagesStillInHistory = _frame.BackStack.Select(entry => entry.Instance).ToHashSet(); pagesStillInHistory.AddRange(_frame.ForwardStack.Select(entry => entry.Instance)); pagesStillInHistory.Add(newPage); - _pageStack.Children.Remove(element => !pagesStillInHistory.Contains(element)); + Children.Remove(element => !pagesStillInHistory.Contains(element)); } } From 046a258c2a9fb3e6a1f35ff0901cf19527d13132 Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Tue, 17 Dec 2024 20:06:30 +0200 Subject: [PATCH 4/5] test: add When_Navigating_NativeFrame_Pages_Get_Collected --- .../Windows_UI_Xaml_Controls/Given_Frame.cs | 109 +++++++++++++++++- 1 file changed, 108 insertions(+), 1 deletion(-) 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..b4d60e76df68 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,85 @@ public async Task When_Exception_In_OnNavigatedTo() Assert.IsTrue(navigationFailed); } + [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); + } + [TestCleanup] public void Cleanup() { @@ -690,3 +772,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); + } +} From ee6b960359e3c37039a068d5bacfc27817e6892c Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Wed, 18 Dec 2024 13:15:21 +0200 Subject: [PATCH 5/5] chore: build errors --- .../Tests/Windows_UI_Xaml_Controls/Given_Frame.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 b4d60e76df68..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 @@ -492,6 +492,7 @@ public async Task When_Exception_In_OnNavigatedTo() Assert.IsTrue(navigationFailed); } +#if HAS_UNO [TestMethod] #if !__ANDROID__ [Ignore("This test specifically tests Android's NativeFramePresenter")] @@ -562,7 +563,7 @@ public async Task When_Navigating_NativeFrame_Pages_Get_Collected(bool backAndFo for (int i = 0; i < 10; i++) { GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced); - GC.WaitForPendingFinalizers(); + GC.WaitForPendingFinalizers(); } Assert.AreEqual(backAndForth ? 2 : 12, FinalizeCounterPage.ConstructorCalls); @@ -570,6 +571,7 @@ public async Task When_Navigating_NativeFrame_Pages_Get_Collected(bool backAndFo // SUT.BackStack.Clear(), but waits for the next SUT.Navigate() Assert.AreEqual(backAndForth ? 0 : 10, FinalizeCounterPage.FinalizerCalls); } +#endif [TestCleanup] public void Cleanup()