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

Move configuration to new YGConfig and pass them down to CalculateLayout #432

Closed
wants to merge 11 commits into from

Conversation

woehrl01
Copy link
Contributor

@woehrl01 woehrl01 commented Feb 22, 2017

Move configuration to new YGConfig and pass them down to CalculateLayout. See #418 .

Adds YGConfigNew() + YGConfigFree, and changed YGSetExperimentalFeatureEnabled to use the config.

New function for calculation is YGNodeCalculateLayoutWithConfig.

Copy link
Contributor

@emilsjolander emilsjolander left a comment

Choose a reason for hiding this comment

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

Minor comments. Looks very good! Thanks a ton for this

yoga/Yoga.h Outdated
WIN_EXPORT YGConfigRef YGConfigNew(void);
WIN_EXPORT void YGConfigFree(const YGConfigRef config);

WIN_EXPORT void YGSetExperimentalFeatureEnabled(YGConfigRef config,
Copy link
Contributor

Choose a reason for hiding this comment

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

const for params please

yoga/Yoga.c Outdated
YGConfigRef YGConfigNew(void) {
const YGConfigRef config = gYGMalloc(sizeof(YGConfig));
YG_ASSERT(config, "Could not allocate memory for config");
gNodeInstanceCount++;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove gNodeInstanceCount++ as this is not allocating a node. I don't think we care about tracking these allocations right now.

yoga/Yoga.c Outdated
static YGConfig gYGConfigDefaults = {
.experimentalFeatures =
{
[YGExperimentalFeatureRounding] = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

please uses spaces for indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

those are spaces, the formater is layouting it like that.

yoga/Yoga.c Outdated
@@ -188,6 +190,14 @@ static YGNode gYGNodeDefaults = {
},
};

static YGConfig gYGConfigDefaults = {
Copy link
Contributor

Choose a reason for hiding this comment

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

const

@@ -94,6 +94,8 @@ typedef struct YGStyle {
float aspectRatio;
} YGStyle;

typedef struct YGConfig { bool experimentalFeatures[YGExperimentalFeatureCount + 1]; } YGConfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you put this on multiple lines / run formatter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, but the layouter is formating it like that, I think we need to add a second value to layout it differently. As soon as you add float pointScale this should be layouted as desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hehe ok. The format config still needs some tweaking i guess. No problem

yoga/Yoga.h Outdated
WIN_EXPORT void YGConfigFree(const YGConfigRef config);

WIN_EXPORT void YGSetExperimentalFeatureEnabled(const YGConfigRef config,
YGExperimentalFeature feature,
Copy link
Contributor

Choose a reason for hiding this comment

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

feature and enabled const please :)

yoga/Yoga.c Outdated
inline bool YGIsExperimentalFeatureEnabled(YGExperimentalFeature feature) {
return experimentalFeatures[feature];
inline bool YGIsExperimentalFeatureEnabled(const YGConfigRef config,
YGExperimentalFeature feature) {
Copy link
Contributor

Choose a reason for hiding this comment

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

const

yoga/Yoga.c Outdated
void YGSetExperimentalFeatureEnabled(YGExperimentalFeature feature, bool enabled) {
experimentalFeatures[feature] = enabled;
void YGSetExperimentalFeatureEnabled(const YGConfigRef config,
YGExperimentalFeature feature,
Copy link
Contributor

Choose a reason for hiding this comment

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

const

@facebook-github-bot
Copy link
Contributor

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

@emilsjolander
Copy link
Contributor

I'll most likely land this next week. It takes a while to integrate bigger changes like this into internal usages 👍 Thanks for the hard work!

@woehrl01
Copy link
Contributor Author

@emilsjolander fully reasonable! Do you also add the other missing languages?

@emilsjolander
Copy link
Contributor

Yup!

@woehrl01
Copy link
Contributor Author

Awesome!

facebook-github-bot pushed a commit to facebook/litho that referenced this pull request Mar 1, 2017
Summary:
Move configuration to new ```YGConfig``` and pass them down to CalculateLayout. See #418 .

Adds ```YGConfigNew()``` + ```YGConfigFree```, and changed ```YGSetExperimentalFeatureEnabled``` to use the config.

New function for calculation is ```YGNodeCalculateLayoutWithConfig```.
Closes facebook/yoga#432

Reviewed By: astreet

Differential Revision: D4611359

Pulled By: emilsjolander

fbshipit-source-id: a1332f0e1b21cec02129dd021ee57408449e10b0
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Mar 1, 2017
Summary:
Move configuration to new ```YGConfig``` and pass them down to CalculateLayout. See #418 .

Adds ```YGConfigNew()``` + ```YGConfigFree```, and changed ```YGSetExperimentalFeatureEnabled``` to use the config.

New function for calculation is ```YGNodeCalculateLayoutWithConfig```.
Closes facebook/yoga#432

Reviewed By: astreet

Differential Revision: D4611359

Pulled By: emilsjolander

fbshipit-source-id: a1332f0e1b21cec02129dd021ee57408449e10b0
vincentriemer pushed a commit to vincentriemer/yoga-dom that referenced this pull request May 9, 2018
Summary:
Move configuration to new ```YGConfig``` and pass them down to CalculateLayout. See #418 .

Adds ```YGConfigNew()``` + ```YGConfigFree```, and changed ```YGSetExperimentalFeatureEnabled``` to use the config.

New function for calculation is ```YGNodeCalculateLayoutWithConfig```.
Closes facebook/yoga#432

Reviewed By: astreet

Differential Revision: D4611359

Pulled By: emilsjolander

fbshipit-source-id: a1332f0e1b21cec02129dd021ee57408449e10b0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants