For discussion - Performance issues on selectExtension.deselect()

RobbWatershed commented 3 years ago

About this issue

Briefly describe the issue

Performance of selectExtension.deselect() is atrocious : users with large collections of items report the app hanging during 20-30s during that sole step of deselecting data.

When calling selectExtension.deselect(selectExtension.getSelections()) instead, FastAdapter only processes actually selected items, and the freeze doesn't happen anymore.

=> Are you certain the use of fastAdapter.recursive when calling deselect() is the best way to go ? Isn't there some kind of optimization that can be done there ?

How can the issue be reproduced / sample code

Details

[ ] Used library version : 5.4.1
[ ] Used support library version : not sure what you mean
[ ] Used gradle build tools version : 4.2.0
[ ] Used tooling / Android Studio version : 4.2
[ ] Other used libraries, potential conflicting libraries : Not that I know of
[ ] The list where it happens uses the paging extension and is fed by livedata

Checklist

[ ] Searched for similar issues Yes
[ ] Checked out the sample application I haven't tried to repro that yet
[ ] Read the README Yes
[ ] Checked out the CHANGELOG Yes
[ ] Checked out the MIGRATION GUIDE Yes

mikepenz commented 3 years ago

@RobbWatershed thank you so much for the issue (and the PR) I hope to be able to come back to it soon. the next 2-3 weeks may be super busy though, so I am sorry in case it's taking me a bit longer to come back to these

RobbWatershed commented 3 years ago

No problem man, take your time, it's not blocking me in any way 😉

mikepenz commented 3 years ago

@RobbWatershed one of the major reasons for the way deselect() works is to really make sure nothing is selected anymore. It does not make assumptions on implementation details (e.g. it will deselect all items) -> for example if sub items can't be selected.

if your usecase does not allow selections in subitems then the second approach would for sure much more fit the requirements.

in the past we had a list keeping track of selections, but ultimately this served to be problematic as it may get out of sync, and ultimately the adapter itself should not necessarily maintain this.

Not sure if this already answers your concern/question?

mikepenz commented 3 years ago

Also congrats to opening the 1000th ticket :D

RobbWatershed commented 3 years ago

if your usecase does not allow selections in subitems then the second approach would for sure much more fit the requirements.

in the past we had a list keeping track of selections, but ultimately this served to be problematic as it may get out of sync, and ultimately the adapter itself should not necessarily maintain this.

That's a perfectly valid answer to me, thanks !

I didn't even think about subitems when considering creating that issue, tbh, as my app doesn't use them.

I'll keep using selectExtension.deselect(selectExtension.getSelections()) for my own needs

mikepenz / FastAdapter