Fix bugs introduced with YogaKit improvements #328

hartbit · 2017-01-07T18:23:31Z

I'm trying to fix some bugs I introduced in my latest PR, but while writing the Unit Tests for them, I saw a really weird behaviour. The following exact piece of code WORKS inside a Yoga C++ unit test, but fails from a Objective-C unit test. I had me completely confused and blocked me in my progression. Any ideas?

From C

TEST(YogaTest, stupid_test) {
  const YGNodeRef node = YGNodeNew();
  YGNodeStyleSetPosition(node, YGEdgeLeft, 1);

  ASSERT_FLOAT_EQ(1, YGNodeStyleGetPosition(node, YGEdgeLeft).value);
  ASSERT_EQ(YGUnitPixel, YGNodeStyleGetPosition(node, YGEdgeLeft).unit);

  YGNodeFree(node);
}

From Objective-C

- (void)testPositionalPropertiesWork
{
  YGNodeRef node = YGNodeNew();
  YGNodeStyleSetPosition(node, YGEdgeLeft, 1);

  XCTAssertEqual(1, YGNodeStyleGetPosition(node, YGEdgeLeft).value);
  XCTAssertEqual(YGUnitPixel, YGNodeStyleGetPosition(node, YGEdgeLeft).unit);

  YGNodeFree(node);
}

hartbit · 2017-01-07T18:25:08Z

Btw, while stepping through the debugger, the Yoga YGNodeStyleGetPosition method seems to calculate the right value, but somehow, in between the value being returned and saved on the stack of the Objective-C method, the YGValue struct is filled with nonsense.

I committed the Objective-C tests if anybody wants to play with this @dshahidehpour @emilsjolander

d16r · 2017-01-07T18:35:45Z

@hartbit I'm patching onto my machine right now to take a look. Did you try running the tests via BUCK and Xcode? It should make absolutely no difference, just curious.

hartbit · 2017-01-07T18:41:34Z

@dshahidehpour I tried both with BUCK and Xcode and got the same results. But as a side note, some other tests do fail when run through Xcode, for two reasons I think:

Xcode runs some tests in parallel, which cause YGNodeGetInstanceCount to return more instances than those of the test calling the function.
Xcode defaults to running the tests on a retina simulator, which causes some of the UILabels to return slightly different sizes when sizeThatFits is called. Those tests should probably be written to not depend on precise values of UILabel sizes.

David.

d16r · 2017-01-07T18:57:10Z

Not totally helpful, but, I tried running this test before the Swift API diff landed, and the test passes, not sure if this tells us anything helpful. I have to go offline, but I'll revisit ASAP!

hartbit · 2017-01-07T19:47:51Z

Travis failed initially because I forgot to commit some minor changes. Lets see what it says now...

hartbit · 2017-01-07T20:00:03Z

Travis won't run because my previous PR was reverted. Anyway, this is the command I run to test YogaKit and which fails on this branch:

buck test //YogaKit:YogaKitTests --config cxx.default_platform=iphonesimulator-x86_64 --config cxx.cflags=-DTRAVIS_CI

d16r · 2017-01-09T17:09:13Z

@hartbit The diff has relanded, are you still seeing this issue after rebasing?

hartbit · 2017-01-09T22:00:06Z

@dshahidehpour The diff fixed the problems I was having and I was able to finish this PR and fix the remaining bugs and add the required tests.

In doing so, I found a problem: the getters for the horizontal, vertical and all properties crashes because the Yoga C API throws an assert when attempting to get those values back, as if it was not expected. I fixed it by removing the assert in the C API and documenting the change by writing C unit tests that demonstrate what the output is supposed to me.

I'm open to discussion.

d16r · 2017-01-09T22:40:20Z

yoga/Yoga.c

 static inline const YGValue *YGComputedEdgeValue(const YGValue edges[YGEdgeCount],
                                                 const YGEdge edge,
                                                 const YGValue *const defaultValue) {
-  YG_ASSERT(edge <= YGEdgeEnd, "Cannot get computed value of multi-edge shorthands");


@emilsjolander any idea why this is needed?

It is not needed i guess. but getting the computed value of "all" or "horizontal edge just does not make sense. e.g.

node.marginHorizontal = 10; node.marginLeft = 20; node.marginRight = 30;

At this point resolving marginHorizontal does not make much sense. I don't think this should be fixed by removing the assert. If we want to return the value set then we should instead have another function to do so.

@emilsjolander So If I'm understanding you right we should:

Add the ASSERT back.

Change all the tests where we call a getter on YGEdgeAll, YGEdgeHorizontal, YGEdgeVertical to access their respective representations.

Remove the new YGEdgeTests since they are testing functionality we don't support.

The tests should test that setting node.marginHorizontal = 10 results in a a node with marginLeft and marginRight set. However there are already tests for this in Yoga so not sure what the value is in adding tests for this in yogakit as well.

Change all the tests where we call a getter on YGEdgeAll, YGEdgeHorizontal, YGEdgeVertical to access their respective representations.

But what would the getter of marginHorizontal do? Right now, I'm not aware of any function in Yoga which I can call which returns the non-computed value. Should I create it? But what should its name be? I wrote these tests as a result of removing the assert because with the assert, the getters of multi-edge shorthands crashed.

I think that would negatively affect the API in Swift and for users of Objective-C which want to use dot-notation. The whole point of the PR I wrote which was recently merged was to make the APIs cleaner by using properties. Reverting that for multi-edge shorthands looks really weird to me:

let view = UIView() view.yoga.isEnabled = true view.yoga.marginLeft = 10 view.yoga.setMarginVertical(20) view.yoga.paddingTop = 15 view.yoga.paddingBottom = 10 view.yoga.setPaddingHorizontal(10)

view.yoga.marginVertical = 20 is just syntax-sugar [view.yoga setMarginVertical:20]. So (for ObjC) we can add - (void)setMarginVertical: and still use the dot-syntax.

Not sure how it translates for the Swift API, though.

I also wouldn't be against removing the computed edges from YogaKit

view.yoga.marginVertical = 20 is just syntax-sugar [view.yoga setMarginVertical:20]. So (for ObjC) we can add - (void)setMarginVertical: and still use the dot-syntax.

It does work, but (1) we loose auto-completion and (2) it's kind of an anti-pattern to use dot-notation on methods.

Not sure how it translates for the Swift API, though.

The code I provided above is how it translates to Swift: we are forced to use the method syntax.

Sorry if I come across as vindicative. I'm just trying to express that I feel this would be a net-loss for the Objective-C and more importantly the Swift API. Can we try to think of solutions which would allow us the retain the getters? Here are some ideas to help me/us think about it:

We could implement the getters to return a value when the short-hand edges have the same value, and return undefined instead:

let view = UIView() view.yoga.marginHorizontal = 10 print(view.yoga.marginHorizontal) // 10 view.yoga.marginStart = 10 print(view.yoga.marginHorizontal) // YGUndefined view.yoga.marginEnd = 10 print(view.yoga.marginHorizontal) // 10 view.yoga.marginStart = 15 print(view.yoga.marginHorizontal) // YGUndefined

Any other ideas?

@hartbit I agree we should keep these as being properties. I like the suggestion of returning YGUndefined when the semantics are not defined. I suggest for this pull request to always return YGUndefined for the property getters of YGEdgeAll/Horizontal/Vertical. That way we are able to land the rest of this PR. In a follow up issue / PR we can discuss improving this by implementing what you suggested above. How does that sound?

d16r · 2017-01-09T22:42:30Z

YogaKit/Tests/YogaKitTests.m

+  UIView *view = [[UIView alloc] initWithFrame:CGRectZero];
+
+  view.yoga.left = 1;
+  XCTAssertEqual(YGNodeStyleGetPosition(view.yoga.node, YGEdgeLeft).value, 1);


I think we can remove the verification that YGNodeRef's setters are working, that shouldn't be our responsibility.

I do think it's valuable to verify that they are set to Pixel values, though.

I do think it's valuable to verify that getter for view.yoga.* works, and that it is a YGUnitPixel would be more than enough. Any chance we can condense these tests so that we simply test that different

I think those tests are important. They were failing because a bug in the macros I wrote was causing the getters/setters to not be defined and Objective-C would therefore auto-synthesise them, causing the YGNodeSet functions to never be called. The tests make sure the macros are defined and called correctly to both (1) define the correct getters and setters and (2) have the setters call YGNodeSet. Its so easy to mess up the macros and never see it.

Thanks for clarifying!

I was just thinking that if we had a unit test that tested layout while using margin/paddng/border, etc, it would become clear that the NodeRef's weren't being properly set because the layout would be in correct. Since this accomplishes the same thing (yet in a slightly more verbose way) I won't argue the point anymore :) Thanks for the test cases!

I started with tests which tested the layout, but it felt like a duplication of tests in the C++ Yoga tests. I felt better with tests which read like what they are testing. But if you feel strongly about it, I don't mind updating the tests :)

emilsjolander

nit: test*PropertiesWork -> test*Properties

emilsjolander · 2017-01-10T14:42:59Z

YogaKit/Tests/YogaKitTests.m

 */

 #import <XCTest/XCTest.h>
-


emilsjolander · 2017-01-10T14:45:00Z

YogaKit/YGLayout+Private.h

+#import <yoga/Yoga.h>

-@interface YGLayout (Private)
+@interface YGLayout ()


Why did you remove the (Private)?

That's required because the private header now contains a property (node, which needs to be accessed in the tests) and which isn't synthesised if declared in a category, compared to a private interface definition.

emilsjolander · 2017-01-10T14:47:16Z

yoga/Yoga.c

 static inline const YGValue *YGComputedEdgeValue(const YGValue edges[YGEdgeCount],
                                                 const YGEdge edge,
                                                 const YGValue *const defaultValue) {
-  YG_ASSERT(edge <= YGEdgeEnd, "Cannot get computed value of multi-edge shorthands");


The tests should test that setting node.marginHorizontal = 10 results in a a node with marginLeft and marginRight set. However there are already tests for this in Yoga so not sure what the value is in adding tests for this in yogakit as well.

emilsjolander · 2017-01-12T10:12:39Z

@hartbit What is the status of this?

hartbit · 2017-01-12T20:03:25Z

@emilsjolander I am waiting for your feedback on my reply on the comment above to see what we do about the getters :)

… the shorthand edge getters return YGUndefined

hartbit · 2017-01-14T00:17:23Z

@emilsjolander All done for me. I did as you suggested and made all shorthand edge getters return YGUndefined for now :)

emilsjolander · 2017-01-15T22:03:02Z

@hartbit travis is failing

hartbit · 2017-01-16T07:54:27Z

@emilsjolander My bad. It's fixed now. Had forgotten to remove the tests I had added to prove that the removable of the ASSERT was behaving as expected. The tests are not necessary now that I put the ASSERT back.

facebook-github-bot · 2017-01-16T18:13:48Z

@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

trying to fix and test bugs introduced with the yogakit improvements

583686e

facebook-github-bot added the CLA Signed label Jan 7, 2017

missing files

4be987c

hartbit added 2 commits January 9, 2017 21:31

Merge branch 'master' into yogakit-edge-bugs

3456e0c

fixed the remaining bugs but had to modify Yoga.c

468191e

d16r reviewed Jan 9, 2017

View reviewed changes

emilsjolander reviewed Jan 10, 2017

View reviewed changes

emilsjolander mentioned this pull request Jan 10, 2017

Carthage and CocoaPods support #309

Closed

hartbit added 3 commits January 13, 2017 17:05

Merge branch 'master' into yogakit-edge-bugs

2e99a5b

working towards applying suggestions

bb19861

rewrote macros to simplify them and satisfy the requirement of having…

6cf6236

… the shorthand edge getters return YGUndefined

revert the tests I had added to test the removal of the ASSERT

a219251

facebook-github-bot closed this in 5404bb1 Jan 18, 2017

hartbit deleted the yogakit-edge-bugs branch January 22, 2017 09:47

Fix bugs introduced with YogaKit improvements #328

Fix bugs introduced with YogaKit improvements #328

Uh oh!

Conversation

hartbit commented Jan 7, 2017

From C

From Objective-C

Uh oh!

hartbit commented Jan 7, 2017

Uh oh!

d16r commented Jan 7, 2017

Uh oh!

hartbit commented Jan 7, 2017

Uh oh!

d16r commented Jan 7, 2017

Uh oh!

hartbit commented Jan 7, 2017

Uh oh!

hartbit commented Jan 7, 2017

Uh oh!

d16r commented Jan 9, 2017

Uh oh!

hartbit commented Jan 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d16r Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d16r Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilsjolander left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilsjolander commented Jan 12, 2017

Uh oh!

hartbit commented Jan 12, 2017

Uh oh!

hartbit commented Jan 14, 2017

Uh oh!

emilsjolander commented Jan 15, 2017

Uh oh!

hartbit commented Jan 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d16r Jan 10, 2017 •

edited

Loading

d16r Jan 10, 2017 •

edited

Loading

hartbit commented Jan 16, 2017 •

edited

Loading