Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ASCollectionView] Fix index space translation of Flow Layout Delegate methods. #467

Merged
merged 4 commits into from
Oct 9, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## master

* Add your own contributions to the next release on the line below this with your name.
- [ASCollectionView] Improve index space translation of Flow Layout Delegate methods. [Scott Goodson](https://github.com/appleguy)
- [ASCollectionView] Add delegate bridging and index space translation for missing UICollectionViewLayout properties. [Scott Goodson](https://github.com/appleguy)
- [ASTextNode2] Add initial implementation for link handling. [Scott Goodson](https://github.com/appleguy) [#396](https://github.com/TextureGroup/Texture/pull/396)
- [ASTextNode2] Provide compile flag to globally enable new implementation of ASTextNode: ASTEXTNODE_EXPERIMENT_GLOBAL_ENABLE. [Scott Goodson](https://github.com/appleguy) [#396](https://github.com/TextureGroup/Texture/pull/410)
Expand Down
2 changes: 2 additions & 0 deletions Source/ASCollectionNode+Beta.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ NS_ASSUME_NONNULL_BEGIN

- (void)endUpdatesAnimated:(BOOL)animated completion:(nullable void (^)(BOOL))completion ASDISPLAYNODE_DEPRECATED_MSG("Use -performBatchUpdates:completion: instead.");

- (void)invalidateFlowLayoutDelegateMetrics;

@end

NS_ASSUME_NONNULL_END
4 changes: 4 additions & 0 deletions Source/ASCollectionNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -836,6 +836,10 @@ NS_ASSUME_NONNULL_BEGIN

- (void)collectionView:(UICollectionView *)collectionView didEndDisplayingCell:(UICollectionViewCell *)cell forItemAtIndexPath:(NSIndexPath *)indexPath;

- (void)collectionView:(UICollectionView *)collectionView willDisplaySupplementaryView:(UICollectionReusableView *)view forElementKind:(NSString *)elementKind atIndexPath:(NSIndexPath *)indexPath;

- (void)collectionView:(UICollectionView *)collectionView didEndDisplayingSupplementaryView:(UICollectionReusableView *)view forElementOfKind:(NSString *)elementKind atIndexPath:(NSIndexPath *)indexPath;

@end

NS_ASSUME_NONNULL_END
7 changes: 7 additions & 0 deletions Source/ASCollectionNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,13 @@ - (void)endUpdatesAnimated:(BOOL)animated completion:(void (^)(BOOL))completion
}
}

- (void)invalidateFlowLayoutDelegateMetrics {
ASDisplayNodeAssertMainThread();
if (self.nodeLoaded) {
[self.view invalidateFlowLayoutDelegateMetrics];
}
}

- (void)insertSections:(NSIndexSet *)sections
{
ASDisplayNodeAssertMainThread();
Expand Down
101 changes: 78 additions & 23 deletions Source/ASCollectionView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ @interface ASCollectionView () <ASRangeControllerDataSource, ASRangeControllerDe
unsigned int interop:1;
unsigned int interopWillDisplayCell:1;
unsigned int interopDidEndDisplayingCell:1;
unsigned int interopWillDisplaySupplementaryView:1;
unsigned int interopdidEndDisplayingSupplementaryView:1;
} _asyncDelegateFlags;

struct {
Expand Down Expand Up @@ -522,6 +524,8 @@ - (void)setAsyncDelegate:(id<ASCollectionDelegate>)asyncDelegate
id<ASCollectionDelegateInterop> interopDelegate = (id<ASCollectionDelegateInterop>)_asyncDelegate;
_asyncDelegateFlags.interopWillDisplayCell = [interopDelegate respondsToSelector:@selector(collectionView:willDisplayCell:forItemAtIndexPath:)];
_asyncDelegateFlags.interopDidEndDisplayingCell = [interopDelegate respondsToSelector:@selector(collectionView:didEndDisplayingCell:forItemAtIndexPath:)];
_asyncDelegateFlags.interopWillDisplaySupplementaryView = [interopDelegate respondsToSelector:@selector(collectionView:willDisplaySupplementaryView:forElementKind:atIndexPath:)];
_asyncDelegateFlags.interopdidEndDisplayingSupplementaryView = [interopDelegate respondsToSelector:@selector(collectionView:didEndDisplayingSupplementaryView:forElementOfKind:atIndexPath:)];
}
}

Expand Down Expand Up @@ -761,6 +765,28 @@ - (void)setUsesSynchronousDataLoading:(BOOL)usesSynchronousDataLoading
self.dataController.usesSynchronousDataLoading = usesSynchronousDataLoading;
}

- (void)invalidateFlowLayoutDelegateMetrics {
UICollectionViewLayout *layout = self.collectionViewLayout;
CGSize defaultSize = ({
UICollectionViewFlowLayout *flowLayout = ASDynamicCast(layout, UICollectionViewFlowLayout);
flowLayout ? flowLayout.itemSize : CGSizeZero;
});

for (ASCollectionElement *element in self.dataController.pendingMap.itemElements) {
ASCellNode *node = element.node;
if (node.shouldUseUIKitCell) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It's probably inconsequential, but maybe we should just ignore calls where the layout isn't a flow layout, rather than using size 0?

  2. Instead of building a giant array – I don't really like the itemElements property and may propose removing it – it's better to enumerate the map directly and if (element.supplementaryElementKind) { continue; } Note to self: add a UICollectionElementCategory elementCategory property to ASCollectionElement to simplify this distinction.

  3. Shouldn't we also refetch header/footer reference sizes here? Meshes well with 2

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Adlai-Holler some merge conflicts have arisen, and I'm afraid my time availability is nearing a local minimum. :( Do you think this PR is acceptable to land as-is if the conflicts are fixed?

  1. I think it's better to have the 0 default, but indeed maybe no one will ever know :)
  2. The iteration suggestion sounds like an improvement for sure. I'm not sure I can commit to doing it only because I would have to get some test cases running to exercise the change.
  3. This seems potentially important, and indeed might motivate me to do Test PR #2.

Could you give me an Accept if you are comfortable with this, and then if I do 2 or 3, I can land without waiting?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Adlai-Holler It took me a while, but I finally got around to implementing your advice — and went quite a bit further. Thanks for suggesting these things, as they are improvements.

In particular, I was intentionally skipping over handling the update of supplementary element sizes, which is not great. That's now fixed and the code is better factored.

If you or @nguyenhuy can give me a sanity check review soon, I'd appreciate that - would like to merge this in pretty soon.

NSIndexPath *indexPath = [self indexPathForNode:node];
CGSize size = CGSizeZero;
if ([_asyncDelegate respondsToSelector:@selector(collectionView:layout:sizeForItemAtIndexPath:)]) {
size = [(id)_asyncDelegate collectionView:self layout:layout sizeForItemAtIndexPath:indexPath];
} else {
size = defaultSize;
}
node.style.preferredSize = size;
}
}
}

#pragma mark Internal

- (void)_configureCollectionViewLayout:(nonnull UICollectionViewLayout *)layout
Expand Down Expand Up @@ -959,15 +985,15 @@ - (NSInteger)collectionView:(UICollectionView *)collectionView numberOfItemsInSe
})

- (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout *)layout
sizeForItemAtIndexPath:(NSIndexPath *)indexPath
sizeForItemAtIndexPath:(NSIndexPath *)indexPath
{
ASDisplayNodeAssertMainThread();
ASCollectionElement *e = [_dataController.visibleMap elementForItemAtIndexPath:indexPath];
return e ? [self sizeForElement:e] : ASFlowLayoutDefault(layout, itemSize, CGSizeZero);
}

- (CGSize)collectionView:(UICollectionView *)cv layout:(UICollectionViewLayout *)l
referenceSizeForHeaderInSection:(NSInteger)section
referenceSizeForHeaderInSection:(NSInteger)section
{
ASDisplayNodeAssertMainThread();
ASElementMap *map = _dataController.visibleMap;
Expand All @@ -977,7 +1003,7 @@ - (CGSize)collectionView:(UICollectionView *)cv layout:(UICollectionViewLayout *
}

- (CGSize)collectionView:(UICollectionView *)cv layout:(UICollectionViewLayout *)l
referenceSizeForFooterInSection:(NSInteger)section
referenceSizeForFooterInSection:(NSInteger)section
{
ASDisplayNodeAssertMainThread();
ASElementMap *map = _dataController.visibleMap;
Expand All @@ -1000,7 +1026,7 @@ - (NSIndexPath *)delegateIndexPathForSection:(NSInteger)section withSelector:(SE
}

- (UIEdgeInsets)collectionView:(UICollectionView *)cv layout:(UICollectionViewLayout *)l
insetForSectionAtIndex:(NSInteger)section
insetForSectionAtIndex:(NSInteger)section
{
NSIndexPath *indexPath = [self delegateIndexPathForSection:section withSelector:_cmd];
if (indexPath) {
Expand All @@ -1010,23 +1036,23 @@ - (UIEdgeInsets)collectionView:(UICollectionView *)cv layout:(UICollectionViewLa
}

- (CGFloat)collectionView:(UICollectionView *)cv layout:(UICollectionViewLayout *)l
minimumInteritemSpacingForSectionAtIndex:(NSInteger)section
minimumInteritemSpacingForSectionAtIndex:(NSInteger)section
{
NSIndexPath *indexPath = [self delegateIndexPathForSection:section withSelector:_cmd];
if (indexPath) {
return [(id)_asyncDelegate collectionView:cv layout:l
minimumInteritemSpacingForSectionAtIndex:indexPath.section];
minimumInteritemSpacingForSectionAtIndex:indexPath.section];
}
return ASFlowLayoutDefault(l, minimumInteritemSpacing, 10.0); // Default is documented as 10.0
}

- (CGFloat)collectionView:(UICollectionView *)cv layout:(UICollectionViewLayout *)l
minimumLineSpacingForSectionAtIndex:(NSInteger)section
minimumLineSpacingForSectionAtIndex:(NSInteger)section
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙂

{
NSIndexPath *indexPath = [self delegateIndexPathForSection:section withSelector:_cmd];
if (indexPath) {
return [(id)_asyncDelegate collectionView:cv layout:l
minimumLineSpacingForSectionAtIndex:indexPath.section];
minimumLineSpacingForSectionAtIndex:indexPath.section];
}
return ASFlowLayoutDefault(l, minimumLineSpacing, 10.0); // Default is documented as 10.0
}
Expand All @@ -1036,7 +1062,7 @@ - (UICollectionReusableView *)collectionView:(UICollectionView *)collectionView
if ([_registeredSupplementaryKinds containsObject:kind] == NO) {
[self registerSupplementaryNodeOfKind:kind];
}

UICollectionReusableView *view = nil;
ASCollectionElement *element = [_dataController.visibleMap supplementaryElementOfKind:kind atIndexPath:indexPath];
ASCellNode *node = element.node;
Expand Down Expand Up @@ -1087,7 +1113,11 @@ - (UICollectionViewCell *)collectionView:(UICollectionView *)collectionView cell
- (void)collectionView:(UICollectionView *)collectionView willDisplayCell:(UICollectionViewCell *)rawCell forItemAtIndexPath:(NSIndexPath *)indexPath
{
if (_asyncDelegateFlags.interopWillDisplayCell) {
[(id <ASCollectionDelegateInterop>)_asyncDelegate collectionView:collectionView willDisplayCell:rawCell forItemAtIndexPath:indexPath];
ASCellNode *node = [self nodeForItemAtIndexPath:indexPath];
NSIndexPath *modelIndexPath = [self indexPathForNode:node];
if (modelIndexPath && node.shouldUseUIKitCell) {
[(id <ASCollectionDelegateInterop>)_asyncDelegate collectionView:collectionView willDisplayCell:rawCell forItemAtIndexPath:modelIndexPath];
}
}

_ASCollectionViewCell *cell = ASDynamicCastStrict(rawCell, _ASCollectionViewCell);
Expand Down Expand Up @@ -1144,7 +1174,11 @@ - (void)collectionView:(UICollectionView *)collectionView willDisplayCell:(UICol
- (void)collectionView:(UICollectionView *)collectionView didEndDisplayingCell:(UICollectionViewCell *)rawCell forItemAtIndexPath:(NSIndexPath *)indexPath
{
if (_asyncDelegateFlags.interopDidEndDisplayingCell) {
[(id <ASCollectionDelegateInterop>)_asyncDelegate collectionView:collectionView didEndDisplayingCell:rawCell forItemAtIndexPath:indexPath];
ASCellNode *node = [self nodeForItemAtIndexPath:indexPath];
NSIndexPath *modelIndexPath = [self indexPathForNode:node];
if (modelIndexPath && node.shouldUseUIKitCell) {
[(id <ASCollectionDelegateInterop>)_asyncDelegate collectionView:collectionView didEndDisplayingCell:rawCell forItemAtIndexPath:modelIndexPath];
}
}

_ASCollectionViewCell *cell = ASDynamicCastStrict(rawCell, _ASCollectionViewCell);
Expand Down Expand Up @@ -1185,6 +1219,14 @@ - (void)collectionView:(UICollectionView *)collectionView didEndDisplayingCell:(

- (void)collectionView:(UICollectionView *)collectionView willDisplaySupplementaryView:(UICollectionReusableView *)rawView forElementKind:(NSString *)elementKind atIndexPath:(NSIndexPath *)indexPath
{
if (_asyncDelegateFlags.interopWillDisplaySupplementaryView) {
ASCellNode *node = [self nodeForItemAtIndexPath:indexPath];
NSIndexPath *modelIndexPath = [self indexPathForNode:node];
if (modelIndexPath && node.shouldUseUIKitCell) {
[(id <ASCollectionDelegateInterop>)_asyncDelegate collectionView:collectionView willDisplaySupplementaryView:rawView forElementKind:elementKind atIndexPath:modelIndexPath];
}
}

_ASCollectionReusableView *view = ASDynamicCastStrict(rawView, _ASCollectionReusableView);
if (view == nil) {
return;
Expand Down Expand Up @@ -1218,6 +1260,14 @@ - (void)collectionView:(UICollectionView *)collectionView willDisplaySupplementa

- (void)collectionView:(UICollectionView *)collectionView didEndDisplayingSupplementaryView:(UICollectionReusableView *)rawView forElementOfKind:(NSString *)elementKind atIndexPath:(NSIndexPath *)indexPath
{
if (_asyncDelegateFlags.interopdidEndDisplayingSupplementaryView) {
ASCellNode *node = [self nodeForItemAtIndexPath:indexPath];
NSIndexPath *modelIndexPath = [self indexPathForNode:node];
if (modelIndexPath && node.shouldUseUIKitCell) {
[(id <ASCollectionDelegateInterop>)_asyncDelegate collectionView:collectionView didEndDisplayingSupplementaryView:rawView forElementOfKind:elementKind atIndexPath:modelIndexPath];
}
}

_ASCollectionReusableView *view = ASDynamicCastStrict(rawView, _ASCollectionReusableView);
if (view == nil) {
return;
Expand Down Expand Up @@ -1416,9 +1466,11 @@ - (void)scrollViewDidScroll:(UIScrollView *)scrollView
[self _checkForBatchFetching];
}

for (_ASCollectionViewCell *cell in _cellsForVisibilityUpdates) {
for (_ASCollectionViewCell *collectionCell in _cellsForVisibilityUpdates) {
// Only nodes that respond to the selector are added to _cellsForVisibilityUpdates
[cell cellNodeVisibilityEvent:ASCellNodeVisibilityEventVisibleRectChanged inScrollView:scrollView];
[[collectionCell node] cellNodeVisibilityEvent:ASCellNodeVisibilityEventVisibleRectChanged
inScrollView:scrollView
withCellFrame:collectionCell.frame];
}
if (_asyncDelegateFlags.scrollViewDidScroll) {
[_asyncDelegate scrollViewDidScroll:scrollView];
Expand Down Expand Up @@ -1705,15 +1757,16 @@ - (ASCellNodeBlock)dataController:(ASDataController *)dataController nodeBlockAt
CGSize preferredSize = CGSizeZero;
SEL sizeForItem = @selector(collectionView:layout:sizeForItemAtIndexPath:);
if ([_asyncDelegate respondsToSelector:sizeForItem]) {
preferredSize = [(id)_asyncDelegate collectionView:self layout:layout sizeForItemAtIndexPath:indexPath];
preferredSize = [(id)_asyncDelegate collectionView:self layout:layout
sizeForItemAtIndexPath:indexPath];
} else {
preferredSize = ASFlowLayoutDefault(layout, itemSize, CGSizeZero);
}
block = ^{
ASCellNode *node = [[ASCellNode alloc] init];
node.shouldUseUIKitCell = YES;
node.style.preferredSize = preferredSize;
return node;
ASCellNode *cell = [[ASCellNode alloc] init];
cell.shouldUseUIKitCell = YES;
cell.style.preferredSize = preferredSize;
return cell;
};
} else {
ASDisplayNodeFailAssert(@"ASCollection could not get a node block for item at index path %@: %@, %@. If you are trying to display a UICollectionViewCell, make sure your dataSource conforms to the <ASCollectionDataSourceInterop> protocol!", indexPath, cell, block);
Expand Down Expand Up @@ -1811,15 +1864,15 @@ - (ASCellNodeBlock)dataController:(ASDataController *)dataController supplementa
SEL sizeForHeader = @selector(collectionView:layout:referenceSizeForHeaderInSection:);
if ([_asyncDelegate respondsToSelector:sizeForHeader]) {
preferredSize = [(id)_asyncDelegate collectionView:self layout:layout
referenceSizeForHeaderInSection:indexPath.section];
referenceSizeForHeaderInSection:indexPath.section];
} else {
preferredSize = ASFlowLayoutDefault(layout, headerReferenceSize, CGSizeZero);
}
} else if ([kind isEqualToString:UICollectionElementKindSectionFooter]) {
SEL sizeForFooter = @selector(collectionView:layout:referenceSizeForFooterInSection:);
if ([_asyncDelegate respondsToSelector:sizeForFooter]) {
preferredSize = [(id)_asyncDelegate collectionView:self layout:layout
referenceSizeForFooterInSection:indexPath.section];
referenceSizeForFooterInSection:indexPath.section];
} else {
preferredSize = ASFlowLayoutDefault(layout, footerReferenceSize, CGSizeZero);
}
Expand Down Expand Up @@ -2196,10 +2249,12 @@ - (void)layer:(CALayer *)layer didChangeBoundsWithOldValue:(CGRect)oldBounds new
BOOL changedInNonScrollingDirection = (fixedHorizontally && newBounds.size.width != lastUsedSize.width) || (fixedVertically && newBounds.size.height != lastUsedSize.height);

if (changedInNonScrollingDirection) {
[_dataController relayoutAllNodes];
[_dataController waitUntilAllUpdatesAreCommitted];
// We need to ensure the size requery is done before we update our layout.
// Because -invalidateLayout doesn't trigger any operations by itself, and we answer queries from UICollectionView using layoutThatFits:,
// we invalidate the layout before we have updated all of the cells. Any cells that the collection needs the size of immediately will get
// -layoutThatFits: with a new constraint, on the main thread, and synchronously calculate them. Meanwhile, relayoutAllNodes will update
// the layout of any remaining nodes on background threads (and fast-return for any nodes that the UICV got to first).
[self.collectionViewLayout invalidateLayout];
[_dataController relayoutAllNodes];
}
}

Expand Down
4 changes: 2 additions & 2 deletions Source/Base/ASAssert.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@
#define ASDisplayNodeConditionalAssert(shouldTestCondition, condition, desc, ...) ASDisplayNodeAssert((!(shouldTestCondition) || (condition)), desc, ##__VA_ARGS__)
#define ASDisplayNodeConditionalCAssert(shouldTestCondition, condition, desc, ...) ASDisplayNodeCAssert((!(shouldTestCondition) || (condition)), desc, ##__VA_ARGS__)

#define ASDisplayNodeCAssertPositiveReal(description, num) ASDisplayNodeCAssert(num >= 0 && num <= CGFLOAT_MAX, @"%@ must be a real positive integer.", description)
#define ASDisplayNodeCAssertInfOrPositiveReal(description, num) ASDisplayNodeCAssert(isinf(num) || (num >= 0 && num <= CGFLOAT_MAX), @"%@ must be infinite or a real positive integer.", description)
#define ASDisplayNodeCAssertPositiveReal(description, num) ASDisplayNodeCAssert(num >= 0 && num <= CGFLOAT_MAX, @"%@ must be a real positive integer: %f.", description, (CGFloat)num)
#define ASDisplayNodeCAssertInfOrPositiveReal(description, num) ASDisplayNodeCAssert(isinf(num) || (num >= 0 && num <= CGFLOAT_MAX), @"%@ must be infinite or a real positive integer: %f.", description, (CGFloat)num)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌


#define ASDisplayNodeErrorDomain @"ASDisplayNodeErrorDomain"
#define ASDisplayNodeNonFatalErrorCode 1
Expand Down
4 changes: 2 additions & 2 deletions Source/Details/ASElementMap.m
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ - (NSString *)description
- (BOOL)sectionIndexIsValid:(NSInteger)section assert:(BOOL)assert
{
NSInteger sectionCount = _sectionsOfItems.count;
if (section >= sectionCount) {
if (section >= sectionCount || section < 0) {
if (assert) {
ASDisplayNodeFailAssert(@"Invalid section index %zd when there are only %zd sections!", section, sectionCount);
}
Expand Down Expand Up @@ -246,7 +246,7 @@ - (BOOL)itemIndexPathIsValid:(NSIndexPath *)indexPath assert:(BOOL)assert item:(

NSInteger itemCount = _sectionsOfItems[section].count;
NSInteger item = indexPath.item;
if (item >= itemCount) {
if (item >= itemCount || item < 0) {
if (assert) {
ASDisplayNodeFailAssert(@"Invalid item index %zd in section %zd which only has %zd items!", item, section, itemCount);
}
Expand Down
5 changes: 5 additions & 0 deletions Source/Private/ASCollectionView+Undeprecated.h
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,11 @@ NS_ASSUME_NONNULL_BEGIN
*/
- (nullable NSIndexPath *)indexPathForNode:(ASCellNode *)cellNode AS_WARN_UNUSED_RESULT;

/**
* Invalidates and recalculates the cached sizes stored for pass-through cells used in interop mode.
*/
- (void)invalidateFlowLayoutDelegateMetrics;

@end

NS_ASSUME_NONNULL_END