From 169c0d6c5480a885b66e9d04dc584473e538d2a4 Mon Sep 17 00:00:00 2001 From: Ryan Nystrom Date: Tue, 13 Dec 2016 13:42:19 -0800 Subject: [PATCH] Prevent item reload, delete, insert collisions Summary: Adds a new test and fixes an item animation collision that `UICollectionView` throws on. Basically we cannot collide reloads with insert+delete. The example in the test is trivial, but real-world situations do appear b/c of the coalescence of `IGListAdapterUpdater`. It can queue reloads and deletes/inserts at the same index paths while waiting. - [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 have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md) Closes https://github.com/Instagram/IGListKit/pull/325 Differential Revision: D4322005 Pulled By: rnystrom fbshipit-source-id: 2247163c13c0873a74ac485efff2d882ca6568f9 --- CHANGELOG.md | 5 +++ Source/IGListAdapter.m | 25 ++++++++++- Tests/IGListAdapterE2ETests.m | 81 +++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fae013ec..38741145 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,8 +8,13 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag This release closes the [2.1.0 milestone](https://github.com/Instagram/IGListKit/milestone/2). ### Enhancements + - Disables `prefetchEnabled` by default on `IGListCollectionView`. [Sven Bacia (#323)](https://github.com/Instagram/IGListKit/pull/323) +### Fixes + +- Avoid `UICollectionView` crashes when queueing a reload and insert/delete on the same item as well as reloading an item in a section that is animating. [Ryan Nystrom](https://github.com/rnystrom) [(#325)](https://github.com/Instagram/IGListKit/pull/325) + 2.0.0 ----- diff --git a/Source/IGListAdapter.m b/Source/IGListAdapter.m index 168284d4..5bda8c1f 100644 --- a/Source/IGListAdapter.m +++ b/Source/IGListAdapter.m @@ -893,8 +893,29 @@ return; } - NSArray *indexPaths = [self indexPathsFromSectionController:sectionController indexes:indexes adjustForUpdateBlock:YES]; - [self.updatingDelegate reloadItemsInCollectionView:collectionView indexPaths:indexPaths]; + if (self.isInUpdateBlock) { + /** + UICollectionView is not designed to support -reloadSections: or -reloadItemsAtIndexPaths: during batch updates. + Internally it appears to convert these operations to a delete+insert. However the transformation is too simple + in that it doesn't account for the item's section being moved (naturally or explicitly) and can queue animation + collisions. + + If you have an object at section 2 with 4 items and attempt to reload item at index 1, you would create an + NSIndexPath at section: 2, item: 1. Within -performBatchUpdates:, UICollectionView converts this to a delete + and insert at the same NSIndexPath. + + If a section were inserted at position 2, the original section 2 has naturally shifted to section 3. However, + the insert NSIndexPath is section: 2, item: 1. Now the UICollectionView has a section animation at section 2, + as well as an item insert animation at section: 2, item: 1, and it will throw an exception. + + IGListAdapter tracks the before/after mapping of section controllers to make precise NSIndexPath conversions. + */ + [self deleteInSectionController:sectionController atIndexes:indexes]; + [self insertInSectionController:sectionController atIndexes:indexes]; + } else { + NSArray *indexPaths = [self indexPathsFromSectionController:sectionController indexes:indexes adjustForUpdateBlock:YES]; + [self.updatingDelegate reloadItemsInCollectionView:collectionView indexPaths:indexPaths]; + } } - (void)insertInSectionController:(IGListSectionController *)sectionController atIndexes:(NSIndexSet *)indexes { diff --git a/Tests/IGListAdapterE2ETests.m b/Tests/IGListAdapterE2ETests.m index 3499af06..89cfd7f2 100644 --- a/Tests/IGListAdapterE2ETests.m +++ b/Tests/IGListAdapterE2ETests.m @@ -1055,4 +1055,85 @@ [self waitForExpectationsWithTimeout:15 handler:nil]; } +- (void)test_whenReloadingItems_withDeleteAndInsertCollision_thatUpdateCanBeApplied { + [self setupWithObjects:@[ + genTestObject(@1, @1), + genTestObject(@2, @5), + genTestObject(@3, @1), + ]]; + + IGListSectionController *section = [self.adapter sectionControllerForObject:self.dataSource.objects[1]]; + + XCTestExpectation *expectation = genExpectation; + [section.collectionContext performBatchAnimated:NO updates:^{ + [section.collectionContext deleteInSectionController:section atIndexes:[NSIndexSet indexSetWithIndexesInRange:NSMakeRange(1, 2)]]; + [section.collectionContext insertInSectionController:section atIndexes:[NSIndexSet indexSetWithIndexesInRange:NSMakeRange(1, 2)]]; + [section.collectionContext reloadInSectionController:section atIndexes:[NSIndexSet indexSetWithIndexesInRange:NSMakeRange(1, 4)]]; + } completion:^(BOOL finished) { + [expectation fulfill]; + }]; + + [self waitForExpectationsWithTimeout:15 handler:nil]; +} + +- (void)test_whenReloadingItems_withSectionInsertedInFront_thatUpdateCanBeApplied { + [self setupWithObjects:@[ + genTestObject(@1, @1), + genTestObject(@2, @5), + genTestObject(@3, @1), + ]]; + + IGListSectionController *section = [self.adapter sectionControllerForObject:self.dataSource.objects[1]]; + + XCTestExpectation *expectation1 = genExpectation; + [section.collectionContext performBatchAnimated:NO updates:^{ + [section.collectionContext reloadInSectionController:section atIndexes:[NSIndexSet indexSetWithIndexesInRange:NSMakeRange(1, 4)]]; + } completion:^(BOOL finished) { + [expectation1 fulfill]; + }]; + + self.dataSource.objects = @[ + genTestObject(@1, @1), + genTestObject(@4, @1), // insert to shift object @2 + genTestObject(@2, @5), + genTestObject(@3, @1), + ]; + + XCTestExpectation *expectation2 = genExpectation; + [self.adapter performUpdatesAnimated:YES completion:^(BOOL finished) { + [expectation2 fulfill]; + }]; + + [self waitForExpectationsWithTimeout:15 handler:nil]; +} + +- (void)test_whenReloadingItems_withSectionDeletedInFront_thatUpdateCanBeApplied { + [self setupWithObjects:@[ + genTestObject(@1, @1), + genTestObject(@2, @5), + genTestObject(@3, @1), + ]]; + + IGListSectionController *section = [self.adapter sectionControllerForObject:self.dataSource.objects[1]]; + + XCTestExpectation *expectation1 = genExpectation; + [section.collectionContext performBatchAnimated:NO updates:^{ + [section.collectionContext reloadInSectionController:section atIndexes:[NSIndexSet indexSetWithIndexesInRange:NSMakeRange(1, 4)]]; + } completion:^(BOOL finished) { + [expectation1 fulfill]; + }]; + + self.dataSource.objects = @[ + genTestObject(@2, @5), + genTestObject(@3, @1), + ]; + + XCTestExpectation *expectation2 = genExpectation; + [self.adapter performUpdatesAnimated:YES completion:^(BOOL finished) { + [expectation2 fulfill]; + }]; + + [self waitForExpectationsWithTimeout:15 handler:nil]; +} + @end