Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Yoga integration improvements #1187

Merged
merged 7 commits into from
Oct 24, 2018
Merged

Yoga integration improvements #1187

merged 7 commits into from
Oct 24, 2018

Conversation

maicki
Copy link
Contributor

@maicki maicki commented Oct 23, 2018

  • Add thread safety for Yoga layout tree manipulation
  • Support baseline alignments for ASYogaLayout
  • Refactor ASLayoutElementYogaBaselineFunc to not require yogaParent (its parent style is set into a private var on ASLayoutElementStyle before layout instead)
  • Only set the accessibility element to nil if the view is loaded
  • Add nodeWillCalculateLayout callback to ASNodeController

@maicki maicki requested a review from appleguy October 23, 2018 15:29
Copy link
Member

@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.

I'd like to get @appleguy 's take since he knows the Yoga integration the best but this is a pretty great diff!

- (ASDisplayNode *)yogaRoot {
ASDisplayNode *yogaRoot = self;
while(yogaRoot.yogaParent) {
yogaRoot = yogaRoot.yogaParent;
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid calling yogaParent twice per node?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, also a space after "while ("

node.yogaLayoutInProgress = YES;
if (node.yogaParent) {
node.style.parentAlignStyle = node.yogaParent.style.alignItems;
Copy link
Member

Choose a reason for hiding this comment

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

Avoid getting parent twice?

@@ -199,6 +199,7 @@ AS_EXTERN void ASDisplayNodePerformBlockOnEveryYogaChild(ASDisplayNode * _Nullab

@property BOOL yogaLayoutInProgress;
@property (nullable, nonatomic) ASLayout *yogaCalculatedLayout;
@property (weak, readonly) ASDisplayNode *yogaRoot;
Copy link
Member

Choose a reason for hiding this comment

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

I might declare this a method, rather than an atomic weak property. Just to be more transparent.

- (ASDisplayNode *)yogaRoot {
ASDisplayNode *yogaRoot = self;
while(yogaRoot.yogaParent) {
yogaRoot = yogaRoot.yogaParent;
Copy link
Member

Choose a reason for hiding this comment

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

Yes, also a space after "while ("

- (void)setYogaChildren:(NSArray *)yogaChildren
{
ASLockScope(self.yogaRoot);
Copy link
Member

Choose a reason for hiding this comment

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

Is this safe? Can't the yogaRoot be changing at this time? I'm pretty sure we also need to lock self, before accessing this and also hold that lock for the duration of the method for it to be re-entrant safe.

@maicki maicki force-pushed the MSYogaImprovements branch from 87f6e7e to f90a74b Compare October 24, 2018 14:51
@maicki maicki merged commit 0977903 into master Oct 24, 2018
@maicki maicki deleted the MSYogaImprovements branch October 24, 2018 22:18
mikezucc pushed a commit to mikezucc/Texture that referenced this pull request Nov 7, 2018
* Thread safety for Yoga layout

* Support baseline alignments for ASYogaLayout

* Refactor ASLayoutElementYogaBaselineFunc to not require yogaParent (its parent style is set into a private var on ASLayoutElementStyle before layout instead)

* Only set the accessibility element if the view is loaded

* Add nodeWillCalculateLayout to ASNodeController

* Update Changelog

* Address first comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants