From ea78ad5794f2e6ccff3fab403b77979dd5b1e455 Mon Sep 17 00:00:00 2001 From: Kevin Petit Date: Tue, 3 Sep 2019 18:25:46 +0200 Subject: [PATCH] [Android] Correctly dispose TabbedPageRenderer (#4974) * Correctly dispose TabbedPageRenderer. The fragments created are now removed and the listeners are now correctly cleaned during dispose. * Added issue UI test. * Fix carousel fragment manager and pager disposing. * Fix test 2338 implementation. * Apply review. * Set to null fragmentManager on dispose. * Use GetFragmentManager extension method. Co-Authored-By: Shane Neuville * Revert some changes. * Apply review comment. --- .../Issue2338.cs | 14 ++-- .../Issue4973.cs | 74 +++++++++++++++++++ ...rin.Forms.Controls.Issues.Shared.projitems | 2 + .../AppCompat/CarouselPageRenderer.cs | 44 +++++++---- .../AppCompat/FormsFragmentPagerAdapter.cs | 50 ++++++++++++- 5 files changed, 161 insertions(+), 23 deletions(-) create mode 100644 Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue4973.cs diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue2338.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue2338.cs index a1be26e8f..bf92faef1 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue2338.cs +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue2338.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Diagnostics; using System.Threading.Tasks; using Xamarin.Forms.CustomAttributes; @@ -157,7 +157,6 @@ namespace Xamarin.Forms.Controls.Issues protected override void OnAppearing() => Debug.WriteLine($"OnAppearing: {_permutations}"); protected override void OnDisappearing() => Debug.WriteLine($"OnDisappearing: {_permutations}"); } - } [Preserve(AllMembers = true)] @@ -170,7 +169,6 @@ namespace Xamarin.Forms.Controls.Issues PushAsync(new InternalPage(30)); var otherPage = new InternalPage(40); - PushAsync(otherPage); otherPage.Appearing += async (object sender, EventArgs e) => { @@ -182,7 +180,9 @@ namespace Xamarin.Forms.Controls.Issues Application.Current.MainPage.BindingContext = new object(); }; + PushAsync(otherPage); } + protected override void OnAppearing() => Debug.WriteLine($"OnAppearing: Issue2338"); protected override void OnDisappearing() => Debug.WriteLine($"OnDisappearing: Issue2338"); @@ -336,10 +336,6 @@ namespace Xamarin.Forms.Controls.Issues await Task.Delay(500); var contentPage = new ContentPage(); -#pragma warning disable 4014 - Detail.Navigation.PushAsync(contentPage); -#pragma warning restore 4014 - contentPage.Appearing += (_, __) => { var navPage = new NavigationPage(new ContentPage() { Title = "Details" }); @@ -350,6 +346,10 @@ namespace Xamarin.Forms.Controls.Issues navPage.PushAsync(new ContentPage() { Title = "Details 2" }); }; + +#pragma warning disable 4014 + Detail.Navigation.PushAsync(contentPage); +#pragma warning restore 4014 } } } diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue4973.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue4973.cs new file mode 100644 index 000000000..8a8908247 --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue4973.cs @@ -0,0 +1,74 @@ +using Xamarin.Forms.CustomAttributes; +using Xamarin.Forms.Internals; + +#if UITEST +using System; +using NUnit.Framework; +#endif + +namespace Xamarin.Forms.Controls.Issues +{ + [Preserve(AllMembers = true)] + [Issue(IssueTracker.Github, 4973, "TabbedPage nav tests", PlatformAffected.Android)] + public class Issue4973 : TestTabbedPage + { + protected override void Init() + { + Children.Add(new TabbedPage + { + Title = "Tab1", + Children = + { + new ContentPage + { + Title = "InnerTab1" + }, + new ContentPage + { + Title = "InnerTab2" + } + } + }); + + Children.Add(new ContentPage + { + Title = "Tab2" + }); + + Children.Add(new ContentPage + { + Title = "Tab3" + }); + + Children.Add(new ContentPage + { + Title = "Tab4" + }); + + Children.Add(new ContentPage + { + Title = "Tab5", + Content = new Label + { + Text = "Test" + } + }); + } + +#if UITEST && __ANDROID__ + [Test] + public void Issue4973Test() + { + RunningApp.Tap(q => q.Text("Tab5")); + + RunningApp.WaitForElement(q => q.Text("Test")); + + GarbageCollectionHelper.Collect(); + + RunningApp.Tap(q => q.Text("Tab1")); + + RunningApp.Tap(q => q.Text("Tab2")); + } +#endif + } +} \ No newline at end of file diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems index 359423ad5..f12c34319 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems @@ -513,6 +513,8 @@ Code + + Issue5057.xaml Code diff --git a/Xamarin.Forms.Platform.Android/AppCompat/CarouselPageRenderer.cs b/Xamarin.Forms.Platform.Android/AppCompat/CarouselPageRenderer.cs index 29b312bc3..444bb7b43 100644 --- a/Xamarin.Forms.Platform.Android/AppCompat/CarouselPageRenderer.cs +++ b/Xamarin.Forms.Platform.Android/AppCompat/CarouselPageRenderer.cs @@ -2,16 +2,18 @@ using System; using System.Collections.Specialized; using System.ComponentModel; using Android.Content; +using Android.Support.V4.App; using Android.Support.V4.View; using Android.Views; namespace Xamarin.Forms.Platform.Android.AppCompat { - public class CarouselPageRenderer : VisualElementRenderer, ViewPager.IOnPageChangeListener + public class CarouselPageRenderer : VisualElementRenderer, ViewPager.IOnPageChangeListener, IManageFragments { bool _disposed; FormsViewPager _viewPager; Page _previousPage; + FragmentManager _fragmentManager; public CarouselPageRenderer(Context context) : base(context) { @@ -35,6 +37,14 @@ namespace Xamarin.Forms.Platform.Android.AppCompat IPageController PageController => Element as IPageController; + FragmentManager FragmentManager => _fragmentManager ?? (_fragmentManager = Context.GetFragmentManager()); + + void IManageFragments.SetFragmentManager(FragmentManager childFragmentManager) + { + if (_fragmentManager == null) + _fragmentManager = childFragmentManager; + } + void ViewPager.IOnPageChangeListener.OnPageSelected(int position) { Element.CurrentPage = Element.Children[position]; @@ -68,7 +78,8 @@ namespace Xamarin.Forms.Platform.Android.AppCompat RemoveAllViews(); _previousPage = null; - + _fragmentManager = null; + if (Element?.Children != null) { foreach (ContentPage pageToRemove in Element.Children) @@ -117,15 +128,22 @@ namespace Xamarin.Forms.Platform.Android.AppCompat if (e.NewElement != null) { - FormsViewPager pager = - _viewPager = - new FormsViewPager(activity) - { - OverScrollMode = OverScrollMode.Never, - EnableGesture = true, - LayoutParameters = new ViewGroup.LayoutParams(ViewGroup.LayoutParams.MatchParent, ViewGroup.LayoutParams.MatchParent), - Adapter = new FormsFragmentPagerAdapter(e.NewElement, activity.SupportFragmentManager) { CountOverride = e.NewElement.Children.Count } - }; + if (_viewPager != null) + { + _viewPager.RemoveOnPageChangeListener(this); + + ViewGroup.RemoveView(_viewPager); + + _viewPager.Dispose(); + } + + FormsViewPager pager = _viewPager = new FormsViewPager(activity) + { + OverScrollMode = OverScrollMode.Never, + EnableGesture = true, + LayoutParameters = new LayoutParams(LayoutParams.MatchParent, LayoutParams.MatchParent), + Adapter = new FormsFragmentPagerAdapter(e.NewElement, FragmentManager) { CountOverride = e.NewElement.Children.Count } + }; pager.Id = Platform.GenerateViewId(); pager.AddOnPageChangeListener(this); @@ -145,7 +163,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat { base.OnElementPropertyChanged(sender, e); - if (e.PropertyName == "CurrentPage") + if (e.PropertyName == nameof(Element.CurrentPage)) ScrollToCurrentPage(); } @@ -180,4 +198,4 @@ namespace Xamarin.Forms.Platform.Android.AppCompat _viewPager.SetCurrentItem(Element.Children.IndexOf(Element.CurrentPage), true); } } -} \ No newline at end of file +} diff --git a/Xamarin.Forms.Platform.Android/AppCompat/FormsFragmentPagerAdapter.cs b/Xamarin.Forms.Platform.Android/AppCompat/FormsFragmentPagerAdapter.cs index 968695df7..d2cc3d46e 100644 --- a/Xamarin.Forms.Platform.Android/AppCompat/FormsFragmentPagerAdapter.cs +++ b/Xamarin.Forms.Platform.Android/AppCompat/FormsFragmentPagerAdapter.cs @@ -1,17 +1,24 @@ -using Android.OS; +using System.Collections.Generic; +using Android.OS; using Android.Support.V4.App; using Java.Lang; using Xamarin.Forms.Internals; +using FragmentTransit = Android.App.FragmentTransit; namespace Xamarin.Forms.Platform.Android.AppCompat { internal class FormsFragmentPagerAdapter : FragmentPagerAdapter where T : Page { MultiPage _page; + FragmentManager _fragmentManager; + List _fragments; + bool _disposed; public FormsFragmentPagerAdapter(MultiPage page, FragmentManager fragmentManager) : base(fragmentManager) { _page = page; + _fragmentManager = fragmentManager; + _fragments = new List(); } public override int Count => CountOverride; @@ -20,7 +27,11 @@ namespace Xamarin.Forms.Platform.Android.AppCompat public override Fragment GetItem(int position) { - return FragmentContainer.CreateInstance(_page.Children[position]); + var fragment = FragmentContainer.CreateInstance(_page.Children[position]); + + _fragments.Add(fragment); + + return fragment; } public override long GetItemId(int position) @@ -31,12 +42,17 @@ namespace Xamarin.Forms.Platform.Android.AppCompat public override int GetItemPosition(Object objectValue) { var fragContainer = objectValue as FragmentContainer; - if (fragContainer != null && fragContainer.Page != null) + + if (fragContainer?.Page != null) { int index = _page.Children.IndexOf(fragContainer.Page); + if (index >= 0) return index; } + + _fragments.Remove(fragContainer); + return PositionNone; } @@ -52,8 +68,36 @@ namespace Xamarin.Forms.Platform.Android.AppCompat protected override void Dispose(bool disposing) { + if (_disposed) + { + return; + } + if (disposing) + { + _disposed = true; + _page = null; + + if (!_fragmentManager.IsDestroyed) + { + FragmentTransaction transaction = _fragmentManager.BeginTransactionEx(); + + foreach (Fragment fragment in _fragments) + { + transaction.RemoveEx(fragment); + transaction.SetTransitionEx((int)FragmentTransit.None); + } + + transaction.CommitAllowingStateLossEx(); + + _fragmentManager.ExecutePendingTransactionsEx(); + + _fragments = null; + _fragmentManager = null; + } + } + base.Dispose(disposing); } }