Create a Pluggable "Tips" System to Help in Development #3270

Adlai-Holler · 2017-04-13T17:37:45Z

To activate: #define AS_ENABLE_TIPS=1
ASDK will create a new window on top of the application window.
At the end of each run loop, tips controller goes through all nodes that appeared during the turn.
Runs all ASTipProvider subclasses against them. Currently we have ASLayerBackingTipProvider.
If the provider generates a tip for that node, a node is added to the overlay window.
If the user taps on a tip, we call a custom tipDisplayBlock with a message. Default is to log a message using NSLog.
You can disable tips on a per-class level. MYDisplayNode.enableTips = NO.
Only requires one hook from ASDisplayNode right now – nodeDidAppear:.

Example Screenshot:

Tapping on one of the big images causes this message to get logged (or shown, if the app sets a custom tipDisplayBlock):

Enable layer backing to improve performance. Node ancestry: (
    "<PIRoundedImageNode: 0x7ff89a031c00>",
    "<PIFlexibleArticleNode: 0x7ff8984a4430>",
    "<PIWrapperCellNode: 0x7ff8985ace40>"
)

Caveats:

As before, you can't have view-backed nodes inside layer-backed nodes, so you need to set layerBacked=YES on the leaf nodes first and work your way up.
When tips are enabled, touches on green areas are not passed to the application window. This is annoying but not too annoying.

Results:

No false positives in Pinterest yet, and all the green is gone in one diff!

Adlai-Holler · 2017-04-13T17:44:28Z

Source/ASDisplayNode.mm

@@ -245,11 +246,6 @@ + (void)load
  ASScreenScale();
 }

-+ (BOOL)layerBackedNodesEnabled
-{
-  return YES;


This method was never made public or used at all. It's been replaced by the instance method -supportsLayerBacking now.

Adlai-Holler · 2017-04-13T17:45:07Z

Source/ASDisplayNode.mm

-    }
-    view = [[_viewClass alloc] init];
+    Class c = _viewClass ?: [_ASDisplayView class];
+    view = [[c alloc] init];


Previously _viewClass would be nil until the view loaded, then it would go to _ASDisplayView. Now _viewClass stays nil unless they used a custom view class.

Adlai-Holler · 2017-04-13T17:55:15Z

Source/ASDisplayNode.mm

- (void)setSynchronous:(BOOL)flag
-{
-  ASDN::MutexLocker l(__instanceLock__);
-  _flags.synchronous = flag;


This was unused so I tossed it in.

levi · 2017-04-13T18:05:10Z

WOW THIS IS AMAZING

Adlai-Holler · 2017-04-13T18:17:03Z

Tests/ASDisplayNodeTests.mm

-  ASTestDisplayNode *node = [[ASTestResponderNode alloc] init];
-  node.layerBacked = YES;
-  XCTAssertTrue([node canBecomeFirstResponder]);
-  XCTAssertFalse([node becomeFirstResponder]);


This test now is invalid because it sets layerBacked=YES on a node that has a custom view class. Can't be done!

Adlai-Holler · 2017-04-13T18:18:35Z

Source/ASDisplayNode.mm

@@ -574,9 +572,6 @@ - (UIView *)_locked_viewToLoad
    _viewBlock = nil;
    _viewClass = [view class];
  } else {
-    if (!_viewClass) {
-      _viewClass = [self.class viewClass];
-    }


These are now established during init so that the values are invariant across the node's lifetime.

garrettmoon · 2017-04-13T18:21:48Z

Source/ASDisplayNode.mm

-
-  if (isLayerBacked != _flags.layerBacked && !_view && !_layer) {
-    _flags.layerBacked = isLayerBacked;
+  if ([self _locked_isNodeLoaded]) {


Nice cleanup :)

garrettmoon · 2017-04-13T18:24:11Z

Source/ASTextNode.mm

+  }
+
+  // If the text contains any links, return NO.
+  NSAttributedString *attributedText = self.attributedText;


Is their any performance concerns that this is called when setting layer backed to YES? Is it called in other paths?

Good call, will modify to only call it if assertions are on.

garrettmoon · 2017-04-13T18:25:44Z

Source/Base/ASDisplayNode+Ancestry.m

+
+@implementation ASNodeAncestryEnumerator {
+  // Enumerators are one-shot optimized – avoid retains/releases/weak
+  __unsafe_unretained ASDisplayNode * _Nullable _node;


This is a helluva optimization, worth it?

Hmmm under normal circumstances I would say yes, but since we allow you to modify node hierarchies from any thread, I guess we have to use weak here, since the "collection" we are enumerating can't be held still.

garrettmoon · 2017-04-13T18:29:02Z

Source/Debug/AsyncDisplayKit+Tips.h

+ * If nil, the default, the message is just logged to the console with the
+ * ancestry of the node.
+ */
+@property (class, nonatomic, copy, null_resettable) ASTipDisplayBlock tipDisplayBlock; 


You know all the fancy annotations.

garrettmoon · 2017-04-13T18:31:38Z

Source/Debug/AsyncDisplayKit+Tips.m

+{
+  NSNumber *result = objc_getAssociatedObject(self, &ASDisplayNodeEnableTipsKey);
+  if (result == nil) {
+    return YES;


I may be reading this wrong, but shouldn't the default be NO?

No the default is YES (RTFM! 😛). The global enable is whether they define AS_ENABLE_TIPS=1. This property is only for disabling tips on a per-class basis, say if we make a false-positive that they find annoying when tips are on.

garrettmoon · 2017-04-13T18:32:59Z

Source/Private/ASDisplayNodeTipState.h

+- (instancetype)init NS_UNAVAILABLE;
+
+/// Unsafe because once the node is deallocated, we will not be able to access the tip state.
+@property (nonatomic, unsafe_unretained, readonly) ASDisplayNode *node;


Not sure I understand why not to use weak here?

So that the node property is truly immutable. If we keep tip states around after nodes are deallocated, then we should crash so that we stop doing that. This acts like an assertion, similar to Swift's unowned storage class.

garrettmoon · 2017-04-13T18:40:16Z

Source/Private/ASTipNode.m

+
+// Prevent compiler from complaining about undeclared selector.
+@interface UIResponder (TipNodeActions)
+- (void)didTapTipNode:(id)sender;


Do you mind updating the comment saying this is defined on the tip window?

I made a protocol to contain this method.

garrettmoon · 2017-04-13T18:40:41Z

Source/Private/ASTipNode.m

+  if (self = [super init]) {
+    self.backgroundColor = [UIColor colorWithRed:0 green:0.7 blue:0.2 alpha:0.3];
+    _tip = tip;
+    [self addTarget:nil action:@selector(didTapTipNode:) forControlEvents:ASControlNodeEventTouchUpInside];


Just curious, is this standard behavior to walk the chain if the target is nil? I see that's what it does in ASControlNode.

Yep! This is the best-kept secret of UIKit – you can set target nil and use the responder chain, instead of tightly binding all your controls to their controllers.

garrettmoon · 2017-04-13T18:44:45Z

Source/Private/ASTipProvider.m

+  static NSArray<ASTipProvider *> *providers;
+  static dispatch_once_t onceToken;
+  dispatch_once(&onceToken, ^{
+    providers = @[ [ASLayerBackingTipProvider new] ];


So the idea is that new tip providers would be added here? Any thoughts on if we wanted to pull these out of ASDK into a separate project?

We could do that in the future, and we have a ton of options. We could scan the runtime for subclasses of ASTipProvider, or we could require subclasses to register themselves, etc.

Adlai-Holler · 2017-04-17T19:07:43Z

PR moved!

facebook-github-bot added the CLA Signed label Apr 13, 2017

Adlai-Holler added the Debuggability label Apr 13, 2017

Adlai-Holler requested a review from nguyenhuy April 13, 2017 17:43

Adlai-Holler commented Apr 13, 2017

View reviewed changes

garrettmoon reviewed Apr 13, 2017

View reviewed changes

Build a Tips system

84eca7e

Adlai-Holler force-pushed the AHTips branch from 5f5d291 to 84eca7e Compare April 13, 2017 23:30

Adlai-Holler mentioned this pull request Apr 17, 2017

Create a Pluggable "Tips" System to Help in Development TextureGroup/Texture#19

Merged

Adlai-Holler closed this Apr 17, 2017

Adlai-Holler deleted the AHTips branch April 17, 2017 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a Pluggable "Tips" System to Help in Development #3270

Create a Pluggable "Tips" System to Help in Development #3270

Adlai-Holler commented Apr 13, 2017 •

edited

Loading

Adlai-Holler Apr 13, 2017

Adlai-Holler Apr 13, 2017

Adlai-Holler Apr 13, 2017

levi commented Apr 13, 2017

Adlai-Holler Apr 13, 2017

Adlai-Holler Apr 13, 2017

garrettmoon Apr 13, 2017

garrettmoon Apr 13, 2017

Adlai-Holler Apr 13, 2017

garrettmoon Apr 13, 2017

Adlai-Holler Apr 13, 2017

garrettmoon Apr 13, 2017

garrettmoon Apr 13, 2017

Adlai-Holler Apr 13, 2017 •

edited

Loading

garrettmoon Apr 13, 2017

Adlai-Holler Apr 13, 2017

garrettmoon Apr 13, 2017

Adlai-Holler Apr 13, 2017

garrettmoon Apr 13, 2017

Adlai-Holler Apr 13, 2017

garrettmoon Apr 13, 2017

Adlai-Holler Apr 13, 2017

Adlai-Holler commented Apr 17, 2017

Create a Pluggable "Tips" System to Help in Development #3270

Create a Pluggable "Tips" System to Help in Development #3270

Conversation

Adlai-Holler commented Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levi commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adlai-Holler Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adlai-Holler commented Apr 17, 2017

Adlai-Holler commented Apr 13, 2017 •

edited

Loading

Adlai-Holler Apr 13, 2017 •

edited

Loading