Add experiment to ensure ASCollectionView's range controller updates on changeset updates (TextureGroup#1976)

rqueue · rqueue · commit b3848d7ccd57 · 2021-03-26T11:51:00.000-07:00
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.
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"
   ];
 }
 

Original file line number	Diff line number	Diff line change
`@@ -2311,6 +2311,9 @@ - (void)rangeController:(ASRangeController *)rangeController updateWithChangeSet`
`2311`	`2311`
`2312`	`2312`	`// Flush any range changes that happened as part of submitting the update.`
`2313`	`2313`	`as_activity_scope(changeSet.rootActivity);`
	`2314`	`+ if (numberOfUpdates > 0 && ASActivateExperimentalFeature(ASExperimentalRangeUpdateOnChangesetUpdate)) {`
	`2315`	`+ [self->_rangeController setNeedsUpdate];`
	`2316`	`+ }`
`2314`	`2317`	`[self->_rangeController updateIfNeeded];`
`2315`	`2318`	`}`
`2316`	`2319`	`});`