Range Controller Explicitly Marks Disappeared Nodes as Invisible (#2805)

* Improve visibility handling for range-managed nodes

* Tweak ASWeakSet & the test

* Put back a few things that are pending a different diff

* Add a test

* Ensure we update visible nodes, even if there are no new ones

Author: Adlai Holler

AsyncDisplayKit/Details/ASRangeController.mm

@@ -34,6 +34,7 @@ @interface ASRangeController ()
   BOOL _layoutControllerImplementsSetVisibleIndexPaths;
   BOOL _layoutControllerImplementsSetViewportSize;
   NSSet<NSIndexPath *> *_allPreviousIndexPaths;
+  ASWeakSet<ASCellNode *> *_visibleNodes;
   ASLayoutRangeMode _currentRangeMode;
   BOOL _preserveCurrentRangeMode;
   BOOL _didRegisterForNodeDisplayNotifications;
@@ -169,6 +170,24 @@ - (void)setDataSource:(id<ASRangeControllerDataSource>)dataSource
   }
 }
 
+// Clear the visible bit from any nodes that disappeared since last update.
+// Currently we guarantee that nodes will not be marked visible when deallocated,
+// but it's OK to be in e.g. the preload range. So for the visible bit specifically,
+// we add this extra mechanism to account for e.g. deleted items.
+//
+// NOTE: There is a minor risk here, if a node is transferred from one range controller
+// to another before the first rc updates and clears the node out of this set. It's a pretty
+// wild scenario that I doubt happens in practice.
+- (void)_setVisibleNodes:(ASWeakSet *)newVisibleNodes
+{
+  for (ASCellNode *node in _visibleNodes) {
+    if (![newVisibleNodes containsObject:node] && node.isVisible) {
+      [node exitInterfaceState:ASInterfaceStateVisible];
+    }
+  }
+  _visibleNodes = newVisibleNodes;
+}
+
 - (void)_updateVisibleNodeIndexPaths
 {
   ASDisplayNodeAssert(_layoutController, @"An ASLayoutController is required by ASRangeController");
@@ -187,8 +206,10 @@ - (void)_updateVisibleNodeIndexPaths
   // TODO: Consider if we need to use this codepath, or can rely on something more similar to the data & display ranges
   // Example: ... = [_layoutController indexPathsForScrolling:scrollDirection rangeType:ASLayoutRangeTypeVisible];
   NSArray<NSIndexPath *> *visibleNodePaths = [_dataSource visibleNodeIndexPathsForRangeController:self];
-  
+  ASWeakSet *newVisibleNodes = [[ASWeakSet alloc] init];
+
   if (visibleNodePaths.count == 0) { // if we don't have any visibleNodes currently (scrolled before or after content)...
+    [self _setVisibleNodes:newVisibleNodes];
     return; // don't do anything for this update, but leave _rangeIsValid == NO to make sure we update it later
   }
   ASProfilingSignpostStart(1, self);
@@ -272,7 +293,7 @@ - (void)_updateVisibleNodeIndexPaths
   ASDisplayNodeAssertTrue([visibleIndexPaths isSubsetOfSet:displayIndexPaths]);
   NSMutableArray<NSIndexPath *> *modifiedIndexPaths = (ASRangeControllerLoggingEnabled ? [NSMutableArray array] : nil);
 #endif
-  
+
   for (NSIndexPath *indexPath in allIndexPaths) {
     // Before a node / indexPath is exposed to ASRangeController, ASDataController should have already measured it.
     // For consistency, make sure each node knows that it should measure itself if something changes.
@@ -326,6 +347,9 @@ - (void)_updateVisibleNodeIndexPaths
         ASDisplayNode *node = currentSectionNodes[row];
 
         ASDisplayNodeAssert(node.hierarchyState & ASHierarchyStateRangeManaged, @"All nodes reaching this point should be range-managed, or interfaceState may be incorrectly reset.");
+        if (ASInterfaceStateIncludesVisible(interfaceState)) {
+          [newVisibleNodes addObject:node];
+        }
         // Skip the many method calls of the recursive operation if the top level cell node already has the right interfaceState.
         if (node.interfaceState != interfaceState) {
 #if ASRangeControllerLoggingEnabled
@@ -345,6 +369,8 @@ - (void)_updateVisibleNodeIndexPaths
       }
     }
   }
+
+  [self _setVisibleNodes:newVisibleNodes];
 
   // TODO: This code is for debugging only, but would be great to clean up with a delegate method implementation.
   if ([ASRangeController shouldShowRangeDebugOverlay]) {

AsyncDisplayKit/Details/ASWeakSet.h

@@ -15,6 +15,11 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
+/**
+ * A class similar to NSSet that stores objects weakly.
+ * Note that this class uses NSPointerFunctionsObjectPointerPersonality –
+ * that is, it uses shifted pointer for hashing, and identity comparison for equality.
+ */
 @interface ASWeakSet<__covariant ObjectType> : NSObject<NSFastEnumeration>
 
 /// Returns YES if the receiver is empty, NO otherwise.

AsyncDisplayKit/Details/ASWeakSet.m

@@ -22,7 +22,7 @@ - (instancetype)init
 {
   self = [super init];
   if (self) {
-    _hashTable = [NSHashTable weakObjectsHashTable];
+    _hashTable = [NSHashTable hashTableWithOptions:NSPointerFunctionsWeakMemory | NSPointerFunctionsObjectPointerPersonality];
   }
   return self;
 }
@@ -59,7 +59,7 @@ - (BOOL)isEmpty
 
 /**
  Note: The `count` property of NSHashTable is unreliable
- in the case of weak-object hash tables because entries
+ in the case of weak-memory hash tables because entries
  that have been deallocated are not removed immediately.
  
  In order to get the true count we have to fall back to using

AsyncDisplayKitTests/ASCollectionViewTests.mm

@@ -815,6 +815,27 @@ - (void)testThatNilBatchUpdatesCanBeSubmitted
   [cn performBatchAnimated:NO updates:nil completion:nil];
 }
 
+- (void)testThatDeletedItemsAreMarkedInvisible
+{
+  UIWindow *window = [[UIWindow alloc] initWithFrame:[UIScreen mainScreen].bounds];
+  ASCollectionViewTestController *testController = [[ASCollectionViewTestController alloc] initWithNibName:nil bundle:nil];
+  window.rootViewController = testController;
+
+  __block NSInteger itemCount = 1;
+  testController.asyncDelegate->_itemCounts = {itemCount};
+  [window makeKeyAndVisible];
+  [window layoutIfNeeded];
+
+  ASCollectionNode *cn = testController.collectionNode;
+  [cn waitUntilAllUpdatesAreCommitted];
+  ASCellNode *node = [cn nodeForItemAtIndexPath:[NSIndexPath indexPathForItem:0 inSection:0]];
+  XCTAssertTrue(node.visible);
+  testController.asyncDelegate->_itemCounts = {0};
+  [cn deleteItemsAtIndexPaths: @[[NSIndexPath indexPathForItem:0 inSection:0]]];
+  [self expectationForPredicate:[NSPredicate predicateWithFormat:@"visible = NO"] evaluatedWithObject:node handler:nil];
+  [self waitForExpectationsWithTimeout:3 handler:nil];
+}
+
 - (void)testThatMultipleBatchFetchesDontHappenUnnecessarily
 {
   UIWindow *window = [[UIWindow alloc] initWithFrame:[UIScreen mainScreen].bounds];