From ea03bc959dcfdc5937655ca471135f874980f0ad Mon Sep 17 00:00:00 2001 From: Maxime Ollivier Date: Tue, 2 Jun 2020 13:02:01 -0700 Subject: [PATCH] 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 --- CHANGELOG.md | 2 + Source/IGListDiffKit/IGListExperiments.h | 2 - Source/IGListKit/IGListAdapter.m | 37 ++------- Tests/IGListAdapterTests.m | 101 ----------------------- 4 files changed, 11 insertions(+), 131 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1998592a7..588d7c513 100644 --- 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 index d20f53f1e..89d4a56b5 100644 --- 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 index c3ab4f959..645c844f0 100644 --- 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 index aa041fadf..86a985ce2 100644 --- 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];