Skip to content

Commit

Permalink
Add experimental collectionView getter fix
Browse files Browse the repository at this point in the history 
Summary:
Adding a fix to the `IGListAdapterUpdater` that requests the `UICollectionView` to perform updates on until just-before we update. This way if the `UICollectionView` is changed between update-queue and execution (b/c updates are async), we guarantee the update is performed on the correct view.

See the accompanying unit test that fails w/out the fix enabled.

Differential Revision: D7889908

fbshipit-source-id: 7178677f34951a1e42986b0289fc4abc708d6946
  • Loading branch information
@facebook-github-bot
Ryan Nystrom authored and facebook-github-bot committed May 11, 2018
1 parent cce5a46 commit 583efb9
Show file tree
Hide file tree
Showing 9 changed files with 226 additions and 136 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -5,6 +5,12 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag
4.0.0 (upcoming release)
-----

### Enhancements

### Fixes

- Experimental fix to get the `UICollectionView` for batch updating immediately before applying the update. [Ryan Nystrom](https://github.com/rnystrom) (tbd)

3.4.0
-----

Expand Down
2 changes: 2 additions & 0 deletions Source/Common/IGListExperiments.h
View file
Expand Up @@ -26,6 +26,8 @@ typedef NS_OPTIONS (NSInteger, IGListExperiment) {
IGListExperimentDedupeItemUpdates = 1 << 5,
/// Test deferring object creation until just before diffing.
IGListExperimentDeferredToObjectCreation = 1 << 6,
/// Test getting collection view at update time.
IGListExperimentGetCollectionViewAtUpdate = 1 << 7,
};

/**
Expand Down
127 changes: 73 additions & 54 deletions Source/IGListAdapter.m
View file
Expand Up @@ -99,6 +99,8 @@ - (void)setCollectionView:(UICollectionView *)collectionView {
_registeredSupplementaryViewIdentifiers = [NSMutableSet new];
_registeredSupplementaryViewNibNames = [NSMutableSet new];

const BOOL settingFirstCollectionView = _collectionView == nil;

_collectionView = collectionView;
_collectionView.dataSource = self;

Expand All @@ -110,7 +112,12 @@ - (void)setCollectionView:(UICollectionView *)collectionView {
[_collectionView.collectionViewLayout invalidateLayout];

[self _updateCollectionViewDelegate];
[self _updateAfterPublicSettingsChange];

// only construct
if (!IGListExperimentEnabled(self.experiments, IGListExperimentGetCollectionViewAtUpdate)
|| settingFirstCollectionView) {
[self _updateAfterPublicSettingsChange];
}
}
}

Expand Down Expand Up @@ -193,11 +200,11 @@ - (void)scrollToObject:(id)object
[collectionView layoutIfNeeded];

NSIndexPath *indexPathFirstElement = [NSIndexPath indexPathForItem:0 inSection:section];

// collect the layout attributes for the cell and supplementary views for the first index
// this will break if there are supplementary views beyond item 0
NSMutableArray<UICollectionViewLayoutAttributes *> *attributes = nil;

const NSInteger numberOfItems = [collectionView numberOfItemsInSection:section];
if (numberOfItems > 0) {
attributes = [self _layoutAttributesForIndexPath:indexPathFirstElement supplementaryKinds:supplementaryKinds].mutableCopy;
Expand Down Expand Up @@ -314,7 +321,7 @@ - (void)performUpdatesAnimated:(BOOL)animated completion:(IGListUpdaterCompletio
id<IGListAdapterDataSource> dataSource = self.dataSource;
UICollectionView *collectionView = self.collectionView;
if (dataSource == nil || collectionView == nil) {
IGLKLog(@"Warning: Your call to %s is ignored as dataSource or collectionView haven't been set.", __PRETTY_FUNCTION__);
IGLKLog(@"Warning: Your call to %s is ignored as dataSource or collectionView haven't been set.", __PRETTY_FUNCTION__);
if (completion) {
completion(NO);
}
Expand All @@ -341,26 +348,26 @@ - (void)performUpdatesAnimated:(BOOL)animated completion:(IGListUpdaterCompletio
}

[self _enterBatchUpdates];
[self.updater performUpdateWithCollectionView:collectionView
fromObjects:fromObjects
toObjectsBlock:toObjectsBlock
animated:animated
objectTransitionBlock:^(NSArray *toObjects) {
// temporarily capture the item map that we are transitioning from in case
// there are any item deletes at the same
weakSelf.previousSectionMap = [weakSelf.sectionMap copy];

[weakSelf _updateObjects:toObjects dataSource:dataSource];
} completion:^(BOOL finished) {
// release the previous items
weakSelf.previousSectionMap = nil;

[weakSelf _notifyDidUpdate:IGListAdapterUpdateTypePerformUpdates animated:animated];
if (completion) {
completion(finished);
}
[weakSelf _exitBatchUpdates];
}];
[self.updater performUpdateWithCollectionViewBlock:[self _collectionViewBlock]
fromObjects:fromObjects
toObjectsBlock:toObjectsBlock
animated:animated
objectTransitionBlock:^(NSArray *toObjects) {
// temporarily capture the item map that we are transitioning from in case
// there are any item deletes at the same
weakSelf.previousSectionMap = [weakSelf.sectionMap copy];

[weakSelf _updateObjects:toObjects dataSource:dataSource];
} completion:^(BOOL finished) {
// release the previous items
weakSelf.previousSectionMap = nil;

[weakSelf _notifyDidUpdate:IGListAdapterUpdateTypePerformUpdates animated:animated];
if (completion) {
completion(finished);
}
[weakSelf _exitBatchUpdates];
}];
}

- (void)reloadDataWithCompletion:(nullable IGListUpdaterCompletion)completion {
Expand All @@ -379,16 +386,17 @@ - (void)reloadDataWithCompletion:(nullable IGListUpdaterCompletion)completion {
NSArray *uniqueObjects = objectsWithDuplicateIdentifiersRemoved([dataSource objectsForListAdapter:self]);

__weak __typeof__(self) weakSelf = self;
[self.updater reloadDataWithCollectionView:collectionView reloadUpdateBlock:^{
// purge all section controllers from the item map so that they are regenerated
[weakSelf.sectionMap reset];
[weakSelf _updateObjects:uniqueObjects dataSource:dataSource];
} completion:^(BOOL finished) {
[weakSelf _notifyDidUpdate:IGListAdapterUpdateTypeReloadData animated:NO];
if (completion) {
completion(finished);
}
}];
[self.updater reloadDataWithCollectionViewBlock:[self _collectionViewBlock]
reloadUpdateBlock:^{
// purge all section controllers from the item map so that they are regenerated
[weakSelf.sectionMap reset];
[weakSelf _updateObjects:uniqueObjects dataSource:dataSource];
} completion:^(BOOL finished) {
[weakSelf _notifyDidUpdate:IGListAdapterUpdateTypeReloadData animated:NO];
if (completion) {
completion(finished);
}
}];
}

- (void)reloadObjects:(NSArray *)objects {
Expand Down Expand Up @@ -447,7 +455,7 @@ - (void)_notifyDidUpdate:(IGListAdapterUpdateType)update animated:(BOOL)animated

- (nullable IGListSectionController *)sectionControllerForSection:(NSInteger)section {
IGAssertMainThread();

return [self.sectionMap sectionControllerForSection:section];
}

Expand Down Expand Up @@ -586,6 +594,16 @@ - (CGSize)sizeForSupplementaryViewOfKind:(NSString *)elementKind atIndexPath:(NS

#pragma mark - Private API

- (IGListCollectionViewBlock)_collectionViewBlock {
if (IGListExperimentEnabled(self.experiments, IGListExperimentGetCollectionViewAtUpdate)) {
__weak __typeof__(self) weakSelf = self;
return ^UICollectionView *{ return weakSelf.collectionView; };
} else {
__weak UICollectionView *collectionView = _collectionView;
return ^UICollectionView *{ return collectionView; };
}
}

// this method is what updates the "source of truth"
// this should only be called just before the collection view is updated
- (void)_updateObjects:(NSArray *)objects dataSource:(id<IGListAdapterDataSource>)dataSource {
Expand Down Expand Up @@ -637,7 +655,7 @@ - (void)_updateObjects:(NSArray *)objects dataSource:(id<IGListAdapterDataSource
[sectionControllers addObject:sectionController];
[validObjects addObject:object];
}

#if DEBUG
IGAssert([NSSet setWithArray:sectionControllers].count == sectionControllers.count,
@"Section controllers array is not filled with unique objects; section controllers are being reused");
Expand Down Expand Up @@ -725,7 +743,7 @@ - (NSIndexPath *)indexPathForSectionController:(IGListSectionController *)contro
}

- (NSArray<UICollectionViewLayoutAttributes *> *)_layoutAttributesForIndexPath:(NSIndexPath *)indexPath
supplementaryKinds:(NSArray<NSString *> *)supplementaryKinds {
supplementaryKinds:(NSArray<NSString *> *)supplementaryKinds {
UICollectionViewLayout *layout = self.collectionView.collectionViewLayout;
NSMutableArray<UICollectionViewLayoutAttributes *> *attributes = [NSMutableArray new];

Expand Down Expand Up @@ -933,17 +951,17 @@ - (void)deselectItemAtIndex:(NSInteger)index
- (void)selectItemAtIndex:(NSInteger)index
sectionController:(IGListSectionController *)sectionController
animated:(BOOL)animated
scrollPosition:(UICollectionViewScrollPosition)scrollPosition {
scrollPosition:(UICollectionViewScrollPosition)scrollPosition {
IGAssertMainThread();
IGParameterAssert(sectionController != nil);
NSIndexPath *indexPath = [self indexPathForSectionController:sectionController index:index usePreviousIfInUpdateBlock:NO];
[self.collectionView selectItemAtIndexPath:indexPath animated:animated scrollPosition:scrollPosition];
}

- (__kindof UICollectionViewCell *)dequeueReusableCellOfClass:(Class)cellClass
withReuseIdentifier:(NSString *)reuseIdentifier
forSectionController:(IGListSectionController *)sectionController
atIndex:(NSInteger)index {
withReuseIdentifier:(NSString *)reuseIdentifier
forSectionController:(IGListSectionController *)sectionController
atIndex:(NSInteger)index {
IGAssertMainThread();
IGParameterAssert(sectionController != nil);
IGParameterAssert(cellClass != nil);
Expand Down Expand Up @@ -1057,9 +1075,9 @@ - (void)performBatchAnimated:(BOOL)animated updates:(void (^)(id<IGListBatchCont
IGAssert(collectionView != nil, @"Performing batch updates without a collection view.");

[self _enterBatchUpdates];

__weak __typeof__(self) weakSelf = self;
[self.updater performUpdateWithCollectionView:collectionView animated:animated itemUpdates:^{
[self.updater performUpdateWithCollectionViewBlock:[self _collectionViewBlock] animated:animated itemUpdates:^{
weakSelf.isInUpdateBlock = YES;
// the adapter acts as the batch context with its API stripped to just the IGListBatchContext protocol
updates(weakSelf);
Expand All @@ -1080,7 +1098,7 @@ - (void)scrollToSectionController:(IGListSectionController *)sectionController
animated:(BOOL)animated {
IGAssertMainThread();
IGParameterAssert(sectionController != nil);

NSIndexPath *indexPath = [self indexPathForSectionController:sectionController index:index usePreviousIfInUpdateBlock:NO];
[self.collectionView scrollToItemAtIndexPath:indexPath atScrollPosition:scrollPosition animated:animated];
}
Expand All @@ -1089,16 +1107,16 @@ - (void)invalidateLayoutForSectionController:(IGListSectionController *)sectionC
completion:(void (^)(BOOL finished))completion{
const NSInteger section = [self sectionForSectionController:sectionController];
const NSInteger items = [_collectionView numberOfItemsInSection:section];

NSMutableArray<NSIndexPath *> *indexPaths = [NSMutableArray new];
for (NSInteger item = 0; item < items; item++) {
[indexPaths addObject:[NSIndexPath indexPathForItem:item inSection:section]];
}

UICollectionViewLayout *layout = _collectionView.collectionViewLayout;
UICollectionViewLayoutInvalidationContext *context = [[[layout.class invalidationContextClass] alloc] init];
[context invalidateItemsAtIndexPaths:indexPaths];

__weak __typeof__(_collectionView) weakCollectionView = _collectionView;

// do not call -[UICollectionView performBatchUpdates:completion:] while already updating. defer it until completed.
Expand Down Expand Up @@ -1228,7 +1246,7 @@ - (void)moveSectionControllerInteractive:(IGListSectionController *)sectionContr
IGAssert(collectionView != nil, @"Moving section %@ without a collection view from index %li to index %li.",
sectionController, (long)fromIndex, (long)toIndex);
IGAssert(self.moveDelegate != nil, @"Moving section %@ without a moveDelegate set", sectionController);

if (fromIndex != toIndex) {
id<IGListAdapterDataSource> dataSource = self.dataSource;

Expand All @@ -1250,26 +1268,26 @@ - (void)moveSectionControllerInteractive:(IGListSectionController *)sectionContr

// inform the data source to update its model
[self.moveDelegate listAdapter:self moveObject:object from:previousObjects to:objects];

// update our model based on that provided by the data source
NSArray<id<IGListDiffable>> *updatedObjects = [dataSource objectsForListAdapter:self];
[self _updateObjects:updatedObjects dataSource:dataSource];
}

// even if from and to index are equal, we need to perform the "move"
// iOS interactively moves items, not sections, so we might have actually moved the item
// to the end of the preceeding section or beginning of the following section
[self.updater moveSectionInCollectionView:collectionView fromIndex:fromIndex toIndex:toIndex];
}

- (void)moveInSectionControllerInteractive:(IGListSectionController *)sectionController
fromIndex:(NSInteger)fromIndex
toIndex:(NSInteger)toIndex NS_AVAILABLE_IOS(9_0) {
IGAssertMainThread();
IGParameterAssert(sectionController != nil);
IGParameterAssert(fromIndex >= 0);
IGParameterAssert(toIndex >= 0);

[sectionController moveObjectFromIndex:fromIndex toIndex:toIndex];
}

Expand All @@ -1278,9 +1296,10 @@ - (void)revertInvalidInteractiveMoveFromIndexPath:(NSIndexPath *)sourceIndexPath
UICollectionView *collectionView = self.collectionView;
IGAssert(collectionView != nil, @"Reverting move without a collection view from %@ to %@.",
sourceIndexPath, destinationIndexPath);

// revert by moving back in the opposite direction
[collectionView moveItemAtIndexPath:destinationIndexPath toIndexPath:sourceIndexPath];
}

@end

0 comments on commit 583efb9

Please sign in to comment.