[Yoga] Rewrite YOGA_TREE_CONTIGUOUS mode with improved behavior and cleaner integration #343

appleguy · 2017-06-09T06:55:33Z

After experimentation with the ASYogaLayoutSpec (or non-contiguous) approach to
integrating Yoga, test results and feedback from the authors of Yoga have shown
that this approach can't be made completely correct,

There are issues with some of the features required to represent Web-style
flexbox; in particular: padding, margins, and border handling have varience.

This diff is a first step towards a truly correct and elegant implementation of
Yoga integration with Texture. In addition to reducing the footprint of
the integration, which is an explicit goal of work at this stage, these changes
already support improved behavior - including mixing between ASLayoutSpecs
even as subnodes of Yoga layout-driven nodes, in addition to above them. Yoga
may be used for any set of nodes.

Because Yoga usage is limited at this time, it's safe to merge this diff and
further improvements will be refinements in this direction.

… ASLayoutSpec. After experimentation with the ASYogaLayoutSpec (or non-contiguous) approach to integrating Yoga, test results and feedback from the authors of Yoga have shown that this approach can't be made completely correct, There are issues with some of the features required to represent Web-style flexbox; in particular: padding, margins, and border handling have varience. This diff is a first step towards a truly correct and elegant implementation of Yoga integration with Texture. In addition to reducing the footprint of the integration, which is an explicit goal of work at this stage, these changes already support improved behavior - including mixing between ASLayoutSpecs even as subnodes of Yoga layout-driven nodes, in addition to above them. Yoga may be used for any set of nodes. Because Yoga usage is limited at this time, it's safe to merge this diff and further improvements will be refinements in this direction.

appleguy · 2017-06-09T07:06:31Z

Source/ASDisplayNode.mm

  }
 #endif /* YOGA */

  // Manual size calculation via calculateSizeThatFits:
-  if (((_methodOverrides & ASDisplayNodeMethodOverrideLayoutSpecThatFits) ||
-      (_layoutSpecBlock != NULL)) == NO) {
+  if (_layoutSpecBlock == NULL && (_methodOverrides & ASDisplayNodeMethodOverrideLayoutSpecThatFits) == 0) {


This condition is equivalent to the previous one, but easier to read the intention of.

appleguy · 2017-06-09T07:07:30Z

Source/Details/ASDataController.mm

@@ -391,7 +391,7 @@ - (void)_insertElementsIntoMap:(ASMutableElementMap *)map
      nodeBlock = [dataSource dataController:self supplementaryNodeBlockOfKind:kind atIndexPath:indexPath];
    }

-    ASSizeRange constrainedSize;
+    ASSizeRange constrainedSize = ASSizeRangeUnconstrained;


This fixes an Xcode 9 static analyzer warning. I think it's the right default - if it is ever encountered - but please take a moment to consider it.

@nguyenhuy would know better, but I believe this is fine because if we don't fetch size ranges at this stage, it means that they're using async layout and they'll perform measurement (and provide a size range) on their own if they want to.

yes, that's a sensible default value. If shouldFetchSizeRanges is false, that means they're using the async layout and the default value will never be used.

appleguy · 2017-06-09T07:08:44Z

Source/Layout/ASLayoutElement.mm

+  else if (propertyName == ASYogaPositionProperty) {
+    ASEdgeInsets position = self.position;
+    YGEdge edge = YGEdgeLeft;
+    for (int i = 0; i < YGEdgeAll + 1; ++i) {


In the future, I can macro-ize / simplify the 4 looping cases below. However, since each of them use a few different property-specific invocations, this level of boilerplate seems reasonable for now.

appleguy · 2017-06-09T07:09:20Z

Source/Layout/ASLayoutElement.mm

+ (void)initialize
+{
+  [super initialize];
+  YGConfigSetUseWebDefaults(YGConfigGetDefault(), true);


I have tested this codepath carefully in the debugger to confirm this code works exactly as intended, so this is ready to go once un-commented. I'd like to enable this in the next ~2-3 weeks.

appleguy · 2017-06-09T07:10:01Z

Source/Layout/ASLayoutElement.mm

+  return _yogaNode;
+}
+
+- (YGNodeRef)yogaNodeCreateIfNeeded


A call to this method is required for Yoga to be used on any given node. Without it, the style properties are not set up on the Yoga node either. This provides an extra level of gating as well.

appleguy · 2017-06-09T07:11:09Z

Source/Private/ASDisplayNodeInternal.h

  __weak ASDisplayNode *_yogaParent;
  ASLayout *_yogaCalculatedLayout;
+  BOOL _yogaLayoutInProgress;


This replaces the ASHierarchyState options. ASHierarchyState propagation turned out to not be desirable, as the Yoga layout may be used on any individual parent+children combination, or contiguous subtrees separated by non-Yoga nodes too.

Reasonable. Should we thread-affine this property? Can yoga layouts proceed in parallel?

Yes, ideally this would be both thread-affined and node-affined.

Considering the other PR doing this with ASLayoutElementContext, I wonder if instead we should have a table inside each ASDisplayNode with an unordered_map from pthread ID to an object context?

appleguy · 2017-06-09T07:12:14Z

Source/ASDisplayNode+Yoga.mm

+    ASDisplayNodeAssert(yogaNode, @"-[ASDisplayNode setYogaMeasureFuncIfNeeded] called with a NULL yogaNode! %@", self);
+    if (yogaNode) {
+      // TODO(appleguy): Use __bridge_transfer to retain nodes and clean up appropriately.
+      YGNodeSetContext(yogaNode, (__bridge void *)self);


Maybe it is appropriate to use ASWeakProxy here, and strong-retain the proxy itself? Otherwise it seems a retain cycle would be created.

Agreed, ASWeakProxy seems to me like the best way to go here. I believe we could only avoid it if we could guarantee that Yoga will never perform any measurement after a node begins deallocation, probably not possible.

Adlai-Holler

Great! I know you're busy as hell so it's great to see significant progress on this front. No objections from me for landing – we should get this in the field as soon as possible and gather info to guide the next step.

Adlai-Holler · 2017-06-10T23:01:09Z

Source/ASDisplayNode+Yoga.mm

+    ASDisplayNodeAssert(yogaNode, @"-[ASDisplayNode setYogaMeasureFuncIfNeeded] called with a NULL yogaNode! %@", self);
+    if (yogaNode) {
+      // TODO(appleguy): Use __bridge_transfer to retain nodes and clean up appropriately.
+      YGNodeSetContext(yogaNode, (__bridge void *)self);


Agreed, ASWeakProxy seems to me like the best way to go here. I believe we could only avoid it if we could guarantee that Yoga will never perform any measurement after a node begins deallocation, probably not possible.

Adlai-Holler · 2017-06-10T23:05:01Z

Source/Details/ASDataController.mm

@@ -391,7 +391,7 @@ - (void)_insertElementsIntoMap:(ASMutableElementMap *)map
      nodeBlock = [dataSource dataController:self supplementaryNodeBlockOfKind:kind atIndexPath:indexPath];
    }

-    ASSizeRange constrainedSize;
+    ASSizeRange constrainedSize = ASSizeRangeUnconstrained;


@nguyenhuy would know better, but I believe this is fine because if we don't fetch size ranges at this stage, it means that they're using async layout and they'll perform measurement (and provide a size range) on their own if they want to.

Adlai-Holler · 2017-06-10T23:05:48Z

Source/Layout/ASLayoutElement.mm

+ (void)initialize
+{
+  [super initialize];
+  YGConfigSetUseWebDefaults(YGConfigGetDefault(), true);


Adlai-Holler · 2017-06-10T23:07:54Z

Source/Private/ASDisplayNodeInternal.h

  __weak ASDisplayNode *_yogaParent;
  ASLayout *_yogaCalculatedLayout;
+  BOOL _yogaLayoutInProgress;


Reasonable. Should we thread-affine this property? Can yoga layouts proceed in parallel?

…layout mode.

…thods.

ghost · 2017-06-15T02:13:58Z

2 Warnings

⚠️ This is a big PR, please consider splitting it up to ease code review.

⚠️

Please ensure license is correct for PhotoCellNode.m:

//
//  PhotoCellNode.m
//  Texture
//
//  Copyright (c) 2014-present, Facebook, Inc.  All rights reserved.
//  This source code is licensed under the BSD-style license found in the
//  LICENSE file in the /ASDK-Licenses directory of this source tree. An additional
//  grant of patent rights can be found in the PATENTS file in the same directory.
//
//  Modifications to this file made after 4/13/2017 are: Copyright (c) 2017-present,
//  Pinterest, Inc.  Licensed under the Apache License, Version 2.0 (the "License");
//  you may not use this file except in compliance with the License.
//  You may obtain a copy of the License at
//
//      http://www.apache.org/licenses/LICENSE-2.0
//

Generated by 🚫 Danger

…leaner integration (TextureGroup#343) * [Yoga] Rewrite YOGA_TREE_CONTIGUOUS mode with support for mixing with ASLayoutSpec. After experimentation with the ASYogaLayoutSpec (or non-contiguous) approach to integrating Yoga, test results and feedback from the authors of Yoga have shown that this approach can't be made completely correct, There are issues with some of the features required to represent Web-style flexbox; in particular: padding, margins, and border handling have varience. This diff is a first step towards a truly correct and elegant implementation of Yoga integration with Texture. In addition to reducing the footprint of the integration, which is an explicit goal of work at this stage, these changes already support improved behavior - including mixing between ASLayoutSpecs even as subnodes of Yoga layout-driven nodes, in addition to above them. Yoga may be used for any set of nodes. Because Yoga usage is limited at this time, it's safe to merge this diff and further improvements will be refinements in this direction. * [ASDKgram] Add Yoga layout implementation for PhotoCellNode. * [Yoga] Final fixes for the upgraded implementation of the Contiguous layout mode. * [Yoga] Add CHANGELOG.md entry and fix for Yoga rounding to screen scale. * [Yoga] Minor cleanup to remove old comments and generalize utility methods.

appleguy self-assigned this Jun 9, 2017

appleguy requested review from maicki and nguyenhuy June 9, 2017 06:55

appleguy commented Jun 9, 2017

View reviewed changes

Adlai-Holler approved these changes Jun 10, 2017

View reviewed changes

appleguy added 6 commits June 12, 2017 21:39

Merge branch 'master' into YogaContiguousRedux

3467dce

[ASDKgram] Add Yoga layout implementation for PhotoCellNode.

70d81ae

[Yoga] Final fixes for the upgraded implementation of the Contiguous …

c2e285d

…layout mode.

Merge branch 'YogaContiguousRedux' into YogaContigMasterTest

818b4b2

[Yoga] Add CHANGELOG.md entry and fix for Yoga rounding to screen scale.

18ea1c2

Merge branch 'master' into YogaContiguousRedux

3546426

appleguy force-pushed the YogaContiguousRedux branch from f579fd6 to 9d378c4 Compare June 15, 2017 01:59

[Yoga] Minor cleanup to remove old comments and generalize utility me…

91a9c77

…thods.

appleguy force-pushed the YogaContiguousRedux branch from 9d378c4 to 91a9c77 Compare June 15, 2017 02:01

appleguy merged commit 55928f3 into master Jun 15, 2017

appleguy deleted the YogaContiguousRedux branch June 15, 2017 02:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Yoga] Rewrite YOGA_TREE_CONTIGUOUS mode with improved behavior and cleaner integration #343

[Yoga] Rewrite YOGA_TREE_CONTIGUOUS mode with improved behavior and cleaner integration #343

appleguy commented Jun 9, 2017

appleguy Jun 9, 2017

appleguy Jun 9, 2017

Adlai-Holler Jun 10, 2017

nguyenhuy Jun 12, 2017

appleguy Jun 9, 2017

appleguy Jun 9, 2017

Adlai-Holler Jun 10, 2017

appleguy Jun 9, 2017

appleguy Jun 9, 2017

Adlai-Holler Jun 10, 2017

appleguy Jun 14, 2017

appleguy Jun 9, 2017

Adlai-Holler Jun 10, 2017

Adlai-Holler left a comment

Adlai-Holler Jun 10, 2017

Adlai-Holler Jun 10, 2017

Adlai-Holler Jun 10, 2017

Adlai-Holler Jun 10, 2017

ghost commented Jun 15, 2017

[Yoga] Rewrite YOGA_TREE_CONTIGUOUS mode with improved behavior and cleaner integration #343

[Yoga] Rewrite YOGA_TREE_CONTIGUOUS mode with improved behavior and cleaner integration #343

Conversation

appleguy commented Jun 9, 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

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 left a 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

ghost commented Jun 15, 2017