-
Notifications
You must be signed in to change notification settings - Fork 3
Add performBatchUpdates(_:) function
#17
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
Conversation
| isPerformingBatchUpdates = true | ||
| updateDelegate?.willBeginUpdating(self) | ||
|
|
||
| updates(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if passing the section provider here is really useful? It might be more confusing than helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't think its a good idea tbh. Also definitely not required (for now anyway) so the minimum API would be better IMHO.
shaps80
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I'm not at all against this, but the current will/did calls were supposed to handle this. Just not via a closure. In fact your imp obvs just wraps those calls.
I picked the minimum implementation and thought its more helpful to not use closures.
Thoughts?
I have tried to move this "up the chain" to the top-level consumer (e.g. the My guess is that the indexes for changes get out of sync (since they're created in the update closures but don't account for further changes e.g. deletes). I think this might be a problem here, and appears to be the reason that If you can see a better way (that's not a large refactor to account for these changing index paths) to insert large numbers of section in a single batch (from the perspective of the collection view) then I'll work that alternative approach. |
|
You seem to have a better view of it currently. I'll trust your judgement. |
|
Closing this in favour of composed-swift/ComposedUI#15. |
When adding a lot of sections (we have a screen which inserts 100~175 on initial load) the performance is quite poor.
Along with the
improve-composed-section-provider-performancebranch (which I want to fully validate before making a PR) performing these changes in batch helps a lot.