Address comments

Author: nguyenhuy

AsyncDisplayKit/ASCellNode.h

@@ -74,12 +74,12 @@ typedef NS_ENUM(NSUInteger, ASCellNodeVisibilityEvent) {
  */
 @property (nonatomic, assign) BOOL neverShowPlaceholders;
 
-//TODO shouldn't this be a generic kind (e.g includes ASDataControllerRowNodeKind)?
 /*
  * The kind of supplementary element this node represents, if any.
  *
  * @return The supplementary element kind, or @c nil if this node does not represent a supplementary element.
  */
+//TODO change this to be a generic "kind" or "elementKind" that exposes `nil` for row kind
 @property (nonatomic, copy, readonly, nullable) NSString *supplementaryElementKind;
 
 /*

AsyncDisplayKit/ASDisplayNode+Beta.h

@@ -64,7 +64,7 @@ typedef struct {
  * This property defaults to NO. It will be removed in a future release.
  */
 + (BOOL)suppressesInvalidCollectionUpdateExceptions AS_WARN_UNUSED_RESULT ASDISPLAYNODE_DEPRECATED_MSG("Collection update exceptions are thrown if assertions are enabled.");
-+ (void)setSuppressesInvalidCollectionUpdateExceptions:(BOOL)suppresses ASDISPLAYNODE_DEPRECATED_MSG("Collection update exceptions are thrown if assertions are enabled.");;
++ (void)setSuppressesInvalidCollectionUpdateExceptions:(BOOL)suppresses ASDISPLAYNODE_DEPRECATED_MSG("Collection update exceptions are thrown if assertions are enabled.");
 
 /**
  * @abstract Recursively ensures node and all subnodes are displayed.

AsyncDisplayKit/Details/ASDataController.h

@@ -93,7 +93,7 @@ extern NSString * const ASCollectionInvalidUpdateException;
  *
  * @param changeSet The change set that includes all updates
  */
-- (void)dataController:(ASDataController * )dataController willUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet;;
+- (void)dataController:(ASDataController *)dataController willUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet;
 
 /**
  * Called for change set updates.

AsyncDisplayKit/Details/ASDataController.mm

@@ -25,8 +25,8 @@
 #import <AsyncDisplayKit/ASDisplayNode+Subclasses.h>
 #import <AsyncDisplayKit/NSIndexSet+ASHelpers.h>
 
-#define LOG(...) NSLog(__VA_ARGS__)
-//#define LOG(...)
+//#define LOG(...) NSLog(__VA_ARGS__)
+#define LOG(...)
 
 #define AS_MEASURE_AVOIDED_DATACONTROLLER_WORK 0
 
@@ -144,10 +144,9 @@ + (NSUInteger)parallelProcessorCount
 
 #pragma mark - Cell Layout
 
-- (void)batchLayoutNodesFromContexts:(NSArray<ASIndexedNodeContext *> *)contexts batchSize:(NSUInteger)batchSize batchCompletion:(ASDataControllerCompletionBlock)batchCompletionHandler
+- (void)batchLayoutNodesFromContexts:(NSArray<ASIndexedNodeContext *> *)contexts batchSize:(NSInteger)batchSize batchCompletion:(ASDataControllerCompletionBlock)batchCompletionHandler
 {
   ASSERT_ON_EDITING_QUEUE;
-  //TODO add Logs and AS_MEASURE_AVOIDED_DATACONTROLLER_WORK back
 #if AS_MEASURE_AVOIDED_DATACONTROLLER_WORK
     [ASDataController _expectToInsertNodes:contexts.count];
 #endif
@@ -197,19 +196,19 @@ - (void)_layoutNode:(ASCellNode *)node withConstrainedSize:(ASSizeRange)constrai
   }
 
   __strong ASCellNode **allocatedNodeBuffer = (__strong ASCellNode **)calloc(nodeCount, sizeof(ASCellNode *));
-  
+
   dispatch_queue_t queue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
   ASDispatchApply(nodeCount, queue, 0, ^(size_t i) {
     RETURN_IF_NO_DATASOURCE();
-    
+
     // Allocate the node.
     ASIndexedNodeContext *context = contexts[i];
     ASCellNode *node = context.node;
     if (node == nil) {
       ASDisplayNodeAssertNotNil(node, @"Node block created nil node; %@, %@", self, self.dataSource);
       node = [[ASCellNode alloc] init]; // Fallback to avoid crash for production apps.
     }
-    
+
     // Layout the node if the size range is valid.
     ASSizeRange sizeRange = context.constrainedSize;
     if (ASSizeRangeHasSignificantArea(sizeRange)) {
@@ -221,9 +220,9 @@ - (void)_layoutNode:(ASCellNode *)node withConstrainedSize:(ASSizeRange)constrai
 #endif
     allocatedNodeBuffer[i] = node;
   });
-  
+
   BOOL canceled = _dataSource == nil;
-  
+
   // Create nodes array
   NSArray *nodes = canceled ? nil : [NSArray arrayWithObjects:allocatedNodeBuffer count:nodeCount];
 
@@ -232,7 +231,7 @@ - (void)_layoutNode:(ASCellNode *)node withConstrainedSize:(ASSizeRange)constrai
     allocatedNodeBuffer[i] = nil;
   }
   free(allocatedNodeBuffer);
-  
+
   return nodes;
 }
 
@@ -289,9 +288,10 @@ - (void)_repopulateSupplementaryNodesForAllSectionsContainingIndexPaths:(NSArray
   // Get all the sections that need to be repopulated
   NSIndexSet *sectionIndexes = [NSIndexSet as_sectionsFromIndexPaths:originalIndexPaths];
   for (NSString *kind in [self supplementaryKindsInSections:sectionIndexes]) {
+    // TODO: Would it make more sense to do _nodeContexts enumerateKeysAndObjectsUsingBlock: for this removal step?
+    // That way we are sure we removed all the old supplementaries, even if that kind isn't present anymore.
+    
     // Step 1: Remove all existing contexts of this kind in these sections
-    // TODO: NSEnumerationConcurrent?
-    // TODO: Consider using a diffing algorithm here
     [_nodeContexts[kind] enumerateObjectsAtIndexes:sectionIndexes options:0 usingBlock:^(NSMutableArray<ASIndexedNodeContext *> * _Nonnull obj, NSUInteger idx, BOOL * _Nonnull stop) {
       [obj removeAllObjects];
     }];
@@ -409,7 +409,7 @@ - (ASSizeRange)constrainedSizeForNodeOfKind:(NSString *)kind atIndexPath:(NSInde
   }
 
   ASDisplayNodeAssert(NO, @"Unknown constrained size for node of kind %@ by data source %@", kind, _dataSource);
-  return ASSizeRangeMake(CGSizeZero, CGSizeZero);
+  return ASSizeRangeZero;
 }
 
 #pragma mark - Batching (External API)
@@ -750,10 +750,10 @@ - (NSIndexPath *)_indexPathForNode:(ASCellNode *)cellNode inContexts:(ASNodeCont
   if (cellNode == nil) {
     return nil;
   }
-  
+
   NSString *kind = cellNode.supplementaryElementKind ?: ASDataControllerRowNodeKind;
   ASNodeContextTwoDimensionalArray *sections = contexts[kind];
-  
+
   // Check if the cached index path is still correct.
   NSIndexPath *indexPath = cellNode.cachedIndexPath;
   if (indexPath != nil) {
@@ -765,7 +765,7 @@ - (NSIndexPath *)_indexPathForNode:(ASCellNode *)cellNode inContexts:(ASNodeCont
       indexPath = nil;
     }
   }
-  
+
   // Loop through each section to look for the node context
   NSInteger sectionIdx = 0;
   for (NSArray<ASIndexedNodeContext *> *section in sections) {

AsyncDisplayKit/Details/ASIndexedNodeContext.h

@@ -22,7 +22,7 @@ NS_ASSUME_NONNULL_BEGIN
  * property too heavily – we should remove it in the future.
  */
 @property (nonatomic, readonly, strong) NSIndexPath *indexPath;
-//TODO shouldn't this be a generic kind (e.g includes ASDataControllerRowNodeKind)?
+//TODO change this to be a generic "kind" or "elementKind" that exposes `nil` for row kind
 @property (nonatomic, readonly, copy, nullable) NSString *supplementaryElementKind;
 @property (nonatomic, assign) ASSizeRange constrainedSize;
 @property (weak, nonatomic) id<ASTraitEnvironment> traitEnvironment;

AsyncDisplayKit/Details/ASRangeController.h

@@ -152,14 +152,14 @@ AS_SUBCLASSING_RESTRICTED
  *
  * @param changeSet The change set that includes all updates
  */
-- (void)rangeController:(ASRangeController * )rangeController willUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet;;
+- (void)rangeController:(ASRangeController *)rangeController willUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet;
 
 /**
  * Called after updating with given change set.
  *
  * @param changeSet The change set that includes all updates
  */
-- (void)rangeController:(ASRangeController * )rangeController didUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet;;
+- (void)rangeController:(ASRangeController *)rangeController didUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet;
 
 @end
 

AsyncDisplayKit/Details/ASTraitCollection.h

@@ -92,7 +92,7 @@ ASDISPLAYNODE_EXTERN_C_END
  * Deprecated and should be replaced by the methods from above
  */
 - (ASEnvironmentTraitCollection)environmentTraitCollection;
-- (void)setEnvironmentTraitCollection:(ASEnvironmentTraitCollection)traitCollection;;
+- (void)setEnvironmentTraitCollection:(ASEnvironmentTraitCollection)traitCollection;
 
 
 @end

AsyncDisplayKit/Layout/ASLayoutElement.h

@@ -77,7 +77,7 @@ ASDISPLAYNODE_EXTERN_C_END
 /**
  * @abstract Returns type of layoutElement
  */
-@property (nonatomic, assign, readonly) ASLayoutElementType layoutElementType;;
+@property (nonatomic, assign, readonly) ASLayoutElementType layoutElementType;
 
 /**
  * @abstract A size constraint that should apply to this ASLayoutElement.

AsyncDisplayKit/Private/Layout/ASStackUnpositionedLayout.mm

@@ -183,7 +183,7 @@ static void computeCrossSizeAndBaseline(const std::vector<ASStackLayoutSpecItem>
   }
 
   // Step 3. The used cross-size of the flex line is the largest of the numbers found in the previous two steps and zero.
-  crossSize = MAX(maxStartToBaselineDistance + maxBaselineToEndDistance, maxItemCrossSize);;
+  crossSize = MAX(maxStartToBaselineDistance + maxBaselineToEndDistance, maxItemCrossSize);
   // Clamp the cross-size to be within the stack's min and max cross-size properties.
   crossSize = MIN(MAX(minCrossSize, crossSize), maxCrossSize);
 

AsyncDisplayKit/Private/_ASHierarchyChangeSet.h

@@ -128,6 +128,7 @@ NSString *NSStringFromASHierarchyChangeType(_ASHierarchyChangeType changeType);
 
 @property (nonatomic, readonly) BOOL completed;
 /// Whether or not changes should be animated.
+// TODO: if any update in this chagne set is non-animated, the whole update should be non-animated.
 @property (nonatomic, readwrite) BOOL animated;
 @property (nonatomic, readonly) BOOL includesReloadData;