-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Prevent a crash when inserting the same index twice #616
Conversation
@rnystrom updated the pull request - view changes |
moveIndexPaths:(NSSet<IGListMoveIndexPath *> *)moveIndexPaths NS_DESIGNATED_INITIALIZER; | ||
insertIndexPaths:(NSArray<NSIndexPath *> *)insertIndexPaths | ||
deleteIndexPaths:(NSArray<NSIndexPath *> *)deleteIndexPaths | ||
moveIndexPaths:(NSArray<IGListMoveIndexPath *> *)moveIndexPaths NS_DESIGNATED_INITIALIZER; |
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.
this class is public, right?
need a changelog for breaking changes?
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.
Oh weird, it is. But, why? This isn't supposed to be used on macOS.
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.
why's that? there's nothing iOS-specific about this class, right? isn't it also needed for diffing?
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.
Not needed for diffing, its like a "give me all your updates and I'll squish them". You're right it doesn't have anything iOS specific tho.
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.
🤷♂️
@rnystrom updated the pull request - view changes |
updated change log. going to import |
@jessesquires has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
cancelled import. something is broken |
Going to re-open this |
re-opened at #634 |
Summary: Original comment: If you insert or delete into the same index twice **and** update your data source to reflect those changes, we will squash the insert/delete into a single update because we're using `NSSet` internally. This becomes very apparent when multiple updates are coalesced. Issue fixed: #483 - [x] All tests pass. Demo project builds and runs. - [x] I added tests, an experiment, or detailed why my change isn't tested. - [x] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes. Closes #634 Differential Revision: D4860479 Pulled By: jessesquires fbshipit-source-id: 3aa271d90fca21b11201f62cefa8d7fbcef6930f
Changes in this pull request
If you insert or delete into the same index twice and update your data source to reflect those changes, we will squash the insert/delete into a single update because we're using
NSSet
internally.This becomes very apparent when multiple updates are coalesced.
Issue fixed: #483
Checklist
CHANGELOG.md
for any breaking changes, enhancements, or bug fixes.