DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

Fix bugs introduced with YogaKit improvements #328

Closed
hartbit wants to merge 8 commits from yogakit-edge-bugs into master
pull from: yogakit-edge-bugs
merge into: DaddyFrosty:master
Conversation 31 Commits 8 Files Changed 4 +412 -38
5 changed files with 412 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit bb19861db5 - Show all commits

228
YogaKit/Tests/YogaKitTests.m
View File

@@ -8,6 +8,7 @@
*/ */
emilsjolander commented 2017-01-10 06:42:59 -08:00 (Migrated from github.com)
Review
Copy Link

revert

revert
hartbit commented 2017-01-13 08:09:16 -08:00 (Migrated from github.com)
Review
Copy Link

Done

Done
#import <XCTest/XCTest.h> #import <XCTest/XCTest.h>
#import <YogaKit/UIView+Yoga.h> #import <YogaKit/UIView+Yoga.h>
#import <YogaKit/YGLayout+Private.h> #import <YogaKit/YGLayout+Private.h>
#import <yoga/Yoga.h> #import <yoga/Yoga.h>
@@ -424,133 +425,172 @@
{ {
UIView *view = [[UIView alloc] initWithFrame:CGRectZero]; UIView *view = [[UIView alloc] initWithFrame:CGRectZero];
view.yoga.marginLeft = 1; view.yoga.margin = 1;
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeLeft).value, 1);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeLeft).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginLeft, 1); XCTAssertEqual(view.yoga.marginLeft, 1);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeLeft).unit, YGUnitPixel);
view.yoga.marginRight = 2; XCTAssertEqual(view.yoga.marginRight, 1);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeRight).value, 2);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeRight).unit, YGUnitPixel); XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeRight).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginRight, 2); XCTAssertEqual(view.yoga.marginStart, 1);
view.yoga.marginTop = 3;
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeTop).value, 3);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeTop).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginTop, 3);
view.yoga.marginBottom = 4;
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeBottom).value, 4);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeBottom).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginBottom, 4);
view.yoga.marginStart = 5;
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeStart).value, 5);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeStart).unit, YGUnitPixel); XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeStart).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginStart, 5); XCTAssertEqual(view.yoga.marginEnd, 1);
view.yoga.marginEnd = 6;
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeEnd).value, 6);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeEnd).unit, YGUnitPixel); XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeEnd).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginEnd, 6); XCTAssertEqual(view.yoga.marginTop, 1);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeTop).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginBottom, 1);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeBottom).unit, YGUnitPixel);
XCTAssertTrue(isnan(view.yoga.marginHorizontal));
XCTAssertTrue(isnan(view.yoga.marginVertical));
XCTAssertTrue(isnan(view.yoga.margin));
view.yoga.marginHorizontal = 7; view.yoga.marginHorizontal = 2;
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeHorizontal).value, 7); XCTAssertEqual(view.yoga.marginLeft, 2);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeHorizontal).unit, YGUnitPixel); XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeLeft).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginHorizontal, 7); XCTAssertEqual(view.yoga.marginRight, 2);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeRight).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginStart, 2);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeStart).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginEnd, 2);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeEnd).unit, YGUnitPixel);
XCTAssertTrue(isnan(view.yoga.marginHorizontal));
view.yoga.marginVertical = 8; view.yoga.marginVertical = 3;
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeVertical).value, 8); XCTAssertEqual(view.yoga.marginTop, 3);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeVertical).unit, YGUnitPixel); XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeTop).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginVertical, 8); XCTAssertEqual(view.yoga.marginBottom, 3);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeBottom).unit, YGUnitPixel);
XCTAssertTrue(isnan(view.yoga.marginVertical));
view.yoga.margin = 9; view.yoga.marginLeft = 4;
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeAll).value, 9); XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeLeft).value, 4);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeAll).unit, YGUnitPixel); XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeLeft).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.margin, 9); XCTAssertEqual(view.yoga.marginLeft, 4);
view.yoga.marginRight = 5;
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeRight).value, 5);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeRight).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginRight, 5);
view.yoga.marginTop = 6;
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeTop).value, 6);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeTop).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginTop, 6);
view.yoga.marginBottom = 7;
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeBottom).value, 7);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeBottom).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginBottom, 7);
view.yoga.marginStart = 8;
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeStart).value, 8);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeStart).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginStart, 8);
view.yoga.marginEnd = 9;
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeEnd).value, 9);
XCTAssertEqual(YGNodeStyleGetMargin(view.yoga.node, YGEdgeEnd).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.marginEnd, 9);
} }
- (void)testPaddingPropertiesWork - (void)testPaddingPropertiesWork
{ {
UIView *view = [[UIView alloc] initWithFrame:CGRectZero]; UIView *view = [[UIView alloc] initWithFrame:CGRectZero];
view.yoga.paddingLeft = 1; view.yoga.padding = 1;
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeLeft).value, 1);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeLeft).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingLeft, 1); XCTAssertEqual(view.yoga.paddingLeft, 1);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeLeft).unit, YGUnitPixel);
view.yoga.paddingRight = 2; XCTAssertEqual(view.yoga.paddingRight, 1);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeRight).value, 2);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeRight).unit, YGUnitPixel); XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeRight).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingRight, 2); XCTAssertEqual(view.yoga.paddingStart, 1);
view.yoga.paddingTop = 3;
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeTop).value, 3);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeTop).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingTop, 3);
view.yoga.paddingBottom = 4;
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeBottom).value, 4);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeBottom).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingBottom, 4);
view.yoga.paddingStart = 5;
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeStart).value, 5);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeStart).unit, YGUnitPixel); XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeStart).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingStart, 5); XCTAssertEqual(view.yoga.paddingEnd, 1);
view.yoga.paddingEnd = 6;
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeEnd).value, 6);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeEnd).unit, YGUnitPixel); XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeEnd).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingEnd, 6); XCTAssertEqual(view.yoga.paddingTop, 1);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeTop).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingBottom, 1);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeBottom).unit, YGUnitPixel);
XCTAssertTrue(isnan(view.yoga.paddingHorizontal));
XCTAssertTrue(isnan(view.yoga.paddingVertical));
XCTAssertTrue(isnan(view.yoga.padding));
view.yoga.paddingHorizontal = 7; view.yoga.paddingHorizontal = 2;
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeHorizontal).value, 7); XCTAssertEqual(view.yoga.paddingLeft, 2);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeHorizontal).unit, YGUnitPixel); XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeLeft).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingHorizontal, 7); XCTAssertEqual(view.yoga.paddingRight, 2);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeRight).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingStart, 2);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeStart).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingEnd, 2);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeEnd).unit, YGUnitPixel);
XCTAssertTrue(isnan(view.yoga.paddingHorizontal));
view.yoga.paddingVertical = 8; view.yoga.paddingVertical = 3;
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeVertical).value, 8); XCTAssertEqual(view.yoga.paddingTop, 3);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeVertical).unit, YGUnitPixel); XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeTop).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingVertical, 8); XCTAssertEqual(view.yoga.paddingBottom, 3);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeBottom).unit, YGUnitPixel);
XCTAssertTrue(isnan(view.yoga.paddingVertical));
view.yoga.padding = 9; view.yoga.paddingLeft = 4;
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeAll).value, 9); XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeLeft).value, 4);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeAll).unit, YGUnitPixel); XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeLeft).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.padding, 9); XCTAssertEqual(view.yoga.paddingLeft, 4);
view.yoga.paddingRight = 5;
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeRight).value, 5);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeRight).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingRight, 5);
view.yoga.paddingTop = 6;
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeTop).value, 6);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeTop).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingTop, 6);
view.yoga.paddingBottom = 7;
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeBottom).value, 7);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeBottom).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingBottom, 7);
view.yoga.paddingStart = 8;
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeStart).value, 8);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeStart).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingStart, 8);
view.yoga.paddingEnd = 9;
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeEnd).value, 9);
XCTAssertEqual(YGNodeStyleGetPadding(view.yoga.node, YGEdgeEnd).unit, YGUnitPixel);
XCTAssertEqual(view.yoga.paddingEnd, 9);
} }
- (void)testBorderWidthPropertiesWork - (void)testBorderWidthPropertiesWork
{ {
UIView *view = [[UIView alloc] initWithFrame:CGRectZero]; UIView *view = [[UIView alloc] initWithFrame:CGRectZero];
view.yoga.borderLeftWidth = 1; view.yoga.borderWidth = 1;
XCTAssertEqual(YGNodeStyleGetBorder(view.yoga.node, YGEdgeLeft), 1);
XCTAssertEqual(view.yoga.borderLeftWidth, 1); XCTAssertEqual(view.yoga.borderLeftWidth, 1);
XCTAssertEqual(view.yoga.borderRightWidth, 1);
XCTAssertEqual(view.yoga.borderStartWidth, 1);
XCTAssertEqual(view.yoga.borderEndWidth, 1);
XCTAssertEqual(view.yoga.borderTopWidth, 1);
XCTAssertEqual(view.yoga.borderBottomWidth, 1);
XCTAssertTrue(isnan(view.yoga.borderWidth));
view.yoga.borderRightWidth = 2; view.yoga.borderLeftWidth = 2;
XCTAssertEqual(YGNodeStyleGetBorder(view.yoga.node, YGEdgeRight), 2); XCTAssertEqual(view.yoga.borderLeftWidth, 2);
XCTAssertEqual(view.yoga.borderRightWidth, 2);
view.yoga.borderTopWidth = 3; view.yoga.borderRightWidth = 3;
XCTAssertEqual(YGNodeStyleGetBorder(view.yoga.node, YGEdgeTop), 3); XCTAssertEqual(view.yoga.borderRightWidth, 3);
XCTAssertEqual(view.yoga.borderTopWidth, 3);
view.yoga.borderBottomWidth = 4; view.yoga.borderTopWidth = 4;
XCTAssertEqual(YGNodeStyleGetBorder(view.yoga.node, YGEdgeBottom), 4); XCTAssertEqual(view.yoga.borderTopWidth, 4);
XCTAssertEqual(view.yoga.borderBottomWidth, 4);
view.yoga.borderStartWidth = 5; view.yoga.borderBottomWidth = 5;
XCTAssertEqual(YGNodeStyleGetBorder(view.yoga.node, YGEdgeStart), 5); XCTAssertEqual(view.yoga.borderBottomWidth, 5);
XCTAssertEqual(view.yoga.borderStartWidth, 5);
view.yoga.borderEndWidth = 6; view.yoga.borderStartWidth = 6;
XCTAssertEqual(YGNodeStyleGetBorder(view.yoga.node, YGEdgeEnd), 6); XCTAssertEqual(view.yoga.borderStartWidth, 6);
XCTAssertEqual(view.yoga.borderEndWidth, 6);
view.yoga.borderWidth = 7; view.yoga.borderEndWidth = 7;
XCTAssertEqual(YGNodeStyleGetBorder(view.yoga.node, YGEdgeAll), 7); XCTAssertEqual(view.yoga.borderEndWidth, 7);
XCTAssertEqual(view.yoga.borderWidth, 7);
} }
@end @end

11
YogaKit/YGLayout.m
View File

@@ -49,12 +49,13 @@
#define YG_STYLE_EDGE_PROPERTY_UNIT_IMPL(objc_lowercased_name, objc_capitalized_name, c_name, edge) \ #define YG_STYLE_EDGE_PROPERTY_UNIT_IMPL(objc_lowercased_name, objc_capitalized_name, c_name, edge) \
- (CGFloat)objc_lowercased_name \ - (CGFloat)objc_lowercased_name \
{ \ { \
YGValue value = YGNodeStyleGet##c_name(self.node, edge); \ if (edge <= YGEdgeEnd) { \
if (value.unit == YGUnitPixel) { \ YGValue value = YGNodeStyleGet##c_name(self.node, edge); \
return value.value; \ if (value.unit == YGUnitPixel) { \
} else { \ return value.value; \
return YGUndefined; \ } \
} \ } \
return YGUndefined; \
} \ } \
\ \
- (void)set##objc_capitalized_name:(CGFloat)objc_lowercased_name \ - (void)set##objc_capitalized_name:(CGFloat)objc_lowercased_name \

2
yoga/Yoga.c
View File

@@ -227,6 +227,8 @@ static YGLogger gLogger = &YGDefaultLog;
static inline const YGValue *YGComputedEdgeValue(const YGValue edges[YGEdgeCount], static inline const YGValue *YGComputedEdgeValue(const YGValue edges[YGEdgeCount],
const YGEdge edge, const YGEdge edge,
const YGValue *const defaultValue) { const YGValue *const defaultValue) {
d16r commented 2017-01-09 14:40:20 -08:00 (Migrated from github.com)
Review
Copy Link

@emilsjolander any idea why this is needed?

@emilsjolander any idea why this is needed?
emilsjolander commented 2017-01-10 03:34:52 -08:00 (Migrated from github.com)
Review
Copy Link

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.

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.
d16r commented 2017-01-10 06:20:57 -08:00 (Migrated from github.com)
Review
Copy Link

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

  1. Add the ASSERT back.
  2. Change all the tests where we call a getter on YGEdgeAll, YGEdgeHorizontal, YGEdgeVertical to access their respective representations.
  3. Remove the new YGEdgeTests since they are testing functionality we don't support.
@emilsjolander So If I'm understanding you right we should: 1. Add the `ASSERT` back. 2. Change all the tests where we call a getter on `YGEdgeAll`, `YGEdgeHorizontal`, `YGEdgeVertical` to access their respective representations. 3. Remove the new `YGEdgeTests` since they are testing functionality we don't support.
emilsjolander commented 2017-01-10 06:47:16 -08:00 (Migrated from github.com)
Review
Copy Link

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.

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.
hartbit commented 2017-01-10 11:51:46 -08:00 (Migrated from github.com)
Review
Copy Link
  1. 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.

> 2. 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.
emilsjolander commented 2017-01-10 11:56:31 -08:00 (Migrated from github.com)
Review
Copy Link

Let's just not have a getter for marginHorizontal and similar for now. We don't allow this is any other versions anyways so if we were to change how those work it should not be part of this PR anyways.

Let's just not have a getter for `marginHorizontal` and similar for now. We don't allow this is any other versions anyways so if we were to change how those work it should not be part of this PR anyways.
hartbit commented 2017-01-10 12:12:26 -08:00 (Migrated from github.com)
Review
Copy Link

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)
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) ```
d16r commented 2017-01-10 13:05:13 -08:00 (Migrated from github.com)
Review
Copy Link

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.

`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.
d16r commented 2017-01-10 13:08:25 -08:00 (Migrated from github.com)
Review
Copy Link

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

I also wouldn't be against removing the computed edges from `YogaKit`
hartbit commented 2017-01-10 13:44:01 -08:00 (Migrated from github.com)
Review
Copy Link

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?

> 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?
emilsjolander commented 2017-01-13 06:48:57 -08:00 (Migrated from github.com)
Review
Copy Link

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

@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?
YG_ASSERT(edge <= YGEdgeEnd, "Cannot get computed value of multi-edge shorthands");
if (edges[edge].unit != YGUnitUndefined) { if (edges[edge].unit != YGUnitUndefined) {
return &edges[edge]; return &edges[edge];
} }