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

Share RecycledViewPool between view pager content fragments #660

Merged
merged 2 commits into from Feb 9, 2018

Conversation

Projects
None yet
2 participants
@chibatching
Contributor

chibatching commented Feb 9, 2018

Issue

  • no issue

Overview (Required)

  • Share RecyclerViewPool between content fragments of Sessions ViewPager
    • It will reduce RecyclerView item inflating count.
    • In my environment view inflate count is reduced to 31 from 95
      • Launch app(room mode - all tab) -> select right each room tab in tab order -> switch mode (Day2 14:00) -> select left each tab to all tab in tab order

Problem

I'm not sure it is appropriate to keep RecyclerViewPool on SessionsViewModel. I think there are three options.

  1. Share through SessionsViewModel (This PR is) or other new ViewModel.
  2. Keep pool on SessionFragment and each content Fragment get from it.
  3. Keep pool on ViewPager and pass pool from ViewPager to content Fragment

Do you have any advice to give to?

Links

Screenshot

Before After
@takahirom

This comment has been minimized.

Member

takahirom commented Feb 9, 2018

🆒

@takahirom takahirom added the awesome label Feb 9, 2018

@@ -49,6 +50,9 @@ class SessionsViewModel @Inject constructor(
val isLoading: LiveData<Boolean> by lazy {
tab.map { it.inProgress }
}
val viewPool: RecyclerView.RecycledViewPool by lazy {

This comment has been minimized.

@takahirom

takahirom Feb 9, 2018

Member

Can you add the comment here?

This comment has been minimized.

@takahirom

takahirom Feb 9, 2018

Member

Like this
// Share RecyclerViewPool between view pager content fragments

This comment has been minimized.

@chibatching

chibatching Feb 9, 2018

Contributor

Added comment on a1f9bbf !

@chibatching chibatching changed the title from Share RecyclerViewPool between view pager content fragments to Share RecycledViewPool between view pager content fragments Feb 9, 2018

@takahirom

This comment has been minimized.

Member

takahirom commented Feb 9, 2018

We try this 👍

@takahirom takahirom merged commit 16edc4d into DroidKaigi:master Feb 9, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
danger/danger All green. Yay.

@chibatching chibatching deleted the chibatching:improve_view_pager_recycler_view_performance branch Feb 9, 2018

@takahirom

This comment has been minimized.

Member

takahirom commented Feb 9, 2018

@chibatching
This is very useful pull request for all apps 👍
Sorry
When merged I was high.
I think this is leaking.
Because ViewModel can exist across the recreation of Activity.
And probably RecyclerViewPool have views that have activity
SessionFragment or MainActivity should have that.
Probably we should create an interface for that.
image

@chibatching

This comment has been minimized.

Contributor

chibatching commented Feb 9, 2018

You may be right.
I''ll check and fix later!
Thanks!

@takahirom

This comment has been minimized.

Member

takahirom commented Feb 10, 2018

@chibatching Thanks🙏

@chibatching

This comment has been minimized.

Contributor

chibatching commented Feb 10, 2018

I fixed at #662 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment