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

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

Closed
5 tasks
Ceees2 opened this issue Nov 22, 2021 · 8 comments · Fixed by #1021
Closed
5 tasks

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

Ceees2 opened this issue Nov 22, 2021 · 8 comments · Fixed by #1021
Assignees
Labels

Comments

@Ceees2
Copy link
Contributor

Ceees2 commented Nov 22, 2021

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

  •  Used library version - Latest
  •  Used support library version - Latest
  •  Used gradle build tools version - Several - 7.2
  •  Used tooling / Android Studio version - Bumblebee
  •  Other used libraries, potential conflicting libraries - inapplicable
@mikepenz mikepenz self-assigned this Nov 22, 2021
@Ceees2
Copy link
Contributor Author

Ceees2 commented Nov 22, 2021

@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
Copy link
Owner

mikepenz commented Nov 22, 2021

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
Copy link
Contributor Author

Ceees2 commented Nov 23, 2021

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
Copy link
Owner

@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
Copy link
Contributor Author

Ceees2 commented Nov 24, 2021

That was fast, thanks for all your work.

@Ceees2
Copy link
Contributor Author

Ceees2 commented Nov 24, 2021

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
Copy link
Owner

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

And yes definitely. Always happy about contributions 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants