Add experiment to ensure ASCollectionView's range controller updates …

…on changeset updates (#1976) This experiment makes sure a ASCollectionView's `rangeController` updates when a changeset WITH updates is applied. Currently it is possible for nodes inserted into the preload range to not get preloaded when performing a batch update. For example, suppose a collection node has: - Tuning parameters with a preload range of 1 screenful for the given range mode. - Nodes A and B where A is visible and B is off screen. Currently if node B is deleted and a new node C is inserted in its place, node C will not get preloaded until the collection node is scrolled. This is because the preloading mechanism relies on a `setNeedsUpdate` call on the range controller as part of the `-collectionView:willDisplayCell:forItemAtIndexPath:` delegate method when the batch update is submitted. However, in the example outlined above, this sometimes doesn't happen automtically, causing the range update to be delayed until the next the view scrolls.
TextureGroup · Mar 25, 2021 · 8f7444e · 8f7444e
1 parent 17d4d13
commit 8f7444e
Show file tree

Hide file tree

Showing 5 changed files with 93 additions and 3 deletions.
diff --git a/Source/ASCollectionView.mm b/Source/ASCollectionView.mm
@@ -2311,6 +2311,9 @@ - (void)rangeController:(ASRangeController *)rangeController updateWithChangeSet
 
       // Flush any range changes that happened as part of submitting the update.
       as_activity_scope(changeSet.rootActivity);
+      if (numberOfUpdates > 0 && ASActivateExperimentalFeature(ASExperimentalRangeUpdateOnChangesetUpdate)) {
+        [self->_rangeController setNeedsUpdate];
+      }
       [self->_rangeController updateIfNeeded];
     }
   });

diff --git a/Source/ASExperimentalFeatures.h b/Source/ASExperimentalFeatures.h
@@ -30,6 +30,7 @@ typedef NS_OPTIONS(NSUInteger, ASExperimentalFeatures) {
   ASExperimentalOptimizeDataControllerPipeline = 1 << 9,                    // exp_optimize_data_controller_pipeline
   ASExperimentalDisableGlobalTextkitLock = 1 << 10,                         // exp_disable_global_textkit_lock
   ASExperimentalMainThreadOnlyDataController = 1 << 11,                     // exp_main_thread_only_data_controller
+  ASExperimentalRangeUpdateOnChangesetUpdate = 1 << 12,                     // exp_range_update_on_changeset_update
   ASExperimentalFeatureAll = 0xFFFFFFFF
 };
 

diff --git a/Source/ASExperimentalFeatures.mm b/Source/ASExperimentalFeatures.mm
@@ -23,7 +23,8 @@
                                       @"exp_drawing_global",
                                       @"exp_optimize_data_controller_pipeline",
                                       @"exp_disable_global_textkit_lock",
-                                      @"exp_main_thread_only_data_controller"]));
+                                      @"exp_main_thread_only_data_controller",
+                                      @"exp_range_update_on_changeset_update"]));
   if (flags == ASExperimentalFeatureAll) {
     return allNames;
   }

diff --git a/Tests/ASCollectionViewTests.mm b/Tests/ASCollectionViewTests.mm
@@ -24,6 +24,7 @@ @interface ASTextCellNodeWithSetSelectedCounter : ASTextCellNode
 
 @property (nonatomic) NSUInteger setSelectedCounter;
 @property (nonatomic) NSUInteger applyLayoutAttributesCount;
+@property (nonatomic) NSUInteger didEnterPreloadStateCount;
 
 @end
 
@@ -40,6 +41,12 @@ - (void)applyLayoutAttributes:(UICollectionViewLayoutAttributes *)layoutAttribut
   _applyLayoutAttributesCount++;
 }
 
+- (void)didEnterPreloadState
+{
+  [super didEnterPreloadState];
+  _didEnterPreloadStateCount++;
+}
+
 @end
 
 @interface ASTestSectionContext : NSObject <ASSectionContext>
@@ -177,7 +184,8 @@ - (void)setUp
 {
   [super setUp];
   ASConfiguration *config = [ASConfiguration new];
-  config.experimentalFeatures = ASExperimentalOptimizeDataControllerPipeline;
+  config.experimentalFeatures = ASExperimentalOptimizeDataControllerPipeline
+                              | ASExperimentalRangeUpdateOnChangesetUpdate;
   [ASConfigurationManager test_resetWithConfiguration:config];
 }
 
@@ -518,6 +526,81 @@ - (void)testThatDeletingAndReloadingASectionThrowsAnException
   } completion:nil]);
 }
 
+- (void)testItemsInsertedIntoThePreloadRangeGetPreloaded
+{
+  updateValidationTestPrologue
+
+  ASRangeTuningParameters minimumPreloadParams = { .leadingBufferScreenfuls = 1, .trailingBufferScreenfuls = 1 };
+  [cn setTuningParameters:minimumPreloadParams forRangeMode:ASLayoutRangeModeMinimum rangeType:ASLayoutRangeTypePreload];
+  [cn updateCurrentRangeWithMode:ASLayoutRangeModeMinimum];
+
+  __weak ASCollectionViewTestController *weakController = testController;
+  NSIndexPath *lastVisibleIndex = [cv indexPathsForVisibleItems].lastObject;
+
+  NSInteger itemCount = weakController.asyncDelegate->_itemCounts[lastVisibleIndex.section];
+  BOOL isLastItemInSection = lastVisibleIndex.row == itemCount - 1;
+  NSInteger nextItemSection = isLastItemInSection ? lastVisibleIndex.section + 1 : lastVisibleIndex.section;
+  NSInteger nextItemRow = isLastItemInSection ? 0 : lastVisibleIndex.row + 1;
+
+  XCTAssertTrue(weakController.asyncDelegate->_itemCounts.size() > nextItemSection, @"There is no items after the last visible item. Update the section/row counts so that there is one for this test to work properly.");
+  XCTAssertTrue(weakController.asyncDelegate->_itemCounts[nextItemSection] > nextItemRow, @"There is no items after the last visible item. Update the section/row counts so that there is one for this test to work properly.");
+
+  NSIndexPath *nextItemIndexPath = [NSIndexPath indexPathForRow:nextItemRow inSection:nextItemSection];
+  ASTextCellNodeWithSetSelectedCounter *nodeBeforeUpdate = (ASTextCellNodeWithSetSelectedCounter *)[cv nodeForItemAtIndexPath:nextItemIndexPath];
+
+  XCTestExpectation *noChangeDone = [self expectationWithDescription:@"Batch update with no changes done and completion block has been called. Tuning params set to 1 screenful."];
+
+  __block ASTextCellNodeWithSetSelectedCounter *nodeAfterUpdate;
+  [cv performBatchUpdates:^{
+  } completion:^(BOOL finished) {
+    nodeAfterUpdate = (ASTextCellNodeWithSetSelectedCounter *)[cv nodeForItemAtIndexPath:nextItemIndexPath];
+    [noChangeDone fulfill];
+  }];
+
+  [self waitForExpectations:@[ noChangeDone ] timeout:1];
+
+  XCTAssertTrue(nodeBeforeUpdate == nodeAfterUpdate, @"Node should not have changed since no updates were made.");
+  XCTAssertTrue(nodeAfterUpdate.didEnterPreloadStateCount == 1, @"Node should have been preloaded.");
+
+  XCTestExpectation *changeDone = [self expectationWithDescription:@"Batch update with changes done and completion block has been called. Tuning params set to 1 screenful."];
+
+  [cv performBatchUpdates:^{
+    NSArray *indexPaths = @[ nextItemIndexPath ];
+    [cv deleteItemsAtIndexPaths:indexPaths];
+    [cv insertItemsAtIndexPaths:indexPaths];
+  } completion:^(BOOL finished) {
+    nodeAfterUpdate = (ASTextCellNodeWithSetSelectedCounter *)[cv nodeForItemAtIndexPath:nextItemIndexPath];
+    [changeDone fulfill];
+  }];
+
+  [self waitForExpectations:@[ changeDone ] timeout:1];
+
+  XCTAssertTrue(nodeBeforeUpdate != nodeAfterUpdate, @"Node should have changed after updating.");
+  XCTAssertTrue(nodeAfterUpdate.didEnterPreloadStateCount == 1, @"New node should have been preloaded.");
+
+  minimumPreloadParams = { .leadingBufferScreenfuls = 0, .trailingBufferScreenfuls = 0 };
+  [cn setTuningParameters:minimumPreloadParams forRangeMode:ASLayoutRangeModeMinimum rangeType:ASLayoutRangeTypePreload];
+  [cn updateCurrentRangeWithMode:ASLayoutRangeModeMinimum];
+
+  XCTestExpectation *changeDoneZeroSreenfuls = [self expectationWithDescription:@"Batch update with changes done and completion block has been called. Tuning params set to 0 screenful."];
+
+  nodeBeforeUpdate = nodeAfterUpdate;
+  __block ASTextCellNodeWithSetSelectedCounter *nodeAfterUpdateZeroSreenfuls;
+  [cv performBatchUpdates:^{
+    NSArray *indexPaths = @[ nextItemIndexPath ];
+    [cv deleteItemsAtIndexPaths:indexPaths];
+    [cv insertItemsAtIndexPaths:indexPaths];
+  } completion:^(BOOL finished) {
+    nodeAfterUpdateZeroSreenfuls = (ASTextCellNodeWithSetSelectedCounter *)[cv nodeForItemAtIndexPath:nextItemIndexPath];
+    [changeDoneZeroSreenfuls fulfill];
+  }];
+
+  [self waitForExpectations:@[ changeDoneZeroSreenfuls ] timeout:1];
+
+  XCTAssertTrue(nodeBeforeUpdate != nodeAfterUpdateZeroSreenfuls, @"Node should have changed after updating.");
+  XCTAssertTrue(nodeAfterUpdateZeroSreenfuls.didEnterPreloadStateCount == 0, @"New node should NOT have been preloaded.");
+}
+
 - (void)testCellNodeLayoutAttributes
 {
   updateValidationTestPrologue

diff --git a/Tests/ASConfigurationTests.mm b/Tests/ASConfigurationTests.mm
@@ -30,6 +30,7 @@
   ASExperimentalOptimizeDataControllerPipeline,
   ASExperimentalDisableGlobalTextkitLock,
   ASExperimentalMainThreadOnlyDataController,
+  ASExperimentalRangeUpdateOnChangesetUpdate,
 };
 
 @interface ASConfigurationTests : ASTestCase <ASConfigurationDelegate>
@@ -53,7 +54,8 @@ + (NSArray *)names {
     @"exp_drawing_global",
     @"exp_optimize_data_controller_pipeline",
     @"exp_disable_global_textkit_lock",
-    @"exp_main_thread_only_data_controller"
+    @"exp_main_thread_only_data_controller",
+    @"exp_range_update_on_changeset_update"
   ];
 }