From e6032c81074db63a209c7ef9d7f5712389606d67 Mon Sep 17 00:00:00 2001 From: Zhisheng Huang Date: Wed, 5 Sep 2018 14:02:09 -0700 Subject: [PATCH] Resolve crash when using preferItemReloadsForSectionReloads and there is section movements Summary: section moves are extremely unsafe and crashy. If we still use the `preferItemReloadsForSectionReloads` on, the item reloads won't be correct when we applies the same logic for cases without section moves. It leads to crash: ``` 2018-09-04 23:28:12.184875-0700 xctest[51440:12103918] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Invalid update: invalid number of items in section 0. The number of items contained in an existing section after the update (1) must be equal to the number of items contained in that section before the update (1), plus or minus the number of items inserted or deleted from that section (0 inserted, 1 deleted) and plus or minus the number of items moved into or out of that section (0 moved in, 0 moved out).' ``` even if we change to use -reloadSections: instead, we still get crash like the following: ``` 2018-09-04 23:21:14.129961-0700 Direct[45654:12078189] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'attempt to perform a delete and a move from the same section (25)' ``` Thus let's only add the moves.count == 0 check before using the prefer item reloads over section reload optimization. Reviewed By: calimarkus Differential Revision: D9661723 fbshipit-source-id: 8c3ddcd5178cb1aa58ccd9ae274cd8e5198f7609 --- Source/IGListAdapterUpdater.m | 3 +- Tests/IGListAdapterUpdaterTests.m | 137 ++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 1 deletion(-) diff --git a/Source/IGListAdapterUpdater.m b/Source/IGListAdapterUpdater.m index 9adf6027..37c2be79 100644 --- a/Source/IGListAdapterUpdater.m +++ b/Source/IGListAdapterUpdater.m @@ -327,7 +327,8 @@ static NSArray *convertSectionReloadToItemUpdates(NSIndexSet *sec moves = [NSSet new]; } - if (self.preferItemReloadsForSectionReloads) { + // Item reloads are not safe, if any section moves happened + if (moves.count == 0 && self.preferItemReloadsForSectionReloads) { [reloads enumerateIndexesUsingBlock:^(NSUInteger sectionIndex, BOOL * _Nonnull stop) { NSMutableIndexSet *localIndexSet = [NSMutableIndexSet indexSetWithIndex:sectionIndex]; if (sectionIndex < [collectionView numberOfSections] diff --git a/Tests/IGListAdapterUpdaterTests.m b/Tests/IGListAdapterUpdaterTests.m index 113ed377..b492e108 100644 --- a/Tests/IGListAdapterUpdaterTests.m +++ b/Tests/IGListAdapterUpdaterTests.m @@ -13,6 +13,7 @@ #import "IGListAdapterUpdaterInternal.h" #import "IGListTestUICollectionViewDataSource.h" #import "IGTestObject.h" +#import "IGListMoveIndexInternal.h" #define genExpectation [self expectationWithDescription:NSStringFromSelector(_cmd)] #define waitExpectation [self waitForExpectationsWithTimeout:30 handler:nil] @@ -689,6 +690,8 @@ [mockDelegate verify]; } +# pragma mark - preferItemReloadsFroSectionReloads + - (void)test_whenReloadIsCalledWithSameItemCount_andPreferItemReload_updateIndexPathsHappen { self.updater.preferItemReloadsForSectionReloads = YES; @@ -753,4 +756,138 @@ [mockDelegate verify]; } +- (void)test_whenReloadIsCalledWithSectionMoveAndUpdate_andPreferItemReload_deleteInsertMoveHappens { + self.updater.preferItemReloadsForSectionReloads = YES; + + IGListBatchUpdateData *expectedBatchUpdateData = [[IGListBatchUpdateData alloc] initWithInsertSections:[NSIndexSet indexSetWithIndex:0] + deleteSections:[NSIndexSet indexSetWithIndex:1] + moveSections:[NSSet setWithArray:@[[[IGListMoveIndex alloc] initWithFrom:0 to:1]]] + insertIndexPaths:@[] + deleteIndexPaths:@[] + updateIndexPaths:@[] + moveIndexPaths:@[]]; + id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)]; + self.updater.delegate = mockDelegate; + [mockDelegate setExpectationOrderMatters:YES]; + [[mockDelegate expect] listAdapterUpdater:self.updater willPerformBatchUpdatesWithCollectionView:self.collectionView]; + [[mockDelegate expect] listAdapterUpdater:self.updater didPerformBatchUpdates:expectedBatchUpdateData collectionView:self.collectionView]; + + XCTestExpectation *expectation = genExpectation; + NSArray *from = @[[IGSectionObject sectionWithObjects:@[@1] identifier:@"id1"], + [IGSectionObject sectionWithObjects:@[@2] identifier:@"id2"]]; + IGListToObjectBlock to = ^NSArray *{ + // move section, and also update the item for "id2" + return @[[IGSectionObject sectionWithObjects:@[@22] identifier:@"id2"], + [IGSectionObject sectionWithObjects:@[@1] identifier:@"id1"]]; + }; + self.dataSource.sections = from; + + [self.updater performUpdateWithCollectionViewBlock:[self collectionViewBlock] fromObjects:from toObjectsBlock:to animated:NO objectTransitionBlock:self.updateBlock completion:^(BOOL finished) { + [expectation fulfill]; + }]; + waitExpectation; + [mockDelegate verify]; +} + +- (void)test_whenReloadIsCalledWithSectionMoveAndUpdate_withDifferentSectionLength_andPreferItemReload_deleteInsertMoveHappens { + self.updater.preferItemReloadsForSectionReloads = YES; + + IGListBatchUpdateData *expectedBatchUpdateData = [[IGListBatchUpdateData alloc] initWithInsertSections:[NSIndexSet indexSetWithIndex:0] + deleteSections:[NSIndexSet indexSetWithIndex:1] + moveSections:[NSSet setWithArray:@[[[IGListMoveIndex alloc] initWithFrom:0 to:1]]] + insertIndexPaths:@[] + deleteIndexPaths:@[] + updateIndexPaths:@[] + moveIndexPaths:@[]]; + id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)]; + self.updater.delegate = mockDelegate; + [mockDelegate setExpectationOrderMatters:YES]; + [[mockDelegate expect] listAdapterUpdater:self.updater willPerformBatchUpdatesWithCollectionView:self.collectionView]; + [[mockDelegate expect] listAdapterUpdater:self.updater didPerformBatchUpdates:expectedBatchUpdateData collectionView:self.collectionView]; + + XCTestExpectation *expectation = genExpectation; + NSArray *from = @[[IGSectionObject sectionWithObjects:@[@1, @2, @3] identifier:@"id1"], + [IGSectionObject sectionWithObjects:@[@2] identifier:@"id2"]]; + IGListToObjectBlock to = ^NSArray *{ + // move section, and also update the item for "id2" + return @[[IGSectionObject sectionWithObjects:@[@22] identifier:@"id2"], + [IGSectionObject sectionWithObjects:@[@1, @2, @3] identifier:@"id1"]]; + }; + self.dataSource.sections = from; + + [self.updater performUpdateWithCollectionViewBlock:[self collectionViewBlock] fromObjects:from toObjectsBlock:to animated:NO objectTransitionBlock:self.updateBlock completion:^(BOOL finished) { + [expectation fulfill]; + }]; + waitExpectation; + [mockDelegate verify]; +} + + +- (void)test_whenReloadIsCalledWithSectionMoveAndUpdate_withThreeSections_deleteInsertMoveHappens { + self.updater.preferItemReloadsForSectionReloads = YES; + + IGListBatchUpdateData *expectedBatchUpdateData = [[IGListBatchUpdateData alloc] initWithInsertSections:[NSIndexSet indexSetWithIndex:0] + deleteSections:[NSIndexSet indexSetWithIndex:1] + moveSections:[NSSet setWithArray:@[[[IGListMoveIndex alloc] initWithFrom:0 to:1]]] + insertIndexPaths:@[] + deleteIndexPaths:@[] + updateIndexPaths:@[] + moveIndexPaths:@[]]; + id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)]; + self.updater.delegate = mockDelegate; + [mockDelegate setExpectationOrderMatters:YES]; + [[mockDelegate expect] listAdapterUpdater:self.updater willPerformBatchUpdatesWithCollectionView:self.collectionView]; + [[mockDelegate expect] listAdapterUpdater:self.updater didPerformBatchUpdates:expectedBatchUpdateData collectionView:self.collectionView]; + + XCTestExpectation *expectation = genExpectation; + NSArray *from = @[[IGSectionObject sectionWithObjects:@[@1] identifier:@"id1"], + [IGSectionObject sectionWithObjects:@[@2] identifier:@"id2"], + [IGSectionObject sectionWithObjects:@[@3] identifier:@"id3"]]; + IGListToObjectBlock to = ^NSArray *{ + // move section, and also update the items for "id2" + return @[[IGSectionObject sectionWithObjects:@[@22, @23] identifier:@"id2"], + [IGSectionObject sectionWithObjects:@[@1] identifier:@"id1"], + [IGSectionObject sectionWithObjects:@[@3] identifier:@"id3"]]; + }; + self.dataSource.sections = from; + + [self.updater performUpdateWithCollectionViewBlock:[self collectionViewBlock] fromObjects:from toObjectsBlock:to animated:NO objectTransitionBlock:self.updateBlock completion:^(BOOL finished) { + [expectation fulfill]; + }]; + waitExpectation; + [mockDelegate verify]; +} + +- (void)test_whenReloadIsCalledWithSectionInsertAndUpdate_andPreferItemReload_orderHappened { + self.updater.preferItemReloadsForSectionReloads = YES; + + IGListBatchUpdateData *expectedBatchUpdateData = [[IGListBatchUpdateData alloc] initWithInsertSections:[NSIndexSet indexSetWithIndex:1] + deleteSections:[NSIndexSet new] + moveSections:[NSSet new] + insertIndexPaths:@[] + deleteIndexPaths:@[] + updateIndexPaths:@[[NSIndexPath indexPathForItem:0 inSection:0]] + moveIndexPaths:@[]]; + id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)]; + self.updater.delegate = mockDelegate; + [mockDelegate setExpectationOrderMatters:YES]; + [[mockDelegate expect] listAdapterUpdater:self.updater willPerformBatchUpdatesWithCollectionView:self.collectionView]; + [[mockDelegate expect] listAdapterUpdater:self.updater didPerformBatchUpdates:expectedBatchUpdateData collectionView:self.collectionView]; + + XCTestExpectation *expectation = genExpectation; + NSArray *from = @[[IGSectionObject sectionWithObjects:@[@1] identifier:@"id1"]]; + IGListToObjectBlock to = ^NSArray *{ + // insert new section id2, and also update for section id1 + return @[[IGSectionObject sectionWithObjects:@[@2] identifier:@"id1"], + [IGSectionObject sectionWithObjects:@[@22] identifier:@"id2"]]; + }; + self.dataSource.sections = from; + + [self.updater performUpdateWithCollectionViewBlock:[self collectionViewBlock] fromObjects:from toObjectsBlock:to animated:NO objectTransitionBlock:self.updateBlock completion:^(BOOL finished) { + [expectation fulfill]; + }]; + waitExpectation; + [mockDelegate verify]; +} + @end