search

airbnb / epoxy

Epoxy is an Android library for building complex screens in a RecyclerView
https://goo.gl/eIK82p
Apache License 2.0
8.5k stars 733 forks source link

Carousel default behavior might leak? #745

Closed franciscoaleixo closed 5 years ago

franciscoaleixo commented 5 years ago

Why does the com.airbnb.epoxy.Carousel EpoxyRecyclerView use setRemoveAdapterWhenDetachedFromWindow(false); by default?

The comment above that line reads // Carousels will be detached when their parent recyclerview is.

Well, if the Carousel is a top level recyclerview won't it leak? And even if it is not (and used in a nested fashion), wouldn't it be better to swapAdapter() anyway, because it will most likely have a sharedViewPool?

At first glance, it would seem more beneficial to make it true by default.

Thanks for the great library.

franciscoaleixo commented 5 years ago

Oh sorry, I just saw https://github.com/airbnb/epoxy/pull/688, BUT I still think that by default it might be better to put it as true for the reasons mentioned. What are your thoughts?

I also wonder if there is no other way to achieve https://github.com/airbnb/epoxy/pull/688 without setting the flag to false. Since the parent generally keeps the state through its LayoutManager (and there's no problems with exit shared element transitions there), it should be possible to also save the nested RV's state without compromising removing the adapter on detach. Infact, doesn't Epoxy already have the ability to save state in these conditions?

elihart commented 5 years ago

Hi, Carousel is mainly designed for nested usage inside another recyclerview, in which case it is unnecessary and slightly expensive to have the setRemoveAdapterWhenDetachedFromWindow behavior (if you are flinging through a list of carousels).

If it is is a nested view, then when the parent's adapter is removed the Carousel will be recycled and cleaned up, which is fine. We don't need the nested Carousel to manually do that. Epoxy does save state of both the top level RecyclerView and nested Carousels, but saved state is unrelated to the transition issue - if the view is cleared in the middle of a transition then it will break it.

We could probably change the default behavior to instead be based on whether the Carousel is nested in another RecyclerView - that could be a reasonable default in EpoxyRecyclerView, which could be set in onAttachedToWindow. Haven't fully thought that through, we need to make sure it would hurt other use cases, but feel free to put up a PR if you are passionate about the fix :)

franciscoaleixo commented 5 years ago

Hi @elihart! Thanks for the response. I think that's a good suggestion, so I might do a PR in the future to make setRemoveAdapterWhenDetachedFromWindow dependent on if it's nested or not.

I was going to argue that it's still beneficial to set it to true on nested situations because swapAdapter() recycles the views back to the pool (good for shared view pools). However I found that since the prefetch feature, they actually already recycle nested views back to the pool for us! This can be seen in GapWorker class on prefetchInnerRecyclerViewWithDeadline method. Given this, I'm with you that for nested cases it will probably be more beneficial to set the flag to false.

Thanks!