Skip to content

Commit

Permalink
avoid layout before IGListAdapter scrollToObject
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Maxime Ollivier authored and facebook-github-bot committed Sep 13, 2019
1 parent cddb297 commit 6faddd9
Show file tree
Hide file tree
Showing 7 changed files with 197 additions and 24 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
-----

Expand Down
2 changes: 2 additions & 0 deletions Source/Common/IGListExperiments.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
67 changes: 47 additions & 20 deletions Source/IGListAdapter.m
Original file line number Diff line number Diff line change
Expand Up @@ -192,13 +192,16 @@ - (void)scrollToObject:(id)object
}

UICollectionView *collectionView = self.collectionView;
UICollectionViewLayout *layout = self.collectionView.collectionViewLayout;

// 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];
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];
}

NSIndexPath *indexPathFirstElement = [NSIndexPath indexPathForItem:0 inSection:section];

Expand All @@ -208,19 +211,21 @@ - (void)scrollToObject:(id)object

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];
}
}
} 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];
}
Expand Down Expand Up @@ -303,7 +308,15 @@ - (void)scrollToObject:(id)object
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);
Expand Down Expand Up @@ -726,18 +739,17 @@ - (NSIndexPath *)indexPathForSectionController:(IGListSectionController *)contro
}
}

- (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];
}
Expand All @@ -746,6 +758,23 @@ - (NSIndexPath *)indexPathForSectionController:(IGListSectionController *)contro
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);
Expand Down Expand Up @@ -1072,7 +1101,6 @@ - (void)performBatchAnimated:(BOOL)animated updates:(void (^)(id<IGListBatchCont
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;
Expand Down Expand Up @@ -1202,11 +1230,11 @@ - (void)invalidateLayoutInSectionController:(IGListSectionController *)sectionCo
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];
Expand Down Expand Up @@ -1317,4 +1345,3 @@ - (void)revertInvalidInteractiveMoveFromIndexPath:(NSIndexPath *)sourceIndexPath
}

@end

8 changes: 4 additions & 4 deletions Source/Internal/IGListAdapter+UICollectionView.m
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,15 @@ - (UICollectionReusableView *)collectionView:(UICollectionView *)collectionView

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 {
Expand Down Expand Up @@ -241,7 +241,7 @@ - (void)collectionView:(UICollectionView *)collectionView didUnhighlightItemAtIn

- (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);
Expand Down
Loading

0 comments on commit 6faddd9

Please sign in to comment.