[RFC] adding height to the measure function #154

2015-11-23T07:29:00-08:00

majak commented

2015-11-23 07:29:00 -08:00

(Migrated from github.com)

The flexbox algorithm is considering text as only thing that does its own layout. Since text usually flows horizontally, the important information for laying it out is the width of its enclosing container (we are assuming its height is unbounded).
In my case I plan to use nodes that do their own layout too, but they are not constrained in same way as text is. (You can imagine this node as a view provided by a different layout engine.)
That means they can be provided height and be unbounded in width. In order to achieve this I need to pass information about height to the measure function as well.
This PR does that and adds some tests too.

Few things to note:

I'm not an expert on flexbox algorithm. I have a vague idea how it works.
My changes were made mostly im mechanical copy-paste fashion to mimick what's happening with width, since I believe there shouldn't be any differences based on the flex direction.
Concept of nodes layed out by a different "system" is foreign to Chrome and therefore I don't think it is possible to test this change with Chrome's CSS implementation. So I've added an option to skip this part of test.
This is ~nonbreaking change in js world, but the generated code (which I haven't tried out yet) is strongly typed and therefore the measure function is going to have a different signature. We could make it support both flavors of measure function, or keep on different branch/fork (not very ideal), or force everyone to fix it (not a big deal imo).

Looking forward to comments and suggestions!

The flexbox algorithm is considering text as only thing that does its own layout. Since text usually flows horizontally, the important information for laying it out is the width of its enclosing container (we are assuming its height is unbounded). In my case I plan to use nodes that do their own layout too, but they are not constrained in same way as text is. (You can imagine this node as a view provided by a different layout engine.) That means they can be provided height and be unbounded in width. In order to achieve this I need to pass information about height to the measure function as well. This PR does that and adds some tests too. Few things to note: 1. I'm not an expert on flexbox algorithm. I have a vague idea how it works. My changes were made mostly im mechanical copy-paste fashion to mimick what's happening with width, since I believe there shouldn't be any differences based on the flex direction. 2. Concept of nodes layed out by a different "system" is foreign to Chrome and therefore I don't think it is possible to test this change with Chrome's CSS implementation. So I've added an option to skip this part of test. 3. This is ~nonbreaking change in js world, but the generated code (which I haven't tried out yet) is strongly typed and therefore the measure function is going to have a different signature. We could make it support both flavors of measure function, or keep on different branch/fork (not very ideal), or force everyone to fix it (not a big deal imo). Looking forward to comments and suggestions!

vjeux commented

2015-11-23 09:38:46 -08:00

(Migrated from github.com)

This sounds good to me. If you port the changes to C and Java I'll gladly merge the pull request :)

majak commented

2015-11-25 16:54:50 -08:00

(Migrated from github.com)

I've fixed all three C, java and C# flavors and their tests, each of them is in a sperate commit.

There are two more things (not directly related to adding the width) I've added:

Custom measure function can be currently used only for leaf nodes. I don't think there is any inherent reason why, but current code implicitely requires it. So I've introduces two helpful errors in case someone would try to introduce such test case. (I've wasted nontrivial time on this, since it's pretty hard to figure out what is really happening. Reason for failure is not obvious at all.)
I had to install mono/xbuild. Looks like it comes with a version of library for unit testing (NUnit3) which is not compatible with the code in the repo. I've changed C# tests so they would compile with the NUnit3. Strangely it doesn't run any of them when I try locally:

    Running "shell:csharpTestExecute" (shell) task
    Tests run: 0, Failures: 0, Not run: 0, Time: 0.001 seconds

I'm not sure why, but they seem to be run by Travis. ¯_(ツ)_/¯

I can submit both of these improvements as a separate PRs if you wish.

I've fixed all three C, java and C# flavors and their tests, each of them is in a sperate commit. There are two more things (not directly related to adding the width) I've added: - Custom measure function can be currently used only for leaf nodes. I don't think there is any inherent reason why, but current code implicitely requires it. So I've introduces two helpful errors in case someone would try to introduce such test case. (I've wasted nontrivial time on this, since it's pretty hard to figure out what is really happening. Reason for failure is not obvious at all.) - I had to install mono/xbuild. Looks like it comes with a version of library for unit testing (NUnit3) which is not compatible with the code in the repo. I've changed C# tests so they would compile with the NUnit3. Strangely it doesn't run any of them when I try locally: ``` Running "shell:csharpTestExecute" (shell) task Tests run: 0, Failures: 0, Not run: 0, Time: 0.001 seconds ``` I'm not sure why, but they seem to be run by Travis. ¯\_(ツ)_/¯ I can submit both of these improvements as a separate PRs if you wish.

pragmatrix commented

2015-11-26 02:59:27 -08:00

(Migrated from github.com)

@majak What probably happened is that nunit-console is an older version that can not pick up the tests. Try to see if its version is >= 2.6 by by running it without any arguments.

I could reproduce this on CentOS7. There are two packages, NUnit, which installs nunit-console-2.6, working just fine, and mono-nunit, which installs nunit-console version 2.4 but fails to pick up the tests.

@majak What probably happened is that `nunit-console` is an older version that can not pick up the tests. Try to see if its version is >= 2.6 by by running it without any arguments. I could reproduce this on CentOS7. There are two packages, `NUnit`, which installs `nunit-console-2.6`, working just fine, and `mono-nunit`, which installs `nunit-console` version 2.4 but fails to pick up the tests.

majak commented

2015-11-26 08:52:16 -08:00

(Migrated from github.com)

@pragmatrix You are right. I have NUnit version 2.4.8.
Since I don't know much about C# do you suggest I drop commit 8af0b2869017ee05e95956a1ee71f89a13166483 that makes it compile with the old NUnit? (as I've learned it has incorrect commit message)

@pragmatrix You are right. I have `NUnit version 2.4.8`. Since I don't know much about C# do you suggest I drop commit 8af0b2869017ee05e95956a1ee71f89a13166483 that makes it compile with the old NUnit? (as I've learned it has incorrect commit message)

pragmatrix commented

2015-11-26 09:34:21 -08:00

(Migrated from github.com)

@majak No need to, these new Assert methods are supported by NUnit 2.6 and your branch compiles in Visual Studio 2015 just fine, nice work!

lucasr commented

2015-11-30 07:48:48 -08:00

(Migrated from github.com)

This is looking good overall, thanks! I'd be more confident about these changes if it contained a more thorough set of unit tests.

majak commented

2015-12-03 08:35:06 -08:00

(Migrated from github.com)

@lucasr I've did changes you asked for. There is a commit for each of them, but I also had to fix issues in existing commits' titles, so I had to force push and all of your inlines are gone...

majak commented

2015-12-07 02:15:19 -08:00

(Migrated from github.com)

ping :)

lucasr commented

2015-12-08 03:59:05 -08:00

(Migrated from github.com)

Looks good to me! @vjeux any objections? Otherwise we can merge this.

(Personally, I'd prefer all these commits to be merged into a single atomic change before merging.)

Looks good to me! @vjeux any objections? Otherwise we can merge this. (Personally, I'd prefer all these commits to be merged into a single atomic change before merging.)

vjeux commented

2015-12-08 08:34:33 -08:00

(Migrated from github.com)

lgtm

majak commented

2015-12-14 07:27:30 -08:00

(Migrated from github.com)

I've squashed everything into a single commit and modified one last place where the maxHeight computation didn't properly mirror maxWidth computation (Layout.js:696-704).

I've squashed everything into a single commit and modified one last place where the `maxHeight` computation didn't properly mirror `maxWidth` computation (Layout.js:696-704).

paramaggarwal commented

2016-02-23 07:00:51 -08:00

(Migrated from github.com)

Thanks a lot @majak!

majak commented

2016-02-23 07:06:50 -08:00

(Migrated from github.com)

@paramaggarwal one caveat: If your node has set both width and height its measure function won't be called.

paramaggarwal commented

2016-02-23 22:20:27 -08:00

(Migrated from github.com)

Yes, makes sense. @majak, but if the measure function does get called, it will still continue to layout the children right? Specifically, if I have a box which has a measure function on it, and then two boxes inside it both with flex: 1, then the inner boxes will layout as per flex (half-half)?

Yes, makes sense. @majak, but if the measure function does get called, it will still continue to layout the children right? Specifically, if I have a box which has a measure function on it, and then two boxes inside it both with `flex: 1`, then the inner boxes will layout as per flex (half-half)?

majak commented

2016-02-24 03:12:52 -08:00

(Migrated from github.com)

@paramaggarwal To be honest I'm not really sure. We didn't care about that in our use-case, so I didn't even try. However based on some chats we had internally, my impression is that it wouldn't work as you'd like :(
Anyway I believe this would be a valuable addition.

@paramaggarwal To be honest I'm not really sure. We didn't care about that in our use-case, so I didn't even try. However based on some chats we had internally, my impression is that it wouldn't work as you'd like :( Anyway I believe this would be a valuable addition.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#154