From 29bf582f479ddf6beb118abc83ba7a8ea87543b0 Mon Sep 17 00:00:00 2001 From: Maxime Ollivier Date: Fri, 26 Jun 2020 10:03:47 -0700 Subject: [PATCH] fix IGListAdapterUpdaterDelegate Summary: Issue: There's a couple of situations where `IGListAdapterUpdater` updates the `collectionView` without notifying the `IGListAdapterUpdater`. * If we call `reloadDataFallback()` because of a missing window, there's no delegate calls. * If we call `reloadDataFallback()` because of too many diff updates, we call `willPerformBatchUpdatesWithCollectionView` but not `didPerformBatchUpdates`. Fix: Lets clean up the delegate calls * If we `[UICollectioView performBatchUpdates:]` lets call `willPerformBatchUpdatesWithCollectionView` & `didPerformBatchUpdates` * If we fallback to `[UICollectionView reloadData]` lets call `willReloadDataWithCollectionView` & `didReloadDataWithCollectionView` * If we fallback to doing nothing, lets call `didFinishWithoutUpdates` Reviewed By: Haud Differential Revision: D22219236 fbshipit-source-id: 1afb4df8dbbaf4725424027bb52c1a5cc5100b30 --- CHANGELOG.md | 2 + Source/IGListKit/IGListAdapterUpdater.m | 69 ++++++++++++------- .../IGListKit/IGListAdapterUpdaterDelegate.h | 14 +++- Tests/IGListAdapterUpdaterTests.m | 39 ++++------- 4 files changed, 72 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 588d7c513..b8893e493 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,8 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag - Remove `[collectionView layoutIfNeeded]` before scrolling in `[IGListAdapter scrollToObject...]` to avoid creating off-screen cells. [Maxime Ollivier](https://github.com/maxolls) (tbd) +- Fixed `IGListAdapterUpdaterDelegate` by 1) calling `willReloadDataWithCollectionView` on fallback reloads and 2) making sure `willPerformBatchUpdatesWithCollectionView` is only called when performing a batch update. [Maxime Ollivier](https://github.com/maxolls) (tbd) + 4.0.0 ----- ### Breaking Changes diff --git a/Source/IGListKit/IGListAdapterUpdater.m b/Source/IGListKit/IGListAdapterUpdater.m index d533bf1d0..dd0511bde 100644 --- a/Source/IGListKit/IGListAdapterUpdater.m +++ b/Source/IGListKit/IGListAdapterUpdater.m @@ -64,6 +64,7 @@ - (void)performReloadDataWithCollectionViewBlock:(IGListCollectionViewBlock)coll if (collectionView == nil) { [self _cleanStateAfterUpdates]; executeCompletionBlocks(NO); + [_delegate listAdapterUpdater:self didFinishWithoutUpdatesWithCollectionView:collectionView]; return; } @@ -89,11 +90,11 @@ - (void)performReloadDataWithCollectionViewBlock:(IGListCollectionViewBlock)coll [self _cleanStateAfterUpdates]; - [delegate listAdapterUpdater:self willReloadDataWithCollectionView:collectionView]; + [delegate listAdapterUpdater:self willReloadDataWithCollectionView:collectionView isFallbackReload:NO]; [collectionView reloadData]; [collectionView.collectionViewLayout invalidateLayout]; [collectionView layoutIfNeeded]; - [delegate listAdapterUpdater:self didReloadDataWithCollectionView:collectionView]; + [delegate listAdapterUpdater:self didReloadDataWithCollectionView:collectionView isFallbackReload:NO]; executeCompletionBlocks(YES); } @@ -128,6 +129,7 @@ - (void)performBatchUpdatesWithCollectionViewBlock:(IGListCollectionViewBlock)co if (collectionView == nil) { [self _cleanStateAfterUpdates]; executeCompletionBlocks(NO); + [_delegate listAdapterUpdater:self didFinishWithoutUpdatesWithCollectionView:collectionView]; return; } @@ -169,26 +171,26 @@ - (void)performBatchUpdatesWithCollectionViewBlock:(IGListCollectionViewBlock)co }; void (^reloadDataFallback)(void) = ^{ + [delegate listAdapterUpdater:self willReloadDataWithCollectionView:collectionView isFallbackReload:YES]; executeUpdateBlocks(); [self _cleanStateAfterUpdates]; [self _performBatchUpdatesItemBlockApplied]; [collectionView reloadData]; [collectionView layoutIfNeeded]; - executeCompletionBlocks(YES); + [delegate listAdapterUpdater:self didReloadDataWithCollectionView:collectionView isFallbackReload:YES]; }; + // disables multiple performBatchUpdates: from happening at the same time + [self _beginPerformBatchUpdatesToObjects:toObjects]; + // if the collection view isn't in a visible window, skip diffing and batch updating. execute all transition blocks, // reload data, execute completion blocks, and get outta here if (self.allowsBackgroundReloading && collectionView.window == nil) { - [self _beginPerformBatchUpdatesToObjects:toObjects]; reloadDataFallback(); return; } - // disables multiple performBatchUpdates: from happening at the same time - [self _beginPerformBatchUpdatesToObjects:toObjects]; - const IGListExperiment experiments = self.experiments; IGListIndexSetResult *(^performDiff)(void) = ^{ @@ -228,6 +230,17 @@ - (void)performBatchUpdatesWithCollectionViewBlock:(IGListCollectionViewBlock)co [self _performBatchUpdatesItemBlockApplied]; }; + // block used as the second param of -[UICollectionView performBatchUpdates:completion:] + void (^fallbackWithoutUpdates)(void) = ^(void) { + executeCompletionBlocks(NO); + + [delegate listAdapterUpdater:self didFinishWithoutUpdatesWithCollectionView:collectionView]; + + // queue another update in case something changed during batch updates. this method will bail next runloop if + // there are no changes + [self _queueUpdateWithCollectionViewBlock:collectionViewBlock]; + }; + // block used as the second param of -[UICollectionView performBatchUpdates:completion:] void (^batchUpdatesCompletionBlock)(BOOL) = ^(BOOL finished) { IGListBatchUpdateData *oldApplyingUpdateData = self.applyingUpdateData; @@ -240,31 +253,37 @@ - (void)performBatchUpdatesWithCollectionViewBlock:(IGListCollectionViewBlock)co [self _queueUpdateWithCollectionViewBlock:collectionViewBlock]; }; - // block that executes the batch update and exception handling void (^performUpdate)(IGListIndexSetResult *) = ^(IGListIndexSetResult *result){ + [delegate listAdapterUpdater:self +willPerformBatchUpdatesWithCollectionView:collectionView + fromObjects:fromObjects + toObjects:toObjects + listIndexSetResult:result]; + if (animated) { + [collectionView performBatchUpdates:^{ + batchUpdatesBlock(result); + } completion:batchUpdatesCompletionBlock]; + } else { + [UIView performWithoutAnimation:^{ + [collectionView performBatchUpdates:^{ + batchUpdatesBlock(result); + } completion:batchUpdatesCompletionBlock]; + }]; + } + }; + + // block that executes the batch update and exception handling + void (^tryToPerformUpdate)(IGListIndexSetResult *) = ^(IGListIndexSetResult *result){ [collectionView layoutIfNeeded]; @try { - [delegate listAdapterUpdater:self -willPerformBatchUpdatesWithCollectionView:collectionView - fromObjects:fromObjects - toObjects:toObjects - listIndexSetResult:result]; if (collectionView.dataSource == nil) { // If the data source is nil, we should not call any collection view update. - batchUpdatesCompletionBlock(NO); + fallbackWithoutUpdates(); } else if (result.changeCount > 100 && IGListExperimentEnabled(experiments, IGListExperimentReloadDataFallback)) { reloadDataFallback(); - } else if (animated) { - [collectionView performBatchUpdates:^{ - batchUpdatesBlock(result); - } completion:batchUpdatesCompletionBlock]; } else { - [UIView performWithoutAnimation:^{ - [collectionView performBatchUpdates:^{ - batchUpdatesBlock(result); - } completion:batchUpdatesCompletionBlock]; - }]; + performUpdate(result); } } @catch (NSException *exception) { [delegate listAdapterUpdater:self @@ -282,12 +301,12 @@ - (void)performBatchUpdatesWithCollectionViewBlock:(IGListCollectionViewBlock)co dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ IGListIndexSetResult *result = performDiff(); dispatch_async(dispatch_get_main_queue(), ^{ - performUpdate(result); + tryToPerformUpdate(result); }); }); } else { IGListIndexSetResult *result = performDiff(); - performUpdate(result); + tryToPerformUpdate(result); } } diff --git a/Source/IGListKit/IGListAdapterUpdaterDelegate.h b/Source/IGListKit/IGListAdapterUpdaterDelegate.h index 9e191b055..e4e7d6953 100644 --- a/Source/IGListKit/IGListAdapterUpdaterDelegate.h +++ b/Source/IGListKit/IGListAdapterUpdaterDelegate.h @@ -121,16 +121,18 @@ willPerformBatchUpdatesWithCollectionView:(UICollectionView *)collectionView @param listAdapterUpdater The adapter updater owning the transition. @param collectionView The collection view that will be reloaded. + @param isFallbackReload The reload was a fallback because we could not performBatchUpdate */ -- (void)listAdapterUpdater:(IGListAdapterUpdater *)listAdapterUpdater willReloadDataWithCollectionView:(UICollectionView *)collectionView; +- (void)listAdapterUpdater:(IGListAdapterUpdater *)listAdapterUpdater willReloadDataWithCollectionView:(UICollectionView *)collectionView isFallbackReload:(BOOL)isFallbackReload; /** Notifies the delegate that the updater successfully called `-[UICollectionView reloadData]`. @param listAdapterUpdater The adapter updater owning the transition. @param collectionView The collection view that reloaded. + @param isFallbackReload The reload was a fallback because we could not performBatchUpdate */ -- (void)listAdapterUpdater:(IGListAdapterUpdater *)listAdapterUpdater didReloadDataWithCollectionView:(UICollectionView *)collectionView; +- (void)listAdapterUpdater:(IGListAdapterUpdater *)listAdapterUpdater didReloadDataWithCollectionView:(UICollectionView *)collectionView isFallbackReload:(BOOL)isFallbackReload; /** Notifies the delegate that the collection view threw an exception in `-[UICollectionView performBatchUpdates:completion:]`. @@ -151,6 +153,14 @@ willPerformBatchUpdatesWithCollectionView:(UICollectionView *)collectionView diffResult:(IGListIndexSetResult *)diffResult updates:(IGListBatchUpdateData *)updates; +/** +Notifies the delegate that the updater finished without performing any batch updates or reloads + +@param listAdapterUpdater The adapter updater owning the transition. +@param collectionView The collection view that reloaded. +*/ +- (void)listAdapterUpdater:(IGListAdapterUpdater *)listAdapterUpdater didFinishWithoutUpdatesWithCollectionView:(nullable UICollectionView *)collectionView; + @end NS_ASSUME_NONNULL_END diff --git a/Tests/IGListAdapterUpdaterTests.m b/Tests/IGListAdapterUpdaterTests.m index 551dd6d58..afce3dad6 100644 --- a/Tests/IGListAdapterUpdaterTests.m +++ b/Tests/IGListAdapterUpdaterTests.m @@ -435,22 +435,20 @@ - (void)test_whenReloadingSection_whenSectionRemoved_thatConvertMethodCorrects { XCTAssertEqual(inserts.count, 0); } -- (void)test_whenReloadingData_withNilCollectionView_thatDelegateEventNotSent { +- (void)test_whenReloadingData_withNilCollectionView_thatDelegateFinishesWithoutUpdates { id mockDelegate = [OCMockObject mockForProtocol:@protocol(IGListAdapterUpdaterDelegate)]; self.updater.delegate = mockDelegate; id compilerFriendlyNil = nil; - [[mockDelegate reject] listAdapterUpdater:self.updater willReloadDataWithCollectionView:compilerFriendlyNil]; - [[mockDelegate reject] listAdapterUpdater:self.updater didReloadDataWithCollectionView:compilerFriendlyNil]; + [[mockDelegate expect] listAdapterUpdater:self.updater didFinishWithoutUpdatesWithCollectionView:nil]; [self.updater performReloadDataWithCollectionViewBlock:^UICollectionView *{ return compilerFriendlyNil; }]; [mockDelegate verify]; } -- (void)test_whenPerformingUpdates_withNilCollectionView_thatDelegateEventNotSent { +- (void)test_whenPerformingUpdates_withNilCollectionView_thatDelegateFinishesWithoutUpdates { id mockDelegate = [OCMockObject mockForProtocol:@protocol(IGListAdapterUpdaterDelegate)]; self.updater.delegate = mockDelegate; id compilerFriendlyNil = nil; - [[mockDelegate reject] listAdapterUpdater:self.updater willPerformBatchUpdatesWithCollectionView:compilerFriendlyNil fromObjects:@[] toObjects:@[] listIndexSetResult:OCMOCK_ANY]; - [[mockDelegate reject] listAdapterUpdater:self.updater didPerformBatchUpdates:[OCMArg any] collectionView:compilerFriendlyNil]; + [[mockDelegate expect] listAdapterUpdater:self.updater didFinishWithoutUpdatesWithCollectionView:nil]; [self.updater performBatchUpdatesWithCollectionViewBlock:^UICollectionView *{ return compilerFriendlyNil; }]; [mockDelegate verify]; } @@ -517,19 +515,15 @@ - (void)test_whenCollectionViewNotInWindow_andBackgroundReloadFlag_isSetNO_diffH [mockDelegate verify]; } -- (void)test_whenCollectionViewNotInWindow_andBackgroundReloadFlag_isDefaultYES_diffDoesNotHappen { +- (void)test_whenCollectionViewNotInWindow_andBackgroundReloadFlag_isDefaultYES_fallbackToReload { [self.collectionView removeFromSuperview]; id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)]; self.updater.delegate = mockDelegate; - // NOTE: The current behavior in this case is for the adapter updater - // simply not to call any delegate methods at all. This may change - // in the future, but we configure the mock delegate to allow any call - // except the batch updates calls. - - [[mockDelegate reject] listAdapterUpdater:self.updater willPerformBatchUpdatesWithCollectionView:self.collectionView fromObjects:@[] toObjects:@[] listIndexSetResult:OCMOCK_ANY]; - [[mockDelegate reject] listAdapterUpdater:self.updater didPerformBatchUpdates:OCMOCK_ANY collectionView:self.collectionView]; + [mockDelegate setExpectationOrderMatters:YES]; + [[mockDelegate expect] listAdapterUpdater:self.updater willReloadDataWithCollectionView:self.collectionView isFallbackReload:YES]; + [[mockDelegate expect] listAdapterUpdater:self.updater didReloadDataWithCollectionView:self.collectionView isFallbackReload:YES]; XCTestExpectation *expectation = genExpectation; IGListToObjectBlock to = ^NSArray *{ @@ -544,13 +538,15 @@ - (void)test_whenCollectionViewNotInWindow_andBackgroundReloadFlag_isDefaultYES_ [mockDelegate verify]; } -- (void)test_whenCollectionViewNotInWindow_andBackgroundReloadFlag_isDefaultYES_andDataSourceWasSetToNilBefore_thatCollectionViewNotCrash { +- (void)test_whenCollectionViewNotInWindow_andBackgroundReloadFlag_isDefaultYES_andDataSourceWasSetToNilBefore_fallbackToReload { [self.collectionView removeFromSuperview]; id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)]; self.updater.delegate = mockDelegate; - [[mockDelegate reject] listAdapterUpdater:self.updater willPerformBatchUpdatesWithCollectionView:self.collectionView fromObjects:@[] toObjects:@[] listIndexSetResult:OCMOCK_ANY]; - [[mockDelegate reject] listAdapterUpdater:self.updater didPerformBatchUpdates:OCMOCK_ANY collectionView:self.collectionView]; + + [mockDelegate setExpectationOrderMatters:YES]; + [[mockDelegate expect] listAdapterUpdater:self.updater willReloadDataWithCollectionView:self.collectionView isFallbackReload:YES]; + [[mockDelegate expect] listAdapterUpdater:self.updater didReloadDataWithCollectionView:self.collectionView isFallbackReload:YES]; XCTestExpectation *expectation = genExpectation; IGListToObjectBlock to = ^NSArray *{ @@ -731,14 +727,7 @@ - (void)test_whenPerformUpdates_dataSourceWasSetToNil_shouldNotCrash { id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)]; self.updater.delegate = mockDelegate; [mockDelegate setExpectationOrderMatters:YES]; - [[mockDelegate expect] listAdapterUpdater:self.updater willPerformBatchUpdatesWithCollectionView:self.collectionView fromObjects:from toObjects:to listIndexSetResult:[OCMArg checkWithBlock:^BOOL(IGListIndexSetResult *result) { - if (result.deletes.count != 0 || result.moves.count != 0) { - return NO; - } - // Make sure we note that index 1 is updated (id1 from @[@1] -> @[@2]), and "id2" was inserted at index 1 - return result.updates.firstIndex == 0 && result.inserts.firstIndex == 1; - }]]; - [[mockDelegate expect] listAdapterUpdater:self.updater didPerformBatchUpdates:OCMOCK_ANY collectionView:self.collectionView]; + [[mockDelegate expect] listAdapterUpdater:self.updater didFinishWithoutUpdatesWithCollectionView:self.collectionView]; XCTestExpectation *expectation = genExpectation;