Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Commit

Permalink
fixes #gh2829
Browse files Browse the repository at this point in the history
* keep track of created ConditionalFocusLayout's so they can all be disposed of
  • Loading branch information
PureWeen committed May 30, 2018
1 parent 012d248 commit 5c4664d
Show file tree
Hide file tree
Showing 6 changed files with 238 additions and 20 deletions.
19 changes: 19 additions & 0 deletions Xamarin.Forms.ControlGallery.Android/CustomRenderers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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))]
Expand All @@ -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<Controls.Effects.AttachedStateEffect>())
{
effect.Detached(Element);
}

base.Dispose(disposing);
}
}

public class NativeDroidMasterDetail : Xamarin.Forms.Platform.Android.AppCompat.MasterDetailPageRenderer
{
MasterDetailPage _page;
Expand Down
Original file line number Diff line number Diff line change
@@ -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
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ protected override void OnCollectionChanged(NotifyCollectionChangedEventArgs e)
{
item.StateChanged -= AttachedStateEffectStateChanged;
}

}
}

Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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<Data>(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<Data>;

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
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@
<Compile Include="$(MSBuildThisFileDirectory)Bugzilla59457.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Bugzilla59580.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Effects\AttachedStateEffect.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Effects\AttachedStateEffectLabel.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Effects\AttachedStateEffectList.cs" />
<Compile Include="$(MSBuildThisFileDirectory)GitHub1878.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Helpers\ISampleNativeControl.cs" />
Expand All @@ -248,6 +249,7 @@
<Compile Include="$(MSBuildThisFileDirectory)Issue1705_2.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Issue1396.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Issue1415.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Issue2829.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Issue2653.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Issue2247.cs" />
<Compile Include="$(MSBuildThisFileDirectory)GroupListViewHeaderIndexOutOfRange.cs" />
Expand Down
57 changes: 39 additions & 18 deletions Xamarin.Forms.Platform.Android/Renderers/ListViewAdapter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ internal class ListViewAdapter : CellAdapter
protected readonly ListView _listView;
readonly AListView _realListView;
readonly Dictionary<DataTemplate, int> _templateToId = new Dictionary<DataTemplate, int>();
readonly List<ConditionalFocusLayout> _layoutsCreated = new List<ConditionalFocusLayout>();
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.
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
}

Expand Down

0 comments on commit 5c4664d

Please sign in to comment.