Revert "New runloop queue to coalesce Interface state update calls. (#…

…788)" This reverts commit 2618c50.
TextureGroup · Mar 28, 2018 · 80efb63 · 80efb63
1 parent cf1c3f6
commit 80efb63
Show file tree

Hide file tree

Showing 13 changed files with 67 additions and 446 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,7 +6,6 @@
 - [ASDisplayNode] Add unit tests for layout z-order changes (with an open issue to fix).
 - [ASWrapperCellNode] Introduce a new class allowing more control of UIKit passthrough cells.
 - [ASDisplayNode] Consolidate main thread initialization and allow apps to invoke it manually instead of +load.
-- [ASRunloopQueue] Introduce new runloop queue(ASCATransactionQueue) to coalesce Interface state update calls for view controller transitions.
 - [ASRangeController] Fix stability of "minimum" rangeMode if the app has more than one layout before scrolling.
 - **Important** ASDisplayNode's cornerRadius is a new thread-safe bridged property that should be preferred over CALayer's. Use the latter at your own risk! [Huy Nguyen](https://github.com/nguyenhuy) [#749](https://github.com/TextureGroup/Texture/pull/749).
 - [ASCellNode] Adds mapping for UITableViewCell focusStyle [Alex Hill](https://github.com/alexhillc) [#727](https://github.com/TextureGroup/Texture/pull/727)

diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm
@@ -65,7 +65,7 @@
 // We have to forward declare the protocol as this place otherwise it will not compile compiling with an Base SDK < iOS 10
 @protocol CALayerDelegate;
 
-@interface ASDisplayNode () <UIGestureRecognizerDelegate, CALayerDelegate, _ASDisplayLayerDelegate, ASCATransactionQueueObserving>
+@interface ASDisplayNode () <UIGestureRecognizerDelegate, CALayerDelegate, _ASDisplayLayerDelegate>
 
 /**
  * See ASDisplayNodeInternal.h for ivars
@@ -2955,7 +2955,7 @@ - (void)setHierarchyState:(ASHierarchyState)newState
   // Entered or exited range managed state.
   if ((newState & ASHierarchyStateRangeManaged) != (oldState & ASHierarchyStateRangeManaged)) {
     if (newState & ASHierarchyStateRangeManaged) {
-      [self enterInterfaceState:self.supernode.pendingInterfaceState];
+      [self enterInterfaceState:self.supernode.interfaceState];
     } else {
       // The case of exiting a range-managed state should be fairly rare.  Adding or removing the node
       // to a view hierarchy will cause its interfaceState to be either fully set or unset (all fields),
@@ -2998,34 +2998,30 @@ - (void)didExitHierarchy
   ASDisplayNodeAssert(_flags.isExitingHierarchy, @"You should never call -didExitHierarchy directly. Appearance is automatically managed by ASDisplayNode");
   ASDisplayNodeAssert(!_flags.isEnteringHierarchy, @"ASDisplayNode inconsistency. __enterHierarchy and __exitHierarchy are mutually exclusive");
   ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
-
-  // This case is important when tearing down hierarchies.  We must deliver a visibileStateDidChange:NO callback, as part our API guarantee that this method can be used for
-  // things like data analytics about user content viewing.  We cannot call the method in the dealloc as any incidental retain operations in client code would fail.
-  // Additionally, it may be that a Standard UIView which is containing us is moving between hierarchies, and we should not send the call if we will be re-added in the
-  // same runloop.  Strategy: strong reference (might be the last!), wait one runloop, and confirm we are still outside the hierarchy (both layer-backed and view-backed).
-  // TODO: This approach could be optimized by only performing the dispatch for root elements + recursively apply the interface state change. This would require a closer
-  // integration with _ASDisplayLayer to ensure that the superlayer pointer has been cleared by this stage (to check if we are root or not), or a different delegate call.
-  if (ASInterfaceStateIncludesVisible(_pendingInterfaceState)) {
-    void(^exitVisibleInterfaceState)(void) = ^{
-      // This block intentionally retains self.
-      __instanceLock__.lock();
-      unsigned isStillInHierarchy = _flags.isInHierarchy;
-      BOOL isVisible = ASInterfaceStateIncludesVisible(_pendingInterfaceState);
-      ASInterfaceState newState = (_pendingInterfaceState & ~ASInterfaceStateVisible);
-      __instanceLock__.unlock();
-
-      if (!isStillInHierarchy && isVisible) {
-        if (![self supportsRangeManagedInterfaceState]) {
-          newState = ASInterfaceStateNone;
+
+  if (![self supportsRangeManagedInterfaceState]) {
+    self.interfaceState = ASInterfaceStateNone;
+  } else {
+    // This case is important when tearing down hierarchies.  We must deliver a visibileStateDidChange:NO callback, as part our API guarantee that this method can be used for
+    // things like data analytics about user content viewing.  We cannot call the method in the dealloc as any incidental retain operations in client code would fail.
+    // Additionally, it may be that a Standard UIView which is containing us is moving between hierarchies, and we should not send the call if we will be re-added in the
+    // same runloop.  Strategy: strong reference (might be the last!), wait one runloop, and confirm we are still outside the hierarchy (both layer-backed and view-backed).
+    // TODO: This approach could be optimized by only performing the dispatch for root elements + recursively apply the interface state change. This would require a closer
+    // integration with _ASDisplayLayer to ensure that the superlayer pointer has been cleared by this stage (to check if we are root or not), or a different delegate call.
+
+    if (ASInterfaceStateIncludesVisible(self.interfaceState)) {
+      dispatch_async(dispatch_get_main_queue(), ^{
+        // This block intentionally retains self.
+        __instanceLock__.lock();
+          unsigned isInHierarchy = _flags.isInHierarchy;
+          BOOL isVisible = ASInterfaceStateIncludesVisible(_interfaceState);
+          ASInterfaceState newState = (_interfaceState & ~ASInterfaceStateVisible);
+        __instanceLock__.unlock();
+
+        if (!isInHierarchy && isVisible) {
+          self.interfaceState = newState;
         }
-        self.interfaceState = newState;
-      }
-    };
-
-    if (!ASCATransactionQueue.sharedQueue.enabled) {
-      dispatch_async(dispatch_get_main_queue(), exitVisibleInterfaceState);
-    } else {
-      exitVisibleInterfaceState();
+      });
     }
   }
 }
@@ -3086,53 +3082,25 @@ - (ASInterfaceState)interfaceState
 }
 
 - (void)setInterfaceState:(ASInterfaceState)newState
-{
-  if (!ASCATransactionQueue.sharedQueue.enabled) {
-    [self applyPendingInterfaceState:newState];
-  } else {
-    ASDN::MutexLocker l(__instanceLock__);
-    if (_pendingInterfaceState != newState) {
-      _pendingInterfaceState = newState;
-      [[ASCATransactionQueue sharedQueue] enqueue:self];
-    }
-  }
-}
-
-- (ASInterfaceState)pendingInterfaceState
-{
-  ASDN::MutexLocker l(__instanceLock__);
-  return _pendingInterfaceState;
-}
-
-- (void)applyPendingInterfaceState:(ASInterfaceState)newPendingState
 {
   //This method is currently called on the main thread. The assert has been added here because all of the
   //did(Enter|Exit)(Display|Visible|Preload)State methods currently guarantee calling on main.
   ASDisplayNodeAssertMainThread();
-
+  // It should never be possible for a node to be visible but not be allowed / expected to display.
+  ASDisplayNodeAssertFalse(ASInterfaceStateIncludesVisible(newState) && !ASInterfaceStateIncludesDisplay(newState));
   // This method manages __instanceLock__ itself, to ensure the lock is not held while didEnter/Exit(.*)State methods are called, thus avoid potential deadlocks
   ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
 
   ASInterfaceState oldState = ASInterfaceStateNone;
-  ASInterfaceState newState = ASInterfaceStateNone;
   {
     ASDN::MutexLocker l(__instanceLock__);
-    // newPendingState will not be used when ASCATransactionQueue is enabled
-    // and use _pendingInterfaceState instead for interfaceState update.
-    if (!ASCATransactionQueue.sharedQueue.enabled) {
-      _pendingInterfaceState = newPendingState;
-    }
-    oldState = _interfaceState;
-    newState = _pendingInterfaceState;
-    if (newState == oldState) {
+    if (_interfaceState == newState) {
       return;
     }
+    oldState = _interfaceState;
     _interfaceState = newState;
   }
 
-  // It should never be possible for a node to be visible but not be allowed / expected to display.
-  ASDisplayNodeAssertFalse(ASInterfaceStateIncludesVisible(newState) && !ASInterfaceStateIncludesDisplay(newState));
-
   // TODO: Trigger asynchronous measurement if it is not already cached or being calculated.
   // if ((newState & ASInterfaceStateMeasureLayout) != (oldState & ASInterfaceStateMeasureLayout)) {
   // }
@@ -3229,12 +3197,6 @@ - (void)applyPendingInterfaceState:(ASInterfaceState)newPendingState
   [self interfaceStateDidChange:newState fromState:oldState];
 }
 
-- (void)prepareForCATransactionCommit
-{
-  // Apply _pendingInterfaceState actual _interfaceState, note that ASInterfaceStateNone is not used.
-  [self applyPendingInterfaceState:ASInterfaceStateNone];
-}
-
 - (void)interfaceStateDidChange:(ASInterfaceState)newState fromState:(ASInterfaceState)oldState
 {
   // Subclass hook

diff --git a/Source/ASRunLoopQueue.h b/Source/ASRunLoopQueue.h
@@ -20,15 +20,8 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
-@protocol ASCATransactionQueueObserving <NSObject>
-- (void)prepareForCATransactionCommit;
-@end
-
-@interface ASAbstractRunLoopQueue : NSObject
-@end
-
 AS_SUBCLASSING_RESTRICTED
-@interface ASRunLoopQueue<ObjectType> : ASAbstractRunLoopQueue <NSLocking>
+@interface ASRunLoopQueue<ObjectType> : NSObject <NSLocking>
 
 /**
  * Create a new queue with the given run loop and handler.
@@ -48,35 +41,13 @@ AS_SUBCLASSING_RESTRICTED
 
 - (void)enqueue:(ObjectType)object;
 
-@property (atomic, readonly) BOOL isEmpty;
+@property (nonatomic, readonly) BOOL isEmpty;
 
 @property (nonatomic, assign) NSUInteger batchSize;           // Default == 1.
 @property (nonatomic, assign) BOOL ensureExclusiveMembership; // Default == YES.  Set-like behavior.
 
 @end
 
-AS_SUBCLASSING_RESTRICTED
-@interface ASCATransactionQueue : ASAbstractRunLoopQueue
-
-@property (atomic, readonly) BOOL isEmpty;
-
-@property (atomic, readonly, getter=isEnabled) BOOL enabled;
-
-/**
- * The queue to run on main run loop before CATransaction commit.
- *
- * @discussion this queue will run after ASRunLoopQueue and before CATransaction commit
- * to get last chance of updating/coalesce info like interface state.
- * Each node will only be called once per transaction commit to reflect interface change.
- */
-@property (class, atomic, readonly) ASCATransactionQueue *sharedQueue;
-+ (ASCATransactionQueue *)sharedQueue NS_RETURNS_RETAINED;
-
-- (void)enqueue:(id<ASCATransactionQueueObserving>)object;
-
-@end
-
-
 AS_SUBCLASSING_RESTRICTED
 @interface ASDeallocQueue : NSObject