diff --git a/Xamarin.Forms.ControlGallery.Android/CustomRenderers.cs b/Xamarin.Forms.ControlGallery.Android/CustomRenderers.cs index 1e7e0be30e8..74095bb0fd3 100644 --- a/Xamarin.Forms.ControlGallery.Android/CustomRenderers.cs +++ b/Xamarin.Forms.ControlGallery.Android/CustomRenderers.cs @@ -25,6 +25,7 @@ using Xamarin.Forms.Controls.Issues; +[assembly: ExportRenderer(typeof(Xamarin.Forms.Controls.Effects.AttachedStateEffectLabel), typeof(AttachedStateEffectLabelRenderer))] [assembly: ExportRenderer(typeof(Issue1942.CustomGrid), typeof(Issue1942GridRenderer))] [assembly: ExportRenderer(typeof(Bugzilla31395.CustomContentView), typeof(CustomContentRenderer))] [assembly: ExportRenderer(typeof(NativeListView), typeof(NativeListViewRenderer))] @@ -48,6 +49,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..324f7da5a5d 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 @@ -3,6 +3,7 @@ using System.Collections.Specialized; using System.Threading.Tasks; using Xamarin.Forms.Internals; +using System.Linq; namespace Xamarin.Forms.Controls.Effects { @@ -41,7 +42,6 @@ protected override void OnCollectionChanged(NotifyCollectionChangedEventArgs e) { item.StateChanged -= AttachedStateEffectStateChanged; } - } } @@ -54,7 +54,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..11bd2977891 --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue2829.cs @@ -0,0 +1,160 @@ +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 a00b760902f..fb828cbb773 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 @@ + @@ -251,6 +252,7 @@ + diff --git a/Xamarin.Forms.Platform.Android/Renderers/ListViewAdapter.cs b/Xamarin.Forms.Platform.Android/Renderers/ListViewAdapter.cs index 9cf072aa813..0d050c59c8f 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) { @@ -463,6 +467,29 @@ protected override void HandleItemClick(AdapterView parent, AView view, int posi Controller.NotifyRowTapped(position, cell); } + + void DisposeOfConditionalFocusLayout(ConditionalFocusLayout layout) + { + var renderedView = layout?.GetChildAt(0); + + var element = (renderedView as INativeElementView)?.Element; + var view = (element as ViewCell)?.View; + + if (view != null) + { + var renderer = Platform.GetRenderer(view); + + if (renderer == renderedView) + element.ClearValue(Platform.RendererProperty); + + renderer?.Dispose(); + renderer = null; + } + + renderedView?.Dispose(); + renderedView = null; + } + void DisposeCells() { var cellCount = _realListView?.ChildCount ?? 0; @@ -474,25 +501,19 @@ void DisposeCells() if (layout == null || layout.IsDisposed()) continue; - var renderedView = layout?.GetChildAt(0); + if (_layoutsCreated.Contains(layout)) + _layoutsCreated.Remove(layout); - var element = (renderedView as INativeElementView)?.Element; - - var view = (element as ViewCell)?.View; - - if (view != null) - { - var renderer = Platform.GetRenderer(view); - - if (renderer == renderedView) - element.ClearValue(Platform.RendererProperty); - - renderer?.Dispose(); - renderer = null; - } + DisposeOfConditionalFocusLayout(layout); + } - renderedView?.Dispose(); - renderedView = null; + // Having just this loop would probably be sufficient but + // I left both just to be overly careful + for (int i = _layoutsCreated.Count - 1; i >= 0; i--) + { + var layout = _layoutsCreated[i]; + _layoutsCreated.Remove(layout); + DisposeOfConditionalFocusLayout(layout); } }