-
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 duplicate item deletes and drop reload collisions #657
Conversation
@rnystrom updated the pull request - view changes |
Generated by 🚫 Danger |
@rnystrom updated the pull request - view changes |
Test failed b/c of timeout |
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.
a few minor nits, but looks good to me!
🎉 🙌 🥇
CHANGELOG.md
Outdated
@@ -58,6 +58,8 @@ This release closes the [3.0.0 milestone](https://github.com/Instagram/IGListKit | |||
|
|||
- `IGListBatchUpdateData` replaced its `NSSet` properties with `NSArray` instead. [Ryan Nystrom](https://github.com/rnystrom) [(#616)](https://github.com/Instagram/IGListKit/pull/616) | |||
|
|||
- `IGListUpdatingDelegate` now has a required method to handle reloading cells between index paths. [Ryan Nystrom](https://github.com/rnystrom) [(#657)](https://github.com/Instagram/IGListKit/pull/657) |
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.
let's note the method
const auto it = map.find(path.section); | ||
if (it != map.end() && it->second != nil) { | ||
[indexPaths removeObject:path]; | ||
[indexPaths removeObjectAtIndex:i]; |
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.
uh.. 🤔 is this mutating while enumerating?
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.
Ya, but reversed the enum so mutations happen at the tail.
*/ | ||
- (void)reloadItemInCollectionView:(UICollectionView *)collectionView | ||
fromIndexPath:(NSIndexPath *)fromIndexPath | ||
toIndexPath:(NSIndexPath *)toIndexPath; |
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.
🎉
*/ | ||
+ (instancetype)new NS_UNAVAILABLE; | ||
|
||
@end |
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.
add nullability macros?
#import <IGListKit/IGListMacros.h> | ||
|
||
IGLK_SUBCLASSING_RESTRICTED | ||
@interface IGListReloadIndexPath : NSObject |
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.
need docs for class
I don't completely understand how this won't cause other items to be deleted within a section that has duplicate deletes for the same index path. |
@@ -29,16 +29,45 @@ static void convertMoveToDeleteAndInsert(NSMutableSet<IGListMoveIndex *> *moves, | |||
|
|||
@implementation IGListBatchUpdateData | |||
|
|||
/** | |||
Finds duplicate index paths and increments increments the item index so eacy operation is unique. Duplicate delete |
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.
double increments
NSIndexPath *nextPath = [NSIndexPath indexPathForItem:path.item + count inSection:path.section]; | ||
NSIndexPath *staggeredPath = staggeredIndexPath(dupes, nextPath); | ||
[dupes addObject:staggeredPath]; | ||
return staggeredPath; |
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 method is a bit confusing to read due to the fact that the return value isn't just the new index path (which happens to be the last one generated, i think) but also the implicit change to dupes
. i'd prefer this to return a struct that returns the NSCountedSet
and NSIndexPath
as elements, even if that set is the same reference as what was passed in.
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.
👍 will add some docs to this.
I think the struct will get really messy w/ the recursion tho
Tests/IGListBatchUpdateDataTests.m
Outdated
moveIndexPaths:@[]]; | ||
XCTAssertEqual(result.deleteIndexPaths.count, 4); | ||
|
||
NSArray *expected = @[newPath(2, 0), newPath(2, 1), newPath(2, 3), newPath(2, 4)]; |
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.
while i understand this matches the logic, won't this end up causing the data source to delete the path (2,3) even though that wasn't an index path that should be deleted?
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.
Ya, but I think that'll be ok b/c they were telling the datasource to delete {2,0} and {2,0} and updating their data source to reflect 2 deletions. So we have to delete 2 things, otherwise UICV will throw. And UICV will bug if we delete {2,0} 2x, so instead {2,0} {2,1} is the next-best thing.
Tho I think there might be a bug here, I think the expectation should be
NSArray *expected = @[newPath(2, 0), newPath(2, 1), newPath(2, 2), newPath(2, 4)];
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.
Made me double-check how this works. There's a bug that if you delete the last item twice, it will throw b/c its deleting an item that doesn't exist...
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.
What about (2,2)? Is it going to be deleted? Won't that be an internal inconsistency since it really wasn't deleted and there is no corresponding insert to ensure that the correct number of items is in the end?
Ok backing out my "stagger" idea b/c if you delete the last item twice you will delete something that doesn't exist. Maybe we could change the API so it validates that deletes are within the range of Changing deletes back to |
Eating my words, I want to fix this for real. Going to need to get clever. Dang u collection view. Here are our options as I see it:
So I'm thinking maybe we keep the staggering stuff, and if it errors it errors? Maybe a follow up is adding our own error and exception handling to be more helpful than the UICV exception nonsense? cc @jessesquires @amonshiz for thoughts |
could we just bounds-check the "double delete" case and ignore the second delete? |
@jessesquires If we drop one of the deletes but a data source is updated assuming 2 deletes are applied, UICV will throw for inconsistency. |
Ok stopgap solution for now:
We've survived shipping IGListKit on Instagram for over a year w/out this, so we're in no hurry to get this totally working. However w/out a fix this is crashing now and causing tests to fail. Let's get this cleaned up and working for now. |
135e624
to
180ff76
Compare
@rnystrom updated the pull request - view changes |
Importing to get internal CI running on this, not landing |
@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Changes in this pull request
a415ef5 exposed a bug in
UICollectionView
where its state gets corrupted when deleting the same index path more than once in a single batch update block. This resulted crashes likeAnd bizarre ones like
This PR prevents that crash and fixes the flakiness of test:
I originally fixed the crash by "staggering" duped delete index paths (see function in
IGListBatchUpdateData
), but that revealed another bug when reloads (which are converted to delete+insert) collide w/ actual deletes. In that case we want to drop the reload entirely. Fixing this required introducing another data structure,IGListReloadIndexPath
.Issue fixed: #651
Internal tasks t17306420, t17422443
Checklist
CHANGELOG.md
for any breaking changes, enhancements, or bug fixes.