[ASCollectionView] Small improvements (TextureGroup#407)

* Minor refactors in ASCollectionView and its private cell classes - `_ASCollectionReusableView` and `_ASCollectionViewCell` no longer expose getters for their collection element and cell node properties. This is to make sure that clients can only obtain elements from the visible map which is always the source of truth. - Since the map can return `nil` for an element request, it's much safer to check and avoid adding/removing a nil pointer to an `NSArray`. - Since we use a special way to check whether an object of kind of `_ASCollectionViewCell` or `_ASCollectionReusableView`, based on the assumption that these classes are subclass restricted, I added cast-or-return macros in the header files, closer to where the restrictions are declared. * Add ASDynamicCastStrict * Add element and node properties back to _ASCollectionReusableView and _ASCollectionViewCell * Assert unexpected nil elements * Always mark an element visible even if it is backed by an UIKit / non-_ASCollection* view * Fix typo * Remove unnecessary changes * Dump mistakes * Update CHANGELOG * Can't track visibility of elements backed by non-_AS views
AtomTickets · Apr 25, 2018 · 97bceab · 97bceab
1 parent 47c7d5b
commit 97bceab
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 61 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@
 - Overhaul logging and add activity tracing support. [Adlai Holler](https://github.com/Adlai-Holler)
 - Fix a crash where scrolling a table view after entering editing mode could lead to bad internal states in the table. [Huy Nguyen](https://github.com/nguyenhuy) [#416](https://github.com/TextureGroup/Texture/pull/416/)
 - Fix a crash in collection view that occurs if batch updates are performed while scrolling [Huy Nguyen](https://github.com/nguyenhuy) [#378](https://github.com/TextureGroup/Texture/issues/378)
+- Some improvements in ASCollectionView [Huy Nguyen](https://github.com/nguyenhuy) [#407](https://github.com/TextureGroup/Texture/pull/407)
 
 ##2.3.4
 - [Yoga] Rewrite YOGA_TREE_CONTIGUOUS mode with improved behavior and cleaner integration [Scott Goodson](https://github.com/appleguy)

diff --git a/Source/ASCollectionView.mm b/Source/ASCollectionView.mm
@@ -952,46 +952,60 @@ - (NSInteger)collectionView:(UICollectionView *)collectionView numberOfItemsInSe
 - (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout *)collectionViewLayout sizeForItemAtIndexPath:(NSIndexPath *)indexPath
 {
   ASDisplayNodeAssertMainThread();
-  ASCellNode *cell = [self nodeForItemAtIndexPath:indexPath];
+  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;
     }
   }
-  ASCollectionElement *e = [_dataController.visibleMap elementForItemAtIndexPath:indexPath];
-  return [self sizeForElement:e];
+
+  return [self sizeForElement:element];
 }
 
 - (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout *)layout referenceSizeForHeaderInSection:(NSInteger)section
 {
   ASDisplayNodeAssertMainThread();
   NSIndexPath *indexPath = [NSIndexPath indexPathForItem:0 inSection:section];
-  ASCellNode *cell = [self supplementaryNodeForElementKind:UICollectionElementKindSectionHeader
-                                               atIndexPath:indexPath];
-  if (cell.shouldUseUIKitCell && _asyncDelegateFlags.interop) {
+  ASCollectionElement *element = [_dataController.visibleMap supplementaryElementOfKind:UICollectionElementKindSectionHeader
+                                                                            atIndexPath:indexPath];
+  if (element == nil) {
+    return CGSizeZero;
+  }
+
+  if (element.node.shouldUseUIKitCell && _asyncDelegateFlags.interop) {
     if ([_asyncDelegate respondsToSelector:@selector(collectionView:layout:referenceSizeForHeaderInSection:)]) {
       return [(id)_asyncDelegate collectionView:collectionView layout:layout referenceSizeForHeaderInSection:section];
     }
   }
-  ASCollectionElement *e = [_dataController.visibleMap supplementaryElementOfKind:UICollectionElementKindSectionHeader atIndexPath:indexPath];
-  return [self sizeForElement:e];
+
+  return [self sizeForElement:element];
 }
 
 - (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout *)layout referenceSizeForFooterInSection:(NSInteger)section
 {
   ASDisplayNodeAssertMainThread();
   NSIndexPath *indexPath = [NSIndexPath indexPathForItem:0 inSection:section];
-  ASCellNode *cell = [self supplementaryNodeForElementKind:UICollectionElementKindSectionFooter
-                                               atIndexPath:indexPath];
-  if (cell.shouldUseUIKitCell && _asyncDelegateFlags.interop) {
+  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];
     }
   }
-  ASCollectionElement *e = [_dataController.visibleMap supplementaryElementOfKind:UICollectionElementKindSectionFooter atIndexPath:indexPath];
-  return [self sizeForElement:e];
+
+  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 = ASDynamicCast(view, _ASCollectionReusableView)) {
+  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 = ASDynamicCast(cell, _ASCollectionViewCell)) {
+  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];
   }
 
-  // Since _ASCollectionViewCell is not available for subclassing, this is faster than isKindOfClass:
-  // We must exit early here, because only _ASCollectionViewCell implements the -node accessor method.
-  if ([rawCell class] != [_ASCollectionViewCell class]) {
+  _ASCollectionViewCell *cell = ASDynamicCastStrict(rawCell, _ASCollectionViewCell);
+  if (cell == nil) {
     [_rangeController setNeedsUpdate];
     return;
   }
-  auto cell = (_ASCollectionViewCell *)rawCell;
-
+
   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 (ASSubclassOverridesSelector([ASCellNode class], [cellNode class], @selector(cellNodeVisibilityEvent:inScrollView:withCellFrame:))) {
+  if ([cell consumesCellNodeVisibilityEvents]) {
     [_cellsForVisibilityUpdates addObject:cell];
   }
 }
@@ -1111,14 +1124,13 @@ - (void)collectionView:(UICollectionView *)collectionView didEndDisplayingCell:(
     [(id <ASCollectionDelegateInterop>)_asyncDelegate collectionView:collectionView didEndDisplayingCell:rawCell forItemAtIndexPath:indexPath];
   }
 
-  // Since _ASCollectionViewCell is not available for subclassing, this is faster than isKindOfClass:
-  // We must exit early here, because only _ASCollectionViewCell implements the -node accessor method.
-  if ([rawCell class] != [_ASCollectionViewCell class]) {
+  _ASCollectionViewCell *cell = ASDynamicCastStrict(rawCell, _ASCollectionViewCell);
+  if (cell == nil) {
     [_rangeController setNeedsUpdate];
     return;
   }
-  auto cell = (_ASCollectionViewCell *)rawCell;
-
+
+  // 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
 {
-  if (rawView.class != [_ASCollectionReusableView class]) {
+  _ASCollectionReusableView *view = ASDynamicCastStrict(rawView, _ASCollectionReusableView);
+  if (view == nil) {
     return;
   }
-  auto view = (_ASCollectionReusableView *)rawView;
 
-  if (view.element) {
-    [_visibleElements addObject:view.element];
+  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;
   }
 
-  // This is a safeguard similar to the behavior for cells in -[ASCollectionView collectionView:willDisplayCell:forItemAtIndexPath:]
-  // It ensures _ASCollectionReusableView receives layoutAttributes and calls applyLayoutAttributes.
+  // 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 = [self supplementaryNodeForElementKind:elementKind atIndexPath:indexPath];
+    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
 {
-  if (rawView.class != [_ASCollectionReusableView class]) {
+  _ASCollectionReusableView *view = ASDynamicCastStrict(rawView, _ASCollectionReusableView);
+  if (view == nil) {
     return;
   }
-  auto view = (_ASCollectionReusableView *)rawView;
 
-  if (view.element) {
-    [_visibleElements removeObject:view.element];
+  // 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 = [self supplementaryNodeForElementKind:elementKind atIndexPath:indexPath];
+    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 *collectionCell in _cellsForVisibilityUpdates) {
+  for (_ASCollectionViewCell *cell in _cellsForVisibilityUpdates) {
     // Only nodes that respond to the selector are added to _cellsForVisibilityUpdates
-    [[collectionCell node] cellNodeVisibilityEvent:ASCellNodeVisibilityEventVisibleRectChanged
-                                      inScrollView:scrollView
-                                     withCellFrame:collectionCell.frame];
+    [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 *collectionCell in _cellsForVisibilityUpdates) {
-    [[collectionCell node] cellNodeVisibilityEvent:ASCellNodeVisibilityEventWillBeginDragging
-                                          inScrollView:scrollView
-                                         withCellFrame:collectionCell.frame];
+  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 *collectionCell in _cellsForVisibilityUpdates) {
-        [[collectionCell node] cellNodeVisibilityEvent:ASCellNodeVisibilityEventDidEndDragging
-                                          inScrollView:scrollView
-                                         withCellFrame:collectionCell.frame];
-    }
-    if (_asyncDelegateFlags.scrollViewDidEndDragging) {
-        [_asyncDelegate scrollViewDidEndDragging:scrollView willDecelerate:decelerate];
-    }
+  for (_ASCollectionViewCell *cell in _cellsForVisibilityUpdates) {
+    [cell cellNodeVisibilityEvent:ASCellNodeVisibilityEventDidEndDragging inScrollView:scrollView];
+  }
+  if (_asyncDelegateFlags.scrollViewDidEndDragging) {
+    [_asyncDelegate scrollViewDidEndDragging:scrollView willDecelerate:decelerate];
+  }
 }
 
 #pragma mark - Scroll Direction.

diff --git a/Source/Base/ASBaseDefines.h b/Source/Base/ASBaseDefines.h
@@ -235,6 +235,12 @@
   ((c *) ([__val isKindOfClass:[c class]] ? __val : nil));\
 })
 
+/// Ensure that class is of certain kind, assuming it is subclass restricted
+#define ASDynamicCastStrict(x, c) ({ \
+  id __val = x;\
+  ((c *) ([__val class] == [c class] ? __val : nil));\
+})
+
 /**
  * Create a new set by mapping `collection` over `work`, ignoring nil.
  */

diff --git a/Source/Details/_ASCollectionReusableView.h b/Source/Details/_ASCollectionReusableView.h
@@ -22,11 +22,13 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
-AS_SUBCLASSING_RESTRICTED
+AS_SUBCLASSING_RESTRICTED // Note: ASDynamicCastStrict is used on instances of this class based on this restriction.
 @interface _ASCollectionReusableView : UICollectionReusableView
+
 @property (nonatomic, strong, readonly, nullable) ASCellNode *node;
 @property (nonatomic, strong, nullable) ASCollectionElement *element;
 @property (nonatomic, strong, nullable) UICollectionViewLayoutAttributes *layoutAttributes;
+
 @end
 
 NS_ASSUME_NONNULL_END
diff --git a/Source/Details/_ASCollectionReusableView.m b/Source/Details/_ASCollectionReusableView.m
@@ -16,9 +16,8 @@
 //
 
 #import "_ASCollectionReusableView.h"
-#import "ASCellNode+Internal.h"
+#import <AsyncDisplayKit/ASCellNode+Internal.h>
 #import <AsyncDisplayKit/ASCollectionElement.h>
-#import <AsyncDisplayKit/AsyncDisplayKit.h>
 
 @implementation _ASCollectionReusableView
 

diff --git a/Source/Details/_ASCollectionViewCell.h b/Source/Details/_ASCollectionViewCell.h
@@ -17,16 +17,27 @@
 
 #import <UIKit/UIKit.h>
 #import <AsyncDisplayKit/ASBaseDefines.h>
+#import <AsyncDisplayKit/ASCellNode.h>
 
-@class ASCellNode, ASCollectionElement;
+@class ASCollectionElement;
 
 NS_ASSUME_NONNULL_BEGIN
 
-AS_SUBCLASSING_RESTRICTED
+AS_SUBCLASSING_RESTRICTED // Note: ASDynamicCastStrict is used on instances of this class based on this restriction.
 @interface _ASCollectionViewCell : UICollectionViewCell
+
 @property (nonatomic, strong, nullable) ASCollectionElement *element;
 @property (nonatomic, strong, readonly, nullable) ASCellNode *node;
 @property (nonatomic, strong, nullable) UICollectionViewLayoutAttributes *layoutAttributes;
+
+/**
+ * Whether or not this cell is interested in cell node visibility events.
+ * -cellNodeVisibilityEvent:inScrollView: should be called only if this property is YES.
+ */
+@property (nonatomic, readonly) BOOL consumesCellNodeVisibilityEvents;
+
+- (void)cellNodeVisibilityEvent:(ASCellNodeVisibilityEvent)event inScrollView:(UIScrollView *)scrollView;
+
 @end
 
 NS_ASSUME_NONNULL_END
diff --git a/Source/Details/_ASCollectionViewCell.m b/Source/Details/_ASCollectionViewCell.m
@@ -16,9 +16,9 @@
 //
 
 #import "_ASCollectionViewCell.h"
-#import "ASCellNode+Internal.h"
-#import <AsyncDisplayKit/AsyncDisplayKit.h>
+#import <AsyncDisplayKit/ASCellNode+Internal.h>
 #import <AsyncDisplayKit/ASCollectionElement.h>
+#import <AsyncDisplayKit/ASInternalHelpers.h>
 
 @implementation _ASCollectionViewCell
 
@@ -38,6 +38,20 @@ - (void)setElement:(ASCollectionElement *)element
   [node __setHighlightedFromUIKit:self.highlighted];
 }
 
+- (BOOL)consumesCellNodeVisibilityEvents
+{
+  ASCellNode *node = self.node;
+  if (node == nil) {
+    return NO;
+  }
+  return ASSubclassOverridesSelector([ASCellNode class], [node class], @selector(cellNodeVisibilityEvent:inScrollView:withCellFrame:));
+}
+
+- (void)cellNodeVisibilityEvent:(ASCellNodeVisibilityEvent)event inScrollView:(UIScrollView *)scrollView
+{
+  [self.node cellNodeVisibilityEvent:event inScrollView:scrollView withCellFrame:self.frame];
+}
+
 - (void)setSelected:(BOOL)selected
 {
   [super setSelected:selected];