ship IGListExperimentAvoidLayoutOnScrollToObject

Summary: Remove `[collectionView layoutIfNeeded]` before scrolling in `[IGListAdapter scrollToObject...]` to avoid creating off-screen cells. Reviewed By: apadalko Differential Revision: D21822496 fbshipit-source-id: 3251beed489c1e3e8d7a872638e37a5580ec0f4f
Instagram · Jun 2, 2020 · ea03bc9 · ea03bc9
1 parent 677ce77
commit ea03bc9
Show file tree

Hide file tree

Showing 4 changed files with 11 additions and 131 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,6 +14,8 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag
 
 - `IGListCollectionViewLayout` should get the section/index counts via `UICollectionView` to stay in sync, instead of the `dataSource` [Maxime Ollivier](https://github.com/maxolls) (tbd)
 
+- Remove `[collectionView layoutIfNeeded]` before scrolling in `[IGListAdapter scrollToObject...]` to avoid creating off-screen cells. [Maxime Ollivier](https://github.com/maxolls) (tbd)
+
 4.0.0
 -----
 ### Breaking Changes

diff --git a/Source/IGListDiffKit/IGListExperiments.h b/Source/IGListDiffKit/IGListExperiments.h
@@ -20,8 +20,6 @@ typedef NS_OPTIONS (NSInteger, IGListExperiment) {
     IGListExperimentBackgroundDiffing = 1 << 2,
     /// Test fallback to reloadData when "too many" update operations.
     IGListExperimentReloadDataFallback = 1 << 3,
-    /// Test removing the layout pass when calling scrollToObject to avoid creating off-screen cells.
-    IGListExperimentAvoidLayoutOnScrollToObject = 1 << 4,
     /// Test fixing a crash when inserting and deleting the same NSIndexPath multiple times.
     IGListExperimentFixIndexPathImbalance = 1 << 5,
     /// Test deferring object creation until just before diffing.

diff --git a/Source/IGListKit/IGListAdapter.m b/Source/IGListKit/IGListAdapter.m
@@ -184,16 +184,10 @@ - (void)scrollToObject:(id)object
     }
 
     UICollectionView *collectionView = self.collectionView;
-    const BOOL avoidLayout = IGListExperimentEnabled(self.experiments, IGListExperimentAvoidLayoutOnScrollToObject);
-
-    // Experiment with skipping the forced layout to avoid creating off-screen cells.
-    // Calling [collectionView layoutIfNeeded] creates the current visible cells that will no longer be visible after the scroll.
-    // We can avoid that by asking the UICollectionView (not the layout object) for the attributes. So if the attributes are not
-    // ready, the UICollectionView will call -prepareLayout, return the attributes, but doesn't generate the cells just yet.
-    if (!avoidLayout) {
-        [collectionView setNeedsLayout];
-        [collectionView layoutIfNeeded];
-    }
+
+    // We avoid calling `[collectionView layoutIfNeeded]` here because that could create cells that will no longer be visible after the scroll.
+    // Note that we get the layout attributes from the `UICollectionView` instead of the `collectionViewLayout`, because that will generate the
+    // necessary attributes without creating the cells just yet.
 
     NSIndexPath *indexPathFirstElement = [NSIndexPath indexPathForItem:0 inSection:section];
 
@@ -300,14 +294,9 @@ - (void)scrollToObject:(id)object
                     contentOffset.y = offsetMin - contentInset.top;
                     break;
             }
-            CGFloat maxHeight;
-            if (avoidLayout) {
-                // If we don't call [collectionView layoutIfNeeded], the collectionView.contentSize does not get updated.
-                // So lets use the layout object, since it should have been updated by now.
-                maxHeight = collectionView.collectionViewLayout.collectionViewContentSize.height;
-            } else {
-                maxHeight = collectionView.contentSize.height;
-            }
+            // If we don't call [collectionView layoutIfNeeded], the collectionView.contentSize does not get updated.
+            // So lets use the layout object, since it should have been updated by now.
+            const CGFloat maxHeight = collectionView.collectionViewLayout.collectionViewContentSize.height;
             const CGFloat maxOffsetY = maxHeight - collectionView.frame.size.height + contentInset.bottom;
             const CGFloat minOffsetY = -contentInset.top;
             contentOffset.y = MIN(contentOffset.y, maxOffsetY);
@@ -752,20 +741,12 @@ - (NSIndexPath *)indexPathForSectionController:(IGListSectionController *)contro
 }
 
 - (nullable UICollectionViewLayoutAttributes *)_layoutAttributesForItemAtIndexPath:(NSIndexPath *)indexPath {
-    if (IGListExperimentEnabled(self.experiments, IGListExperimentAvoidLayoutOnScrollToObject)) {
-        return [self.collectionView layoutAttributesForItemAtIndexPath:indexPath];
-    } else {
-        return [self.collectionView.collectionViewLayout layoutAttributesForItemAtIndexPath:indexPath];
-    }
+    return [self.collectionView layoutAttributesForItemAtIndexPath:indexPath];
 }
 
 - (nullable UICollectionViewLayoutAttributes *)_layoutAttributesForSupplementaryViewOfKind:(NSString *)elementKind
                                                                                atIndexPath:(NSIndexPath *)indexPath {
-    if (IGListExperimentEnabled(self.experiments, IGListExperimentAvoidLayoutOnScrollToObject)) {
-        return [self.collectionView layoutAttributesForSupplementaryElementOfKind:elementKind atIndexPath:indexPath];
-    } else {
-        return [self.collectionView.collectionViewLayout layoutAttributesForSupplementaryViewOfKind:elementKind atIndexPath:indexPath];
-    }
+    return [self.collectionView layoutAttributesForSupplementaryElementOfKind:elementKind atIndexPath:indexPath];
 }
 
 - (void)mapView:(UICollectionReusableView *)view toSectionController:(IGListSectionController *)sectionController {

diff --git a/Tests/IGListAdapterTests.m b/Tests/IGListAdapterTests.m
@@ -655,15 +655,6 @@ - (void)test_whenAdapterUpdated_thatVisibleCellsForNilObjectIsEmpty {
 }
 
 - (void)test_whenScrollVerticallyToItem {
-    [self performTest_whenScrollVerticallyToItem];
-}
-
-- (void)test_whenScrollVerticallyToItem_withExperiment {
-    self.adapter.experiments = self.adapter.experiments | IGListExperimentAvoidLayoutOnScrollToObject;
-    [self performTest_whenScrollVerticallyToItem];
-}
-
-- (void)performTest_whenScrollVerticallyToItem {
     // # of items for each object == [item integerValue], so @2 has 2 items (cells)
     self.dataSource.objects = @[@1, @2, @3, @4, @5, @6];
     [self.adapter reloadDataWithCompletion:nil];
@@ -684,15 +675,6 @@ - (void)performTest_whenScrollVerticallyToItem {
 }
 
 - (void)test_whenScrollVerticallyToItemInASectionWithNoCellsAndNoSupplymentaryView {
-    [self performTest_whenScrollVerticallyToItemInASectionWithNoCellsAndNoSupplymentaryView];
-}
-
-- (void)test_whenScrollVerticallyToItemInASectionWithNoCellsAndNoSupplymentaryView_withExperiment {
-    self.adapter.experiments = self.adapter.experiments | IGListExperimentAvoidLayoutOnScrollToObject;
-    [self performTest_whenScrollVerticallyToItemInASectionWithNoCellsAndNoSupplymentaryView];
-}
-
-- (void)performTest_whenScrollVerticallyToItemInASectionWithNoCellsAndNoSupplymentaryView {
     self.dataSource.objects = @[@1, @0, @300];
     [self.adapter reloadDataWithCompletion:nil];
     XCTAssertEqual([self.collectionView numberOfSections], 3);
@@ -705,15 +687,6 @@ - (void)performTest_whenScrollVerticallyToItemInASectionWithNoCellsAndNoSupplyme
 }
 
 - (void)test_whenScrollVerticallyToItemInASectionWithNoCellsButAHeaderSupplymentaryView {
-    [self performTest_whenScrollVerticallyToItemInASectionWithNoCellsButAHeaderSupplymentaryView];
-}
-
-- (void)test_whenScrollVerticallyToItemInASectionWithNoCellsButAHeaderSupplymentaryView_withExperiment {
-    self.adapter.experiments = self.adapter.experiments | IGListExperimentAvoidLayoutOnScrollToObject;
-    [self performTest_whenScrollVerticallyToItemInASectionWithNoCellsButAHeaderSupplymentaryView];
-}
-
-- (void)performTest_whenScrollVerticallyToItemInASectionWithNoCellsButAHeaderSupplymentaryView {
     self.dataSource.objects = @[@1, @0, @300];
     [self.adapter reloadDataWithCompletion:nil];
 
@@ -739,15 +712,6 @@ - (void)performTest_whenScrollVerticallyToItemInASectionWithNoCellsButAHeaderSup
 }
 
 - (void)test_whenScrollVerticallyToItemWithPositionning {
-    [self performTest_whenScrollVerticallyToItemWithPositionning];
-}
-
-- (void)test_whenScrollVerticallyToItemWithPositionning_withExperiment {
-    self.adapter.experiments = self.adapter.experiments | IGListExperimentAvoidLayoutOnScrollToObject;
-    [self performTest_whenScrollVerticallyToItemWithPositionning];
-}
-
-- (void)performTest_whenScrollVerticallyToItemWithPositionning {
     self.dataSource.objects = @[@1, @100, @200];
     [self.adapter reloadDataWithCompletion:nil];
     [self.adapter scrollToObject:@1 supplementaryKinds:nil scrollDirection:UICollectionViewScrollDirectionVertical scrollPosition:UICollectionViewScrollPositionNone animated:NO];
@@ -773,15 +737,6 @@ - (void)performTest_whenScrollVerticallyToItemWithPositionning {
 }
 
 - (void)test_whenScrollVerticallyToBottom_withContentInsets_thatBottomFlushWithCollectionViewBounds {
-    [self performTest_whenScrollVerticallyToBottom_withContentInsets_thatBottomFlushWithCollectionViewBounds];
-}
-
-- (void)test_whenScrollVerticallyToBottom_withContentInsets_thatBottomFlushWithCollectionViewBounds_withExperiment {
-    self.adapter.experiments = self.adapter.experiments | IGListExperimentAvoidLayoutOnScrollToObject;
-    [self performTest_whenScrollVerticallyToBottom_withContentInsets_thatBottomFlushWithCollectionViewBounds];
-}
-
-- (void)performTest_whenScrollVerticallyToBottom_withContentInsets_thatBottomFlushWithCollectionViewBounds {
     self.dataSource.objects = @[@100];
     [self.adapter reloadDataWithCompletion:nil];
 
@@ -810,15 +765,6 @@ - (void)performTest_whenScrollVerticallyToBottom_withContentInsets_thatBottomFlu
 }
 
 - (void)test_whenScrollHorizontalToRight_withContentInsets_thatRightFlushWithCollectionViewBounds {
-    [self performTest_whenScrollHorizontalToRight_withContentInsets_thatRightFlushWithCollectionViewBounds];
-}
-
-- (void)test_whenScrollHorizontalToRight_withContentInsets_thatRightFlushWithCollectionViewBounds_withExperiment {
-    self.adapter.experiments = self.adapter.experiments | IGListExperimentAvoidLayoutOnScrollToObject;
-    [self performTest_whenScrollHorizontalToRight_withContentInsets_thatRightFlushWithCollectionViewBounds];
-}
-
-- (void)performTest_whenScrollHorizontalToRight_withContentInsets_thatRightFlushWithCollectionViewBounds {
     self.dataSource.objects = @[@100];
     [self.adapter reloadDataWithCompletion:nil];
 
@@ -851,15 +797,6 @@ - (void)performTest_whenScrollHorizontalToRight_withContentInsets_thatRightFlush
 }
 
 - (void)test_whenScrollHorizontallyToItem {
-    [self performTest_whenScrollHorizontallyToItem];
-}
-
-- (void)test_whenScrollHorizontallyToItem_withExperiment {
-    self.adapter.experiments = self.adapter.experiments | IGListExperimentAvoidLayoutOnScrollToObject;
-    [self performTest_whenScrollHorizontallyToItem];
-}
-
-- (void)performTest_whenScrollHorizontallyToItem {
     // # of items for each object == [item integerValue], so @2 has 2 items (cells)
     IGListTestAdapterHorizontalDataSource *dataSource = [[IGListTestAdapterHorizontalDataSource alloc] init];
     self.adapter.dataSource = dataSource;
@@ -885,15 +822,6 @@ - (void)performTest_whenScrollHorizontallyToItem {
 }
 
 - (void)test_whenScrollToItem_thatSupplementarySourceSupportsSingleHeader {
-    [self performTest_whenScrollToItem_thatSupplementarySourceSupportsSingleHeader];
-}
-
-- (void)test_whenScrollToItem_thatSupplementarySourceSupportsSingleHeader_withExperiment {
-    self.adapter.experiments = self.adapter.experiments | IGListExperimentAvoidLayoutOnScrollToObject;
-    [self performTest_whenScrollToItem_thatSupplementarySourceSupportsSingleHeader];
-}
-
-- (void)performTest_whenScrollToItem_thatSupplementarySourceSupportsSingleHeader {
     self.dataSource.objects = @[@1, @2];
     [self.adapter reloadDataWithCompletion:nil];
 
@@ -916,15 +844,6 @@ - (void)performTest_whenScrollToItem_thatSupplementarySourceSupportsSingleHeader
 }
 
 - (void)test_whenScrollToItem_thatSupplementarySourceSupportsHeaderAndFooter {
-    [self performTest_whenScrollToItem_thatSupplementarySourceSupportsHeaderAndFooter];
-}
-
-- (void)test_whenScrollToItem_thatSupplementarySourceSupportsHeaderAndFooter_withExperiment {
-    self.adapter.experiments = self.adapter.experiments | IGListExperimentAvoidLayoutOnScrollToObject;
-    [self performTest_whenScrollToItem_thatSupplementarySourceSupportsHeaderAndFooter];
-}
-
-- (void)performTest_whenScrollToItem_thatSupplementarySourceSupportsHeaderAndFooter {
     self.dataSource.objects = @[@1, @2];
     [self.adapter reloadDataWithCompletion:nil];
 
@@ -948,15 +867,6 @@ - (void)performTest_whenScrollToItem_thatSupplementarySourceSupportsHeaderAndFoo
 }
 
 - (void)test_whenScrollVerticallyToItem_thatFeedIsEmpty {
-    [self performTest_whenScrollVerticallyToItem_thatFeedIsEmpty];
-}
-
-- (void)test_whenScrollVerticallyToItem_thatFeedIsEmpty_withExperiment {
-    self.adapter.experiments = self.adapter.experiments | IGListExperimentAvoidLayoutOnScrollToObject;
-    [self performTest_whenScrollVerticallyToItem_thatFeedIsEmpty];
-}
-
-- (void)performTest_whenScrollVerticallyToItem_thatFeedIsEmpty {
     self.dataSource.objects = @[];
     [self.adapter reloadDataWithCompletion:nil];
     XCTAssertEqual([self.collectionView numberOfSections], 0);
@@ -965,15 +875,6 @@ - (void)performTest_whenScrollVerticallyToItem_thatFeedIsEmpty {
 }
 
 - (void)test_whenScrollVerticallyToItem_thatItemNotInFeed {
-    [self performTest_whenScrollVerticallyToItem_thatItemNotInFeed];
-}
-
-- (void)test_whenScrollVerticallyToItem_thatItemNotInFeed_withExperiment {
-    self.adapter.experiments = self.adapter.experiments | IGListExperimentAvoidLayoutOnScrollToObject;
-    [self performTest_whenScrollVerticallyToItem_thatItemNotInFeed];
-}
-
-- (void)performTest_whenScrollVerticallyToItem_thatItemNotInFeed {
     // # of items for each object == [item integerValue], so @2 has 2 items (cells)
     self.dataSource.objects = @[@1, @2, @3, @4];
     [self.adapter reloadDataWithCompletion:nil];
@@ -991,8 +892,6 @@ - (void)performTest_whenScrollVerticallyToItem_thatItemNotInFeed {
 }
 
 - (void)test_whenScrollToItem_thatNonVisibleCellsDidNotAppear {
-    self.adapter.experiments = self.adapter.experiments | IGListExperimentAvoidLayoutOnScrollToObject;
-
     // Regenerate the source with existing objects
     self.dataSource = [IGListTestAdapterDataSource new];
     self.dataSource.objects = @[@20, @22];
-Original file line number
+Diff line change
@@ Expand Up @@
     - `IGListCollectionViewLayout` should get the section/index counts via `UICollectionView` to stay in sync, instead of the `dataSource` [Maxime Ollivier](https://github.com/maxolls) (tbd)
+    - Remove `[collectionView layoutIfNeeded]` before scrolling in `[IGListAdapter scrollToObject...]` to avoid creating off-screen cells. [Maxime Ollivier](https://github.com/maxolls) (tbd)
 .0.0
     -----
     ### Breaking Changes
@@ Expand Down @@