RFR: 8350149: VBox ignores bias of child controls when fillWidth is set to false [v2]

[John Hendrikx]

On Fri, 28 Feb 2025 19:54:22 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> John Hendrikx has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
>> 
>>  - Merge branch 'openjdk:master' into feature/vbox-fillwidth-bug-fix
>>  - Make computeChildMin/Pref/MaxAreaHeight accept a fillWidth parameter
>
> modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 1953:
> 
>> 1951: 
>> 1952:         double alt = -1;
>> 1953:         if (availableWidth != -1 & child.isResizable() && child.getContentBias() == Orientation.HORIZONTAL) { // height depends on width
> 
> & or && ?

Oops, definitely `&&` :)

> modules/javafx.graphics/src/main/java/javafx/scene/layout/VBox.java line 477:
> 
>> 475:         double[] usedHeights = areaHeights[0];
>> 476:         double[] temp = areaHeights[1];
>> 477:         final boolean isFillWidth = isFillWidth();
> 
> isn't it effectively final here?

Yeah, I think I "copied" the code style surrounding it... you normally wouldn't catch me writing `final` on locals

> modules/javafx.graphics/src/shims/java/javafx/scene/layout/RegionShim.java line 59:
> 
>> 57:         Node child, Insets margin) {
>> 58:     return r.computeChildMinAreaHeight(child, margin);
>> 59: }
> 
> there is something wrong with indentation here.
> and ... in this class
> 
> since it's a shim, I would rather auto-format the whole thing...

I'll see what I can do

> modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderPaneTest.java line 349:
> 
>> 347:         assertEquals(240 /* l + r + c*/, borderpane.prefHeight(-1), 1e-10);
>> 348:         assertEquals(110, borderpane.minWidth(-1), 1e-100); /* min center + 2x pref width (l, r) */
>> 349:         assertEquals(20 /*t*/ + 200 /*c*/ + 20 /*b*/, borderpane.minHeight(-1), 1e-10);
> 
> minor: Do you think it'll be easier to define the constants explicitly, line
> 
> double L = 40;
> double C = 200;
> 
> and use those?

I just copied the style that was being used, but can make any changes desired of course (at the expense of increasing the diff and amount of code to review).

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1976010610
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1976011044
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1976011363
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1976012048