[RFC] adding height to the measure function

[majak]

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]

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

[majak]

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]

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

@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 8af0b28 that makes it compile with the old NUnit? (as I've learned it has incorrect commit message)

[pragmatrix]

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

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]

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

ping :)

[lucasr]

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]

lgtm

[majak]

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]

Thanks a lot @majak!

[majak]

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

[paramaggarwal]

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]

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