Call out to AnimatedImage subclass hook in the next runloop cycle (Te…

…xtureGroup#1087)

CHANGELOG.md

@@ -99,6 +99,7 @@
 - Standardize property declaration style. [Adlai Holler](https://github.com/Adlai-Holler)
 - [ASTableView] Fix an issue that causes table view to use one of a cell's invalid layouts instead of generating a new one. [Huy Nguyen](https://github.com/nguyenhuy) [#942](https://github.com/TextureGroup/Texture/pull/942)
 - Introduce let / var macros and some further cleanup. [Michael Schneider](https://github.com/maicki) [#1012](https://github.com/TextureGroup/Texture/pull/1012)
+- Unlock before cleanup and calling out to subclass hooks for animated images. [Michael Schneider](https://github.com/maicki) [#1087](https://github.com/TextureGroup/Texture/pull/1087)
 
 ## 2.6
 - [Xcode 9] Updated to require Xcode 9 (to fix warnings) [Garrett Moon](https://github.com/garrettmoon)

Source/ASImageNode+AnimatedImage.mm

@@ -44,12 +44,12 @@ - (void)setAnimatedImage:(id <ASAnimatedImageProtocol>)animatedImage
 - (void)_locked_setAnimatedImage:(id <ASAnimatedImageProtocol>)animatedImage
 {
   ASAssertLocked(__instanceLock__);
-  
+
   if (ASObjectIsEqual(_animatedImage, animatedImage) && (animatedImage == nil || animatedImage.playbackReady)) {
     return;
   }
 
-  id <ASAnimatedImageProtocol> previousAnimatedImage = _animatedImage;
+  __block id <ASAnimatedImageProtocol> previousAnimatedImage = _animatedImage;
 
   _animatedImage = animatedImage;
 
@@ -70,22 +70,35 @@ - (void)_locked_setAnimatedImage:(id <ASAnimatedImageProtocol>)animatedImage
       [self _locked_setShouldAnimate:YES];
     }
   } else {
-      // Clean up after ourselves.
-      self.contents = nil;
-      [self setCoverImage:nil];
-  }
-
-  [self animatedImageSet:_animatedImage previousAnimatedImage:previousAnimatedImage];
+    // Clean up after ourselves.
 
-  // Animated image can take while to dealloc, do it off the main queue
-  if (previousAnimatedImage != nil) {
-    ASPerformBackgroundDeallocation(&previousAnimatedImage);
+    // Don't bother using a `_locked` version for setting contnst as it should be pretty safe calling it with
+    // reaquire the lock and would add overhead to introduce this version
+    self.contents = nil;
+    [self _locked_setCoverImage:nil];
   }
+
+  // Push calling subclass to the next runloop cycle
+  // We have to schedule the block on the common modes otherwise the tracking mode will not be included and it will
+  // not fire e.g. while scrolling down
+  CFRunLoopPerformBlock(CFRunLoopGetCurrent(), kCFRunLoopCommonModes, ^(void) {
+    [self animatedImageSet:animatedImage previousAnimatedImage:previousAnimatedImage];
+
+    // Animated image can take while to dealloc, do it off the main queue
+    if (previousAnimatedImage != nil) {
+      ASPerformBackgroundDeallocation(&previousAnimatedImage);
+    }
+  });
+  // Don't need to wakeup the runloop as the current is already running
+  // CFRunLoopWakeUp(runLoop); // Should not be necessary
 }
 
 - (void)animatedImageSet:(id <ASAnimatedImageProtocol>)newAnimatedImage previousAnimatedImage:(id <ASAnimatedImageProtocol>)previousAnimatedImage
 {
-  //Subclasses may override
+  // Subclass hook should not be called with the lock held
+  ASAssertUnlocked(__instanceLock__);
+
+  // Subclasses may override
 }
 
 - (id <ASAnimatedImageProtocol>)animatedImage

Source/ASImageNode.h

@@ -173,9 +173,8 @@ typedef UIImage * _Nullable (^asimagenode_modification_block_t)(UIImage *image);
  *
  * @discussion This method is for subclasses to override so they can know if an animated image
  * has been set on the node.
- * @warning this method is called with the node's lock held.
  */
-- (void)animatedImageSet:(id <ASAnimatedImageProtocol>)newAnimatedImage previousAnimatedImage:(id <ASAnimatedImageProtocol>)previousAnimatedImage;
+- (void)animatedImageSet:(nullable id <ASAnimatedImageProtocol>)newAnimatedImage previousAnimatedImage:(nullable id <ASAnimatedImageProtocol>)previousAnimatedImage ASDISPLAYNODE_REQUIRES_SUPER;
 
 @end