From 6faddd99c95428cf42bb38684464b458ef1455c0 Mon Sep 17 00:00:00 2001 From: Maxime Ollivier Date: Fri, 13 Sep 2019 14:55:51 -0700 Subject: [PATCH] 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 | 2 + Source/Common/IGListExperiments.h | 2 + Source/IGListAdapter.m | 65 ++++++--- .../Internal/IGListAdapter+UICollectionView.m | 8 +- Tests/IGListAdapterTests.m | 123 ++++++++++++++++++ Tests/Objects/IGListTestSection.h | 1 + Tests/Objects/IGListTestSection.m | 18 +++ 7 files changed, 196 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bfb7996..8e97f0e2 100644 --- a/CHANGELOG.md +++ b/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 ----- diff --git a/Source/Common/IGListExperiments.h b/Source/Common/IGListExperiments.h index cc1d663c..7a82ecf0 100644 --- a/Source/Common/IGListExperiments.h +++ b/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. diff --git a/Source/IGListAdapter.m b/Source/IGListAdapter.m index 95099d9e..b895e150 100644 --- a/Source/IGListAdapter.m +++ b/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 *)_layoutAttributesForIndexPath:(NSIndexPath *)indexPath - supplementaryKinds:(NSArray *)supplementaryKinds { - UICollectionViewLayout *layout = self.collectionView.collectionViewLayout; +- (NSArray *)_layoutAttributesForItemAndSupplementaryViewAtIndexPath:(NSIndexPath *)indexPath + supplementaryKinds:(NSArray *)supplementaryKinds { NSMutableArray *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 - diff --git a/Source/Internal/IGListAdapter+UICollectionView.m b/Source/Internal/IGListAdapter+UICollectionView.m index fcef6388..c7a44cb9 100644 --- a/Source/Internal/IGListAdapter+UICollectionView.m +++ b/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); diff --git a/Tests/IGListAdapterTests.m b/Tests/IGListAdapterTests.m index 9d96caca..08cce1ef 100644 --- a/Tests/IGListAdapterTests.m +++ b/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]; diff --git a/Tests/Objects/IGListTestSection.h b/Tests/Objects/IGListTestSection.h index 0899e938..ca40f3d7 100644 --- a/Tests/Objects/IGListTestSection.h +++ b/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 diff --git a/Tests/Objects/IGListTestSection.m b/Tests/Objects/IGListTestSection.m index 7010b760..a114fbfb 100644 --- a/Tests/Objects/IGListTestSection.m +++ b/Tests/Objects/IGListTestSection.m @@ -9,11 +9,15 @@ #import "IGListTestSection.h" +@interface IGListTestSection () +@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