On Sat, 22 Feb 2025 15:57:28 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 values adjusted. This is becaus...
Not a finished review, only a few initial comments - I need to do a bit more
testing.
This change however looks exceptionally good.
modules/javafx.graphics/src/main/java/javafx/scene/layout/GridPane.java line
1450:
> 1448: int start = getNodeRowIndex(child);
> 1449: int end = getNodeRowEndConvertRemaining(child);
> 1450: double childPrefAreaHeight = computeChildPrefAreaHeight(
thank you for fixing the indentation! much more readable now.
modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 2020:
> 2018: double baseline = child.getBaselineOffset();
> 2019: if (child.isResizable() && baseline ==
> BASELINE_OFFSET_SAME_AS_HEIGHT) {
> 2020: return top +
> snapSizeY(boundedSize(child.minHeight(alt), max, Double.MAX_VALUE)) + bottom
here and elsewhere: a wise man once said that the sum of snapped values may not
come out snapped. Should we start snapping the result of any algebraic
operation that involves snapped values?
modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 2044:
> 2042: * # content width/heights:
> 2043: *
> 2044: * The space allocated to a child, minus its margins. These are
> never -1.
or always `> 0.0` ?
modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 2049:
> 2047: *
> 2048: * The space allocated to a child, minus its margins, adjusted
> according to
> 2049: * its constraints (min <= X <= max). These are never -1.
always `> 0.0` here?
modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 2093:
> 2091: double right = margin != null ? snapSpaceX(margin.getRight(),
> snap) : 0;
> 2092:
> 2093: return width - left - right;
snap?
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/RegionTest.java
line 1025:
> 1023: */
> 1024:
> 1025: assertEquals(12, RegionShim.computeChildMinAreaWidth(pane, c2,
> -1, new Insets(1), -1, false), 1e-100);
might as well declare a static `computeChildMinAreaWidth()` which delegates to
`RegionShim.computeChildMinAreaWidth`...
-------------
PR Review: https://git.openjdk.org/jfx/pull/1723#pullrequestreview-2637850093
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1968064369
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1968088719
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1968091482
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1968104020
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1968090525
PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1968114907
