diff --git a/CHANGELOG.md b/CHANGELOG.md index e26c62f899..fb5adc85a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ - [Yoga] Implement ASYogaLayoutSpec, a simplified integration strategy for Yoga-powered layout calculation. [Scott Goodson](https://github.com/appleguy) - Fixed an issue where calls to setNeedsDisplay and setNeedsLayout would stop working on loaded nodes. [Garrett Moon](https://github.com/garrettmoon) - Migrated unit tests to OCMock 3.4 (from 2.2) and improved the multiplex image node tests. [Adlai Holler](https://github.com/Adlai-Holler) +- Fix CollectionNode double-load issue. This should significantly improve performance in cases where a collection node has content immediately available on first layout i.e. not fetched from the network. [Adlai Holler](https://github.com/Adlai-Holler) ## 2.3.3 - [ASTextKitFontSizeAdjuster] Replace use of NSAttributedString's boundingRectWithSize:options:context: with NSLayoutManager's boundingRectForGlyphRange:inTextContainer: [Ricky Cancro](https://github.com/rcancro) diff --git a/Source/ASCollectionNode.mm b/Source/ASCollectionNode.mm index 3412dd67e3..6c7e3799c5 100644 --- a/Source/ASCollectionNode.mm +++ b/Source/ASCollectionNode.mm @@ -28,6 +28,7 @@ #import #import #import +#import #import #import #import @@ -682,9 +683,17 @@ - (void)reloadDataWithCompletion:(void (^)())completion { ASDisplayNodeAssertMainThread(); - if (self.nodeLoaded) { - [self.view reloadDataWithCompletion:completion]; + if (!self.nodeLoaded) { + return; } + + [self performBatchUpdates:^{ + [self.view.changeSet reloadData]; + } completion:^(BOOL finished){ + if (completion) { + completion(); + } + }]; } - (void)reloadData @@ -692,14 +701,19 @@ [self reloadDataWithCompletion:nil]; } -- (void)relayoutItems -{ - [self.view relayoutItems]; -} - - (void)reloadDataImmediately { - [self.view reloadDataImmediately]; + ASDisplayNodeAssertMainThread(); + [self reloadData]; + [self waitUntilAllUpdatesAreCommitted]; +} + +- (void)relayoutItems +{ + ASDisplayNodeAssertMainThread(); + if (self.nodeLoaded) { + [self.view relayoutItems]; + } } - (void)beginUpdates diff --git a/Source/ASCollectionView.h b/Source/ASCollectionView.h index 2972ced495..b2f015cf57 100644 --- a/Source/ASCollectionView.h +++ b/Source/ASCollectionView.h @@ -266,14 +266,14 @@ NS_ASSUME_NONNULL_BEGIN * the main thread. * @warning This method is substantially more expensive than UICollectionView's version. */ -- (void)reloadDataWithCompletion:(nullable void (^)())completion ASDISPLAYNODE_DEPRECATED_MSG("Use ASCollectionNode method instead."); +- (void)reloadDataWithCompletion:(nullable void (^)())completion AS_UNAVAILABLE("Use ASCollectionNode method instead."); /** * Reload everything from scratch, destroying the working range and all cached nodes. * * @warning This method is substantially more expensive than UICollectionView's version. */ -- (void)reloadData ASDISPLAYNODE_DEPRECATED_MSG("Use ASCollectionNode method instead."); +- (void)reloadData AS_UNAVAILABLE("Use ASCollectionNode method instead."); /** * Reload everything from scratch entirely on the main thread, destroying the working range and all cached nodes. @@ -281,7 +281,7 @@ NS_ASSUME_NONNULL_BEGIN * @warning This method is substantially more expensive than UICollectionView's version and will block the main thread * while all the cells load. */ -- (void)reloadDataImmediately ASDISPLAYNODE_DEPRECATED_MSG("Use ASCollectionNode's -reloadDataWithCompletion: followed by -waitUntilAllUpdatesAreCommitted instead."); +- (void)reloadDataImmediately AS_UNAVAILABLE("Use ASCollectionNode method instead."); /** * Triggers a relayout of all nodes. diff --git a/Source/ASCollectionView.mm b/Source/ASCollectionView.mm index 68b4c34680..906268349b 100644 --- a/Source/ASCollectionView.mm +++ b/Source/ASCollectionView.mm @@ -144,11 +144,6 @@ static NSString * const kReuseIdentifier = @"_ASCollectionReuseIdentifier"; * (0 sections) we always check at least once after each update (initial reload is the first update.) */ BOOL _hasEverCheckedForBatchFetchingDueToUpdate; - - /** - * The change set that we're currently building, if any. - */ - _ASHierarchyChangeSet *_changeSet; /** * Counter used to keep track of nested batch updates. @@ -333,31 +328,23 @@ static NSString * const kReuseIdentifier = @"_ASCollectionReuseIdentifier"; #pragma mark - #pragma mark Overrides. -- (void)reloadDataWithCompletion:(void (^)())completion -{ - ASDisplayNodeAssertMainThread(); - - if (! _dataController.initialReloadDataHasBeenCalled) { - // If this is the first reload, forward to super immediately to prevent it from triggering more "initial" loads while our data controller is working. - _superIsPendingDataLoad = YES; - [super reloadData]; - } - - void (^batchUpdatesCompletion)(BOOL); - if (completion) { - batchUpdatesCompletion = ^(BOOL) { - completion(); - }; - } - - [self performBatchUpdates:^{ - [_changeSet reloadData]; - } completion:batchUpdatesCompletion]; -} - +/** + * This method is not available to be called by the public i.e. + * it should only be called by UICollectionView itself. UICollectionView + * does this e.g. during the first layout pass, or if you call -numberOfSections + * before its content is loaded. + */ - (void)reloadData { - [self reloadDataWithCompletion:nil]; + [super reloadData]; + + // UICollectionView calls -reloadData during first layoutSubviews and when the data source changes. + // This fires off the first load of cell nodes. + if (_asyncDataSource != nil && !self.dataController.initialReloadDataHasBeenCalled) { + [self performBatchUpdates:^{ + [_changeSet reloadData]; + } completion:nil]; + } } - (void)scrollToItemAtIndexPath:(NSIndexPath *)indexPath atScrollPosition:(UICollectionViewScrollPosition)scrollPosition animated:(BOOL)animated @@ -367,13 +354,6 @@ static NSString * const kReuseIdentifier = @"_ASCollectionReuseIdentifier"; } } -- (void)reloadDataImmediately -{ - ASDisplayNodeAssertMainThread(); - [self reloadData]; - [self waitUntilAllUpdatesAreCommitted]; -} - - (void)relayoutItems { [_dataController relayoutAllNodes]; @@ -1721,7 +1701,6 @@ static NSString * const kReuseIdentifier = @"_ASCollectionReuseIdentifier"; } UICollectionViewLayoutAttributes *attributes = [self layoutAttributesForItemAtIndexPath:indexPath]; return CGSizeEqualToSizeWithIn(attributes.size, size, FLT_EPSILON); - } #pragma mark - ASDataControllerSource optional methods diff --git a/Source/Details/ASCollectionInternal.h b/Source/Details/ASCollectionInternal.h index bfab0bf949..28e62f8270 100644 --- a/Source/Details/ASCollectionInternal.h +++ b/Source/Details/ASCollectionInternal.h @@ -31,6 +31,11 @@ NS_ASSUME_NONNULL_BEGIN @property (nonatomic, strong, readonly) ASDataController *dataController; @property (nonatomic, strong, readonly) ASRangeController *rangeController; +/** + * The change set that we're currently building, if any. + */ +@property (nonatomic, strong, nullable, readonly) _ASHierarchyChangeSet *changeSet; + /** * @see ASCollectionNode+Beta.h for full documentation. */ diff --git a/Source/Private/ASCollectionView+Undeprecated.h b/Source/Private/ASCollectionView+Undeprecated.h index 2764d479a8..be3a3f1a0b 100644 --- a/Source/Private/ASCollectionView+Undeprecated.h +++ b/Source/Private/ASCollectionView+Undeprecated.h @@ -163,30 +163,6 @@ NS_ASSUME_NONNULL_BEGIN */ - (void)performBatchUpdates:(nullable AS_NOESCAPE void (^)())updates completion:(nullable void (^)(BOOL finished))completion; -/** - * Reload everything from scratch, destroying the working range and all cached nodes. - * - * @param completion block to run on completion of asynchronous loading or nil. If supplied, the block is run on - * the main thread. - * @warning This method is substantially more expensive than UICollectionView's version. - */ -- (void)reloadDataWithCompletion:(nullable void (^)())completion; - -/** - * Reload everything from scratch, destroying the working range and all cached nodes. - * - * @warning This method is substantially more expensive than UICollectionView's version. - */ -- (void)reloadData; - -/** - * Reload everything from scratch entirely on the main thread, destroying the working range and all cached nodes. - * - * @warning This method is substantially more expensive than UICollectionView's version and will block the main thread - * while all the cells load. - */ -- (void)reloadDataImmediately; - /** * Triggers a relayout of all nodes. * diff --git a/Tests/ASCollectionModernDataSourceTests.m b/Tests/ASCollectionModernDataSourceTests.m index 4a53bda1ad..ec97e8c644 100644 --- a/Tests/ASCollectionModernDataSourceTests.m +++ b/Tests/ASCollectionModernDataSourceTests.m @@ -151,26 +151,30 @@ - (void)loadInitialData { - /// BUG: these methods are called twice in a row i.e. this for-loop shouldn't be here. https://github.com/TextureGroup/Texture/issues/351 + // Count methods are called twice in a row for first data load. + // Since -reloadData is routed through our batch update system, + // the batch update latches the "old data source counts" if needed at -beginUpdates time + // and then verifies them against the "new data source counts" after the updates. + // This isn't ideal, but the cost is very small and the system works well. for (int i = 0; i < 2; i++) { // It reads all the counts [self expectDataSourceCountMethods]; + } - // It reads each section object. - for (NSInteger section = 0; section < sections.count; section++) { - [self expectContextMethodForSection:section]; - } + // It reads each section object. + for (NSInteger section = 0; section < sections.count; section++) { + [self expectContextMethodForSection:section]; + } - // It reads the contents for each item. - for (NSInteger section = 0; section < sections.count; section++) { - NSArray *viewModels = sections[section].viewModels; + // It reads the contents for each item. + for (NSInteger section = 0; section < sections.count; section++) { + NSArray *viewModels = sections[section].viewModels; - // For each item: - for (NSInteger i = 0; i < viewModels.count; i++) { - NSIndexPath *indexPath = [NSIndexPath indexPathForItem:i inSection:section]; - [self expectViewModelMethodForItemAtIndexPath:indexPath viewModel:viewModels[i]]; - [self expectNodeBlockMethodForItemAtIndexPath:indexPath]; - } + // For each item: + for (NSInteger i = 0; i < viewModels.count; i++) { + NSIndexPath *indexPath = [NSIndexPath indexPathForItem:i inSection:section]; + [self expectViewModelMethodForItemAtIndexPath:indexPath viewModel:viewModels[i]]; + [self expectNodeBlockMethodForItemAtIndexPath:indexPath]; } } diff --git a/Tests/ASCollectionViewTests.mm b/Tests/ASCollectionViewTests.mm index b201f63a07..00cf58d2b5 100644 --- a/Tests/ASCollectionViewTests.mm +++ b/Tests/ASCollectionViewTests.mm @@ -1,5 +1,5 @@ // -// ASCollectionViewTests.m +// ASCollectionViewTests.mm // Texture // // Copyright (c) 2014-present, Facebook, Inc. All rights reserved. @@ -259,7 +259,8 @@ [window setRootViewController:testController]; [window makeKeyAndVisible]; - [testController.collectionView reloadDataImmediately]; + [testController.collectionNode reloadData]; + [testController.collectionNode waitUntilAllUpdatesAreCommitted]; [testController.collectionView layoutIfNeeded]; NSIndexPath *indexPath = [NSIndexPath indexPathForItem:0 inSection:0]; @@ -390,12 +391,13 @@ ASCollectionViewTestController *testController = [[ASCollectionViewTestController alloc] initWithNibName:nil bundle:nil];\ __unused ASCollectionViewTestDelegate *del = testController.asyncDelegate;\ __unused ASCollectionView *cv = testController.collectionView;\ - __unused ASCollectionNode *cn = testController.collectionNode;\ + ASCollectionNode *cn = testController.collectionNode;\ UIWindow *window = [[UIWindow alloc] initWithFrame:[[UIScreen mainScreen] bounds]];\ [window makeKeyAndVisible]; \ window.rootViewController = testController;\ \ - [testController.collectionView reloadDataImmediately];\ + [cn reloadData];\ + [cn waitUntilAllUpdatesAreCommitted]; \ [testController.collectionView layoutIfNeeded]; - (void)testThatSubmittingAValidInsertDoesNotThrowAnException @@ -620,7 +622,7 @@ for (NSInteger i = 0; i < 2; i++) { // NOTE: waitUntilAllUpdatesAreCommitted or reloadDataImmediately is not sufficient here!! XCTestExpectation *done = [self expectationWithDescription:[NSString stringWithFormat:@"Reload #%td complete", i]]; - [cv reloadDataWithCompletion:^{ + [cn reloadDataWithCompletion:^{ [done fulfill]; }]; [self waitForExpectationsWithTimeout:1 handler:nil]; @@ -752,7 +754,8 @@ updateValidationTestPrologue del.sectionGeneration++; - [cv reloadDataImmediately]; + [cn reloadData]; + [cn waitUntilAllUpdatesAreCommitted]; NSInteger sectionCount = del->_itemCounts.size(); for (NSInteger section = 0; section < sectionCount; section++) { diff --git a/Tests/ASDisplayNodeLayoutTests.mm b/Tests/ASDisplayNodeLayoutTests.mm index 202b3b4fa2..8688e5cec6 100644 --- a/Tests/ASDisplayNodeLayoutTests.mm +++ b/Tests/ASDisplayNodeLayoutTests.mm @@ -1,5 +1,5 @@ // -// ASDisplayNodeLayoutTests.m +// ASDisplayNodeLayoutTests.mm // Texture // // Copyright (c) 2014-present, Facebook, Inc. All rights reserved.