Fix double-load issue with ASCollectionNode (#372)

* Fix double-reload issue with ASCollectionNode

* Add a message to the change pile

* Fix some license headers

* Address feedback

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)

Source/ASCollectionNode.mm

@@ -28,6 +28,7 @@
 #import <AsyncDisplayKit/ASDisplayNode+FrameworkPrivate.h>
 #import <AsyncDisplayKit/ASInternalHelpers.h>
 #import <AsyncDisplayKit/ASCellNode+Internal.h>
+#import <AsyncDisplayKit/_ASHierarchyChangeSet.h>
 #import <AsyncDisplayKit/AsyncDisplayKit+Debug.h>
 #import <AsyncDisplayKit/ASSectionContext.h>
 #import <AsyncDisplayKit/ASDataController.h>
@@ -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

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.

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

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.
  */

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.
  *

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];
     }
   }
 

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++) {

Tests/ASDisplayNodeLayoutTests.mm

@@ -1,5 +1,5 @@
 //
-//  ASDisplayNodeLayoutTests.m
+//  ASDisplayNodeLayoutTests.mm
 //  Texture
 //
 //  Copyright (c) 2014-present, Facebook, Inc.  All rights reserved.