avoid layout before IGListAdapter scrollToObject

Summary:
* Issue: `[IGListAdapter scrollToObject ...]` currently calls `[collectionView layoutIfNeeded]` before scrolling which creates the current visible cells that will no longer be visible after the scroll. This causes perf issues when we scroll immediately after creating the view controller.
* Fix: Instead of asking the layout object for the attributes, lets go throught the `UICollectionView`. So if the attributes are not ready, the `UICollectionView` will call `-prepareLayout`, return the attributes, but doesn't generate the cells just yet.

Differential Revision: D17326459

fbshipit-source-id: 745942225e0311fea7c3efb07ac1e8b8e0a82996

CHANGELOG.md

@@ -53,6 +53,8 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag
 
 - Fixed crash when calling `[UICollectionView layoutAttributesForSupplementaryElementOfKind...]` with `IGListCollectionViewLayout` and the section controller doesn't actually return a supplementary view [Maxime Ollivier](https://github.com/maxolls) (tbd)
 
+- Added `IGListExperimentAvoidLayoutOnScrollToObject` to avoid creating off-screen cells when calling `[IGListAdapter scrollToObject ...]`. [Maxime Ollivier](https://github.com/maxolls) (tbd)
+
 3.4.0
 -----
 

Source/Common/IGListExperiments.h

@@ -20,6 +20,8 @@ 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 deferring object creation until just before diffing.
     IGListExperimentDeferredToObjectCreation = 1 << 6,
     /// Test getting collection view at update time.

Source/IGListAdapter.m

@@ -192,13 +192,16 @@
     }
 
     UICollectionView *collectionView = self.collectionView;
-    UICollectionViewLayout *layout = self.collectionView.collectionViewLayout;
+    const BOOL avoidLayout = IGListExperimentEnabled(self.experiments, IGListExperimentAvoidLayoutOnScrollToObject);
 
-    // force layout before continuing
-    // this method is typcially called before pushing a view controller
-    // thus, before the layout process has actually happened
-    [collectionView setNeedsLayout];
-    [collectionView layoutIfNeeded];
+    // 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];
+    }
 
     NSIndexPath *indexPathFirstElement = [NSIndexPath indexPathForItem:0 inSection:section];
 
@@ -208,11 +211,13 @@
 
     const NSInteger numberOfItems = [collectionView numberOfItemsInSection:section];
     if (numberOfItems > 0) {
-        attributes = [self _layoutAttributesForIndexPath:indexPathFirstElement supplementaryKinds:supplementaryKinds].mutableCopy;
+        attributes = [self _layoutAttributesForItemAndSupplementaryViewAtIndexPath:indexPathFirstElement
+                                                                supplementaryKinds:supplementaryKinds].mutableCopy;
 
         if (numberOfItems > 1) {
             NSIndexPath *indexPathLastElement = [NSIndexPath indexPathForItem:(numberOfItems - 1) inSection:section];
-            UICollectionViewLayoutAttributes *lastElementattributes = [self _layoutAttributesForIndexPath:indexPathLastElement supplementaryKinds:supplementaryKinds].firstObject;
+            UICollectionViewLayoutAttributes *lastElementattributes = [self _layoutAttributesForItemAndSupplementaryViewAtIndexPath:indexPathLastElement
+                                                                                                                 supplementaryKinds:supplementaryKinds].firstObject;
             if (lastElementattributes != nil) {
                 [attributes addObject:lastElementattributes];
             }
@@ -220,7 +225,7 @@
     } else {
         NSMutableArray *supplementaryAttributes = [NSMutableArray new];
         for (NSString* supplementaryKind in supplementaryKinds) {
-            UICollectionViewLayoutAttributes *supplementaryAttribute = [layout layoutAttributesForSupplementaryViewOfKind:supplementaryKind atIndexPath:indexPathFirstElement];
+            UICollectionViewLayoutAttributes *supplementaryAttribute = [self _layoutAttributesForSupplementaryViewOfKind:supplementaryKind atIndexPath:indexPathFirstElement];
             if (supplementaryAttribute != nil) {
                 [supplementaryAttributes addObject: supplementaryAttribute];
             }
@@ -303,7 +308,15 @@
                     contentOffset.y = offsetMin - contentInset.top;
                     break;
             }
-            const CGFloat maxOffsetY = collectionView.contentSize.height - collectionView.frame.size.height + contentInset.bottom;
+            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;
+            }
+            const CGFloat maxOffsetY = maxHeight - collectionView.frame.size.height + contentInset.bottom;
             const CGFloat minOffsetY = -contentInset.top;
             contentOffset.y = MIN(contentOffset.y, maxOffsetY);
             contentOffset.y = MAX(contentOffset.y, minOffsetY);
@@ -726,18 +739,17 @@
     }
 }
 
-- (NSArray<UICollectionViewLayoutAttributes *> *)_layoutAttributesForIndexPath:(NSIndexPath *)indexPath
-                                                            supplementaryKinds:(NSArray<NSString *> *)supplementaryKinds {
-    UICollectionViewLayout *layout = self.collectionView.collectionViewLayout;
+- (NSArray<UICollectionViewLayoutAttributes *> *)_layoutAttributesForItemAndSupplementaryViewAtIndexPath:(NSIndexPath *)indexPath
+                                                                                      supplementaryKinds:(NSArray<NSString *> *)supplementaryKinds {
     NSMutableArray<UICollectionViewLayoutAttributes *> *attributes = [NSMutableArray new];
 
-    UICollectionViewLayoutAttributes *cellAttributes = [layout layoutAttributesForItemAtIndexPath:indexPath];
+    UICollectionViewLayoutAttributes *cellAttributes = [self _layoutAttributesForItemAtIndexPath:indexPath];
     if (cellAttributes) {
         [attributes addObject:cellAttributes];
     }
 
     for (NSString *kind in supplementaryKinds) {
-        UICollectionViewLayoutAttributes *supplementaryAttributes = [layout layoutAttributesForSupplementaryViewOfKind:kind atIndexPath:indexPath];
+        UICollectionViewLayoutAttributes *supplementaryAttributes = [self _layoutAttributesForSupplementaryViewOfKind:kind atIndexPath:indexPath];
         if (supplementaryAttributes) {
             [attributes addObject:supplementaryAttributes];
         }
@@ -746,6 +758,23 @@
     return attributes;
 }
 
+- (nullable UICollectionViewLayoutAttributes *)_layoutAttributesForItemAtIndexPath:(NSIndexPath *)indexPath {
+    if (IGListExperimentEnabled(self.experiments, IGListExperimentAvoidLayoutOnScrollToObject)) {
+        return [self.collectionView layoutAttributesForItemAtIndexPath:indexPath];
+    } else {
+        return [self.collectionView.collectionViewLayout 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];
+    }
+}
+
 - (void)mapView:(UICollectionReusableView *)view toSectionController:(IGListSectionController *)sectionController {
     IGAssertMainThread();
     IGParameterAssert(view != nil);
@@ -1072,7 +1101,6 @@
     IGAssert(self.collectionView != nil, @"Performing batch updates without a collection view.");
 
     [self _enterBatchUpdates];
-    
     __weak __typeof__(self) weakSelf = self;
     [self.updater performUpdateWithCollectionViewBlock:[self _collectionViewBlock] animated:animated itemUpdates:^{
         weakSelf.isInUpdateBlock = YES;
@@ -1202,11 +1230,11 @@
     IGParameterAssert(sectionController != nil);
     UICollectionView *collectionView = self.collectionView;
     IGAssert(collectionView != nil, @"Invalidating items from %@ without a collection view at indexes %@.", sectionController, indexes);
-    
+
     if (indexes.count == 0) {
         return;
     }
-    
+
     NSArray *indexPaths = [self indexPathsFromSectionController:sectionController indexes:indexes usePreviousIfInUpdateBlock:NO];
     UICollectionViewLayout *layout = collectionView.collectionViewLayout;
     UICollectionViewLayoutInvalidationContext *context = [[[layout.class invalidationContextClass] alloc] init];
@@ -1317,4 +1345,3 @@
 }
 
 @end
-

Source/Internal/IGListAdapter+UICollectionView.m

@@ -60,15 +60,15 @@
 
     return view;
 }
-    
+
 - (BOOL)collectionView:(UICollectionView *)collectionView canMoveItemAtIndexPath:(NSIndexPath *)indexPath {
     const NSInteger sectionIndex = indexPath.section;
     const NSInteger itemIndex = indexPath.item;
-    
+
     IGListSectionController *sectionController = [self sectionControllerForSection:sectionIndex];
     return [sectionController canMoveItemAtIndex:itemIndex];
 }
-    
+
 - (void)collectionView:(UICollectionView *)collectionView
    moveItemAtIndexPath:(NSIndexPath *)sourceIndexPath
            toIndexPath:(NSIndexPath *)destinationIndexPath {
@@ -241,7 +241,7 @@
 
 - (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout *)collectionViewLayout sizeForItemAtIndexPath:(NSIndexPath *)indexPath {
     IGAssert(![self.collectionViewDelegate respondsToSelector:_cmd], @"IGListAdapter is consuming method also implemented by the collectionViewDelegate: %@", NSStringFromSelector(_cmd));
-    
+
     CGSize size = [self sizeForItemAtIndexPath:indexPath];
     IGAssert(!isnan(size.height), @"IGListAdapter returned NaN height = %f for item at indexPath <%@>", size.height, indexPath);
     IGAssert(!isnan(size.width), @"IGListAdapter returned NaN width = %f for item at indexPath <%@>", size.width, indexPath);

Tests/IGListAdapterTests.m

@@ -655,6 +655,15 @@
 }
 
 - (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];
@@ -675,6 +684,15 @@
 }
 
 - (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);
@@ -687,6 +705,15 @@
 }
 
 - (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];
 
@@ -712,6 +739,15 @@
 }
 
 - (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];
@@ -737,6 +773,15 @@
 }
 
 - (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];
     
@@ -765,6 +810,15 @@
 }
 
 - (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];
     
@@ -797,6 +851,15 @@
 }
 
 - (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;
@@ -822,6 +885,15 @@
 }
 
 - (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];
 
@@ -844,6 +916,15 @@
 }
 
 - (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];
 
@@ -867,6 +948,15 @@
 }
 
 - (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);
@@ -875,6 +965,15 @@
 }
 
 - (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];
@@ -891,6 +990,30 @@
     IGAssertEqualPoint([self.collectionView contentOffset], 0, 0);
 }
 
+- (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];
+    self.adapter.dataSource = self.dataSource;
+
+    // # of items for each object == [item integerValue], so @2 has 2 items (cells)
+    // Assumptions: UICollectionView size is (100,100), each cell size is (100,10)
+    [self.adapter scrollToObject:@22 supplementaryKinds:nil scrollDirection:UICollectionViewScrollDirectionVertical scrollPosition:UICollectionViewScrollPositionTop animated:NO];
+
+    // Force the layout, which creates the cells
+    [self.collectionView layoutIfNeeded];
+
+    IGListTestSection *firstSection = [self.adapter sectionControllerForObject:@20];
+    XCTAssertNotNil(firstSection);
+    IGListTestSection *lastSection = [self.adapter sectionControllerForObject:@22];
+    XCTAssertNotNil(lastSection);
+
+    XCTAssertFalse(firstSection.wasDisplayed);
+    XCTAssertTrue(lastSection.wasDisplayed);
+}
+
 - (void)test_whenQueryingIndexPath_withOOBSectionController_thatNilReturned {
     self.dataSource.objects = @[@0, @1, @2];
     [self.adapter reloadDataWithCompletion:nil];

Tests/Objects/IGListTestSection.h

@@ -20,5 +20,6 @@
 @property (nonatomic, assign) BOOL wasDeselected;
 @property (nonatomic, assign) BOOL wasHighlighted;
 @property (nonatomic, assign) BOOL wasUnhighlighted;
+@property (nonatomic, assign) BOOL wasDisplayed;
 
 @end

Tests/Objects/IGListTestSection.m

@@ -9,11 +9,15 @@
 
 #import "IGListTestSection.h"
 
+@interface IGListTestSection () <IGListDisplayDelegate>
+@end
+
 @implementation IGListTestSection
 
 - (instancetype)init {
     if (self = [super init]) {
         _size = CGSizeMake(100, 10);
+        self.displayDelegate = self;
     }
     return self;
 }
@@ -58,4 +62,18 @@
     self.wasUnhighlighted = YES;
 }
 
+#pragma mark - IGListDisplayDelegate
+
+- (void)listAdapter:(IGListAdapter *)listAdapter willDisplaySectionController:(IGListSectionController *)sectionController {
+    _wasDisplayed = YES;
+}
+
+- (void)listAdapter:(IGListAdapter *)listAdapter didEndDisplayingSectionController:(IGListSectionController *)sectionController {}
+- (void)listAdapter:(IGListAdapter *)listAdapter willDisplaySectionController:(IGListSectionController *)sectionController
+               cell:(UICollectionViewCell *)cell
+            atIndex:(NSInteger)index {}
+- (void)listAdapter:(IGListAdapter *)listAdapter didEndDisplayingSectionController:(IGListSectionController *)sectionController
+               cell:(UICollectionViewCell *)cell
+            atIndex:(NSInteger)index {}
+
 @end