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

[Android] Keep track of created ConditionalFocusLayouts so they can all be disposed of #2857

Merged
merged 1 commit into from
Jun 7, 2018

Conversation

PureWeen
Copy link
Contributor

@PureWeen PureWeen commented May 29, 2018

Description of Change

Currently DisposeCells only iterates using GetChildAt which will only return visible cells. When you would scroll a listview this would cause a cell to not be visible and would thus return NULL for GetChildAt but the ConditionalFocusLayout would still exist in memory. This fix keeps track of created Layouts in an internal list to ensure that all resources get disposed of

I can't say for sure if this is the only cause of #2829 but it definitely fixes a case where Dispose wasn't being called for a given cell.

Bugs Fixed

fixes #2829

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

@PureWeen PureWeen requested review from samhouts and hartez May 29, 2018 16:19
@samhouts samhouts changed the title keep track of created ConditionalFocusLayout's so they can all be disposed of [Android] Keep track of created ConditionalFocusLayouts so they can all be disposed of May 29, 2018
@samhouts samhouts added this to the 3.1.0 milestone May 29, 2018
@samhouts samhouts changed the title [Android] Keep track of created ConditionalFocusLayouts so they can all be disposed of [Android] Renderers associated with ListView cells are occasionaly not being disposed of which causes left over events to propagate to disposed views May 29, 2018
@samhouts samhouts added i/critical retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. labels May 29, 2018
@samhouts samhouts changed the title [Android] Renderers associated with ListView cells are occasionaly not being disposed of which causes left over events to propagate to disposed views [Android] Keep track of created ConditionalFocusLayouts so they can all be disposed of May 29, 2018
@@ -3,6 +3,7 @@
using System.Collections.Specialized;
using System.Threading.Tasks;
using Xamarin.Forms.Internals;
using System.Linq;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@PureWeen PureWeen changed the base branch from master to 3.1.0 May 30, 2018 00:06
@PureWeen PureWeen removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label May 30, 2018
hartez
hartez previously requested changes Jun 5, 2018
Copy link
Contributor

@hartez hartez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, let's just loop once on Dispose for simplicity.


renderedView?.Dispose();
renderedView = null;
// Having just this loop would probably be sufficient but
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this covers everything the previous loop does (and more); let's just loop once. It'll make the code easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hartez fixed

* keep track of created ConditionalFocusLayout's so they can all be disposed of
@PureWeen
Copy link
Contributor Author

PureWeen commented Jun 6, 2018

build --uitests

@rmarinho rmarinho dismissed hartez’s stale review June 7, 2018 17:14

Fixed the Disposed

@rmarinho rmarinho merged commit 82d69c2 into 3.1.0 Jun 7, 2018
@StephaneDelcroix StephaneDelcroix deleted the fix-gh2829 branch December 19, 2018 15:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants