From ca3ae34d9d00a3151b6632f9327e42bd1285a7ab Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Tue, 5 Jun 2018 21:34:45 -0700 Subject: [PATCH] fixes #gh2829 * keep track of created ConditionalFocusLayout's so they can all be disposed of --- .../CustomRenderers.cs | 19 +++ .../Effects/AttachedStateEffectLabel.cs | 15 ++ .../Effects/AttachedStateEffectList.cs | 5 +- .../Issue2829.cs | 159 ++++++++++++++++++ ...rin.Forms.Controls.Issues.Shared.projitems | 2 + .../Renderers/ListViewAdapter.cs | 48 +++--- 6 files changed, 227 insertions(+), 21 deletions(-) create mode 100644 Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Effects/AttachedStateEffectLabel.cs create mode 100644 Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue2829.cs diff --git a/Xamarin.Forms.ControlGallery.Android/CustomRenderers.cs b/Xamarin.Forms.ControlGallery.Android/CustomRenderers.cs index 623f27a9a30..fd4ac9e52d1 100644 --- a/Xamarin.Forms.ControlGallery.Android/CustomRenderers.cs +++ b/Xamarin.Forms.ControlGallery.Android/CustomRenderers.cs @@ -23,6 +23,8 @@ using Android.Text.Method; using Xamarin.Forms.Controls.Issues; + +[assembly: ExportRenderer(typeof(Xamarin.Forms.Controls.Effects.AttachedStateEffectLabel), typeof(AttachedStateEffectLabelRenderer))] [assembly: ExportRenderer(typeof(Bugzilla31395.CustomContentView), typeof(CustomContentRenderer))] [assembly: ExportRenderer(typeof(NativeListView), typeof(NativeListViewRenderer))] [assembly: ExportRenderer(typeof(NativeListView2), typeof(NativeAndroidListViewRenderer))] @@ -45,6 +47,23 @@ #endif namespace Xamarin.Forms.ControlGallery.Android { + public class AttachedStateEffectLabelRenderer : LabelRenderer + { + public AttachedStateEffectLabelRenderer(Context context) : base(context) + { + } + + protected override void Dispose(bool disposing) + { + foreach(var effect in Element.Effects.OfType()) + { + effect.Detached(Element); + } + + base.Dispose(disposing); + } + } + public class NativeDroidMasterDetail : Xamarin.Forms.Platform.Android.AppCompat.MasterDetailPageRenderer { MasterDetailPage _page; diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Effects/AttachedStateEffectLabel.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Effects/AttachedStateEffectLabel.cs new file mode 100644 index 00000000000..fe4643dcb59 --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Effects/AttachedStateEffectLabel.cs @@ -0,0 +1,15 @@ +using System; +using System.Collections.Generic; +using System.Text; +using Xamarin.Forms.Internals; + +namespace Xamarin.Forms.Controls.Effects +{ + [Preserve(AllMembers = true)] + public class AttachedStateEffectLabel : Label + { + // Android renderers don't detach effects when the renderers get disposed + // so this is a hack setup to detach those effects when testing if dispose is called from a renderer + // https://github.com/xamarin/Xamarin.Forms/issues/2520 + } +} diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Effects/AttachedStateEffectList.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Effects/AttachedStateEffectList.cs index 2c111773b65..5ce6773ea6f 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Effects/AttachedStateEffectList.cs +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Effects/AttachedStateEffectList.cs @@ -41,7 +41,6 @@ protected override void OnCollectionChanged(NotifyCollectionChangedEventArgs e) { item.StateChanged -= AttachedStateEffectStateChanged; } - } } @@ -54,7 +53,9 @@ private async void AttachedStateEffectStateChanged(object sender, EventArgs e) foreach (AttachedStateEffect item in this) { allAttached &= item.State == AttachedStateEffect.AttachedState.Attached; - allDetached &= item.State == AttachedStateEffect.AttachedState.Detached; + + if (item.State != AttachedStateEffect.AttachedState.Unknown) + allDetached &= item.State == AttachedStateEffect.AttachedState.Detached; } if (allAttached) diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue2829.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue2829.cs new file mode 100644 index 00000000000..549b9ab02e3 --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue2829.cs @@ -0,0 +1,159 @@ +using System; +using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.ComponentModel; +using System.Linq; +using System.Text; +using Xamarin.Forms.Controls.Effects; +using Xamarin.Forms.CustomAttributes; +using Xamarin.Forms.Internals; + +#if UITEST +using Xamarin.UITest; +using NUnit.Framework; +#endif + +namespace Xamarin.Forms.Controls.Issues +{ + [Preserve(AllMembers = true)] + [Issue(IssueTracker.Github, 2829, "[Android] Renderers associated with ListView cells are occasionaly not being disposed of which causes left over events to propagate to disposed views", + PlatformAffected.Android)] + public class Issue2829 : TestNavigationPage + { + AttachedStateEffectList attachedStateEffectList = new AttachedStateEffectList(); + const string kScrollMe = "kScrollMe"; + const string kSuccess = "SUCCESS"; + const string kCreateListViewButton = "kCreateListViewButton"; + StackLayout layout = null; + + protected override void Init() + { + var label = new Label() { Text = "Click the button then click back" }; + + layout = new StackLayout() + { + Children = + { + label, + new Button() + { + AutomationId = kCreateListViewButton, + Text = "Create ListView", + Command = new Command(() => + { + attachedStateEffectList.ToList().ForEach(x=> attachedStateEffectList.Remove(x)); + label.Text = "FAILURE"; + Navigation.PushAsync(CreateListViewPage()); + }) + } + } + }; + + var page = new ContentPage() + { + Content = layout + }; + + + PushAsync(page); + attachedStateEffectList.AllEventsDetached += (_, __) => + { + label.Text = kSuccess; + }; + } + + ListView CreateListView() + { + ListView view = new ListView(ListViewCachingStrategy.RecycleElement); + view.ItemTemplate = new DataTemplate(() => + { + ViewCell cell = new ViewCell(); + AttachedStateEffectLabel label = new AttachedStateEffectLabel(); + label.TextColor = Color.Black; + label.BackgroundColor = Color.White; + label.SetBinding(Label.TextProperty, "Text"); + attachedStateEffectList.Add(label); + label.BindingContextChanged += (_, __) => + { + if (label.AutomationId == null) + label.AutomationId = ((Data)label.BindingContext).Text.ToString(); + }; + + cell.View = new ContentView() + { + Content = new StackLayout() + { + Orientation = StackOrientation.Horizontal, + Children = + { + label, + new Image{ Source = "coffee.png"} + } + }, + HeightRequest = 40 + }; + + return cell; + }); + var data = new ObservableCollection(Enumerable.Range(0, 72).Select(index => new Data() { Text = index.ToString() })); + view.ItemsSource = data; + + return view; + } + + Page CreateListViewPage() + { + var view = CreateListView(); + var data = view.ItemsSource as ObservableCollection; + + Button scrollMe = new Button() + { + Text = "Scroll ListView", + AutomationId = kScrollMe, + Command = new Command(() => + { + view.ScrollTo(data.Last(), ScrollToPosition.MakeVisible, true); + }) + }; + + return new ContentPage() + { + Content = new StackLayout() + { + Children = { scrollMe, view } + } + }; + } + + [Preserve(AllMembers = true)] + public class Data : INotifyPropertyChanged + { + private string _text; + + public string Text + { + get => _text; + set + { + _text = value; + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Text))); + } + } + + public event PropertyChangedEventHandler PropertyChanged; + } + +#if UITEST + [Test] + public void ViewCellsAllDisposed() + { + RunningApp.Tap(kCreateListViewButton); + RunningApp.WaitForElement("0"); + RunningApp.Tap(kScrollMe); + RunningApp.WaitForElement("70"); + RunningApp.Back(); + RunningApp.WaitForElement(kSuccess); + } +#endif + } +} 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 8f296a5c6cb..66bce43e843 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 @@ -238,6 +238,7 @@ + @@ -249,6 +250,7 @@ + diff --git a/Xamarin.Forms.Platform.Android/Renderers/ListViewAdapter.cs b/Xamarin.Forms.Platform.Android/Renderers/ListViewAdapter.cs index 9cf072aa813..16d89f873cc 100644 --- a/Xamarin.Forms.Platform.Android/Renderers/ListViewAdapter.cs +++ b/Xamarin.Forms.Platform.Android/Renderers/ListViewAdapter.cs @@ -27,6 +27,7 @@ internal class ListViewAdapter : CellAdapter protected readonly ListView _listView; readonly AListView _realListView; readonly Dictionary _templateToId = new Dictionary(); + readonly List _layoutsCreated = new List(); int _dataTemplateIncrementer = 2; // lets start at not 0 because ... // We will use _dataTemplateIncrementer to get the proper ViewType key for the item's DataTemplate and store these keys in _templateToId. @@ -181,7 +182,7 @@ public override int GetItemViewType(int position) _templateToId[itemTemplate] = key; } - if (key >= ViewTypeCount) + if (key >= ViewTypeCount) { throw new Exception($"ItemTemplate count has exceeded the limit of {ViewTypeCount}" + Environment.NewLine + "Please make sure to reuse DataTemplate objects"); @@ -226,7 +227,10 @@ public override AView GetView(int position, AView convertView, ViewGroup parent) convertView = layout.GetChildAt(0); } else + { layout = new ConditionalFocusLayout(_context) { Orientation = Orientation.Vertical }; + _layoutsCreated.Add(layout); + } if (((cachingStrategy & ListViewCachingStrategy.RecycleElement) != 0) && convertView != null) { @@ -465,35 +469,41 @@ protected override void HandleItemClick(AdapterView parent, AView view, int posi void DisposeCells() { - var cellCount = _realListView?.ChildCount ?? 0; + var cellCount = _layoutsCreated.Count; + for (int i = 0; i < cellCount; i++) { - var layout = _realListView.GetChildAt(i) as ConditionalFocusLayout; + var layout = _layoutsCreated[i]; - // Headers and footers will be skipped. They are disposed elsewhere. - if (layout == null || layout.IsDisposed()) + if (layout.IsDisposed()) continue; + + DisposeOfConditionalFocusLayout(layout); + } - var renderedView = layout?.GetChildAt(0); - - var element = (renderedView as INativeElementView)?.Element; + _layoutsCreated.Clear(); + } - var view = (element as ViewCell)?.View; + void DisposeOfConditionalFocusLayout(ConditionalFocusLayout layout) + { + var renderedView = layout?.GetChildAt(0); - if (view != null) - { - var renderer = Platform.GetRenderer(view); + var element = (renderedView as INativeElementView)?.Element; + var view = (element as ViewCell)?.View; - if (renderer == renderedView) - element.ClearValue(Platform.RendererProperty); + if (view != null) + { + var renderer = Platform.GetRenderer(view); - renderer?.Dispose(); - renderer = null; - } + if (renderer == renderedView) + element.ClearValue(Platform.RendererProperty); - renderedView?.Dispose(); - renderedView = null; + renderer?.Dispose(); + renderer = null; } + + renderedView?.Dispose(); + renderedView = null; } // TODO: We can optimize this by storing the last position, group index and global index