On Fri, 28 Feb 2025 19:54:22 GMT, Andy Goryachev <ango...@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
