Merge pull request #1398 from facebook/Placeholders

[ASDisplayNode] Placeholders should always be recreated if returning to cleared nodes, and remain until ASNetwork/MultiplexImageNode finish loading their image from the network.

AsyncDisplayKit/ASDisplayNode+Beta.h

@@ -91,4 +91,11 @@ ASDISPLAYNODE_EXTERN_C_END
                          shouldMeasureAsync:(BOOL)shouldMeasureAsync
                       measurementCompletion:(void(^)())completion;
 
+
+/**
+ * @abstract Currently used by ASNetworkImageNode and ASMultiplexImageNode to allow their placeholders to stay if they are loading an image from the network.
+ * Otherwise, a display pass is scheduled and completes, but does not actually draw anything - and ASDisplayNode considers the element finished.
+ */
+- (BOOL)placeholderShouldPersist;
+
 @end

AsyncDisplayKit/ASDisplayNode.mm

@@ -426,6 +426,7 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c)
 {
   CALayer *layer;
   ASDN::MutexLocker l(_propertyLock);
+  ASDisplayNodeAssert(_flags.layerBacked, @"_layerToLoad is only for layer-backed nodes");
 
   if (_layerBlock) {
     layer = _layerBlock();
@@ -481,10 +482,6 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c)
     TIME_SCOPED(_debugTimeForDidLoad);
     [self __didLoad];
   }
-
-  if (self.placeholderEnabled) {
-    [self _setupPlaceholderLayer];
-  }
 }
 
 - (UIView *)view
@@ -749,16 +746,19 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c)
   ASDN::MutexLocker l(_propertyLock);
   [self calculatedLayoutDidChange];
 
-  // we generate placeholders at measureWithSizeRange: time so that a node is guaranteed
-  // to have a placeholder ready to go. Also, if a node has no size it should not have a placeholder
-  if (self.placeholderEnabled && [self _displaysAsynchronously] &&
-      _layout.size.width > 0.0 && _layout.size.height > 0.0) {
-    if (!_placeholderImage) {
-      _placeholderImage = [self placeholderImage];
+  // We generate placeholders at measureWithSizeRange: time so that a node is guaranteed to have a placeholder ready to go.
+  // This is also because measurement is usually asynchronous, but placeholders need to be set up synchronously.
+  // First measurement is guaranteed to be before the node is onscreen, so we can create the image async. but still have it appear sync.
+  if (_placeholderEnabled && [self _displaysAsynchronously] && self.contents == nil) {
+    
+    // Zero-sized nodes do not require a placeholder.
+    CGSize layoutSize = (_layout ? _layout.size : CGSizeZero);
+    if (CGSizeEqualToSize(layoutSize, CGSizeZero)) {
+      return;
     }
 
-    if (_placeholderLayer) {
-      [self _setupPlaceholderLayerContents];
+    if (!_placeholderImage) {
+      _placeholderImage = [self placeholderImage];
     }
   }
 }
@@ -1010,13 +1010,14 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c)
 {
   ASDisplayNodeAssertMainThread();
   ASDN::MutexLocker l(_propertyLock);
-  if (CGRectEqualToRect(self.bounds, CGRectZero)) {
+  CGRect bounds = self.bounds;
+  if (CGRectEqualToRect(bounds, CGRectZero)) {
     // Performing layout on a zero-bounds view often results in frame calculations
     // with negative sizes after applying margins, which will cause
     // measureWithSizeRange: on subnodes to assert.
     return;
   }
-  _placeholderLayer.frame = self.bounds;
+  _placeholderLayer.frame = bounds;
   [self layout];
   [self layoutDidFinish];
 }
@@ -1545,9 +1546,20 @@ static NSInteger incrementIfFound(NSInteger i) {
     }
     _flags.isEnteringHierarchy = NO;
 
+    
+    // If we don't have contents finished drawing by the time we are on screen, immediately add the placeholder (if it is enabled and we do have something to draw).
+    if (self.contents == nil) {
       CALayer *layer = self.layer;
-    if (!layer.contents) {
       [layer setNeedsDisplay];
+      
+      if ([self _shouldHavePlaceholderLayer]) {
+        [CATransaction begin];
+        [CATransaction setDisableActions:YES];
+        [self _setupPlaceholderLayerIfNeeded];
+        _placeholderLayer.opacity = 1.0;
+        [CATransaction commit];
+        [self.layer addSublayer:_placeholderLayer];
+      }
     }
   }
 }
@@ -1677,10 +1689,9 @@ static NSInteger incrementIfFound(NSInteger i) {
 
   [_pendingDisplayNodes removeObject:node];
 
-  // only trampoline if there is a placeholder and nodes are done displaying
-  if ([self _pendingDisplayNodesHaveFinished] && _placeholderLayer.superlayer) {
+  if (_pendingDisplayNodes.count == 0 && _placeholderLayer.superlayer && ![self placeholderShouldPersist]) {
     void (^cleanupBlock)() = ^{
-      [self _tearDownPlaceholderLayer];
+      [_placeholderLayer removeFromSuperlayer];
     };
 
     if (_placeholderFadeDuration > 0.0 && ASInterfaceStateIncludesVisible(self.interfaceState)) {
@@ -1695,33 +1706,46 @@ static NSInteger incrementIfFound(NSInteger i) {
   }
 }
 
-// Helper method to check that all nodes that the current node is waiting to display are finished
-// Use this method to check to remove any placeholder layers
-- (BOOL)_pendingDisplayNodesHaveFinished
-{
-  return _pendingDisplayNodes.count == 0;
-}
-
 // Helper method to summarize whether or not the node run through the display process
 - (BOOL)__implementsDisplay
 {
   return _flags.implementsDrawRect || _flags.implementsImageDisplay || self.shouldRasterizeDescendants || _flags.implementsInstanceDrawRect || _flags.implementsInstanceImageDisplay;
 }
 
-- (void)_setupPlaceholderLayer
+- (BOOL)placeholderShouldPersist
+{
+  return NO;
+}
+
+- (BOOL)_shouldHavePlaceholderLayer
+{
+  return (_placeholderEnabled && [self __implementsDisplay]);
+}
+
+- (void)_setupPlaceholderLayerIfNeeded
 {
   ASDisplayNodeAssertMainThread();
 
+  if (!_placeholderLayer) {
     _placeholderLayer = [CALayer layer];
     // do not set to CGFLOAT_MAX in the case that something needs to be overtop the placeholder
     _placeholderLayer.zPosition = 9999.0;
   }
 
-- (void)_tearDownPlaceholderLayer
-{
-  ASDisplayNodeAssertMainThread();
-
-  [_placeholderLayer removeFromSuperlayer];
+  if (_placeholderLayer.contents == nil) {
+    if (!_placeholderImage) {
+      _placeholderImage = [self placeholderImage];
+    }
+    if (_placeholderImage) {
+      BOOL stretchable = !UIEdgeInsetsEqualToEdgeInsets(_placeholderImage.capInsets, UIEdgeInsetsZero);
+      if (stretchable) {
+        ASDisplayNodeSetupLayerContentsWithResizableImage(_placeholderLayer, _placeholderImage);
+      } else {
+        _placeholderLayer.contentsScale = self.contentsScale;
+        _placeholderLayer.contents = (id)_placeholderImage.CGImage;
+      }
+    }
+  }
 }
 
 void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock)
@@ -2258,26 +2282,6 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock)
   [self _pendingNodeWillDisplay:self];
 
   [_supernode subnodeDisplayWillStart:self];
-
-  if (_placeholderImage && _placeholderLayer && self.layer.contents == nil) {
-    [CATransaction begin];
-    [CATransaction setDisableActions:YES];
-    [self _setupPlaceholderLayerContents];
-    _placeholderLayer.opacity = 1.0;
-    [CATransaction commit];
-    [self.layer addSublayer:_placeholderLayer];
-  }
-}
-
-- (void)_setupPlaceholderLayerContents
-{
-  BOOL stretchable = !UIEdgeInsetsEqualToEdgeInsets(_placeholderImage.capInsets, UIEdgeInsetsZero);
-  if (stretchable) {
-    ASDisplayNodeSetupLayerContentsWithResizableImage(_placeholderLayer, _placeholderImage);
-  } else {
-    _placeholderLayer.contentsScale = self.contentsScale;
-    _placeholderLayer.contents = (id)_placeholderImage.CGImage;
-  }
 }
 
 - (void)displayDidFinish

AsyncDisplayKit/ASImageNode.mm

@@ -25,6 +25,7 @@
 
 @interface _ASImageNodeDrawParameters : NSObject
 
+@property (nonatomic, retain) UIImage *image;
 @property (nonatomic, assign) BOOL opaque;
 @property (nonatomic, assign) CGRect bounds;
 @property (nonatomic, assign) CGFloat contentsScale;
@@ -36,11 +37,17 @@
 // TODO: eliminate explicit parameters with a set of keys copied from the node
 @implementation _ASImageNodeDrawParameters
 
-- (id)initWithBounds:(CGRect)bounds opaque:(BOOL)opaque contentsScale:(CGFloat)contentsScale backgroundColor:(UIColor *)backgroundColor contentMode:(UIViewContentMode)contentMode
+- (id)initWithImage:(UIImage *)image
+             bounds:(CGRect)bounds
+             opaque:(BOOL)opaque
+      contentsScale:(CGFloat)contentsScale
+    backgroundColor:(UIColor *)backgroundColor
+        contentMode:(UIViewContentMode)contentMode
 {
-  self = [self init];
-  if (!self) return nil;
+  if (!(self = [self init]))
+    return nil;
 
+  _image = image;
   _opaque = opaque;
   _bounds = bounds;
   _contentsScale = contentsScale;
@@ -133,8 +140,13 @@
     _image = image;
 
     _imageLock.unlock();
+    
     [self invalidateCalculatedLayout];
+    if (image) {
       [self setNeedsDisplay];
+    } else {
+      self.contents = nil;
+    }
   } else {
     _imageLock.unlock(); // We avoid using MutexUnlocker as it needlessly re-locks at the end of the scope.
   }
@@ -156,7 +168,8 @@
 
 - (NSObject *)drawParametersForAsyncLayer:(_ASDisplayLayer *)layer
 {
-  return [[_ASImageNodeDrawParameters alloc] initWithBounds:self.bounds
+  return [[_ASImageNodeDrawParameters alloc] initWithImage:self.image
+                                                    bounds:self.bounds
                                                     opaque:self.opaque
                                              contentsScale:self.contentsScaleForDisplay
                                            backgroundColor:self.backgroundColor
@@ -171,21 +184,26 @@
 
 - (UIImage *)displayWithParameters:(_ASImageNodeDrawParameters *)parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled
 {
-  UIImage *image;
-  BOOL cropEnabled;
-  BOOL forceUpscaling;
-  CGFloat contentsScale;
-  CGRect cropDisplayBounds;
-  CGRect cropRect;
-  asimagenode_modification_block_t imageModificationBlock;
-  
-  {
-    ASDN::MutexLocker l(_imageLock);
-    image = _image;
+  UIImage *image = parameters.image;
   if (!image) {
     return nil;
   }
   
+  BOOL forceUpscaling           = NO;
+  BOOL cropEnabled              = NO;
+  BOOL isOpaque                 = parameters.opaque;
+  UIColor *backgroundColor      = parameters.backgroundColor;
+  UIViewContentMode contentMode = parameters.contentMode;
+  CGFloat contentsScale         = 0.0;
+  CGRect cropDisplayBounds      = CGRectZero;
+  CGRect cropRect               = CGRectZero;
+  asimagenode_modification_block_t imageModificationBlock;
+  
+  {
+    ASDN::MutexLocker l(_imageLock);
+    
+    // FIXME: There is a small risk of these values changing between the main thread creation of drawParameters, and the execution of this method.
+    // We should package these up into the draw parameters object.  Might be easiest to create a struct for the non-objects and make it one property.
     cropEnabled = _cropEnabled;
     forceUpscaling = _forceUpscaling;
     contentsScale = _contentsScaleForDisplay;
@@ -194,16 +212,12 @@
     imageModificationBlock = _imageModificationBlock;
   }
   
+  BOOL hasValidCropBounds = cropEnabled && !CGRectIsNull(cropDisplayBounds) && !CGRectIsEmpty(cropDisplayBounds);
+  CGRect bounds = (hasValidCropBounds ? cropDisplayBounds : parameters.bounds);
+  
   ASDisplayNodeContextModifier preContextBlock = self.willDisplayNodeContentWithRenderingContext;
   ASDisplayNodeContextModifier postContextBlock = self.didDisplayNodeContentWithRenderingContext;
   
-  BOOL hasValidCropBounds = cropEnabled && !CGRectIsNull(cropDisplayBounds) && !CGRectIsEmpty(cropDisplayBounds);
-  
-  CGRect bounds = (hasValidCropBounds ? cropDisplayBounds : parameters.bounds);
-  BOOL isOpaque = parameters.opaque;
-  UIColor *backgroundColor = parameters.backgroundColor;
-  UIViewContentMode contentMode = parameters.contentMode;
-  
   ASDisplayNodeAssert(contentsScale > 0, @"invalid contentsScale at display time");
   
   // if the image is resizable, bail early since the image has likely already been configured
@@ -232,17 +246,15 @@
     }
   }
   
-  BOOL contentModeSupported =    contentMode == UIViewContentModeScaleAspectFill
-  || contentMode == UIViewContentModeScaleAspectFit
-  || contentMode == UIViewContentModeCenter;
+  BOOL contentModeSupported = contentMode == UIViewContentModeScaleAspectFill ||
+                              contentMode == UIViewContentModeScaleAspectFit ||
+                              contentMode == UIViewContentModeCenter;
   
-  CGSize backingSize;
-  CGRect imageDrawRect;
+  CGSize backingSize   = CGSizeZero;
+  CGRect imageDrawRect = CGRectZero;
   
-  if (boundsSizeInPixels.width * contentsScale < 1.0f ||
-      boundsSizeInPixels.height * contentsScale < 1.0f ||
-      imageSizeInPixels.width < 1.0f ||
-      imageSizeInPixels.height < 1.0f) {
+  if (boundsSizeInPixels.width * contentsScale < 1.0f || boundsSizeInPixels.height * contentsScale < 1.0f ||
+      imageSizeInPixels.width < 1.0f                  || imageSizeInPixels.height < 1.0f) {
     return nil;
   }
   
@@ -260,10 +272,8 @@
                                                  &imageDrawRect);
   }
   
-  if (backingSize.width <= 0.0f ||
-      backingSize.height <= 0.0f ||
-      imageDrawRect.size.width <= 0.0f ||
-      imageDrawRect.size.height <= 0.0f) {
+  if (backingSize.width <= 0.0f        || backingSize.height <= 0.0f ||
+      imageDrawRect.size.width <= 0.0f || imageDrawRect.size.height <= 0.0f) {
     return nil;
   }
   

AsyncDisplayKit/ASMultiplexImageNode.mm

@@ -265,6 +265,11 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent
   }
 }
 
+- (BOOL)placeholderShouldPersist
+{
+  return (self.image == nil && self.imageIdentifiers.count > 0);
+}
+
 /* displayWillStart in ASNetworkImageNode has a very similar implementation. Changes here are likely necessary
  in ASNetworkImageNode as well. */
 - (void)displayWillStart

AsyncDisplayKit/ASNetworkImageNode.mm

@@ -173,6 +173,12 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0};
   return _delegate;
 }
 
+- (BOOL)placeholderShouldPersist
+{
+  ASDN::MutexLocker l(_lock);
+  return (self.image == nil && _URL != nil);
+}
+
 /* displayWillStart in ASMultiplexImageNode has a very similar implementation. Changes here are likely necessary
  in ASMultiplexImageNode as well. */
 - (void)displayWillStart
@@ -334,6 +340,7 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0};
 
 - (void)_lazilyLoadImageIfNecessary
 {
+  // FIXME: We should revisit locking in this method (e.g. to access the instance variables at the top, and holding lock while calling delegate)
   if (!_imageLoaded && _URL != nil && _downloadIdentifier == nil) {
     {
       ASDN::MutexLocker l(_lock);