Elgato_dark/IGListKit
1
0
Fork 0
mirror of https://github.com/Instagram/IGListKit synced 2026-05-23 09:18:29 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

fix IGListAdapterUpdaterDelegate

Browse source 
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
This commit is contained in:
Maxime Ollivier 2020-06-26 10:03:47 -07:00 committed by Facebook GitHub Bot
parent 4cc65c6b7f
commit 29bf582f47
4 changed files with 75 additions and 55 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
CHANGELOG.md
View file

@ -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

75
Source/IGListKit/IGListAdapterUpdater.m
View file

@ -64,6 +64,7 @@
if (collectionView == nil) {
[self _cleanStateAfterUpdates];
executeCompletionBlocks(NO);
[_delegate listAdapterUpdater:self didFinishWithoutUpdatesWithCollectionView:collectionView];
return;
}
@ -89,11 +90,11 @@
[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 @@
if (collectionView == nil) {
[self _cleanStateAfterUpdates];
executeCompletionBlocks(NO);
[_delegate listAdapterUpdater:self didFinishWithoutUpdatesWithCollectionView:collectionView];
return;
}
@ -169,26 +171,26 @@
};
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 @@
[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 @@
[self _queueUpdateWithCollectionViewBlock:collectionViewBlock];
};
// block that executes the batch update and exception handling
void (^performUpdate)(IGListIndexSetResult *) = ^(IGListIndexSetResult *result){
[collectionView layoutIfNeeded];
@try {
[delegate listAdapterUpdater:self
[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);
} else if (result.changeCount > 100 && IGListExperimentEnabled(experiments, IGListExperimentReloadDataFallback)) {
reloadDataFallback();
} else if (animated) {
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 {
if (collectionView.dataSource == nil) {
// If the data source is nil, we should not call any collection view update.
fallbackWithoutUpdates();
} else if (result.changeCount > 100 && IGListExperimentEnabled(experiments, IGListExperimentReloadDataFallback)) {
reloadDataFallback();
} else {
[UIView performWithoutAnimation:^{
[collectionView performBatchUpdates:^{
batchUpdatesBlock(result);
} completion:batchUpdatesCompletionBlock];
}];
performUpdate(result);
}
} @catch (NSException *exception) {
[delegate listAdapterUpdater:self
@ -282,12 +301,12 @@ willPerformBatchUpdatesWithCollectionView:collectionView
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);
}
}

14
Source/IGListKit/IGListAdapterUpdaterDelegate.h
View file

@ -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

39
Tests/IGListAdapterUpdaterTests.m
View file

@ -435,22 +435,20 @@
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 @@
[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 @@
[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 @@
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;