Add experiment to have the collection view layout check the collection view instead of the collection view layout [2/2]

Summary:
The collection view layout here should not be accessing the data source directly but instead asking the collection view for the number of sections and number of items. This is because the collection view holds an internal cache and if there is a discrepancy between when the collection view cache and the data source, preparing the layout will crash the app because we will send a layout down to the collection view with a items that might not exist in the collection view's cache but exist in the data source.

This diff adds an experiment to see the impact of fixing this check so that when we prepare layouts we don't ask the data source directly but ask the collection view instead. The collection view will then update it's cache and check the data source so it does not crash when we prepare a layout.

```
NSInternalInconsistencyException at __45-[UICollectionViewData validateLayoutInRect:]_block_invoke:
UICollectionView received layout attributes for a cell with an index path that does not exist:
<NSIndexPath: 0xc198a8c3ea391fbb> {length = 2, path = 4 - 0}
```

Reviewed By: lorixx

Differential Revision: D15830610

fbshipit-source-id: f3dae3e87c55b86c3b686309c1144ccfff1375bf
This commit is contained in:
Aaron Pang 2019-06-14 16:19:38 -07:00 committed by Facebook Github Bot
parent 8a0c69ba29
commit 78cca5bc43
4 changed files with 100 additions and 88 deletions

View file

@ -26,6 +26,8 @@ typedef NS_OPTIONS (NSInteger, IGListExperiment) {
IGListExperimentGetCollectionViewAtUpdate = 1 << 7,
/// Test invalidating layout when cell reloads/updates in IGListBindingSectionController.
IGListExperimentInvalidateLayoutForUpdates = 1 << 8,
/// Test using the collection view when asking for layout instead of accessing the data source. Only apply to IGListCollectionViewLayout.
IGListExperimentUseCollectionViewInsteadOfDataSourceInLayout = 1 << 9
};
/**

View file

@ -1072,7 +1072,7 @@
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;

View file

@ -8,6 +8,7 @@
#import <UIKit/UIKit.h>
#import <IGListKit/IGListMacros.h>
#import <IGListKit/IGListExperiments.h>
#import "IGListCollectionViewLayoutCompatible.h"
@ -93,6 +94,11 @@ NS_SWIFT_NAME(ListCollectionViewLayout)
*/
@property (nonatomic, assign) BOOL showHeaderWhenEmpty;
/**
A bitmask of experiments to conduct on the adapter.
*/
@property (nonatomic, assign) IGListExperiment experiments;
/**
Create and return a new collection view layout.

View file

@ -75,7 +75,7 @@ static NSInteger IGListMergeMinimumInvalidatedSection(NSInteger section, NSInteg
} else if (otherSection == NSNotFound) {
return section;
}
return MIN(section, otherSection);
}
@ -86,28 +86,28 @@ struct IGListSectionEntry {
to build layout attributes given a rect.
*/
CGRect bounds;
// The insets for the section. Used to find total content size of the section.
UIEdgeInsets insets;
// The RESTING frame of the header view (e.g. when the header is not sticking to the top of the scroll view).
CGRect headerBounds;
// The RESTING frame of the footer view
CGRect footerBounds;
// An array of frames for each cell in the section.
std::vector<CGRect> itemBounds;
// last item distance in scroll direction, used for partial invalidation
CGFloat lastItemCoordInScrollDirection;
// last item distance in fixed direction, used for partial invalidation
CGFloat lastItemCoordInFixedDirection;
// last next row distance in scroll direction, used for partial invalidation
CGFloat lastNextRowCoordInScrollDirection;
// Returns YES when the section has visible content (header and/or items).
BOOL isValid() {
return !CGSizeEqualToSize(bounds.size, CGSizeZero);
@ -121,7 +121,7 @@ struct IGListSectionEntry {
static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attributes) {
const NSInteger maxZIndexPerSection = 1000;
const NSInteger baseZIndex = attributes.indexPath.section * maxZIndexPerSection;
switch (attributes.representedElementCategory) {
case UICollectionElementCategoryCell:
attributes.zIndex = baseZIndex + attributes.indexPath.item;
@ -154,10 +154,10 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
@implementation IGListCollectionViewLayout {
std::vector<IGListSectionEntry> _sectionData;
NSMutableDictionary<NSIndexPath *, UICollectionViewLayoutAttributes *> *_attributesCache;
// invalidate starting at this section
NSInteger _minimumInvalidatedSection;
/**
The workflow for getting sticky headers working:
1. Use a custom invalidation context to mark supplementary attributes invalid.
@ -231,17 +231,17 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
- (NSArray<UICollectionViewLayoutAttributes *> *)layoutAttributesForElementsInRect:(CGRect)rect {
IGAssertMainThread();
NSMutableArray *result = [NSMutableArray new];
const NSRange range = [self _rangeOfSectionsInRect:rect];
if (range.location == NSNotFound) {
return nil;
}
for (NSInteger section = range.location; section < NSMaxRange(range); section++) {
const NSInteger itemCount = _sectionData[section].itemBounds.size();
// do not add headers if there are no items
if (itemCount > 0 || self.showHeaderWhenEmpty) {
for (NSString *elementKind in _supplementaryAttributesCache.allKeys) {
@ -256,7 +256,7 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
}
}
}
// add all cells within the rect, return early if it starts iterating outside
for (NSInteger item = 0; item < itemCount; item++) {
NSIndexPath *indexPath = [NSIndexPath indexPathForItem:item inSection:section];
@ -266,19 +266,19 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
}
}
}
return result;
}
- (UICollectionViewLayoutAttributes *)layoutAttributesForItemAtIndexPath:(NSIndexPath *)indexPath {
IGAssertMainThread();
IGParameterAssert(indexPath != nil);
UICollectionViewLayoutAttributes *attributes = _attributesCache[indexPath];
if (attributes != nil) {
return attributes;
}
// avoid OOB errors
const NSInteger section = indexPath.section;
const NSInteger item = indexPath.item;
@ -286,7 +286,7 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
|| item >= _sectionData[section].itemBounds.size()) {
return nil;
}
attributes = [[[self class] layoutAttributesClass] layoutAttributesForCellWithIndexPath:indexPath];
attributes.frame = _sectionData[indexPath.section].itemBounds[indexPath.item];
adjustZIndexForAttributes(attributes);
@ -297,30 +297,30 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
- (UICollectionViewLayoutAttributes *)layoutAttributesForSupplementaryViewOfKind:(NSString *)elementKind atIndexPath:(NSIndexPath *)indexPath {
IGAssertMainThread();
IGParameterAssert(indexPath != nil);
UICollectionViewLayoutAttributes *attributes = _supplementaryAttributesCache[elementKind][indexPath];
if (attributes != nil) {
return attributes;
}
// avoid OOB errors
const NSInteger section = indexPath.section;
if (section >= _sectionData.size()) {
return nil;
}
UICollectionView *collectionView = self.collectionView;
const IGListSectionEntry entry = _sectionData[section];
const CGFloat minOffset = CGRectGetMinInDirection(entry.bounds, self.scrollDirection);
CGRect frame = CGRectZero;
if ([elementKind isEqualToString:UICollectionElementKindSectionHeader]) {
frame = entry.headerBounds;
if (self.stickyHeaders) {
CGFloat offset = CGPointGetCoordinateInDirection(collectionView.contentOffset, self.scrollDirection) + self.topContentInset + self.stickyHeaderYOffset;
if (section + 1 == _sectionData.size()) {
offset = MAX(minOffset, offset);
} else {
@ -339,7 +339,7 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
} else if ([elementKind isEqualToString:UICollectionElementKindSectionFooter]) {
frame = entry.footerBounds;
}
attributes = [UICollectionViewLayoutAttributes layoutAttributesForSupplementaryViewOfKind:elementKind withIndexPath:indexPath];
attributes.frame = frame;
adjustZIndexForAttributes(attributes);
@ -350,13 +350,13 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
- (CGSize)collectionViewContentSize {
IGAssertMainThread();
const NSInteger sectionCount = _sectionData.size();
if (sectionCount == 0) {
return CGSizeZero;
}
const IGListSectionEntry section = _sectionData[sectionCount - 1];
UICollectionView *collectionView = self.collectionView;
const UIEdgeInsets contentInset = collectionView.ig_contentInset;
@ -370,7 +370,7 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
return CGSizeMake(width, CGRectGetHeight(collectionView.bounds) - contentInset.top - contentInset.bottom);
}
}
}
- (void)invalidateLayoutWithContext:(IGListCollectionViewLayoutInvalidationContext *)context {
@ -378,7 +378,7 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
if ([context respondsToSelector:@selector(invalidatedItemIndexPaths)]) {
hasInvalidatedItemIndexPaths = [context invalidatedItemIndexPaths].count > 0;
}
if (hasInvalidatedItemIndexPaths
|| [context invalidateEverything]
|| context.ig_invalidateAllAttributes) {
@ -388,11 +388,11 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
// invalidate all if count changed and we don't have information on the minimum invalidated section
_minimumInvalidatedSection = 0;
}
if (context.ig_invalidateSupplementaryAttributes) {
[self _resetSupplementaryAttributesCache];
}
[super invalidateLayoutWithContext:context];
}
@ -402,9 +402,9 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
- (UICollectionViewLayoutInvalidationContext *)invalidationContextForBoundsChange:(CGRect)newBounds {
const CGRect oldBounds = self.collectionView.bounds;
IGListCollectionViewLayoutInvalidationContext *context =
(IGListCollectionViewLayoutInvalidationContext *)[super invalidationContextForBoundsChange:newBounds];
(IGListCollectionViewLayoutInvalidationContext *)[super invalidationContextForBoundsChange:newBounds];
context.ig_invalidateSupplementaryAttributes = YES;
if (!CGSizeEqualToSize(oldBounds.size, newBounds.size)) {
context.ig_invalidateAllAttributes = YES;
@ -414,17 +414,17 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
- (BOOL)shouldInvalidateLayoutForBoundsChange:(CGRect)newBounds {
const CGRect oldBounds = self.collectionView.bounds;
// always invalidate for size changes
if (!CGSizeEqualToSize(oldBounds.size, newBounds.size)) {
return YES;
}
// if the y origin has changed, only invalidate when using sticky headers
if (CGRectGetMinInDirection(newBounds, self.scrollDirection) != CGRectGetMinInDirection(oldBounds, self.scrollDirection)) {
return self.stickyHeaders;
}
return NO;
}
@ -436,10 +436,10 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
- (void)setStickyHeaderYOffset:(CGFloat)stickyHeaderYOffset {
IGAssertMainThread();
if (_stickyHeaderYOffset != stickyHeaderYOffset) {
_stickyHeaderYOffset = stickyHeaderYOffset;
IGListCollectionViewLayoutInvalidationContext *invalidationContext = [IGListCollectionViewLayoutInvalidationContext new];
invalidationContext.ig_invalidateSupplementaryAttributes = YES;
[self invalidateLayoutWithContext:invalidationContext];
@ -452,28 +452,30 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
if (_minimumInvalidatedSection == NSNotFound) {
return;
}
// purge attribute caches so they are rebuilt
[_attributesCache removeAllObjects];
[self _resetSupplementaryAttributesCache];
UICollectionView *collectionView = self.collectionView;
id<UICollectionViewDataSource> dataSource = collectionView.dataSource;
id<UICollectionViewDelegateFlowLayout> delegate = (id<UICollectionViewDelegateFlowLayout>)collectionView.delegate;
const NSInteger sectionCount = [dataSource numberOfSectionsInCollectionView:collectionView];
const NSInteger sectionCount = (IGListExperimentEnabled(_experiments, IGListExperimentUseCollectionViewInsteadOfDataSourceInLayout)
? [collectionView numberOfSections]
: [dataSource numberOfSectionsInCollectionView:collectionView]);
const UIEdgeInsets contentInset = collectionView.ig_contentInset;
const CGRect contentInsetAdjustedCollectionViewBounds = UIEdgeInsetsInsetRect(collectionView.bounds, contentInset);
_sectionData.resize(sectionCount);
CGFloat itemCoordInScrollDirection = 0.0;
CGFloat itemCoordInFixedDirection = 0.0;
CGFloat nextRowCoordInScrollDirection = 0.0;
// union item frames and optionally the header to find a bounding box of the entire section
CGRect rollingSectionBounds = CGRectZero;
// populate last valid section information
const NSInteger lastValidSection = _minimumInvalidatedSection - 1;
if (lastValidSection >= 0 && lastValidSection < sectionCount) {
@ -482,19 +484,21 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
nextRowCoordInScrollDirection = _sectionData[lastValidSection].lastNextRowCoordInScrollDirection;
rollingSectionBounds = _sectionData[lastValidSection].bounds;
}
for (NSInteger section = _minimumInvalidatedSection; section < sectionCount; section++) {
const NSInteger itemCount = [dataSource collectionView:collectionView numberOfItemsInSection:section];
const NSInteger itemCount = (IGListExperimentEnabled(_experiments, IGListExperimentUseCollectionViewInsteadOfDataSourceInLayout)
? [collectionView numberOfItemsInSection:section]
: [dataSource collectionView:collectionView numberOfItemsInSection:section]);
const BOOL itemsEmpty = itemCount == 0;
const BOOL hideHeaderWhenItemsEmpty = itemsEmpty && !self.showHeaderWhenEmpty;
_sectionData[section].itemBounds = std::vector<CGRect>(itemCount);
const CGSize headerSize = [delegate collectionView:collectionView layout:self referenceSizeForHeaderInSection:section];
const CGSize footerSize = [delegate collectionView:collectionView layout:self referenceSizeForFooterInSection:section];
const UIEdgeInsets insets = [delegate collectionView:collectionView layout:self insetForSectionAtIndex:section];
const CGFloat lineSpacing = [delegate collectionView:collectionView layout:self minimumLineSpacingForSectionAtIndex:section];
const CGFloat interitemSpacing = [delegate collectionView:collectionView layout:self minimumInteritemSpacingForSectionAtIndex:section];
const CGSize paddedCollectionViewSize = UIEdgeInsetsInsetRect(contentInsetAdjustedCollectionViewBounds, insets).size;
const UICollectionViewScrollDirection fixedDirection = self.scrollDirection == UICollectionViewScrollDirectionHorizontal ? UICollectionViewScrollDirectionVertical : UICollectionViewScrollDirectionHorizontal;
const CGFloat paddedLengthInFixedDirection = CGSizeGetLengthInDirection(paddedCollectionViewSize, fixedDirection);
@ -502,24 +506,24 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
const CGFloat footerLengthInScrollDirection = hideHeaderWhenItemsEmpty ? 0 : CGSizeGetLengthInDirection(footerSize, self.scrollDirection);
const BOOL headerExists = headerLengthInScrollDirection > 0;
const BOOL footerExists = footerLengthInScrollDirection > 0;
// start the section accounting for the header size
// header length in scroll direction is subtracted from the sectionBounds when calculating the header bounds after items are done
// this bumps the first row of items over enough to make room for the header
itemCoordInScrollDirection += headerLengthInScrollDirection;
nextRowCoordInScrollDirection += headerLengthInScrollDirection;
// add the leading inset in fixed direction in case the section falls on the same row as the previous
// if the section is newlined then the coord in fixed direction is reset
itemCoordInFixedDirection += UIEdgeInsetsLeadingInsetInDirection(insets, fixedDirection);
// the farthest in the fixed direction the frame of an item in this section can go
const CGFloat maxCoordinateInFixedDirection = CGRectGetLengthInDirection(contentInsetAdjustedCollectionViewBounds, fixedDirection) - UIEdgeInsetsTrailingInsetInDirection(insets, fixedDirection);
for (NSInteger item = 0; item < itemCount; item++) {
NSIndexPath *indexPath = [NSIndexPath indexPathForItem:item inSection:section];
const CGSize size = [delegate collectionView:collectionView layout:self sizeForItemAtIndexPath:indexPath];
IGAssert(CGSizeGetLengthInDirection(size, fixedDirection) <= paddedLengthInFixedDirection
|| fabs(CGSizeGetLengthInDirection(size, fixedDirection) - paddedLengthInFixedDirection) < FLT_EPSILON,
@"%@ of item %li in section %li (%.0f pt) must be less than or equal to container (%.0f pt) accounting for section insets %@",
@ -529,9 +533,9 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
CGSizeGetLengthInDirection(size, fixedDirection),
CGRectGetLengthInDirection(contentInsetAdjustedCollectionViewBounds, fixedDirection),
NSStringFromUIEdgeInsets(insets));
CGFloat itemLengthInFixedDirection = MIN(CGSizeGetLengthInDirection(size, fixedDirection), paddedLengthInFixedDirection);
// if the origin and length in fixed direction of the item busts the size of the container
// or if this is the first item and the header has a non-zero size
// newline to the next row and reset
@ -541,38 +545,38 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
|| (item == 0 && headerExists)) {
itemCoordInScrollDirection = nextRowCoordInScrollDirection;
itemCoordInFixedDirection = UIEdgeInsetsLeadingInsetInDirection(insets, fixedDirection);
// if newlining, always append line spacing unless its the very first item of the section
if (item > 0) {
itemCoordInScrollDirection += lineSpacing;
}
}
const CGFloat distanceToEdge = paddedLengthInFixedDirection - (itemCoordInFixedDirection + itemLengthInFixedDirection);
if (self.stretchToEdge && distanceToEdge > 0 && distanceToEdge <= epsilon) {
itemLengthInFixedDirection = paddedLengthInFixedDirection - itemCoordInFixedDirection;
}
const CGRect rawFrame = (self.scrollDirection == UICollectionViewScrollDirectionVertical) ?
CGRectMake(itemCoordInFixedDirection,
itemCoordInScrollDirection + insets.top,
itemLengthInFixedDirection,
size.height) :
CGRectMake(itemCoordInScrollDirection + insets.left,
itemCoordInFixedDirection,
size.width,
itemLengthInFixedDirection);
CGRectMake(itemCoordInFixedDirection,
itemCoordInScrollDirection + insets.top,
itemLengthInFixedDirection,
size.height) :
CGRectMake(itemCoordInScrollDirection + insets.left,
itemCoordInFixedDirection,
size.width,
itemLengthInFixedDirection);
const CGRect frame = IGListRectIntegralScaled(rawFrame);
_sectionData[section].itemBounds[item] = frame;
// track the max size of the row to find the coord of the next row, adjust for leading inset while iterating items
nextRowCoordInScrollDirection = MAX(CGRectGetMaxInDirection(frame, self.scrollDirection) - UIEdgeInsetsLeadingInsetInDirection(insets, self.scrollDirection), nextRowCoordInScrollDirection);
// increase the rolling coord in fixed direction appropriately and add item spacing for all items on the same row
itemCoordInFixedDirection += itemLengthInFixedDirection + interitemSpacing;
// union the rolling section bounds
if (item == 0) {
rollingSectionBounds = frame;
@ -580,7 +584,7 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
rollingSectionBounds = CGRectUnion(rollingSectionBounds, frame);
}
}
const CGRect headerBounds = self.scrollDirection == UICollectionViewScrollDirectionVertical ?
CGRectMake(insets.left,
itemsEmpty ? CGRectGetMaxY(rollingSectionBounds) : CGRectGetMinY(rollingSectionBounds) - headerSize.height,
@ -608,7 +612,7 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
paddedLengthInFixedDirection);
_sectionData[section].footerBounds = footerBounds;
// union the header before setting the bounds of the section
// only do this when the header has a size, otherwise the union stretches to box empty space
if (headerExists) {
@ -617,28 +621,28 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
if (footerExists) {
rollingSectionBounds = CGRectUnion(rollingSectionBounds, footerBounds);
}
_sectionData[section].bounds = rollingSectionBounds;
_sectionData[section].insets = insets;
// bump the coord for the next section with the right insets
itemCoordInFixedDirection += UIEdgeInsetsTrailingInsetInDirection(insets, fixedDirection);
// find the farthest point in the section and add the trailing inset to find the next row's coord
nextRowCoordInScrollDirection = MAX(nextRowCoordInScrollDirection, CGRectGetMaxInDirection(rollingSectionBounds, self.scrollDirection) + UIEdgeInsetsTrailingInsetInDirection(insets, self.scrollDirection));
// keep track of coordinates for partial invalidation
_sectionData[section].lastItemCoordInScrollDirection = itemCoordInScrollDirection;
_sectionData[section].lastItemCoordInFixedDirection = itemCoordInFixedDirection;
_sectionData[section].lastNextRowCoordInScrollDirection = nextRowCoordInScrollDirection;
}
_minimumInvalidatedSection = NSNotFound;
}
- (NSRange)_rangeOfSectionsInRect:(CGRect)rect {
NSRange result = NSMakeRange(NSNotFound, 0);
const NSInteger sectionCount = _sectionData.size();
for (NSInteger section = 0; section < sectionCount; section++) {
IGListSectionEntry entry = _sectionData[section];
@ -651,7 +655,7 @@ static void adjustZIndexForAttributes(UICollectionViewLayoutAttributes *attribut
}
}
}
return result;
}