[ASDisplayNode] Add delegate to ASDisplayNode, allowing ASViewController layout

[levi]

Sketching out an initial direction of exposing layout and layout spec definition in an ASViewController. Introduced a delegate property on ASDisplayNode that allows external layout calculation, similar to the CALayer delegate design.

Note that this is currently not building, as we have a delegate property on a few display node subclasses which will need to inherit ASDisplayNodeDelegate. I wanted to put this up for feedback/discussion before I head too far down the road of bringing the other classes up to speed. Moreover, would like to include a few unit tests for this patch.

Fixes #969.

[appleguy]

@levi very cool!! We can't use the name delegate, because many apps have this property in their subclasses.

/Users/travis/build/facebook/AsyncDisplayKit/AsyncDisplayKit/ASEditableTextNode.mm:208:13: error: type of property 'delegate' ('id<ASEditableTextNodeDelegate,ASDisplayNodeDelegate>') does not match type of instance variable '_delegate' ('__weak id')
@synthesize delegate = _delegate;

This is an interesting idea, and indeed one I almost added a couple days ago when desiring to monitor the new interfaceStateDidChange:fromState: and visibilityDidChange: notifications from ASViewController's base class. I haven't reviewed fully yet and am not yet sure what the name should be, but would be curious to hear ideas! We should probably think ahead to as many plausible usages of the delegate as possible to see if there is a name that can encompass them which adds at least one more word alongside delegate in the property name.

[levi]

interfaceDelegate, layoutDelegate, layoutContext, displayHandler, to name a few suggestions

[appleguy]

@levi one concern - if this delegate is public, which it probably should be, then ASViewController can't necessarily take it over. If it did, it would fail if a user were to set the delegate of the node to their object later...

[levi]

Can't the same argument be made about CALayer#delegate?

On Dec 24, 2015, at 17:22, appleguy notifications@github.com wrote:

@levi one concern - if this delegate is public, which it probably should be, then ASViewController can't necessarily take it over. If it did, it would fail if a user were to set the delegate of the node to their object later...

—
Reply to this email directly or view it on GitHub.

[appleguy]

@levi Definitely interested to look to Apple APIs for comparison / precedent. CALayer's delegate is set to UIView when attached to a view, but this is prominently documented (in UIView) - and more importantly, the CALayer delegate has essentially no functionality that can't be handled in UIView itself (https://developer.apple.com/library/ios/documentation/QuartzCore/Reference/CALayerDelegate_protocol/index.html#//apple_ref/doc/uid/TP40012871) Also, UIView and CALayer are very similar objects conceptually, so subclassing one and doing the operations there is not as strange or unexpected as requiring the implementation be in a different type of object (ASViewController vs ASDisplayNode).

This consideration simple underscores the expected value of this delegate - not only the initial set of features, but in the future it could become quite an essential construct! The solution is probably just an ASDelegateProxy owned by ASViewController, in addition to a name besides "delegate".

[levi]

Back from vacation. Totally forgot about this PR. Will look at it over the weekend to see how to bring it up to speed.

[levi]

OK, I just pushed some updates that bring this patch up to speed with the feedback and also provides unit test coverage for the new functionally. I'm having some issues running the entire test suite when it comes to the snapshot tests. They seem to be looking for an image asset to compare to, but unable to find one. I haven't looked too closely into the issue, but hoping this is something others have encountered (cc/ @appleguy, @nguyenhuy).

[nguyenhuy]

Assuming you are having problems with existing tests, definitely check for the architecture that you are running the tests on. Reference images are available for 64bit only.

Great to see a PR that includes tests! Definitely way to go haha 💃 💃 💃

[levi]

@nguyenhuy, hrm it looks like I'm running them in 64bit locally. Are you able to pull the patch down on your end to confirm?

[nguyenhuy]

Are you running on @3x devices (iPhone 6S and 6S+)? We don't have @3x snapshots yet. Should be easy to add though.

[nguyenhuy]

For the record, I pulled your patch and all tests passed on a @2x 64bit simulator.

(Gonna go out for some hours to do touristy things around SF!)

[rcancro]

I just took a quick glance at this, but wondered since ASViewController will probably be used most often as a base class for a custom VC, does it make sense to add this functionality more like the way UIViewController does? i.e., with methods like:

- (ASLayoutSpec *)layoutSpecForSubnodes;
- (ASLayout *)layoutForSubnodes;

that a subclass can implement?

[levi]

Thanks for the feedback, @rcancro. Are you specifically asking for a naming change? As far as the existing functionality stands, subclasses of ASViewController will be able to override these methods to be used in the way that you're describing.

[appleguy]

Continuing to discuss this offline, I'm going to close this for now but we can certainly find it to reopen or pull the code as a base for a further change!