Skip to content

Commit

Permalink
fix IGListAdapterUpdaterDelegate
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@maxolls @facebook-github-bot
maxolls authored and facebook-github-bot committed Jun 26, 2020
1 parent 4cc65c6 commit 29bf582
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 52 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
69 changes: 44 additions & 25 deletions Source/IGListKit/IGListAdapterUpdater.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ - (void)performReloadDataWithCollectionViewBlock:(IGListCollectionViewBlock)coll
if (collectionView == nil) {
[self _cleanStateAfterUpdates];
executeCompletionBlocks(NO);
[_delegate listAdapterUpdater:self didFinishWithoutUpdatesWithCollectionView:collectionView];
return;
}

Expand All @@ -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);
}
Expand Down Expand Up @@ -128,6 +129,7 @@ - (void)performBatchUpdatesWithCollectionViewBlock:(IGListCollectionViewBlock)co
if (collectionView == nil) {
[self _cleanStateAfterUpdates];
executeCompletionBlocks(NO);
[_delegate listAdapterUpdater:self didFinishWithoutUpdatesWithCollectionView:collectionView];
return;
}

Expand Down Expand Up @@ -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) = ^{
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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);
}
}

Expand Down
14 changes: 12 additions & 2 deletions Source/IGListKit/IGListAdapterUpdaterDelegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:]`.
Expand All @@ -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
39 changes: 14 additions & 25 deletions Tests/IGListAdapterUpdaterTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
Expand Down Expand Up @@ -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 *{
Expand All @@ -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 *{
Expand Down Expand Up @@ -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;

Expand Down

0 comments on commit 29bf582

Please # to comment.