[ASCollectionView] Small improvements #407
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -952,46 +952,60 @@ - (NSInteger)collectionView:(UICollectionView *)collectionView numberOfItemsInSe | |
| - (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout *)collectionViewLayout sizeForItemAtIndexPath:(NSIndexPath *)indexPath | ||
| { | ||
| ASDisplayNodeAssertMainThread(); | ||
| ASCollectionElement *element = [_dataController.visibleMap elementForItemAtIndexPath:indexPath]; | ||
| if (element == nil) { | ||
| ASDisplayNodeAssert(NO, @"Unexpected nil element for collectionView:layout:sizeForItemAtIndexPath: %@, %@, %@", self, collectionViewLayout, indexPath); | ||
| return CGSizeZero; | ||
| } | ||
|
|
||
| ASCellNode *cell = element.node; | ||
| if (cell.shouldUseUIKitCell) { | ||
| if ([_asyncDelegate respondsToSelector:@selector(collectionView:layout:sizeForItemAtIndexPath:)]) { | ||
| CGSize size = [(id)_asyncDelegate collectionView:collectionView layout:collectionViewLayout sizeForItemAtIndexPath:indexPath]; | ||
| cell.style.preferredSize = size; | ||
| return size; | ||
| } | ||
| } | ||
|
|
||
| return [self sizeForElement:element]; | ||
| } | ||
|
|
||
| - (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout *)layout referenceSizeForHeaderInSection:(NSInteger)section | ||
| { | ||
| ASDisplayNodeAssertMainThread(); | ||
| NSIndexPath *indexPath = [NSIndexPath indexPathForItem:0 inSection:section]; | ||
| ASCollectionElement *element = [_dataController.visibleMap supplementaryElementOfKind:UICollectionElementKindSectionHeader | ||
| atIndexPath:indexPath]; | ||
| if (element == nil) { | ||
| return CGSizeZero; | ||
|
There was a problem hiding this comment. @appleguy In case of interop, should we reach out to the delegate here? Note that since element is There was a problem hiding this comment. @nguyenhuy this is a very good and probably important question to resolve some of the crashes I've been trying to get past. I think the answer will come from...in what case can we get here but have a nil element? If that is expected to be possible / valid, then we probably do need to call the delegate. If it is believed to be invalid, then we should probably add an assertion in the return CGSizeZero case, and not worry about calling the delegate. There was a problem hiding this comment.
I was assuming that clients that use interop can return an item/supplementary view that is not of type It turns out that, there is a race condition in |
||
| } | ||
|
|
||
| if (element.node.shouldUseUIKitCell && _asyncDelegateFlags.interop) { | ||
| if ([_asyncDelegate respondsToSelector:@selector(collectionView:layout:referenceSizeForHeaderInSection:)]) { | ||
| return [(id)_asyncDelegate collectionView:collectionView layout:layout referenceSizeForHeaderInSection:section]; | ||
| } | ||
| } | ||
|
|
||
| return [self sizeForElement:element]; | ||
| } | ||
|
|
||
| - (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout *)layout referenceSizeForFooterInSection:(NSInteger)section | ||
| { | ||
| ASDisplayNodeAssertMainThread(); | ||
| NSIndexPath *indexPath = [NSIndexPath indexPathForItem:0 inSection:section]; | ||
| ASCollectionElement *element = [_dataController.visibleMap supplementaryElementOfKind:UICollectionElementKindSectionFooter | ||
| atIndexPath:indexPath]; | ||
| if (element == nil) { | ||
| return CGSizeZero; | ||
| } | ||
|
|
||
| if (element.node.shouldUseUIKitCell && _asyncDelegateFlags.interop) { | ||
| if ([_asyncDelegate respondsToSelector:@selector(collectionView:layout:referenceSizeForFooterInSection:)]) { | ||
| return [(id)_asyncDelegate collectionView:collectionView layout:layout referenceSizeForFooterInSection:section]; | ||
| } | ||
| } | ||
|
|
||
| return [self sizeForElement:element]; | ||
| } | ||
|
|
||
| - (UICollectionReusableView *)collectionView:(UICollectionView *)collectionView viewForSupplementaryElementOfKind:(NSString *)kind atIndexPath:(NSIndexPath *)indexPath | ||
|
|
@@ -1013,7 +1027,7 @@ - (UICollectionReusableView *)collectionView:(UICollectionView *)collectionView | |
| view = [self dequeueReusableSupplementaryViewOfKind:kind withReuseIdentifier:kReuseIdentifier forIndexPath:indexPath]; | ||
| } | ||
|
|
||
| if (_ASCollectionReusableView *reusableView = ASDynamicCastStrict(view, _ASCollectionReusableView)) { | ||
| reusableView.element = element; | ||
| } | ||
|
|
||
|
|
@@ -1039,7 +1053,7 @@ - (UICollectionViewCell *)collectionView:(UICollectionView *)collectionView cell | |
|
|
||
| ASDisplayNodeAssert(element != nil, @"Element should exist. indexPath = %@, collectionDataSource = %@", indexPath, self); | ||
|
|
||
| if (_ASCollectionViewCell *asCell = ASDynamicCastStrict(cell, _ASCollectionViewCell)) { | ||
| asCell.element = element; | ||
| [_rangeController configureContentView:cell.contentView forCellNode:node]; | ||
| } | ||
|
|
@@ -1053,16 +1067,15 @@ - (void)collectionView:(UICollectionView *)collectionView willDisplayCell:(UICol | |
| [(id <ASCollectionDelegateInterop>)_asyncDelegate collectionView:collectionView willDisplayCell:rawCell forItemAtIndexPath:indexPath]; | ||
| } | ||
|
|
||
| _ASCollectionViewCell *cell = ASDynamicCastStrict(rawCell, _ASCollectionViewCell); | ||
| if (cell == nil) { | ||
| [_rangeController setNeedsUpdate]; | ||
| return; | ||
| } | ||
|
|
||
| ASCollectionElement *element = cell.element; | ||
| if (element) { | ||
| ASDisplayNodeAssertTrue([_dataController.visibleMap elementForItemAtIndexPath:indexPath] == element); | ||
| [_visibleElements addObject:element]; | ||
| } else { | ||
| ASDisplayNodeAssert(NO, @"Unexpected nil element for willDisplayCell: %@, %@, %@", rawCell, self, indexPath); | ||
|
|
@@ -1100,7 +1113,7 @@ - (void)collectionView:(UICollectionView *)collectionView willDisplayCell:(UICol | |
|
|
||
| [_rangeController setNeedsUpdate]; | ||
|
|
||
| if ([cell consumesCellNodeVisibilityEvents]) { | ||
|
There was a problem hiding this comment. This is potentially valuable if made into an API that can be implemented optionally on UIKit cells too. |
||
| [_cellsForVisibilityUpdates addObject:cell]; | ||
| } | ||
| } | ||
|
|
@@ -1111,14 +1124,13 @@ - (void)collectionView:(UICollectionView *)collectionView didEndDisplayingCell:( | |
| [(id <ASCollectionDelegateInterop>)_asyncDelegate collectionView:collectionView didEndDisplayingCell:rawCell forItemAtIndexPath:indexPath]; | ||
| } | ||
|
|
||
| _ASCollectionViewCell *cell = ASDynamicCastStrict(rawCell, _ASCollectionViewCell); | ||
| if (cell == nil) { | ||
| [_rangeController setNeedsUpdate]; | ||
| return; | ||
| } | ||
|
|
||
| // Retrieve the element from cell instead of visible map because at this point visible map could have been updated and no longer holds the element. | ||
| ASCollectionElement *element = cell.element; | ||
| if (element) { | ||
| [_visibleElements removeObject:element]; | ||
|
|
@@ -1150,49 +1162,56 @@ - (void)collectionView:(UICollectionView *)collectionView didEndDisplayingCell:( | |
|
|
||
| - (void)collectionView:(UICollectionView *)collectionView willDisplaySupplementaryView:(UICollectionReusableView *)rawView forElementKind:(NSString *)elementKind atIndexPath:(NSIndexPath *)indexPath | ||
| { | ||
| _ASCollectionReusableView *view = ASDynamicCastStrict(rawView, _ASCollectionReusableView); | ||
| if (view == nil) { | ||
| return; | ||
| } | ||
|
|
||
| ASCollectionElement *element = view.element; | ||
| if (element) { | ||
| ASDisplayNodeAssertTrue([_dataController.visibleMap supplementaryElementOfKind:elementKind atIndexPath:indexPath] == view.element); | ||
| [_visibleElements addObject:element]; | ||
| } else { | ||
| ASDisplayNodeAssert(NO, @"Unexpected nil element for willDisplaySupplementaryView: %@, %@, %@", rawView, self, indexPath); | ||
| return; | ||
| } | ||
|
|
||
| // Under iOS 10+, cells may be removed/re-added to the collection view without | ||
| // receiving prepareForReuse/applyLayoutAttributes, as an optimization for e.g. | ||
| // if the user is scrolling back and forth across a small set of items. | ||
| // In this case, we have to fetch the layout attributes manually. | ||
| // This may be possible under iOS < 10 but it has not been observed yet. | ||
| if (view.layoutAttributes == nil) { | ||
| view.layoutAttributes = [collectionView layoutAttributesForSupplementaryElementOfKind:elementKind atIndexPath:indexPath]; | ||
| } | ||
|
|
||
| if (_asyncDelegateFlags.collectionNodeWillDisplaySupplementaryElement) { | ||
| GET_COLLECTIONNODE_OR_RETURN(collectionNode, (void)0); | ||
| ASCellNode *node = element.node; | ||
| ASDisplayNodeAssert([node.supplementaryElementKind isEqualToString:elementKind], @"Expected node for supplementary element to have kind '%@', got '%@'.", elementKind, node.supplementaryElementKind); | ||
| [_asyncDelegate collectionNode:collectionNode willDisplaySupplementaryElementWithNode:node]; | ||
| } | ||
| } | ||
|
|
||
| - (void)collectionView:(UICollectionView *)collectionView didEndDisplayingSupplementaryView:(UICollectionReusableView *)rawView forElementOfKind:(NSString *)elementKind atIndexPath:(NSIndexPath *)indexPath | ||
| { | ||
| _ASCollectionReusableView *view = ASDynamicCastStrict(rawView, _ASCollectionReusableView); | ||
| if (view == nil) { | ||
| return; | ||
| } | ||
|
|
||
| // Retrieve the element from cell instead of visible map because at this point visible map could have been updated and no longer holds the element. | ||
| ASCollectionElement *element = view.element; | ||
| if (element) { | ||
| [_visibleElements removeObject:element]; | ||
| } else { | ||
| ASDisplayNodeAssert(NO, @"Unexpected nil element for didEndDisplayingSupplementaryView: %@, %@, %@", rawView, self, indexPath); | ||
| return; | ||
| } | ||
|
|
||
| if (_asyncDelegateFlags.collectionNodeDidEndDisplayingSupplementaryElement) { | ||
| GET_COLLECTIONNODE_OR_RETURN(collectionNode, (void)0); | ||
| ASCellNode *node = element.node; | ||
| ASDisplayNodeAssert([node.supplementaryElementKind isEqualToString:elementKind], @"Expected node for supplementary element to have kind '%@', got '%@'.", elementKind, node.supplementaryElementKind); | ||
| [_asyncDelegate collectionNode:collectionNode didEndDisplayingSupplementaryElementWithNode:node]; | ||
| } | ||
|
|
@@ -1374,11 +1393,9 @@ - (void)scrollViewDidScroll:(UIScrollView *)scrollView | |
| [self _checkForBatchFetching]; | ||
| } | ||
|
|
||
| for (_ASCollectionViewCell *cell in _cellsForVisibilityUpdates) { | ||
| // Only nodes that respond to the selector are added to _cellsForVisibilityUpdates | ||
| [cell cellNodeVisibilityEvent:ASCellNodeVisibilityEventVisibleRectChanged inScrollView:scrollView]; | ||
| } | ||
| if (_asyncDelegateFlags.scrollViewDidScroll) { | ||
| [_asyncDelegate scrollViewDidScroll:scrollView]; | ||
|
|
@@ -1414,10 +1431,8 @@ - (void)scrollViewDidEndDecelerating:(UIScrollView *)scrollView | |
|
|
||
| - (void)scrollViewWillBeginDragging:(UIScrollView *)scrollView | ||
| { | ||
| for (_ASCollectionViewCell *cell in _cellsForVisibilityUpdates) { | ||
| [cell cellNodeVisibilityEvent:ASCellNodeVisibilityEventWillBeginDragging inScrollView:scrollView]; | ||
| } | ||
| if (_asyncDelegateFlags.scrollViewWillBeginDragging) { | ||
| [_asyncDelegate scrollViewWillBeginDragging:scrollView]; | ||
|
|
@@ -1426,14 +1441,12 @@ - (void)scrollViewWillBeginDragging:(UIScrollView *)scrollView | |
|
|
||
| - (void)scrollViewDidEndDragging:(UIScrollView *)scrollView willDecelerate:(BOOL)decelerate | ||
| { | ||
| for (_ASCollectionViewCell *cell in _cellsForVisibilityUpdates) { | ||
|
There was a problem hiding this comment. Is it valid to use this type when iterating over _cellsForVisibilityUpdates? The gating factor to add things to this array is whether the cell consumes visibility events, but this might theoretically happen for non-_AS cells too. Maybe we should make the _cellsForVisibilityUpdates collection typed to <id > or something. If this already works correctly because of the early-returns checking for the _AS classes that occur before the cells are added, then that's OK for now. There was a problem hiding this comment. I'm gonna keep this as is as we do make sure only |
||
| [cell cellNodeVisibilityEvent:ASCellNodeVisibilityEventDidEndDragging inScrollView:scrollView]; | ||
| } | ||
| if (_asyncDelegateFlags.scrollViewDidEndDragging) { | ||
| [_asyncDelegate scrollViewDidEndDragging:scrollView willDecelerate:decelerate]; | ||
| } | ||
| } | ||
|
|
||
| #pragma mark - Scroll Direction. | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because
-supplementaryNodeForElementKind:atIndexPathinternally calls[_dataController.visibleMap supplementaryElementOfKind:atIndexPath].node, we're literally asking for the element twice in this method. Let's directly reach out to the visible map.