From 4b4388d967cef14c824ca5478f0e776b0ff1c66b Mon Sep 17 00:00:00 2001 From: Maxime Ollivier Date: Tue, 8 Sep 2020 09:06:16 -0700 Subject: [PATCH] safer collectionView and dataSource changes Summary: Changing the `IGListAdapter`'s `collectionView` or `dataSource` isn't safe. For example: * If we change the `dataSource`, the `collectionView` will be out of sync with the `IGListAdapter`'s data and can easily crash. * If we change the `collectionView` or `dataSource` during background diffing, the diffing result will be out of date by the time we get back to to the main main thread. To fix this: * On `[IGListAdapter setDataSource: ...]`, lets also change the `UICollectionView`'s `dataSource` to invalidate any section/item counts. We don't have to call `[UICollectionView reloadData]` just yet, we just want to let the collectionView know that the counts might have changed. * On `[IGListAdapter setDataSource: ...]` and `[IGListAdapter setCollectionView:...]`, we should cancel any pending or on-going updates. The tricky part is that we should still execute the `itemUpdateBlocks` and `completionBlocks` in the right order. I kind of want to make the `collectionView` readonly, but I think it would be too large of a public API change at this point. Reviewed By: patters Differential Revision: D23151919 fbshipit-source-id: 61cf025127706acaf22f153ec148ac0ea575bc98 --- Source/IGListKit/IGListAdapter.m | 87 ++- .../IGListExperimentalAdapterUpdater.m | 61 ++- Source/IGListKit/IGListUpdatingDelegate.h | 4 + .../IGListUpdatingDelegateExperimental.h | 8 + .../Internal/IGListBatchUpdateTransaction.m | 26 + .../IGListDataSourceChangeTransaction.h | 30 ++ .../IGListDataSourceChangeTransaction.m | 106 ++++ .../Internal/IGListReloadTransaction.m | 7 + .../Internal/IGListUpdateTransactable.h | 3 + .../Internal/IGListUpdateTransactionBuilder.h | 14 + .../Internal/IGListUpdateTransactionBuilder.m | 116 +++- Tests/IGListExperimentalAdapterE2ETests.m | 509 ++++++++++++++++++ 12 files changed, 920 insertions(+), 51 deletions(-) create mode 100644 Source/IGListKit/Internal/IGListDataSourceChangeTransaction.h create mode 100644 Source/IGListKit/Internal/IGListDataSourceChangeTransaction.m diff --git a/Source/IGListKit/IGListAdapter.m b/Source/IGListKit/IGListAdapter.m index b89cfc01..e097f59b 100644 --- a/Source/IGListKit/IGListAdapter.m +++ b/Source/IGListKit/IGListAdapter.m @@ -100,8 +100,33 @@ const BOOL settingFirstCollectionView = _collectionView == nil; - _collectionView = collectionView; - _collectionView.dataSource = self; + if (_experimentalUpdater) { + // We can't just swap out the collectionView, because we might have on-going or pending updates. + // `_experimentalUpdater` can take care of that by wrapping the change in `performDataSourceChange`. + + [_experimentalUpdater performDataSourceChange:^{ + if (self->_collectionView.dataSource == self) { + // Since we're not going to sync the previous collectionView anymore, lets not be its dataSource. + self->_collectionView.dataSource = nil; + } + self->_collectionView = collectionView; + self->_collectionView.dataSource = self; + + [self _updateCollectionViewDelegate]; + + // Sync the dataSource <> adapter for a couple of reasons: + // 1. We might not have synced on -setDataSource, so now is the time to try again. + // 2. Any in-flight `performUpdatesAnimated` were cancelled, so lets make sure we have the latest data. + [self _updateObjects]; + + // The sync between the collectionView <> adapter will happen automically, since + // we just changed the `collectionView.dataSource`. + }]; + } else { + _collectionView = collectionView; + _collectionView.dataSource = self; + [self _updateCollectionViewDelegate]; + } if (@available(iOS 10.0, tvOS 10, *)) { _collectionView.prefetchingEnabled = NO; @@ -110,19 +135,34 @@ [_collectionView.collectionViewLayout ig_hijackLayoutInteractiveReorderingMethodForAdapter:self]; [_collectionView.collectionViewLayout invalidateLayout]; - [self _updateCollectionViewDelegate]; - - // only construct - if (settingFirstCollectionView) { - [self _updateAfterPublicSettingsChange]; + if (!_experimentalUpdater && settingFirstCollectionView) { + [self _updateObjectsIfHasDataSource]; } } } - (void)setDataSource:(id)dataSource { - if (_dataSource != dataSource) { + if (_dataSource == dataSource) { + return; + } + + if (_experimentalUpdater) { + [_experimentalUpdater performDataSourceChange:^{ + self->_dataSource = dataSource; + + // Invalidate the collectionView internal section & item counts, as if its dataSource changed. + self->_collectionView.dataSource = nil; + self->_collectionView.dataSource = self; + + // Sync the dataSource <> adapter + [self _updateObjects]; + + // The sync between the collectionView <> adapter will happen automically, since + // we just changed the `collectionView.dataSource`. + }]; + } else { _dataSource = dataSource; - [self _updateAfterPublicSettingsChange]; + [self _updateObjectsIfHasDataSource]; } } @@ -147,14 +187,23 @@ } } -- (void)_updateAfterPublicSettingsChange { - id dataSource = _dataSource; - if (_collectionView != nil && dataSource != nil) { - NSArray *uniqueObjects = objectsWithDuplicateIdentifiersRemoved([dataSource objectsForListAdapter:self]); - [self _updateObjects:uniqueObjects dataSource:dataSource]; +- (void)_updateObjectsIfHasDataSource { + // This is to keep the existing logic while testing `experimentalUpdater` + if (_dataSource != nil) { + [self _updateObjects]; } } +- (void)_updateObjects { + if (_collectionView == nil) { + // If we don't have a collectionView, we can't do much. + return; + } + id dataSource = _dataSource; + NSArray *uniqueObjects = objectsWithDuplicateIdentifiersRemoved([dataSource objectsForListAdapter:self]); + [self _updateObjects:uniqueObjects dataSource:dataSource]; +} + - (void)_createProxyAndUpdateCollectionViewDelegate { // there is a known bug with accessibility and using an NSProxy as the delegate that will cause EXC_BAD_ACCESS // when voiceover is enabled. it will hold an unsafe ref to the delegate @@ -628,7 +677,13 @@ } - (IGListTransitionData *)_generateTransitionDataWithObjects:(NSArray *)objects dataSource:(id)dataSource { - IGParameterAssert(dataSource != nil); + IGListSectionMap *map = self.sectionMap; + + if (!dataSource) { + return [[IGListTransitionData alloc] initFromObjects:map.objects + toObjects:@[] + toSectionControllers:@[]]; + } #if DEBUG for (id object in objects) { @@ -648,8 +703,6 @@ validObjects = [NSMutableArray new]; } - IGListSectionMap *map = self.sectionMap; - // push the view controller and collection context into a local thread container so they are available on init // for IGListSectionController subclasses after calling [super init] IGListSectionControllerPushThread(self.viewController, self); diff --git a/Source/IGListKit/IGListExperimentalAdapterUpdater.m b/Source/IGListKit/IGListExperimentalAdapterUpdater.m index ee31467c..bed69f28 100644 --- a/Source/IGListKit/IGListExperimentalAdapterUpdater.m +++ b/Source/IGListKit/IGListExperimentalAdapterUpdater.m @@ -20,7 +20,9 @@ @interface IGListExperimentalAdapterUpdater () @property (nonatomic, strong) IGListUpdateTransactionBuilder *transactionBuilder; +@property (nonatomic, strong, nullable) IGListUpdateTransactionBuilder *lastTransactionBuilder; @property (nonatomic, strong, nullable) id transaction; +@property (nonatomic, assign) BOOL hasQueuedUpdate; @end @implementation IGListExperimentalAdapterUpdater @@ -50,16 +52,22 @@ return [self.transactionBuilder hasChanges]; } -- (void)_queueUpdate { +- (void)_queueUpdateIfNeeded { IGAssertMainThread(); + if (self.hasQueuedUpdate || !self.transactionBuilder.hasChanges) { + return; + } + __weak __typeof__(self) weakSelf = self; // dispatch_async to give the main queue time to collect more batch updates so that a minimum amount of work // (diffing, etc) is done on main. dispatch_async does not garauntee a full runloop turn will pass though. // see -performUpdateWithCollectionView:fromObjects:toObjects:animated:objectTransitionBlock:completion: for more // details on how coalescence is done. + self.hasQueuedUpdate = YES; dispatch_async(dispatch_get_main_queue(), ^{ + weakSelf.hasQueuedUpdate = NO; [weakSelf update]; }); } @@ -86,6 +94,7 @@ id transaction = [self.transactionBuilder buildWithConfig:config delegate:_delegate updater:self]; self.transaction = transaction; + self.lastTransactionBuilder = self.transactionBuilder; self.transactionBuilder = [IGListUpdateTransactionBuilder new]; if (!transaction) { @@ -96,11 +105,17 @@ __weak __typeof__(self) weakSelf = self; __weak __typeof__(transaction) weakTransaction = transaction; [transaction addCompletionBlock:^(BOOL finished) { - if (weakSelf.transaction == weakTransaction) { - weakSelf.transaction = nil; + __typeof__(self) strongSelf = weakSelf; + if (strongSelf == nil) { + return; + } + if (strongSelf.transaction == weakTransaction) { + strongSelf.transaction = nil; + strongSelf.lastTransactionBuilder = nil; + // queue another update in case something changed during batch updates. this method will bail next runloop if // there are no changes - [weakSelf _queueUpdate]; + [strongSelf _queueUpdateIfNeeded]; } }]; [transaction begin]; @@ -154,7 +169,7 @@ static NSUInteger IGListIdentifierHash(const void *item, NSUInteger (*size)(cons applyDataBlock:applyDataBlock completion:completion]; - [self _queueUpdate]; + [self _queueUpdateIfNeeded]; } @@ -179,7 +194,7 @@ static NSUInteger IGListIdentifierHash(const void *item, NSUInteger (*size)(cons itemUpdates:itemUpdates completion:completion]; - [self _queueUpdate]; + [self _queueUpdateIfNeeded]; } } @@ -194,7 +209,39 @@ static NSUInteger IGListIdentifierHash(const void *item, NSUInteger (*size)(cons reloadBlock:reloadUpdateBlock completion:completion]; - [self _queueUpdate]; + [self _queueUpdateIfNeeded]; +} + +- (void)performDataSourceChange:(IGListDataSourceChangeBlock)block { + // Unlike the other "performs", we need the dataSource change to be synchronous. + // Which means we need to cancel the current transaction, flatten the changes from + // both the current transtion and builder, and execute that new transaction. + + if (!self.transaction && ![self.transactionBuilder hasChanges]) { + // If nothing is going on, lets take a shortcut. + block(); + return; + } + + IGListUpdateTransactionBuilder *builder = [IGListUpdateTransactionBuilder new]; + [builder addDataSourceChange:block]; + + // Lets try to cancel any current transactions. + if ([self.transaction cancel] && self.lastTransactionBuilder) { + // We still need to apply the item-updates and completion-blocks, so lets merge the builders. + [builder addChangesFromBuilder:(IGListUpdateTransactionBuilder *)self.lastTransactionBuilder]; + } + + // Lets merge pending changes + [builder addChangesFromBuilder:self.transactionBuilder]; + + // Clear the current state + self.transaction = nil; + self.lastTransactionBuilder = nil; + self.transactionBuilder = builder; + + // Update synchronously + [self update]; } - (void)insertItemsIntoCollectionView:(UICollectionView *)collectionView indexPaths:(NSArray *)indexPaths { diff --git a/Source/IGListKit/IGListUpdatingDelegate.h b/Source/IGListKit/IGListUpdatingDelegate.h index dad60334..8b4505b4 100644 --- a/Source/IGListKit/IGListUpdatingDelegate.h +++ b/Source/IGListKit/IGListUpdatingDelegate.h @@ -43,6 +43,10 @@ typedef NSArray * _Nullable (^IGListToObjectBlock)(void); NS_SWIFT_NAME(ListCollectionViewBlock) typedef UICollectionView * _Nullable (^IGListCollectionViewBlock)(void); +/// A block that applies a `UICollectionView` dataSource change +NS_SWIFT_NAME(ListDataSourceChangeBlock) +typedef void (^IGListDataSourceChangeBlock)(void); + /** Implement this protocol in order to handle both section and row based update events. Implementation should forward or coalesce these events to a backing store or collection. diff --git a/Source/IGListKit/IGListUpdatingDelegateExperimental.h b/Source/IGListKit/IGListUpdatingDelegateExperimental.h index 0e7d44b6..d7888a07 100644 --- a/Source/IGListKit/IGListUpdatingDelegateExperimental.h +++ b/Source/IGListKit/IGListUpdatingDelegateExperimental.h @@ -41,6 +41,14 @@ NS_SWIFT_NAME(ListUpdatingDelegateExperimental) applyDataBlock:(IGListTransitionDataApplyBlock)applyDataBlock completion:(nullable IGListUpdatingCompletion)completion; +/** + Perform a `[UICollectionView setDataSource:...]` swap within this block. It gives the updater the chance to cancel or + execute any on-going updates. The block will be executed synchronously. + + @param block The block that will actuallty change the `dataSource` + */ +- (void)performDataSourceChange:(IGListDataSourceChangeBlock)block; + @end NS_ASSUME_NONNULL_END diff --git a/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m b/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m index 301654a4..e7cb9184 100644 --- a/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m +++ b/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m @@ -20,6 +20,12 @@ #import "IGListTransitionData.h" #import "UICollectionView+IGListBatchUpdateData.h" +typedef NS_ENUM (NSInteger, IGListBatchUpdateTransactionMode) { + IGListBatchUpdateTransactionModeCancellable, + IGListBatchUpdateTransactionModeNotCancellable, + IGListBatchUpdateTransactionModeCancelled, +}; + @interface IGListBatchUpdateTransaction () // Given @property (nonatomic, copy, readonly) UICollectionView *collectionView; @@ -35,6 +41,7 @@ @property (nonatomic, strong, readonly) IGListItemUpdatesCollector *inUpdateItemCollector; @property (nonatomic, copy, readonly) NSMutableArray *inUpdateCompletionBlocks; @property (nonatomic, assign, readwrite) IGListBatchUpdateState state; +@property (nonatomic, assign, readwrite) IGListBatchUpdateTransactionMode mode; @property (nonatomic, strong, readwrite, nullable) IGListBatchUpdateData *actualCollectionViewUpdates; @end @@ -62,6 +69,7 @@ _inUpdateItemCollector = [IGListItemUpdatesCollector new]; _state = IGListBatchUpdateStateIdle; + _mode = IGListBatchUpdateTransactionModeCancellable; } return self; } @@ -116,6 +124,14 @@ } - (void)_didDiff:(IGListIndexSetResult *)diffResult { + if (self.mode == IGListBatchUpdateTransactionModeCancelled) { + // Cancelling should have already taken care of the completion blocks + return; + } + + // After this point, we can assume that the update has began and there's no turning back. + self.mode = IGListBatchUpdateTransactionModeNotCancellable; + [self.delegate listAdapterUpdater:self.updater didDiffWithResults:diffResult onBackgroundThread:self.config.allowBackgroundDiffing]; @try { @@ -255,6 +271,16 @@ willPerformBatchUpdatesWithCollectionView:self.collectionView [self _completeAsFinished:NO]; } +#pragma mark - Cancel + +- (BOOL)cancel { + if (_mode != IGListBatchUpdateTransactionModeCancellable) { + return NO; + } + _mode = IGListBatchUpdateTransactionModeCancelled; + return YES; +} + #pragma mark - Item updates - (void)insertItemsAtIndexPaths:(NSArray *)indexPaths { diff --git a/Source/IGListKit/Internal/IGListDataSourceChangeTransaction.h b/Source/IGListKit/Internal/IGListDataSourceChangeTransaction.h new file mode 100644 index 00000000..20f78640 --- /dev/null +++ b/Source/IGListKit/Internal/IGListDataSourceChangeTransaction.h @@ -0,0 +1,30 @@ +/* +* Copyright (c) Facebook, Inc. and its affiliates. +* +* This source code is licensed under the MIT license found in the +* LICENSE file in the root directory of this source tree. +*/ + +#import + +#import +#import + +#import "IGListUpdateTransactable.h" + +NS_ASSUME_NONNULL_BEGIN + +/// Handles a `UICollectionView` `dataSource` change +IGLK_SUBCLASSING_RESTRICTED +@interface IGListDataSourceChangeTransaction : NSObject + +- (instancetype)initWithChangeBlock:(IGListDataSourceChangeBlock)block + itemUpdateBlocks:(NSArray *)itemUpdateBlocks + completionBlocks:(NSArray *)completionBlocks NS_DESIGNATED_INITIALIZER; + +- (instancetype)init NS_UNAVAILABLE; ++ (instancetype)new NS_UNAVAILABLE; + +@end + +NS_ASSUME_NONNULL_END diff --git a/Source/IGListKit/Internal/IGListDataSourceChangeTransaction.m b/Source/IGListKit/Internal/IGListDataSourceChangeTransaction.m new file mode 100644 index 00000000..264b39a1 --- /dev/null +++ b/Source/IGListKit/Internal/IGListDataSourceChangeTransaction.m @@ -0,0 +1,106 @@ +/* +* Copyright (c) Facebook, Inc. and its affiliates. +* +* This source code is licensed under the MIT license found in the +* LICENSE file in the root directory of this source tree. +*/ + +#import "IGListDataSourceChangeTransaction.h" + +@implementation IGListDataSourceChangeTransaction { + // Given + IGListDataSourceChangeBlock _block; + NSArray *_itemUpdateBlocks; + NSArray *_completionBlocks; + + // Internal + NSMutableArray *_inUpdateCompletionBlocks; + IGListBatchUpdateState _state; +} + +- (instancetype)initWithChangeBlock:(IGListDataSourceChangeBlock)block + itemUpdateBlocks:(NSArray *)itemUpdateBlocks + completionBlocks:(NSArray *)completionBlocks { + if (self = [super init]) { + _block = block; + _itemUpdateBlocks = itemUpdateBlocks; + _completionBlocks = completionBlocks; + } + return self; +} + +- (IGListBatchUpdateState)state { + return _state; +} + +#pragma mark - Update + +- (void)begin { + // Item updates must not send mutations to the collection view while we are reloading + _state = IGListBatchUpdateStateExecutingBatchUpdateBlock; + + // Execute all stored item update blocks even if all cells will get reloaded. the actual collection view + // mutations will be discarded, but clients are encouraged to put their actual /data/ mutations inside the + // update block as well, so if we don't execute the block the changes will never happen + for (IGListItemUpdateBlock itemUpdateBlock in _itemUpdateBlocks) { + itemUpdateBlock(); + } + + _state = IGListBatchUpdateStateExecutedBatchUpdateBlock; + + // Apply dataSource change + if (_block) { + _block(); + } + + for (IGListUpdatingCompletion completion in _completionBlocks) { + completion(YES); + } + + // Execute any completion blocks from item updates. Added after item blocks are executed in order to capture any + // re-entrant updates. + NSArray *inUpdateCompletionBlocks = [_inUpdateCompletionBlocks copy]; + for (IGListUpdatingCompletion completion in inUpdateCompletionBlocks) { + completion(YES); + } + + _state = IGListBatchUpdateStateIdle; +} + +#pragma mark - Cancel + +- (BOOL)cancel { + // This transaction is synchronous + return NO; +} + +#pragma mark - Item updates + +- (void)insertItemsAtIndexPaths:(NSArray *)indexPaths { + // no-op because changing the UICollectionView's dataSource invalidates section/item counts +} + +- (void)deleteItemsAtIndexPaths:(NSArray *)indexPaths { + // no-op because changing the UICollectionView's dataSource invalidates section/item counts +} + +- (void)moveItemFromIndexPath:(NSIndexPath *)fromIndexPath toIndexPath:(NSIndexPath *)toIndexPath { + // no-op because changing the UICollectionView's dataSource invalidates section/item counts +} + +- (void)reloadItemFromIndexPath:(NSIndexPath *)fromIndexPath toIndexPath:(NSIndexPath *)toIndexPath { + // no-op because changing the UICollectionView's dataSource invalidates section/item counts +} + +- (void)reloadSections:(NSIndexSet *)sections { + // no-op because changing the UICollectionView's dataSource invalidates section/item counts +} + +- (void)addCompletionBlock:(IGListUpdatingCompletion)completion { + if (!_inUpdateCompletionBlocks) { + _inUpdateCompletionBlocks = [NSMutableArray new]; + } + [_inUpdateCompletionBlocks addObject:completion]; +} + +@end diff --git a/Source/IGListKit/Internal/IGListReloadTransaction.m b/Source/IGListKit/Internal/IGListReloadTransaction.m index 950a3b5c..94ce5fff 100644 --- a/Source/IGListKit/Internal/IGListReloadTransaction.m +++ b/Source/IGListKit/Internal/IGListReloadTransaction.m @@ -93,6 +93,13 @@ self.state = IGListBatchUpdateStateIdle; } +#pragma mark - Cancel + +- (BOOL)cancel { + // This transaction is syncronous + return NO; +} + #pragma mark - Item updates - (void)insertItemsAtIndexPaths:(NSArray *)indexPaths { diff --git a/Source/IGListKit/Internal/IGListUpdateTransactable.h b/Source/IGListKit/Internal/IGListUpdateTransactable.h index 10c08ff7..a3cb45fa 100644 --- a/Source/IGListKit/Internal/IGListUpdateTransactable.h +++ b/Source/IGListKit/Internal/IGListUpdateTransactable.h @@ -28,6 +28,9 @@ typedef struct { /// Begin the transaction. We expect all completion blocks to be called once finished. - (void)begin; +/// Cancel any on going updates. +- (BOOL)cancel; + /// Current state of the transaction - (IGListBatchUpdateState)state; diff --git a/Source/IGListKit/Internal/IGListUpdateTransactionBuilder.h b/Source/IGListKit/Internal/IGListUpdateTransactionBuilder.h index 254b4c3e..595a031b 100644 --- a/Source/IGListKit/Internal/IGListUpdateTransactionBuilder.h +++ b/Source/IGListKit/Internal/IGListUpdateTransactionBuilder.h @@ -61,6 +61,20 @@ Completely reload data in the collection. reloadBlock:(IGListReloadUpdateBlock)reloadBlock completion:(nullable IGListUpdatingCompletion)completion; +/** +Change the `UICollectionView` dataSource + +@param block A block that applies a `UICollectionView` dataSource change +*/ +- (void)addDataSourceChange:(IGListDataSourceChangeBlock)block; + +/** + Add the changes from another builder. + +@param builder Add the changes from this builder +*/ +- (void)addChangesFromBuilder:(IGListUpdateTransactionBuilder *)builder; + /** Build a transaction based on the changes addded. */ diff --git a/Source/IGListKit/Internal/IGListUpdateTransactionBuilder.m b/Source/IGListKit/Internal/IGListUpdateTransactionBuilder.m index c63a0531..d7df65f6 100644 --- a/Source/IGListKit/Internal/IGListUpdateTransactionBuilder.m +++ b/Source/IGListKit/Internal/IGListUpdateTransactionBuilder.m @@ -8,8 +8,21 @@ #import "IGListUpdateTransactionBuilder.h" #import "IGListBatchUpdateTransaction.h" +#import "IGListDataSourceChangeTransaction.h" #import "IGListReloadTransaction.h" +/** + Modes in ascending order of priority. + */ +typedef NS_ENUM (NSInteger, IGListUpdateTransactionBuilderMode) { + /// The lowest priority is a batch-update, because a reload or dataSource take care of any changes. + IGListUpdateTransactionBuilderModeBatchUpdate, + /// The second priority is reloading all data. + IGListUpdateTransactionBuilderModeReload, + /// The highest priority is changing the `UICollectionView` dataSource. + IGListUpdateTransactionBuilderModeDataSourceChange, +}; + @interface IGListUpdateTransactionBuilder () // Batch updates @property (nonatomic, copy, readwrite, nullable) IGListTransitionDataBlock dataBlock; @@ -17,9 +30,11 @@ @property (nonatomic, strong, readonly) NSMutableArray *itemUpdateBlocks; @property (nonatomic, assign, readwrite) BOOL animated; // Reload -@property (nonatomic, assign, readwrite) BOOL hasReloadData; @property (nonatomic, copy, readwrite, nullable) IGListReloadUpdateBlock reloadBlock; +// DataSource change +@property (nonatomic, copy, readwrite, nullable) IGListDataSourceChangeBlock dataSourceChangeBlock; // Both +@property (nonatomic, assign, readwrite) IGListUpdateTransactionBuilderMode mode; @property (nonatomic, copy, readwrite, nullable) IGListCollectionViewBlock collectionViewBlock; @property (nonatomic, strong, readonly) NSMutableArray *completionBlocks; @end @@ -42,6 +57,8 @@ dataBlock:(IGListTransitionDataBlock)dataBlock applyDataBlock:(IGListTransitionDataApplyBlock)applyDataBlock completion:(IGListUpdatingCompletion)completion { + self.mode = MAX(self.mode, IGListUpdateTransactionBuilderModeBatchUpdate); + // disabled animations will always take priority // reset to YES in -cleanupState self.animated = self.animated && animated; @@ -63,6 +80,8 @@ collectionViewBlock:(IGListCollectionViewBlock)collectionViewBlock itemUpdates:(IGListItemUpdateBlock)itemUpdates completion:(nullable IGListUpdatingCompletion)completion { + self.mode = MAX(self.mode, IGListUpdateTransactionBuilderModeBatchUpdate); + // disabled animations will always take priority // reset to YES in -cleanupState self.animated = self.animated && animated; @@ -79,7 +98,8 @@ - (void)addReloadDataWithCollectionViewBlock:(IGListCollectionViewBlock)collectionViewBlock reloadBlock:(IGListReloadUpdateBlock)reloadBlock completion:(nullable IGListUpdatingCompletion)completion { - self.hasReloadData = YES; + self.mode = MAX(self.mode, IGListUpdateTransactionBuilderModeReload); + self.collectionViewBlock = collectionViewBlock; self.reloadBlock = reloadBlock; @@ -89,42 +109,84 @@ } } -- (BOOL)hasChanges { - return self.hasReloadData - || self.itemUpdateBlocks.count > 0 - || self.dataBlock != nil; +- (void)addDataSourceChange:(IGListDataSourceChangeBlock)block { + self.mode = MAX(self.mode, IGListUpdateTransactionBuilderModeDataSourceChange); + + self.dataSourceChangeBlock = block; +} + +- (void)addChangesFromBuilder:(IGListUpdateTransactionBuilder *)builder { + if (!builder) { + return; + } + + self.mode = MAX(self.mode, builder.mode); + + // Section update + self.animated = self.animated && builder.animated; + self.dataBlock = self.dataBlock ?: builder.dataBlock; + self.applyDataBlock = self.applyDataBlock ?: builder.applyDataBlock; + + // Item updates + [self.itemUpdateBlocks addObjectsFromArray:builder.itemUpdateBlocks]; + + // Reload + self.reloadBlock = self.reloadBlock ?: builder.reloadBlock; + + // All + self.collectionViewBlock = self.collectionViewBlock ?: builder.collectionViewBlock; + [self.completionBlocks addObjectsFromArray:builder.completionBlocks]; } - (nullable id)buildWithConfig:(IGListUpdateTransactationConfig)config delegate:(nullable id)delegate updater:(id)updater { - IGListCollectionViewBlock collectionViewBlock = _collectionViewBlock; + IGListCollectionViewBlock collectionViewBlock = self.collectionViewBlock; if (!collectionViewBlock) { return nil; } - if (_hasReloadData) { - IGListReloadUpdateBlock reloadBlock = self.reloadBlock; - if (!reloadBlock) { - return nil; + switch (self.mode) { + case IGListUpdateTransactionBuilderModeBatchUpdate: { + return [[IGListBatchUpdateTransaction alloc] initWithCollectionViewBlock:collectionViewBlock + updater:updater + delegate:delegate + config:config + animated:self.animated + dataBlock:self.dataBlock + applyDataBlock:self.applyDataBlock + itemUpdateBlocks:self.itemUpdateBlocks + completionBlocks:self.completionBlocks]; + } + case IGListUpdateTransactionBuilderModeReload: { + IGListReloadUpdateBlock reloadBlock = self.reloadBlock; + if (!reloadBlock) { + return nil; + } + return [[IGListReloadTransaction alloc] initWithCollectionViewBlock:collectionViewBlock + updater:updater + delegate:delegate + reloadBlock:reloadBlock + itemUpdateBlocks:self.itemUpdateBlocks + completionBlocks:self.completionBlocks]; + } + case IGListUpdateTransactionBuilderModeDataSourceChange: { + IGListDataSourceChangeBlock dataSourceChangeBlock = self.dataSourceChangeBlock; + if (!dataSourceChangeBlock) { + return nil; + } + return [[IGListDataSourceChangeTransaction alloc] initWithChangeBlock:dataSourceChangeBlock + itemUpdateBlocks:self.itemUpdateBlocks + completionBlocks:self.completionBlocks]; } - return [[IGListReloadTransaction alloc] initWithCollectionViewBlock:collectionViewBlock - updater:updater - delegate:delegate - reloadBlock:reloadBlock - itemUpdateBlocks:self.itemUpdateBlocks - completionBlocks:self.completionBlocks]; - } else { - return [[IGListBatchUpdateTransaction alloc] initWithCollectionViewBlock:collectionViewBlock - updater:updater - delegate:delegate - config:config - animated:self.animated - dataBlock:self.dataBlock - applyDataBlock:self.applyDataBlock - itemUpdateBlocks:self.itemUpdateBlocks - completionBlocks:self.completionBlocks]; } } +- (BOOL)hasChanges { + return self.mode == IGListUpdateTransactionBuilderModeReload + || self.mode == IGListUpdateTransactionBuilderModeDataSourceChange + || self.itemUpdateBlocks.count > 0 + || self.dataBlock != nil; +} + @end diff --git a/Tests/IGListExperimentalAdapterE2ETests.m b/Tests/IGListExperimentalAdapterE2ETests.m index dd26700f..03053507 100644 --- a/Tests/IGListExperimentalAdapterE2ETests.m +++ b/Tests/IGListExperimentalAdapterE2ETests.m @@ -14,6 +14,7 @@ #import "IGListAdapterInternal.h" #import "IGListAdapterUpdateTester.h" #import "IGListExperimentalAdapterUpdater.h" +#import "IGListExperimentalAdapterUpdaterInternal.h" #import "IGListTestCase.h" #import "IGListTestHelpers.h" #import "IGListTestOffsettingLayout.h" @@ -2002,4 +2003,512 @@ [self waitForExpectationsWithTimeout:30 handler:nil]; } +#pragma mark - Changing the collectionView/dataSource + +- (void)test_whenChangingDataSourceWithADifferentCount_thenPerformBatchUpdate_thatLastestDataIsApplied { + [self setupWithObjects:@[ + genTestObject(@0, @"Foo") + ]]; + + // STATE + // DataSource: 1 section + // Adapter: 1 section + // CollectionView: 1 section + + self.dataSource = [IGTestDelegateDataSource new]; + self.dataSource.objects = @[ + genTestObject(@0, @"Foo"), + genTestObject(@1, @"Bar") + ]; + self.adapter.dataSource = self.dataSource; + + // STATE + // DataSource: 2 sections + // Adapter: 2 sections + // CollectionView: Invalidated count + + // Schedule update + XCTestExpectation *expectation2 = genExpectation; + [self.adapter performUpdatesAnimated:NO completion:^(BOOL finished) { + XCTAssertTrue(finished); + XCTAssertEqual([self.collectionView numberOfSections], 2); + XCTAssertEqual(self.adapter.objects.count, 2); + + // STATE + // DataSource: 2 sections + // Adapter: 2 sections + // CollectionView: 2 sections + + [expectation2 fulfill]; + }]; + + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + +- (void)test_whenChangingCollectionView_thenScheduleSectionUpdate_thatLastestDataIsApplied { + [self setupWithObjects:@[ + genTestObject(@0, @"Foo") + ]]; + + // STATE + // DataSource: 1 section + // Adapter: 1 section + // CollectionView: 1 section + + // Force dataSource <> adapater sync by changing the collection view + self.layout = [UICollectionViewFlowLayout new]; + self.collectionView = [[UICollectionView alloc] initWithFrame:self.frame + collectionViewLayout:self.layout]; + self.adapter.collectionView = self.collectionView; + + // STATE + // DataSource: 1 sections + // Adapter: 1 sections + // CollectionView: Invalidated count + + XCTAssertEqual([self.collectionView numberOfSections], 1); + XCTAssertEqual(self.adapter.objects.count, 1); + + // STATE + // DataSource: 1 sections + // Adapter: 1 sections + // CollectionView: 1 sections +} + +- (void)test_settingCollectionViewAndDataSource_thatDontCreateCellsUntilLayout { + self.dataSource.objects = @[ + genTestObject(@0, @"Foo") + ]; + self.adapter.collectionView = self.collectionView; + self.adapter.dataSource = self.dataSource; + + // Make sure we didn't create the cells just yet, since we might want to scroll way without animating. + XCTAssertNil([self.collectionView cellForItemAtIndexPath:[NSIndexPath indexPathForItem:0 inSection:0]]); + + [self.collectionView layoutIfNeeded]; + XCTAssertNotNil([self.collectionView cellForItemAtIndexPath:[NSIndexPath indexPathForItem:0 inSection:0]]); +} + +#pragma mark - Changing the collectionView/dataSource with pending SECTION updates + +- (void)test_whenSchedulingSectionUpdate_thenChangeCollectionView_thatLastestDataIsApplied { + [self setupWithObjects:@[ + genTestObject(@0, @"Foo") + ]]; + + // STATE + // DataSource: 1 section + // Adapter: 1 section + // CollectionView: 1 section + + self.dataSource.objects = @[ + genTestObject(@0, @"Foo"), + genTestObject(@1, @"Bar") + ]; + + // STATE + // DataSource: 2 sections + // Adapter: 1 section + // CollectionView: 1 section + + // Schedule update + XCTestExpectation *expectation = genExpectation; + [self.adapter performUpdatesAnimated:NO completion:^(BOOL finished) { + + // STATE + // DataSource: 2 sections + // Adapter: 2 sections + // CollectionView: Invalidated count + + // Force collectionView <> adapter sync + XCTAssertEqual([self.collectionView numberOfSections], 2); + XCTAssertEqual(self.adapter.objects.count, 2); + XCTAssertTrue(finished); + + // STATE + // DataSource: 2 sections + // Adapter: 2 sections + // CollectionView: 2 sections + + [expectation fulfill]; + }]; + + // Force dataSource <> adapater sync by changing the collection view + self.layout = [UICollectionViewFlowLayout new]; + self.collectionView = [[UICollectionView alloc] initWithFrame:self.frame + collectionViewLayout:self.layout]; + self.adapter.collectionView = self.collectionView; + + // Although all the syncs should have been checked by now, lets still make + // sure the counts are right. + XCTAssertEqual([self.collectionView numberOfSections], 2); + XCTAssertEqual(self.adapter.objects.count, 2); + + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + +- (void)test_whenSchedulingSectionUpdate_thenChangeTheDataSource_thatLastestDataIsApplied { + [self setupWithObjects:@[ + genTestObject(@0, @"Foo") + ]]; + + // STATE + // DataSource: 1 section + // Adapter: 1 section + // CollectionView: 1 section + + self.dataSource.objects = @[ + genTestObject(@0, @"Foo"), + genTestObject(@1, @"Bar") + ]; + + // STATE + // DataSource: 2 section + // Adapter: 1 section + // CollectionView: 1 section + + // Schedule update + XCTestExpectation *expectation2 = genExpectation; + [self.adapter performUpdatesAnimated:NO completion:^(BOOL finished) { + // STATE + // DataSource: 3 sections + // Adapter: 3 sections + // CollectionView: Invalidated count + + XCTAssertTrue(finished); + XCTAssertEqual([self.collectionView numberOfSections], 3); + XCTAssertEqual(self.adapter.objects.count, 3); + + // STATE + // DataSource: 3 sections + // Adapter: 3 sections + // CollectionView: 3 sections + + [expectation2 fulfill]; + }]; + + // Force dataSource <> adapater sync by changing the dataSource + self.dataSource = [IGTestDelegateDataSource new]; + self.dataSource.objects = @[ + genTestObject(@0, @"Foo"), + genTestObject(@1, @"Bar"), + genTestObject(@2, @"Baz") + ]; + self.adapter.dataSource = self.dataSource; + + // Although all the syncs should have been checked by now, lets still make + // sure the counts are right. + XCTAssertEqual([self.collectionView numberOfSections], 3); + XCTAssertEqual(self.adapter.objects.count, 3); + + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + +#pragma mark - Changing the collectionView/dataSource with pending ITEM updates + +- (void)test_whenSchedulingItemUpdate_thenChangeCollectionView_thatLastestDataIsApplied { + [self setupWithObjects:@[ + genTestObject(@0, @1) + ]]; + + // STATE + // Section Controller: 1 cell + // CollectionView: 1 cell + + IGTestDelegateController *contoller = (IGTestDelegateController *)[self.adapter sectionControllerForSection:0]; + XCTAssertNotNil(contoller); + XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 1); + + XCTestExpectation *expectation1 = genExpectation; + [contoller.collectionContext performBatchAnimated:NO updates:^(id _Nonnull batchContext) { + // Just change the item count for section 0 + contoller.item = genTestObject(@0, @2); + [batchContext insertInSectionController:contoller atIndexes:[NSIndexSet indexSetWithIndex:0]]; + } completion:^(BOOL finished) { + XCTAssertTrue(finished); + XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 2); + [expectation1 fulfill]; + }]; + + // Force dataSource <> adapater sync by changing the collection view + self.layout = [UICollectionViewFlowLayout new]; + self.collectionView = [[UICollectionView alloc] initWithFrame:self.frame + collectionViewLayout:self.layout]; + self.adapter.collectionView = self.collectionView; + + // STATE + // Section Controller: 2 cells + // CollectionView: Invalidated count + + XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 2); + + // STATE + // Section Controller: 2 cells + // CollectionView: 2 cells + + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + +- (void)test_whenSchedulingItemUpdate_thenChangeDataSource_thatLastestDataIsApplied { + [self setupWithObjects:@[ + genTestObject(@0, @1) + ]]; + + // STATE + // Section Controller: 1 cell + // CollectionView: 1 cell + + IGTestDelegateController *contoller = (IGTestDelegateController *)[self.adapter sectionControllerForSection:0]; + XCTAssertNotNil(contoller); + XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 1); + + XCTestExpectation *expectation1 = genExpectation; + [contoller.collectionContext performBatchAnimated:NO updates:^(id _Nonnull batchContext) { + // Just change the item count for section 0 + contoller.item = genTestObject(@0, @2); + [batchContext insertInSectionController:contoller atIndexes:[NSIndexSet indexSetWithIndex:0]]; + } completion:^(BOOL finished) { + XCTAssertTrue(finished); + XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 2); + [expectation1 fulfill]; + }]; + + // Force dataSource <> adapater sync by changing the dataSource. + // Note that we keep the old object here, but that should not matter since + // it didn't change, it won't call -didUpdateToObject on that section-controller. + IGTestDelegateDataSource *oldDataSource = self.dataSource; + self.dataSource = [IGTestDelegateDataSource new]; + self.dataSource.objects = oldDataSource.objects; + self.adapter.dataSource = self.dataSource; + + // STATE + // Section Controller: 2 cells + // CollectionView: Invalidated count + + XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 2); + + // STATE + // Section Controller: 2 cells + // CollectionView: 2 cells + + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + +#pragma mark - Changing the collectionView/dataSource in middle of diffing + +- (void)test_whenSchedulingSectionUpdate_thenBeginDiffing_thenChangeCollectionView_thatLastestDataIsApplied { + IGListExperimentalAdapterUpdater *updater = (IGListExperimentalAdapterUpdater *)self.updater; + updater.experiments |= IGListExperimentBackgroundDiffing; + + [self setupWithObjects:@[ + genTestObject(@0, @"Foo") + ]]; + + // STATE + // DataSource: 1 section + // Adapter: 1 section + // CollectionView: 1 section + + self.dataSource.objects = @[ + genTestObject(@0, @"Foo"), + genTestObject(@1, @"Bar") + ]; + + // STATE + // DataSource: 2 sections + // Adapter: 1 section + // CollectionView: 1 section + + // Schedule update + XCTestExpectation *expectation = genExpectation; + [self.adapter performUpdatesAnimated:NO completion:^(BOOL finished) { + // STATE + // DataSource: 2 sections + // Adapter: 2 sections + // CollectionView: Invalidated count + + XCTAssertTrue(finished); + XCTAssertEqual([self.collectionView numberOfSections], 2); + XCTAssertEqual(self.adapter.objects.count, 2); + + // STATE + // DataSource: 2 sections + // Adapter: 2 sections + // CollectionView: 2 sections + + [expectation fulfill]; + }]; + + // Force the update to happen right way, so that the diffing starts + [updater update]; + + // Force dataSource <> adapater sync by changing the collection view + self.layout = [UICollectionViewFlowLayout new]; + self.collectionView = [[UICollectionView alloc] initWithFrame:self.frame + collectionViewLayout:self.layout]; + self.adapter.collectionView = self.collectionView; + + // Although all the syncs should have been checked by now, lets still make + // sure the counts are right. + XCTAssertEqual([self.collectionView numberOfSections], 2); + XCTAssertEqual(self.adapter.objects.count, 2); + + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + +- (void)test_whenSchedulingSectionUpdate_thenBeginDiffing_thenChangeTheDataSource_thatLastestDataIsApplied { + IGListExperimentalAdapterUpdater *updater = (IGListExperimentalAdapterUpdater *)self.updater; + updater.experiments |= IGListExperimentBackgroundDiffing; + + [self setupWithObjects:@[ + genTestObject(@0, @"Foo") + ]]; + + // STATE + // DataSource: 1 section + // Adapter: 1 section + // CollectionView: 1 section + + self.dataSource.objects = @[ + genTestObject(@0, @"Foo"), + genTestObject(@1, @"Bar") + ]; + + // STATE + // DataSource: 2 sections + // Adapter: 1 section + // CollectionView: 1 section + + // Schedule update + XCTestExpectation *expectation = genExpectation; + [self.adapter performUpdatesAnimated:NO completion:^(BOOL finished) { + // STATE + // DataSource: 3 sections + // Adapter: 3 sections + // CollectionView: Invalidated count + + XCTAssertTrue(finished); + XCTAssertEqual([self.collectionView numberOfSections], 3); + XCTAssertEqual(self.adapter.objects.count, 3); + + // STATE + // DataSource: 3 sections + // Adapter: 3 sections + // CollectionView: 3 sections + + [expectation fulfill]; + }]; + + // Force the update to happen right way, so that the diffing starts + [updater update]; + + // Force dataSource <> adapater sync by changing the dataSource + self.dataSource = [IGTestDelegateDataSource new]; + self.dataSource.objects = @[ + genTestObject(@0, @"Foo"), + genTestObject(@1, @"Bar"), + genTestObject(@2, @"Baz") + ]; + self.adapter.dataSource = self.dataSource; + + // Although all the syncs should have been checked by now, lets still make + // sure the counts are right. + XCTAssertEqual([self.collectionView numberOfSections], 3); + XCTAssertEqual(self.adapter.objects.count, 3); + + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + +#pragma mark - Sync the collectionView before setting a adapter.dataSource + +- (void)test_whenCollectionViewSyncsBeforeTheAdapterDataSourceIsSet_thatLastestDataIsApplied { + self.adapter.collectionView = self.collectionView; + + // Force the adapter <> collectionView to sync + XCTAssertEqual([self.collectionView numberOfSections], 0); + XCTAssertEqual([self.adapter objects].count, 0); + + // STATE + // DataSource: Nil + // Adapter: 0 sections + // CollectionView: 0 sections + + // Changing the `adapter.dataSource` will sync the adapter <> dataSource, and + // invalidate the collectionView's internal section/item counts. + self.dataSource.objects = @[genTestObject(@1, @"Foo")]; + self.adapter.dataSource = self.dataSource; + + // STATE + // DataSource: 1 section + // Adapter: 1 section + // CollectionView: Invalidated counts (UICollectionView will ask for counts on next layout) + + XCTAssertEqual([self.collectionView numberOfSections], 1); + XCTAssertEqual([self.adapter objects].count, 1); + + // Test that collectionView syncs with the adapter + [self.collectionView layoutIfNeeded]; + XCTAssertNotNil([self.collectionView cellForItemAtIndexPath:[NSIndexPath indexPathForItem:0 inSection:0]]); + + // STATE + // DataSource: 1 section + // Adapter: 1 section + // CollectionView: 1 section +} + +- (void)test_whenCollectionViewSyncsBeforeTheAdapterDataSourceIsSet_thenSchedulingSectionUpdate_thatLastestDataIsApplied { + self.adapter.collectionView = self.collectionView; + + // Force the adapter <> collectionView to sync + XCTAssertEqual([self.collectionView numberOfSections], 0); + XCTAssertEqual([self.adapter objects].count, 0); + + // STATE + // DataSource: Nil + // Adapter: 0 sections + // CollectionView: 0 sections + + // Changing the `adapter.dataSource` will sync the adapter <> dataSource, and + // invalidate the collectionView's internal section/item counts. + self.dataSource.objects = @[genTestObject(@0, @"Foo")]; + self.adapter.dataSource = self.dataSource; + + // STATE + // DataSource: 1 section + // Adapter: 1 section + // CollectionView: Invalidated counts (UICollectionView will ask for counts on next layout) + + XCTAssertEqual([self.adapter objects].count, 1); + + // Adding an object + self.dataSource.objects = @[ + genTestObject(@0, @"Foo"), + genTestObject(@1, @"Bar"), + ]; + + // STATE + // DataSource: 2 sections + // Adapter: 1 section + // CollectionView: Invalidated counts (Still) + + // Test that a batchUpdate from 1 -> 2 objects works, even though + // the collectionView has not synced yet. + XCTestExpectation *expectation = genExpectation; + [self.adapter performUpdatesAnimated:NO completion:^(BOOL finished) { + // Checked that the update worked + XCTAssertTrue(finished); + // Check that the we have the correct counts + XCTAssertEqual([self.collectionView numberOfSections], 2); + XCTAssertEqual(self.adapter.objects.count, 2); + [expectation fulfill]; + + // STATE + // DataSource: 2 sections + // Adapter: 2 section + // CollectionView: 2 sections + }]; + + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + @end