Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fragments leaking on orientation change #681

Closed
ahmedre opened this issue Sep 4, 2016 · 1 comment
Closed

Fragments leaking on orientation change #681

ahmedre opened this issue Sep 4, 2016 · 1 comment

Comments

@ahmedre
Copy link
Contributor

ahmedre commented Sep 4, 2016

this only happens on tablets with tablet mode.

start a tablet in portrait, then switch to landscape (with the setting to see 2 pages side by side enabled). do a heap dump and look for instances of HighlightingImageView - while you'd expect to only find 6 (2 per page, and ViewPager keeps 3 pages), we instead find 9, 3 of which are from portrait orientation.

@ahmedre ahmedre closed this as completed in 5a03036 Sep 6, 2016
ahmedre added a commit that referenced this issue Sep 6, 2016
This patch greatly improves tablet orientation change by making a few
fixes - first, it stops the ViewPager from restoring its page number
when the pages are different between portrait and landscape. Second, it
stops calling setCurrentItem more than once. See #681 for further
discussion.
@ahmedre
Copy link
Contributor Author

ahmedre commented Sep 6, 2016

this really needs a blog post, but the tldr is that:

  1. FragmentStatePagerAdapter is better than FragmentPagerAdapter for large sets of pages because it does remove on the Fragments that are going away instead of just detach. the problem, however, is that on orientation change, we don't get destroyItem, and so the FragmentManager hangs on to those Fragments (with the idea that it'll need to restore them when orientation changes). since our Fragments in landscape are different than the ones in portrait, this is problematic, since we keep a reference to the old fragments.
  2. the ViewPager stores the page it was on as part of its saved state - that's great when we're showing the same number of pages in portrait and landscape, but terrible otherwise because it requires us to load pages that we'll soon after throw away.

the fix implemented here is two fold:

  1. modify FragmentStatePagerAdapter to detect the mode change and explicitly remove the old fragments in restoreState.
  2. hack ViewPager to not restore the page on its own in said cases.

the future intended fix is likely:

  1. switch to views, since problem 1 will just fix itself.
  2. subclass ViewPager, override onRestoreInstanceState, optionally handle our own instance state, and call super.onRestoreInstanceState(null).

an alternative to 2 above is to instead signal to the adapter that we're restoring instance state, at which point the adapter can either return lightweight Views that it knows will be destroyed immediately thereafter, or do the mapping and return the proper View that will be requested shortly thereafter.

ahmedre added a commit that referenced this issue Oct 14, 2017
This patch introduces a "mode" that is passed to
FragmentStatePagerAdapter, which determines whether we are in single or
dual page mode. When switching from one to the other, remove all the
fragments from FragmentManager since there is no reason to cache them.
Fixes #681.
ahmedre added a commit that referenced this issue Oct 14, 2017
This patch greatly improves tablet orientation change by making a few
fixes - first, it stops the ViewPager from restoring its page number
when the pages are different between portrait and landscape. Second, it
stops calling setCurrentItem more than once. See #681 for further
discussion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant