On Mon, 24 Feb 2025 20:30:17 GMT, John Hendrikx <[email protected]> wrote:
>> Fixes the case where `VBox` will ignore the (horizontal) bias of a child
>> when its fill width property is set to `false`. This manifests itself with
>> labels that have their wrap text property set to `true`, and the container
>> is not wide enough to hold the text on a single line (in other words, the
>> label is potentially far wider, and fill width should have no effect). No
>> reflow would occur before this fix.
>>
>> The solution is to ensure the `computeChildAreaMin/Pref/MaxHeight` functions
>> are provided with a `fillWidth` parameter, to be used in the case a
>> horizontally biased control is encountered (several of the parameters to
>> these compute functions are only used for those special cases and ignored
>> otherwise).
>>
>> With this additional information, the compute functions can decide between
>> the preferred width of a control or the available width of the container.
>> In the old implementation this decision was made *before* these functions
>> were called, and the available width was reset to -1 to indicate the case
>> that the preferred width should be used. However, there are three cases,
>> and so setting a single parameter to -1 can't effectively communicate that:
>>
>> Assuming a control that is horizontally biased, and is resizable there are
>> three cases when calculating a **height**:
>>
>> 1. There is no available width: bias is ignored (as there is no value to use
>> as a dependent value) and the height is then simply calculated ignoring
>> available width (which is -1) and fill width settings (same as unbiased
>> controls).
>> 2. There is an available width and fill width is false; the bias logic must
>> first find a reasonable width before it can call any height function; with
>> fill width false, this reasonable width will be the preferred width of the
>> control, unless it exceeds its maximum width or the available width, in
>> which case the smallest of these three values is used. The final width
>> calculated is then used to determine the height.
>> 3. There is an available width and fill width is true; this is the same as
>> case 2, except the available width is used as a dependent value (unless its
>> greater than the child's maximum width, in which case the smaller is used).
>> The final width calculated is then used to determine the height.
>>
>> All in all, this PR removes several inconsistencies between horizontal and
>> vertical computations. The end result is that the horizontal and vertical
>> bias calculations now more closely mirror each other.
>>
>> **Note**: some tests have had their va...
>
> 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 && ?
modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 2140:
> 2138:
> 2139: double computeMaxPrefAreaWidth(List<Node> children, Callback<Node,
> Insets> childMargins,
> 2140: double[] childHeights, boolean fillHeight) {
+1
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?
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...
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?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1975942819
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1975944566
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1975947322
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1975949775
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1975951774
