mikepenz / FastAdapter

The bullet proof, fast and easy to use adapter library, which minimizes developing time to a fraction...
https://mikepenz.dev
Apache License 2.0
3.84k stars 494 forks source link

CreateBinding is being called each time item is being collapsed or expanded. #1018

Closed Ceees2 closed 2 years ago

Ceees2 commented 2 years ago

About this issue

When using AbstractBindingItem and the expandable extension, expanding/collapsing an item causes createBinding to be called each time. I feel like this is not intended behaviour, since the actual clicked item is not dissapearing/appearing from/on the screen. And also because when using AbstractItem a new view holder is not being created newly each time on expanding/collapsing. Correct me if I'm wrong. I've previously created a pull request to "fix" something caused by this, but rather than fixing it, it was more a work around and preventing us from using notifying about item changes.

If this is indeed wrong/unintended behaviour i'd be happy to help out fixing it.

Details

mikepenz commented 2 years ago

@Ceees2 you can modify this behaviour by configuring notifyOnAutoToggleExpandable within the ExpandableExtension

https://github.com/mikepenz/FastAdapter/blob/develop/fastadapter-extensions-expandable/src/main/java/com/mikepenz/fastadapter/expandable/ExpandableExtension.kt#L103

It's used here when we toggle the expandable state for the clicked item: https://github.com/mikepenz/FastAdapter/blob/develop/fastadapter-extensions-expandable/src/main/java/com/mikepenz/fastadapter/expandable/ExpandableExtension.kt#L168 to: https://github.com/mikepenz/FastAdapter/blob/develop/fastadapter-extensions-expandable/src/main/java/com/mikepenz/fastadapter/expandable/ExpandableExtension.kt#L309 to: https://github.com/mikepenz/FastAdapter/blob/develop/fastadapter-extensions-expandable/src/main/java/com/mikepenz/fastadapter/expandable/ExpandableExtension.kt#L401-L404

If you don't notify this item, you'd need to make sure alternatively to highlight any indicator that it's expanded

Ceees2 commented 2 years ago

@Ceees2 you can modify this behaviour by configuring notifyOnAutoToggleExpandable within the ExpandableExtension

https://github.com/mikepenz/FastAdapter/blob/develop/fastadapter-extensions-expandable/src/main/java/com/mikepenz/fastadapter/expandable/ExpandableExtension.kt#L103

It's used here when we toggle the expandable state for the clicked item: https://github.com/mikepenz/FastAdapter/blob/develop/fastadapter-extensions-expandable/src/main/java/com/mikepenz/fastadapter/expandable/ExpandableExtension.kt#L168 to: https://github.com/mikepenz/FastAdapter/blob/develop/fastadapter-extensions-expandable/src/main/java/com/mikepenz/fastadapter/expandable/ExpandableExtension.kt#L309 to: https://github.com/mikepenz/FastAdapter/blob/develop/fastadapter-extensions-expandable/src/main/java/com/mikepenz/fastadapter/expandable/ExpandableExtension.kt#L401-L404

If you don't notify this item, you'd need to make sure alternatively to highlight any indicator that it's expanded

I know, that was my proposed solution in the linked PR, but I feel like that is more a work around than an actual solution. notifyOnAutoToggleExpandable false is fixing the animation issue, but causes the inability to used the extensions expand or collapse since click listeners wont be called and thus no animations. notifyOnAutoToggleExpandable true would somewhat fix that issue but can cause any unwanted animation behaviour. The linked PR also shows this behaviour is actually present in the current example app.

Might also just be that I'm trying to do something thats just not possible, but not giving up yet.

mikepenz commented 2 years ago

Sorry, missed that this was the exact thing you had linked.

One potential solution would be to do the notify with a payload in this case, so you could manually handle the bind in case of the existence of the payload, allowing you to update the UI?

https://developer.android.com/reference/androidx/recyclerview/widget/RecyclerView.Adapter#notifyItemChanged(int,%20java.lang.Object)

Ceees2 commented 2 years ago

Sorry, missed that this was the exact thing you had linked.

One potential solution would be to do the notify with a payload in this case, so you could manually handle the bind in case of the existence of the payload, allowing you to update the UI?

https://developer.android.com/reference/androidx/recyclerview/widget/RecyclerView.Adapter#notifyItemChanged(int,%20java.lang.Object)

I thought about that, but that would mean updating the expand and collapse methods wouldn't it?

Would it also be possible to call bindView/unbindView with the current view instead of creating a new view. Seems like that's also causing issues for animations if the starting state of the view (when starting the animation) is different than when the view is created initially. eg the jumping of the expand icon; when the item is expanded and you want to collapse, the icon jumps to the collapsed rotation(since the view is being recreated), then immediately back to the expanded rotation and then the animation starts to rotate it back to collapse.

mikepenz commented 2 years ago

@Ceees2 it will mean we have to update the extension, but it would still be a good option, and consumers of the SDK can start acting on it optionally.

No it's not anticipated to call bindView/unbindView with the current view as those methods are called by the RecyclerView as the RV will properly handle recycling the re-use of views, and any interception of that is most likely causing significant issues.

The animation parts can all be handled by making use of the payload, as within the bindView the RV will make sure the right view is provided (reused or new) and based on the payload you can then run the animation

Ceees2 commented 2 years ago

That was fast, thanks for all your work.

Ceees2 commented 2 years ago

Works like a charm, I've completely removed the click listener triggering the animation and am using the payload now to determine if animations should be shown, and no need to use notifyOnAutoToggleExpandable = false anymore. All in all nice change, thanks a bunch. 👌

If wanted I can also open a PR updating and applying these changes in the expand example in the fast adapter example app @mikepenz

mikepenz commented 2 years ago

@Ceees2 that's awesome news. Glad it works that great for you.

And yes definitely. Always happy about contributions 😀