[ASViewController] Add support for ASTraitCollection #1592

rcancro · 2016-04-26T00:10:29Z

Initial attempt to get display traits working with ASEnvironment.

To get proper ASDisplayTraits support, you must use an ASViewController. The ASViewController implements UITraitCollection-related methods (traitCollectionDidChange:, willTransitionToTraitCollection:withTransitionCoordinator:, viewWillTransitionToSize:withTransitionCoordinator) to update the internal ASDisplayTraits and propagate them to subnodes.

ASTableNode and ASCollectionNode don't actually have their cells as subnodes, so a little bit of trickery is involved (on setEnvironment: the table/collection node gets its data controller's completedNodes and propagates the new traits. see ASDisplayTraitsCollectionTableSetEnvironmentState). The data controller also has the ability to get the current display traits when creating new cells through a new protocol ASDataControllerEnvironmentDelegate.

ASViewController also supports the ability to return a custom set of display traits regardless its UITraitCollection. For example, if you have a modal dialog that should always have a compact size classes, you can set the override block before displaying the VC (overrideDisplayTraitsWithTraitCollection and overrideDisplayTraitsWithWindowSize).

A new example, called Display Traits, has been added. It shows how display traits can be used in a ASViewController with a normal ASDisplayNode as its root, as well as in ASViewControllers hosting a table node or collection node. There is also an example of overriding the default display traits of a VC.

Please provide any suggestions that you have!

maicki · 2016-05-02T02:39:21Z

AsyncDisplayKit/Details/ASEnvironment.h

+#define ASDisplayTraitsCollectionTableSetEnvironmentState \
+- (void)setEnvironmentState:(ASEnvironmentState)environmentState\
+{\


Do we need locking for setting and propagation?

maicki · 2016-05-02T03:27:35Z

@rcancro Overall seems good to me. Just one thing I'm thinking about is that the naming of the ASDisplayTraits is a bit different as all the other states that we start with "ASEnvironment..." Just thinking if we should internally use ASEnvironmentDisplayTraitsState and for the public API we expose it as displayTraits as we already do?

cc @appleguy

rcancro · 2016-05-04T05:09:49Z

@maicki Thanks for the review! I've addressed most of your comments except for the request to make a NSObject-based class that wraps the struct. I'll look at that tomorrow

levi · 2016-05-04T19:11:28Z

AsyncDisplayKit/ASViewController.mm

+  [self progagateNewDisplayTraits:displayTraits];
+}
+
+- (void)viewWillTransitionToSize:(CGSize)size withTransitionCoordinator:(id<UIViewControllerTransitionCoordinator>)coordinator


Why do we need to implement all three of these notifications? willTransitionToTraitCollection: should suffice, as it is called in transitionToSize: call and is invoked before traitCollectionDidChange: is called. I'd love to learn more about the reasons behind this implementation.

I noticed that on start up of the app, traitCollectionDidChange: is called, but not willTransitionToTraitCollection:.

I also feel like i've seen the case where a window changes size (but not trait collection) and viewWillTransitionToSize: is called but not willTransitionToTraitCollection:.

progagateNewDisplayTraits: does check to make sure that the traits are different before propagating, so even in the case where all three of these are called we only propagate once.

appleguy · 2016-05-05T03:07:08Z

@rcancro I am re-running, but I think this may be a real failure. Probably worth opening the CarthageBuildTest to run it locally and see what the detailed failure is, and confirm the build server's result that before this change, it builds. It is possible that there is a new problem in master that hasn't been noticed yet though, as we did just adopt a new PINRemoteImage which apparently has some issue with Carthage. cc @garrettmoon

▸ Building Sample/CarthageExample [Debug]
▸ Check Dependencies
▸ Compiling main.m
▸ Compiling AppDelegate.m
❌ /Users/travis/build/facebook/AsyncDisplayKit/examples/CarthageBuildTest/CarthageExample/AppDelegate.m:9:9: could not build module 'AsyncDisplayKit'
@import AsyncDisplayKit;

appleguy · 2016-05-05T03:09:23Z

@rcancro @levi if ASDisplayTraits is directly analogous to UITraitCollection, can we call it ASTraitCollection instead? I assume it will be a struct directly inside ASEnvironment struct.

Sincere apologies that I have not been able to review this yet. I really badly want to, but haven't been able to reach the minimum sleep quota recently even without touching ASDK diffs in the last week :(. Will plan to review on the plane on Friday or on return flight.

appleguy · 2016-05-05T22:42:17Z

examples/BackgroundPropertySetting/Sample/ViewController.swift

@@ -24,8 +24,8 @@ final class ViewController: ASViewController, ASCollectionDelegate, ASCollection
 		layout.minimumInteritemSpacing = padding
 		layout.minimumLineSpacing = padding
 		super.init(node: ASCollectionNode(collectionViewLayout: layout))
-		navigationItem.rightBarButtonItem = UIBarButtonItem(title: "Color", style: .Plain, target: self, action: "didTapColorsButton")
-		navigationItem.leftBarButtonItem = UIBarButtonItem(title: "Layout", style: .Plain, target: self, action: "didTapLayoutButton")
+        navigationItem.rightBarButtonItem = UIBarButtonItem(title: "Color", style: .Plain, target: self, action: #selector(didTapColorsButton))


Needs more indentation?

levi · 2016-05-10T00:30:00Z

@rcancro looks like this patch got messed up with a rebase or merge.

Initial attempt to get display traits working with ASEnvironment. To get proper ASDisplayTraits support, you must use an ASViewController. The ASViewController implements UITraitCollection-related methods (`traitCollectionDidChange:`, `willTransitionToTraitCollection:withTransitionCoordinator:`, viewWillTransitionToSize:withTransitionCoordinator`) to update the internal ASDisplayTraits and propagate them to subnodes. ASTableNode and ASCollectionNode don't actually have their cells as subnodes, so a little bit of trickery is involved (on `setEnvironment:` the table/collection node gets its data controllers completedNodes and propagates the new traits. see `ASDisplayTraitsCollectionTableSetEnvironmentState`). The data controller also passes the current display traits when creating new cells. ASViewController also supports the ability to return a custom set of display traits. So if you have a modal dialog that should always be told it is in a compact size class, you can set the override block before displaying the VC. A new example, called Display Traits, has been added. It shows how display traits can be used in a ASViewController with a normal ASDisplayNode as its root, as well as in ASViewControllers hosting table nodes and collection nodes. There is also an example of overriding the default display traits of a VC. Please provide feedback!

levi · 2016-05-10T23:06:47Z

AsyncDisplayKit/ASViewController.mm

+
+  if (ASEnvironmentTraitCollectionIsEqualToASEnvironmentTraitCollection(traitCollection, oldTraitCollection) == NO) {
+    environmentState.traitCollection = traitCollection;
+    [self.node setEnvironmentState:environmentState];


Are you able to use dot syntax here?

you mean self.node.environmentState = environmentState?

levi · 2016-05-10T23:12:12Z

AsyncDisplayKit/ASViewController.mm

+    traitCollection.isMutable = NO;
+    return [traitCollection environmentTraitCollection];
+  }
+  return self.node.environmentState.traitCollection;


Would be nice to reduce the amount of references here. Perhaps add a shortcut to asdisplaynode to get the underlying environmentState trait collection?

@maicki had the idea to expose the trait collection as ASTraitCollection to the user, but store it as a struct behind the scenes. When providing a custom trait collection the user overrides one of two blocks, both of which return ASTraitCollection* not the struct.

I guess my point is that we are trying to make it difficult for the user to get to the struct trait collection since most obj-c developers are more familiar using classes. By adding a getter to the struct traits we kind of break that.

We could introduce it as a private method for these use cases.

levi · 2016-05-11T20:51:46Z

AsyncDisplayKit/ASViewController.h

+ *  ASVC keeps a strong reference to the context to make sure that it stays alive. If you change this value
+ *  it will propagate the change to the subnodes.
+ */
+@property (nonatomic, strong) id _Nullable traitColectionContext;


d'oh! good catch

levi · 2016-05-11T20:53:49Z

Ricky, this is straight up poetry.

rcancro · 2016-05-11T21:12:05Z

Rotation, oh my
iPad, 6 plus and pad pro
trait collections yay!

appleguy · 2016-05-12T04:11:41Z

AsyncDisplayKit/Details/ASEnvironment.h

+- (void)setEnvironmentState:(ASEnvironmentState)environmentState\
+{\
+  ASDN::MutexLocker l(lock);\


Pretty damn fancy, sir @rcancro

appleguy · 2016-05-12T04:19:50Z

Phenomenal work, @rcancro ! Once again, you've fundamentally extended the capabilities of the framework. I sincerely appreciate your attention to detail both in architecting and executing this. There's great care to efficiency / performance, but simultaneously a clear priority placed on a friendly object-based API. Love it.

The only followup that seems important for the next 1-2 days is checking on the locking sites I commented at, but otherwise I think this is a mic drop. :)

Summary: facebookarchive#1592 Reviewers: levi, scottg, garrett Reviewed By: garrett Differential Revision: https://phabricator.pinadmin.com/D90514

ghost added the CLA Signed label Apr 26, 2016

maicki reviewed May 2, 2016
View reviewed changes

levi reviewed May 4, 2016
View reviewed changes

appleguy added feature — core API improvement ASViewController labels May 5, 2016

appleguy added this to the 1.9.8 milestone May 5, 2016

appleguy reviewed May 5, 2016
View reviewed changes

rcancro added 11 commits May 10, 2016 14:44

addressed some comments

b4a6f87

add locking to collection state propagation

35820e5

added nsobject based ASDisplayTraits class

f828d07

include displayContext in equality check

a04cbb6

fix build errors

223a5f0

fix build?

51f79f3

changed DisplayTraits to TraitCollection

9d622c7

change ASCOllectionNode back .mm file

0806f52

fix indentation

92ed02f

add ASTraitCollection to umbrella header

2995271

rcancro force-pushed the displayTraits branch from 0a4df41 to 2995271 Compare May 10, 2016 21:47

levi reviewed May 10, 2016
View reviewed changes

Levi's comments

4b54144

levi reviewed May 11, 2016
View reviewed changes

rcancro changed the title ~~Initial attempt at implementing Display Traits~~ [ASViewController] Add support for ASTraitCollection May 11, 2016

typo and comment

cfa9dcd

appleguy reviewed May 12, 2016
View reviewed changes

appleguy merged commit efc81f5 into facebookarchive:master May 12, 2016

nguyenhuy mentioned this pull request Jan 22, 2018

[ASPrimitiveTraitCollection] Always treat preferredContentSize as a potential nil #trivial TextureGroup/Texture#757

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASViewController] Add support for ASTraitCollection #1592

[ASViewController] Add support for ASTraitCollection #1592

rcancro commented Apr 26, 2016

maicki May 2, 2016

maicki commented May 2, 2016

rcancro commented May 4, 2016

levi May 4, 2016

rcancro May 4, 2016

appleguy commented May 5, 2016

appleguy commented May 5, 2016

appleguy May 5, 2016

levi commented May 10, 2016

levi May 10, 2016

rcancro May 10, 2016

levi May 10, 2016

levi May 10, 2016

rcancro May 10, 2016

levi May 10, 2016

levi May 11, 2016

rcancro May 11, 2016

levi commented May 11, 2016

rcancro commented May 11, 2016

appleguy May 12, 2016

appleguy commented May 12, 2016

[ASViewController] Add support for ASTraitCollection #1592

[ASViewController] Add support for ASTraitCollection #1592

Conversation

rcancro commented Apr 26, 2016

Choose a reason for hiding this comment

maicki commented May 2, 2016

rcancro commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appleguy commented May 5, 2016

appleguy commented May 5, 2016

Choose a reason for hiding this comment

levi commented May 10, 2016

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

levi commented May 11, 2016

rcancro commented May 11, 2016

Choose a reason for hiding this comment

appleguy commented May 12, 2016