Merge pull request #1816 from hannahmbanana/selectionBug

[ASCellNode] Fix selection / highlight implementation

AsyncDisplayKit/ASCellNode+Internal.h

@@ -12,7 +12,7 @@
 
 #import "ASCellNode.h"
 
-@protocol ASCellNodeLayoutDelegate <NSObject>
+@protocol ASCellNodeInteractionDelegate <NSObject>
 
 /**
  * Notifies the delegate that the specified cell node has done a relayout.
@@ -27,18 +27,26 @@
  */
 - (void)nodeDidRelayout:(ASCellNode *)node sizeChanged:(BOOL)sizeChanged;
 
+/*
+ * Methods to be called whenever the selection or highlight state changes
+ * on ASCellNode. UIKit internally stores these values to update reusable cells.
+ */
+
+- (void)nodeSelectedStateDidChange:(ASCellNode *)node;
+- (void)nodeHighlightedStateDidChange:(ASCellNode *)node;
+
 @end
 
 @interface ASCellNode ()
 
-/*
- * A delegate to be notified (on main thread) after a relayout.
- */
-@property (nonatomic, weak) id<ASCellNodeLayoutDelegate> layoutDelegate;
+@property (nonatomic, weak) id <ASCellNodeInteractionDelegate> interactionDelegate;
 
 /*
  * Back-pointer to the containing scrollView instance, set only for visible cells.  Used for Cell Visibility Event callbacks.
  */
 @property (nonatomic, weak) UIScrollView *scrollView;
 
+- (void)__setSelectedFromUIKit:(BOOL)selected;
+- (void)__setHighlightedFromUIKit:(BOOL)highlighted;
+
 @end

AsyncDisplayKit/ASCellNode.h

@@ -81,14 +81,16 @@ typedef NS_ENUM(NSUInteger, ASCellNodeVisibilityEvent) {
 @property (nonatomic) UITableViewCellSelectionStyle selectionStyle;
 
 /**
- * A Boolean value that indicates whether the node is selected.
+ * A Boolean value that is synchronized with the underlying collection or tableView cell property.
+ * Setting this value is equivalent to calling selectItem / deselectItem on the collection or table.
  */
-@property (nonatomic, assign) BOOL selected;
+@property (nonatomic, assign, getter=isSelected) BOOL selected;
 
 /**
- * A Boolean value that indicates whether the node is highlighted.
+ * A Boolean value that is synchronized with the underlying collection or tableView cell property.
+ * Setting this value is equivalent to calling highlightItem / unHighlightItem on the collection or table.
  */
-@property (nonatomic, assign) BOOL highlighted;
+@property (nonatomic, assign, getter=isHighlighted) BOOL highlighted;
 
 /*
  * ASCellNode must forward touch events in order for UITableView and UICollectionView tap handling to work. Overriding
@@ -122,6 +124,16 @@ typedef NS_ENUM(NSUInteger, ASCellNodeVisibilityEvent) {
 
 @end
 
+@interface ASCellNode (Deprecated)
+
+/**
+ * Previous versions of ASDK did not include "is" in the name of the getter for these properties.
+ * These older accessor methods don't match UIKit naming, and will be removed in a future version.
+ */
+- (BOOL)selected ASDISPLAYNODE_DEPRECATED;
+- (BOOL)highlighted ASDISPLAYNODE_DEPRECATED;
+
+@end
 
 /**
  * Simple label-style cell node.  Read its source for an example of custom <ASCellNode>s.

AsyncDisplayKit/ASCellNode.mm

@@ -31,12 +31,13 @@
   ASDisplayNodeDidLoadBlock _viewControllerDidLoadBlock;
   ASDisplayNode *_viewControllerNode;
   UIViewController *_viewController;
+  BOOL _suspendInteractionDelegate;
 }
 
 @end
 
 @implementation ASCellNode
-@synthesize layoutDelegate = _layoutDelegate;
+@synthesize interactionDelegate = _interactionDelegate;
 
 - (instancetype)init
 {
@@ -170,14 +171,62 @@
 
 - (void)didRelayoutFromOldSize:(CGSize)oldSize toNewSize:(CGSize)newSize
 {
-  if (_layoutDelegate != nil) {
+  if (_interactionDelegate != nil) {
     ASPerformBlockOnMainThread(^{
       BOOL sizeChanged = !CGSizeEqualToSize(oldSize, newSize);
-      [_layoutDelegate nodeDidRelayout:self sizeChanged:sizeChanged];
+      [_interactionDelegate nodeDidRelayout:self sizeChanged:sizeChanged];
     });
   }
 }
 
+- (void)setSelected:(BOOL)selected
+{
+  if (_selected != selected) {
+    _selected = selected;
+    if (!_suspendInteractionDelegate) {
+      [_interactionDelegate nodeSelectedStateDidChange:self];
+    }
+  }
+}
+
+- (void)setHighlighted:(BOOL)highlighted
+{
+  if (_highlighted != highlighted) {
+    _highlighted = highlighted;
+    if (!_suspendInteractionDelegate) {
+      [_interactionDelegate nodeHighlightedStateDidChange:self];
+    }
+  }
+}
+
+- (void)__setSelectedFromUIKit:(BOOL)selected;
+{
+  if (selected != _selected) {
+    _suspendInteractionDelegate = YES;
+    self.selected = selected;
+    _suspendInteractionDelegate = NO;
+  }
+}
+
+- (void)__setHighlightedFromUIKit:(BOOL)highlighted;
+{
+  if (highlighted != _highlighted) {
+    _suspendInteractionDelegate = YES;
+    self.highlighted = highlighted;
+    _suspendInteractionDelegate = NO;
+  }
+}
+
+- (BOOL)selected
+{
+  return self.isSelected;
+}
+
+- (BOOL)highlighted
+{
+  return self.isSelected;
+}
+
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wobjc-missing-super-calls"
 

AsyncDisplayKit/ASCollectionView.mm

@@ -53,20 +53,27 @@ static NSString * const kCellReuseIdentifier = @"_ASCollectionViewCell";
 - (void)setNode:(ASCellNode *)node
 {
   _node = node;
-  node.selected = self.selected;
-  node.highlighted = self.highlighted;
+  [node __setSelectedFromUIKit:self.selected];
+  [node __setHighlightedFromUIKit:self.highlighted];
 }
 
 - (void)setSelected:(BOOL)selected
 {
   [super setSelected:selected];
-  _node.selected = selected;
+  [_node __setSelectedFromUIKit:selected];
 }
 
 - (void)setHighlighted:(BOOL)highlighted
 {
   [super setHighlighted:highlighted];
-  _node.highlighted = highlighted;
+  [_node __setHighlightedFromUIKit:highlighted];
+}
+
+- (void)prepareForReuse
+{
+  // Need to clear node pointer before UIKit calls setSelected:NO / setHighlighted:NO on its cells
+  self.node = nil;
+  [super prepareForReuse];
 }
 
 - (void)applyLayoutAttributes:(UICollectionViewLayoutAttributes *)layoutAttributes
@@ -79,7 +86,7 @@ static NSString * const kCellReuseIdentifier = @"_ASCollectionViewCell";
 #pragma mark -
 #pragma mark ASCollectionView.
 
-@interface ASCollectionView () <ASRangeControllerDataSource, ASRangeControllerDelegate, ASDataControllerSource, ASCellNodeLayoutDelegate, ASDelegateProxyInterceptor, ASBatchFetchingScrollView, ASDataControllerEnvironmentDelegate> {
+@interface ASCollectionView () <ASRangeControllerDataSource, ASRangeControllerDelegate, ASDataControllerSource, ASCellNodeInteractionDelegate, ASDelegateProxyInterceptor, ASBatchFetchingScrollView, ASDataControllerEnvironmentDelegate> {
   ASCollectionViewProxy *_proxyDataSource;
   ASCollectionViewProxy *_proxyDelegate;
   
@@ -908,8 +915,8 @@ static NSString * const kCellReuseIdentifier = @"_ASCollectionViewCell";
     return ^{
       __typeof__(self) strongSelf = weakSelf;
       [node enterHierarchyState:ASHierarchyStateRangeManaged];
-      if (node.layoutDelegate == nil) {
-        node.layoutDelegate = strongSelf;
+      if (node.interactionDelegate == nil) {
+        node.interactionDelegate = strongSelf;
       }
       return node;
     };
@@ -923,8 +930,8 @@ static NSString * const kCellReuseIdentifier = @"_ASCollectionViewCell";
 
     ASCellNode *node = block();
     [node enterHierarchyState:ASHierarchyStateRangeManaged];
-    if (node.layoutDelegate == nil) {
-      node.layoutDelegate = strongSelf;
+    if (node.interactionDelegate == nil) {
+      node.interactionDelegate = strongSelf;
     }
     return node;
   };
@@ -1171,6 +1178,25 @@ static NSString * const kCellReuseIdentifier = @"_ASCollectionViewCell";
 }
 
 #pragma mark - ASCellNodeDelegate
+- (void)nodeSelectedStateDidChange:(ASCellNode *)node
+{
+  NSIndexPath *indexPath = [self indexPathForNode:node];
+  if (indexPath) {
+    if (node.isSelected) {
+      [self selectItemAtIndexPath:indexPath animated:NO scrollPosition:UICollectionViewScrollPositionNone];
+    } else {
+      [self deselectItemAtIndexPath:indexPath animated:NO];
+    }
+  }
+}
+
+- (void)nodeHighlightedStateDidChange:(ASCellNode *)node
+{
+  NSIndexPath *indexPath = [self indexPathForNode:node];
+  if (indexPath) {
+    [self cellForItemAtIndexPath:indexPath].highlighted = node.isHighlighted;
+  }
+}
 
 - (void)nodeDidRelayout:(ASCellNode *)node sizeChanged:(BOOL)sizeChanged
 {

AsyncDisplayKit/ASTableView.mm

@@ -67,20 +67,27 @@ static NSString * const kCellReuseIdentifier = @"_ASTableViewCell";
 - (void)setNode:(ASCellNode *)node
 {
   _node = node;
-  node.selected = self.selected;
-  node.highlighted = self.highlighted;
+  [node __setSelectedFromUIKit:self.selected];
+  [node __setHighlightedFromUIKit:self.highlighted];
 }
 
 - (void)setSelected:(BOOL)selected animated:(BOOL)animated
 {
   [super setSelected:selected animated:animated];
-  _node.selected = selected;
+  [_node __setSelectedFromUIKit:selected];
 }
 
 - (void)setHighlighted:(BOOL)highlighted animated:(BOOL)animated
 {
   [super setHighlighted:highlighted animated:animated];
-  _node.highlighted = highlighted;
+  [_node __setHighlightedFromUIKit:highlighted];
+}
+
+- (void)prepareForReuse
+{
+  // Need to clear node pointer before UIKit calls setSelected:NO / setHighlighted:NO on its cells
+  self.node = nil;
+  [super prepareForReuse];
 }
 
 @end
@@ -92,7 +99,7 @@ static NSString * const kCellReuseIdentifier = @"_ASTableViewCell";
 - (instancetype)_initWithTableView:(ASTableView *)tableView;
 @end
 
-@interface ASTableView () <ASRangeControllerDataSource, ASRangeControllerDelegate, ASDataControllerSource, _ASTableViewCellDelegate, ASCellNodeLayoutDelegate, ASDelegateProxyInterceptor, ASBatchFetchingScrollView, ASDataControllerEnvironmentDelegate>
+@interface ASTableView () <ASRangeControllerDataSource, ASRangeControllerDelegate, ASDataControllerSource, _ASTableViewCellDelegate, ASCellNodeInteractionDelegate, ASDelegateProxyInterceptor, ASBatchFetchingScrollView, ASDataControllerEnvironmentDelegate>
 {
   ASTableViewProxy *_proxyDataSource;
   ASTableViewProxy *_proxyDelegate;
@@ -1069,8 +1076,8 @@ static NSString * const kCellReuseIdentifier = @"_ASTableViewCell";
     return ^{
       __typeof__(self) strongSelf = weakSelf;
       [node enterHierarchyState:ASHierarchyStateRangeManaged];
-      if (node.layoutDelegate == nil) {
-        node.layoutDelegate = strongSelf;
+      if (node.interactionDelegate == nil) {
+        node.interactionDelegate = strongSelf;
       }
       return node;
     };
@@ -1082,8 +1089,8 @@ static NSString * const kCellReuseIdentifier = @"_ASTableViewCell";
     __typeof__(self) strongSelf = weakSelf;
     ASCellNode *node = block();
     [node enterHierarchyState:ASHierarchyStateRangeManaged];
-    if (node.layoutDelegate == nil) {
-      node.layoutDelegate = strongSelf;
+    if (node.interactionDelegate == nil) {
+      node.interactionDelegate = strongSelf;
     }
     return node;
   };
@@ -1158,7 +1165,27 @@ static NSString * const kCellReuseIdentifier = @"_ASTableViewCell";
   }
 }
 
-#pragma mark - ASCellNodeLayoutDelegate
+#pragma mark - ASCellNodeDelegate
+
+- (void)nodeSelectedStateDidChange:(ASCellNode *)node
+{
+  NSIndexPath *indexPath = [self indexPathForNode:node];
+  if (indexPath) {
+    if (node.isSelected) {
+      [self selectRowAtIndexPath:indexPath animated:NO scrollPosition:UITableViewScrollPositionNone];
+    } else {
+      [self deselectRowAtIndexPath:indexPath animated:NO];
+    }
+  }
+}
+
+- (void)nodeHighlightedStateDidChange:(ASCellNode *)node
+{
+  NSIndexPath *indexPath = [self indexPathForNode:node];
+  if (indexPath) {
+    [self cellForRowAtIndexPath:indexPath].highlighted = node.isHighlighted;
+  }
+}
 
 - (void)nodeDidRelayout:(ASCellNode *)node sizeChanged:(BOOL)sizeChanged
 {

AsyncDisplayKitTests/ASCollectionViewTests.m

@@ -13,6 +13,22 @@
 #import "ASCollectionDataController.h"
 #import "ASCollectionViewFlowLayoutInspector.h"
 
+@interface ASTextCellNodeWithSetSelectedCounter : ASTextCellNode
+
+@property (nonatomic, assign) NSUInteger setSelectedCounter;
+
+@end
+
+@implementation ASTextCellNodeWithSetSelectedCounter
+
+- (void)setSelected:(BOOL)selected
+{
+  [super setSelected:selected];
+  _setSelectedCounter++;
+}
+
+@end
+
 @interface ASCollectionViewTestDelegate : NSObject <ASCollectionViewDataSource, ASCollectionViewDelegate>
 
 @property (nonatomic, assign) NSInteger numberOfSections;
@@ -32,7 +48,7 @@
 }
 
 - (ASCellNode *)collectionView:(ASCollectionView *)collectionView nodeForItemAtIndexPath:(NSIndexPath *)indexPath {
-  ASTextCellNode *textCellNode = [ASTextCellNode new];
+  ASTextCellNodeWithSetSelectedCounter *textCellNode = [ASTextCellNodeWithSetSelectedCounter new];
   textCellNode.text = indexPath.description;
 
   return textCellNode;
@@ -41,7 +57,7 @@
 
 - (ASCellNodeBlock)collectionView:(ASCollectionView *)collectionView nodeBlockForItemAtIndexPath:(NSIndexPath *)indexPath {
   return ^{
-    ASTextCellNode *textCellNode = [ASTextCellNode new];
+    ASTextCellNodeWithSetSelectedCounter *textCellNode = [ASTextCellNodeWithSetSelectedCounter new];
     textCellNode.text = indexPath.description;
     return textCellNode;
   };
@@ -134,6 +150,55 @@
   [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:1.0]];
 }
 
+- (void)testSelection
+{
+  ASCollectionViewTestController *testController = [[ASCollectionViewTestController alloc] initWithNibName:nil bundle:nil];
+  UIWindow *window = [[UIWindow alloc] initWithFrame:[[UIScreen mainScreen] bounds]];
+  [window setRootViewController:testController];
+  [window makeKeyAndVisible];
+  
+  [testController.collectionView reloadDataImmediately];
+  [testController.collectionView layoutIfNeeded];
+  
+  NSIndexPath *indexPath = [NSIndexPath indexPathForItem:0 inSection:0];
+  ASCellNode *node = [testController.collectionView nodeForItemAtIndexPath:indexPath];
+  
+  // selecting node should select cell
+  node.selected = YES;
+  XCTAssertTrue([[testController.collectionView indexPathsForSelectedItems] containsObject:indexPath], @"Selecting node should update cell selection.");
+  
+  // deselecting node should deselect cell
+  node.selected = NO;
+  XCTAssertTrue([[testController.collectionView indexPathsForSelectedItems] isEqualToArray:@[]], @"Deselecting node should update cell selection.");
+
+  // selecting cell via collectionView should select node
+  [testController.collectionView selectItemAtIndexPath:indexPath animated:NO scrollPosition:UICollectionViewScrollPositionNone];
+  XCTAssertTrue(node.isSelected == YES, @"Selecting cell should update node selection.");
+  
+  // deselecting cell via collectionView should deselect node
+  [testController.collectionView deselectItemAtIndexPath:indexPath animated:NO];
+  XCTAssertTrue(node.isSelected == NO, @"Deselecting cell should update node selection.");
+  
+  // select the cell again, scroll down and back up, and check that the state persisted
+  [testController.collectionView selectItemAtIndexPath:indexPath animated:NO scrollPosition:UICollectionViewScrollPositionNone];
+  XCTAssertTrue(node.isSelected == YES, @"Selecting cell should update node selection.");
+  
+  // reload cell (-prepareForReuse is called) & check that selected state is preserved
+  [testController.collectionView setContentOffset:CGPointMake(0,testController.collectionView.bounds.size.height)];
+  [testController.collectionView layoutIfNeeded];
+  [testController.collectionView setContentOffset:CGPointMake(0,0)];
+  [testController.collectionView layoutIfNeeded];
+  XCTAssertTrue(node.isSelected == YES, @"Reloaded cell should preserve state.");
+  
+  // deselecting cell should deselect node
+  UICollectionViewCell *cell = [testController.collectionView cellForItemAtIndexPath:indexPath];
+  cell.selected = NO;
+  XCTAssertTrue(node.isSelected == NO, @"Deselecting cell should update node selection.");
+  
+  // check setSelected not called extra times
+  XCTAssertTrue([(ASTextCellNodeWithSetSelectedCounter *)node setSelectedCounter] == 6, @"setSelected: should not be called on node multiple times.");
+}
+
 - (void)testTuningParametersWithExplicitRangeMode
 {
   UICollectionViewFlowLayout *layout = [[UICollectionViewFlowLayout alloc] init];
@@ -174,5 +239,4 @@
   XCTAssertTrue(ASRangeTuningParametersEqualToRangeTuningParameters(preloadParams, [collectionView tuningParametersForRangeType:ASLayoutRangeTypeFetchData]));
 }
 
-
 @end