Skip to content
This repository was archived by the owner on Feb 2, 2023. It is now read-only.

[Layout] Improve Layout System Abstraction#2941

Merged
maicki merged 2 commits intofacebookarchive:masterfrom
maicki:MSImproveLayoutAbstractionTakeTwo
Feb 3, 2017
Merged

[Layout] Improve Layout System Abstraction#2941
maicki merged 2 commits intofacebookarchive:masterfrom
maicki:MSImproveLayoutAbstractionTakeTwo

Conversation

@maicki
Copy link
Copy Markdown
Contributor

@maicki maicki commented Jan 28, 2017 •
edited
Loading

Currently our layout system is pretty coupled to the overall ASDK system. This PR is the first step to loosen up this coupling.

For now it:

  1. Cleans up the protocol situation around ASLayoutElement.
  2. Removes the dependency on ASEnvironment and introduces trait collection support that would allow us to more easier compile out in certain cases.
  3. Move ASLayoutElementStyleExtensions out from ASEnvironment

TODO:

  • Move ASLayoutElementTraitCollection related code to ASTraitCollection
  • Locking situation for layout element extensions in _extensions
  • Reenable logging in ASLayoutElement
  • Re-add removed ASEnvironmentTraitCollection as deprecated
  • Remove all ASEnvironment related files from project
  • Rename ASLayoutElementTraitCollection to ASPrimitiveTraitCollection

@maicki maicki mentioned this pull request Jan 28, 2017
Closed
4 tasks
@maicki maicki mentioned this pull request Jan 28, 2017
Closed
1 task
@appleguy
Copy link
Copy Markdown
Contributor

appleguy commented Jan 28, 2017

test this please

@appleguy
Copy link
Copy Markdown
Contributor

appleguy commented Jan 28, 2017 •
edited
Loading

@maicki do you really think we should remove all of ASEnvironment? I remember when you wrote it (and we talked about its design together), and I still think it is one of the most interesting and unique systems in ASDK.

Although adoption was not completed, I feel it would be very valuable (and is great, flexible code) if we can get ASInterfaceState and ASHierarchyState into the ASEnvironment struct, as was originally intended / in-progress when we had to jump to other projects. This wouldn't need to be done right now, and I do still agree that removing layout propagation from ASEnvironment makes sense.

One reason I say this is because the integration idea I have for Yoga will almost certainly be able to use ASEnvironment as a central mechanism for keeping the Yoga tree up to date with the ASDK children tree.

@maicki
Copy link
Copy Markdown
Contributor Author

maicki commented Jan 30, 2017

@appleguy Yes for now we should get rid of ASEnvironment it is trivial to write this kind of tree traversal without having to hook into another system like ASEnvironment. Look how I replaced the trait collection system that used to use ASEnvironment.

@maicki
Copy link
Copy Markdown
Contributor Author

maicki commented Jan 30, 2017

Ready for first review. CC @nguyenhuy

@Adlai-Holler
Copy link
Copy Markdown
Contributor

Adlai-Holler commented Jan 31, 2017 •
edited
Loading

Nice! Thoughts:

  • Since we don't support macOS and there aren't plans to do so, I think we shouldn't land any OSX-oriented code.
  • Some of the names are verbose. How about:
    • ASLayoutElementTraitCollection -> ASPrimitiveTraitCollection. This conveys that it is a struct version of the class ASTraitCollection.
    • ASLayoutElementTraitEnvironment -> ASTraitEnvironment. This mirrors the UITraitEnvironment protocol and the word Trait is unambiguous within ASDK.
    • progagateNewLayoutElementTraitCollection -> propagateNewTraitCollection. Since the format used for propagation doesn't matter, we can omit it from the name.

@maicki

@maicki
Copy link
Copy Markdown
Contributor Author

maicki commented Feb 1, 2017

@Adlai-Holler

  • I think it would be valuable to let the macOS related code in there as the layout system can be used on macOS without having the coupling with ASDK
  • Agree with naming of trait collections is not optimal. Will change the naming.

Adlai-Holler
Adlai-Holler approved these changes Feb 2, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@Adlai-Holler Adlai-Holler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple notes on naming and then we can land.

Comment thread AsyncDisplayKit/Details/ASTraitCollection.h Outdated
/**
* Abstraction on top of UITraitCollection for propagation within AsyncDisplayKit-Layout
*/
@protocol ASPrimitiveTraitEnvironment <NSObject>
Copy link
Copy Markdown
Contributor

@Adlai-Holler Adlai-Holler Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's name this protocol ASTraitEnvironment.

Comment thread AsyncDisplayKit/ASViewController.mm Outdated
ASEnvironmentTraitCollection traitCollection = [self environmentTraitCollectionForUITraitCollection:self.traitCollection];
[self progagateNewEnvironmentTraitCollection:traitCollection];
ASPrimitiveTraitCollection traitCollection = [self primitiveTraitCollectionForUITraitCollection:self.traitCollection];
[self progagateNewPrimitiveTraitCollection:traitCollection];
Copy link
Copy Markdown
Contributor

@Adlai-Holler Adlai-Holler Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: let's call this method -propagateNewTraitCollection:.

Comment thread AsyncDisplayKit/ASDisplayNode.mm Outdated

// Now that we have a supernode, propagate its traits to self.
ASEnvironmentStatePropagateDown(self, [newSupernode environmentTraitCollection]);
ASPrimitiveTraitCollectionPropagateDown(self, newSupernode.primitiveTraitCollection);
Copy link
Copy Markdown
Contributor

@Adlai-Holler Adlai-Holler Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this ASTraitCollectionPropagateDown. In cases where there won't be ambiguity, we should omit the word Primitive because it's non-essential.

maicki added 2 commits February 3, 2017 10:50
@maicki
Improve Layout Abstraction
820dab8
@maicki
Address naming comments
efdc30a 
- Rename ASPrimitiveTraitEnvironment to ASTraitEnvironment
- Rename ASPrimitiveTraitCollectionPropagateDown to ASTraitCollectionPropagateDown
- Rename progagateNewPrimitiveTraitCollection: to propagateNewTraitCollection:
@maicki
Copy link
Copy Markdown
Contributor Author

maicki commented Feb 3, 2017

@Adlai-Holler Addressed your comments ready for next review. Thanks!

Adlai-Holler
Adlai-Holler approved these changes Feb 3, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@Adlai-Holler Adlai-Holler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boom!

@maicki maicki merged commit 12e4e5b into facebookarchive:master Feb 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@appleguy appleguy Awaiting requested review from appleguy

2 more reviewers

@nguyenhuy nguyenhuy nguyenhuy approved these changes

@Adlai-Holler Adlai-Holler Adlai-Holler approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maicki @appleguy @Adlai-Holler @nguyenhuy @facebook-github-bot