Merge pull request #961 from facebook/AutomagicalNodeHierarchyHookups

[ASDisplayNode] Automatically find and hook up node hierarchies, even when -addSubview: is used directly.

Author: appleguy

AsyncDisplayKit/ASDisplayNode.mm

@@ -996,13 +996,14 @@ - (void)_insertSubnode:(ASDisplayNode *)subnode atSubnodeIndex:(NSInteger)subnod
   if (isMovingEquivalentParents) {
     [subnode __incrementVisibilityNotificationsDisabled];
   }
+  
   [subnode removeFromSupernode];
-
+  [oldSubnode removeFromSupernode];
+  
   if (!_subnodes)
     _subnodes = [[NSMutableArray alloc] init];
-
-  [oldSubnode removeFromSupernode];
   [_subnodes insertObject:subnode atIndex:subnodeIndex];
+  [subnode __setSupernode:self];
 
   // Don't bother inserting the view/layer if in a rasterized subtree, because there are no layers in the hierarchy and none of this could possibly work.
   if (!_flags.shouldRasterizeDescendants && [self __shouldLoadViewOrLayer]) {
@@ -1030,8 +1031,6 @@ - (void)_insertSubnode:(ASDisplayNode *)subnode atSubnodeIndex:(NSInteger)subnod
   if (isMovingEquivalentParents) {
     [subnode __decrementVisibilityNotificationsDisabled];
   }
-
-  [subnode __setSupernode:self];
 }
 
 - (void)replaceSubnode:(ASDisplayNode *)oldSubnode withSubnode:(ASDisplayNode *)replacementSubnode
@@ -1225,17 +1224,24 @@ - (void)removeFromSupernode
   if (!_supernode)
     return;
 
+  // Check to ensure that our view or layer is actually inside of our supernode; otherwise, don't remove it.
+  // Though _ASDisplayView decouples the supernode if it is inserted inside another view hierarchy, this is
+  // more difficult to guarantee with _ASDisplayLayer because CoreAnimation doesn't have a -didMoveToSuperlayer.
+  BOOL shouldRemoveFromSuperviewOrSuperlayer = NO;
+  
+  if (self.nodeLoaded && _supernode.nodeLoaded) {
+    if (_flags.layerBacked || _supernode.layerBacked) {
+      shouldRemoveFromSuperviewOrSuperlayer = (_layer.superlayer == _supernode.layer);
+    } else {
+      shouldRemoveFromSuperviewOrSuperlayer = (_view.superview == _supernode.view);
+    }
+  }
+
   // Do this before removing the view from the hierarchy, as the node will clear its supernode pointer when its view is removed from the hierarchy.
   [_supernode _removeSubnode:self];
 
-  if (ASDisplayNodeThreadIsMain()) {
-    if (_flags.layerBacked) {
-      [_layer removeFromSuperlayer];
-    } else {
-      [_view removeFromSuperview];
-    }
-  } else {
-    dispatch_async(dispatch_get_main_queue(), ^{
+  if (shouldRemoveFromSuperviewOrSuperlayer) {
+    ASPerformBlockOnMainThread(^{
       if (_flags.layerBacked) {
         [_layer removeFromSuperlayer];
       } else {
@@ -1301,7 +1307,7 @@ - (void)__enterHierarchy
     _flags.isEnteringHierarchy = NO;
 
     CALayer *layer = self.layer;
-    if (!self.layer.contents) {
+    if (!layer.contents) {
       [layer setNeedsDisplay];
     }
   }
@@ -2317,22 +2323,33 @@ @implementation CALayer (ASDisplayNodeInternal)
 
 @implementation UIView (AsyncDisplayKit)
 
-- (void)addSubnode:(ASDisplayNode *)node
+- (void)addSubnode:(ASDisplayNode *)subnode
 {
-  if (node.layerBacked) {
-    [self.layer addSublayer:node.layer];
+  if (subnode.layerBacked) {
+    // Call -addSubnode: so that we use the asyncdisplaykit_node path if possible.
+    [self.layer addSubnode:subnode];
   } else {
-    [self addSubview:node.view];
+    ASDisplayNode *selfNode = self.asyncdisplaykit_node;
+    if (selfNode) {
+      [selfNode addSubnode:subnode];
+    } else {
+      [self addSubview:subnode.view];
+    }
   }
 }
 
 @end
 
 @implementation CALayer (AsyncDisplayKit)
 
-- (void)addSubnode:(ASDisplayNode *)node
+- (void)addSubnode:(ASDisplayNode *)subnode
 {
-  [self addSublayer:node.layer];
+  ASDisplayNode *selfNode = self.asyncdisplaykit_node;
+  if (selfNode) {
+    [selfNode addSubnode:subnode];
+  } else {
+    [self addSublayer:subnode.layer];
+  }
 }
 
 @end

AsyncDisplayKit/Details/_ASDisplayView.mm

@@ -63,6 +63,22 @@ - (id)initWithFrame:(CGRect)frame
   return self;
 }
 
+- (void)willMoveToWindow:(UIWindow *)newWindow
+{
+  BOOL visible = (newWindow != nil);
+  if (visible && !_node.inHierarchy) {
+    [_node __enterHierarchy];
+  }
+}
+
+- (void)didMoveToWindow
+{
+  BOOL visible = (self.window != nil);
+  if (!visible && _node.inHierarchy) {
+    [_node __exitHierarchy];
+  }
+}
+
 - (void)willMoveToSuperview:(UIView *)newSuperview
 {
   // Keep the node alive while the view is in a view hierarchy.  This helps ensure that async-drawing views can always
@@ -76,28 +92,74 @@ - (void)willMoveToSuperview:(UIView *)newSuperview
   else if (currentSuperview && !newSuperview) {
     self.keepalive_node = nil;
   }
-}
-
-- (void)willMoveToWindow:(UIWindow *)newWindow
-{
-  BOOL visible = newWindow != nil;
-  if (visible && !_node.inHierarchy) {
-    [_node __enterHierarchy];
-  } else if (!visible && _node.inHierarchy) {
-    [_node __exitHierarchy];
+  
+  if (newSuperview) {
+    ASDisplayNode *supernode = _node.supernode;
+    BOOL supernodeLoaded = supernode.nodeLoaded;
+    ASDisplayNodeAssert(!supernode.isLayerBacked, @"Shouldn't be possible for _ASDisplayView's supernode to be layer-backed.");
+    
+    BOOL needsSupernodeUpdate = NO;
+
+    if (supernode) {
+      if (supernodeLoaded) {
+        if (supernode.layerBacked) {
+          // See comment in -didMoveToSuperview.  This case should be avoided, but is possible with app-level coding errors.
+          needsSupernodeUpdate = (supernode.layer != newSuperview.layer);
+        } else {
+          // If we have a supernode, compensate for users directly messing with views by hitching up to any new supernode.
+          needsSupernodeUpdate = (supernode.view != newSuperview);
+        }
+      } else {
+        needsSupernodeUpdate = YES;
+      }
+    } else {
+      // If we have no supernode and we are now in a view hierarchy, check to see if we can hook up to a supernode.
+      needsSupernodeUpdate = (newSuperview != nil);
+    }
+
+    if (needsSupernodeUpdate) {
+      // -removeFromSupernode is called by -addSubnode:, if it is needed.
+      [newSuperview.asyncdisplaykit_node addSubnode:_node];
+    }
   }
+
 }
 
 - (void)didMoveToSuperview
 {
-  // FIXME maybe move this logic into ASDisplayNode addSubnode/removeFromSupernode
-  UIView *superview = self.superview;
-
-  // If superview's node is different from supernode's view, fix it by setting supernode to the new superview's node.  Got that?
-  if (!superview)
-    [_node __setSupernode:nil];
-  else if (superview != _node.supernode.view)
-    [_node __setSupernode:superview.asyncdisplaykit_node];
+  ASDisplayNode *supernode = _node.supernode;
+  ASDisplayNodeAssert(!supernode.isLayerBacked, @"Shouldn't be possible for superview's node to be layer-backed.");
+  
+  if (supernode) {
+    ASDisplayNodeAssertTrue(_node.nodeLoaded);
+    UIView *superview = self.superview;
+    BOOL supernodeLoaded = supernode.nodeLoaded;
+    BOOL needsSupernodeRemoval = NO;
+    
+    if (superview) {
+      // If our new superview is not the same as the supernode's view, or the supernode has no view, disconnect.
+      if (supernodeLoaded) {
+        if (supernode.layerBacked) {
+          // As asserted at the top, this shouldn't be possible, but in production with assertions disabled it can happen.
+          // We try to make such code behave as well as feasible because it's not that hard of an error to make if some deep
+          // child node of a layer-backed node happens to be view-backed, but it is not supported and should be avoided.
+          needsSupernodeRemoval = (supernode.layer != superview.layer);
+        } else {
+          needsSupernodeRemoval = (supernode.view != superview);
+        }
+      } else {
+        needsSupernodeRemoval = YES;
+      }
+    } else {
+      // If supernode is loaded but our superview is nil, the user manually removed us, so disconnect supernode.
+      needsSupernodeRemoval = supernodeLoaded;
+    }
+    
+    if (needsSupernodeRemoval) {
+      // The node will only disconnect from its supernode, not removeFromSuperview, in this condition.
+      [_node removeFromSupernode];
+    }
+  }
 }
 
 - (void)setNeedsDisplay

AsyncDisplayKitTests/ASDisplayNodeTests.m

@@ -1342,16 +1342,20 @@ - (void)testInsertSubviewAtIndexWithMeddlingViewsAndLayersViewBacked
   [parent insertSubnode:c belowSubnode:b];
   XCTAssertEqualObjects(orderStringFromSublayers(parent.layer), @"a,e,d,c,b", @"Didn't match");
 
-  XCTAssertEqual(3u, parent.subnodes.count, @"Should have the right subnode count");
+  XCTAssertEqual(4u, parent.subnodes.count, @"Should have the right subnode count");
   XCTAssertEqual(4u, parent.view.subviews.count, @"Should have the right subview count");
   XCTAssertEqual(5u, parent.layer.sublayers.count, @"Should have the right sublayer count");
+  
+  [e removeFromSuperlayer];
+  XCTAssertEqual(4u, parent.layer.sublayers.count, @"Should have the right sublayer count");
 
   //TODO: assert that things deallocate immediately and don't have latent autoreleases in here
   [parent release];
   [a release];
   [b release];
   [c release];
   [d release];
+  [e release];
 }
 
 - (void)testAppleBugInsertSubview
@@ -1415,11 +1419,11 @@ - (void)testInsertSubviewAtIndexWithMeddlingView
   [parent.view insertSubview:d aboveSubview:a.view];
   XCTAssertEqualObjects(orderStringFromSublayers(parent.layer), @"a,d,b", @"Didn't match");
 
-  // (a,e,d,b) => (a,d,>c<,b)
+  // (a,d,b) => (a,d,>c<,b)
   [parent insertSubnode:c belowSubnode:b];
   XCTAssertEqualObjects(orderStringFromSublayers(parent.layer), @"a,d,c,b", @"Didn't match");
 
-  XCTAssertEqual(3u, parent.subnodes.count, @"Should have the right subnode count");
+  XCTAssertEqual(4u, parent.subnodes.count, @"Should have the right subnode count");
   XCTAssertEqual(4u, parent.view.subviews.count, @"Should have the right subview count");
   XCTAssertEqual(4u, parent.layer.sublayers.count, @"Should have the right sublayer count");