Add a replaceAll(List<RenderInfo>) method to RecyclerBinder. #451
Conversation
|
Will add tests obviously, if you all think this is an acceptable API. |
|
What is the difference between this and calling clear() and then adding all the new items? |
|
2 things:
|
|
When you say transition animations, are you talking about ItemAnimator animations or Litho animations? |
|
ItemAnimator animations |
|
So, from the ItemAnimator, you should be able to get the ComponentTreeHolder and RenderInfo -- can you put custom info on the RenderInfo that indicates that the current changes shouldn't animate (this would only really work for insert/change animations though)? I'd really like to not add this API if possible so I'm trying to think of other options. |
|
It's more that the ItemAnimator would have to know that the RV is backed by Litho or not, which is more awkward imo. I'm curious what you think the issues are against adding this API? If someone is integrating at the RecyclerBinder level (vs sections), they're already digging pretty deep into RV+RB interactions and this is basically the only point where RB can't offer the same API. While I understand the API is far from ideal... there are some performance advantages, especially in the "mostly not litho views" case (like ours). I do want to look into making a proper "lazy RenderInfo" API to use instead of this, so this may very well be temporary. |
|
@vinc3m1 I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
|
ping @astreet any thoughts? |
|
Hey, sorry, this seems reasonable to me: one thing I think you should change in the implementation is to create new ComponentTrees instead of reusing some: I think it's more correct to model this like a clear+addAll, and that's what that would do. It hopefully doesn't matter, but re-using a ComponentTree/ComponentTreeHolder means reusing the StateHandler, as well as indicating the the framework that semantically they are actually the same row (e.g. for the purposes of internal animations). Fix that up and I think we're good to land, thanks for the context |
|
Oh and a test if you don't mind :) |
|
The problem I see here is mostly with State. The way we are updating ComponentTreeHolders will transfer state just based on the index of the item and that's basically wrong. You really want to be removing all the ComponentTreeHolders and create new ones for the entire list |
|
Ah yeah this is outdated, our current version only reused the ComponentTreeHolder when both old and new versions render Views. Will update and add tests |
|
@vinc3m1 I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
litho-widget/src/main/java/com/facebook/litho/widget/RecyclerBinder.java
Outdated
Show resolved
Hide resolved
|
See the inline -- any reason we want to reuse holders instead of just dropping the old list and creating a new one? It seems simpler and maybe less bug-prone |
For compatibility reasons, there's currently no way to hook up RecyclerView.notifyDatasetChanged() calls to RecyclerBinder without potentially triggering transition animations. This opens up an API to support the equivalent of replacing all the items in a RecyclerBinder and calling notifyDatasetChanged().
|
lgtm! I'll import now |
There was a problem hiding this comment.
@astreet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@vinc3m1 I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
For compatibility reasons, there's currently no way to hook up
RecyclerView.notifyDatasetChanged() calls to RecyclerBinder without
potentially triggering transition animations. This opens up an API to
support the equivalent of replacing all the items in a RecyclerBinder
and calling notifyDatasetChanged().