On Tue, 25 Aug 2026 19:31:09 GMT, Andy Goryachev <[email protected]> wrote:
>> Michael Strauß has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - don't use epsilon
>> - review changes
>
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java
> line 413:
>
>> 411: final double topInset = snappedTopInset();
>> 412: final double bottomInset = snappedBottomInset();
>> 413: final double insideWidth = width < 0 ? -1 : snapSpaceX(width -
>> snappedLeftInset() - snappedRightInset());
>
> can the result be negative?
Yes, I've also fixed this in the height case.
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java
> line 472:
>
>> 470: final double topInset = snappedTopInset();
>> 471: final double bottomInset = snappedBottomInset();
>> 472: final double insideWidth = width < 0 ? -1 : snapSpaceX(width -
>> snappedLeftInset() - snappedRightInset());
>
> negative result?
Yes, I've also fixed this in the height case.
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java
> line 483:
>
>> 481: double leftPrefWidth = getAreaWidth(getLeft(), -1, false);
>> 482: double rightPrefWidth = getAreaWidth(getRight(), -1, false);
>> 483: double centerWidth = snapSpaceX(Math.max(0, insideWidth -
>> leftPrefWidth - rightPrefWidth));
>
> negative result?
No, there's already a `Math.max(0, ...)` call in there. Snapping a non-negative
result can't make it negative.
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java
> line 522:
>
>> 520: final double insideY = snappedTopInset();
>> 521: final double insideWidth = snapSpaceX(width - insideX -
>> snappedRightInset());
>> 522: final double insideHeight = snapSpaceY(height - insideY -
>> snappedBottomInset());
>
> negative result L521, 522 ?
These can be negative, but I don't think it needs to be clamped. For example,
we could explicitly override a pane's minimum size to something smaller than
its positive insets. That is not an unsupported configuration: applications are
generally permitted to override a pane's size constraints. JavaFX panes do not
clip their contents, and `BorderPane` explicitly documents that children can
extend beyond its bounds when their minimum dimensions can't fit.
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java
> line 550:
>
>> 548: Insets bottomMargin = getNodeMargin(b);
>> 549: double adjustedWidth =
>> snapSpaceX(adjustWidthByMargin(insideWidth, bottomMargin));
>> 550: double remainingHeight = snapSpaceY(insideHeight -
>> topHeight);
>
> negative result?
This can be negative when the child's minimum or preferred allocation exceeds
the available span. However, these values are passed to
`boundedNodeSizeWithBias`, where a negative available amount causes the child
to retain at least its minimum size and overflow. So I think we're good here.
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java
> line 558:
>
>> 556: bottomHeight =
>> snapSpaceY(snapSpaceY(bottomMargin.getBottom()) + bottomHeight +
>> snapSpaceY(bottomMargin.getTop()));
>> 557: Pos alignment = getAlignment(b);
>> 558: positionInArea(b, insideX, snapPositionY(insideY +
>> insideHeight - bottomHeight),
>
> possible negative result?
If a bottom or right child is larger than the available area, anchoring it to
the bottom or right edge can place its opposite edge at a negative coordinate.
That is normal overflow behavior.
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java
> line 589:
>
>> 587: if (r != null && r.isManaged()) {
>> 588: Insets rightMargin = getNodeMargin(r);
>> 589: double remainingWidth = snapSpaceX(insideWidth - leftWidth);
>
> possible negative result?
Same as with L550. The values are passed into `boundedNodeSizeWithBias`, so
this is okay.
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java
> line 598:
>
>> 596: rightWidth = snapSpaceX(snapSpaceX(rightMargin.getLeft()) +
>> rightWidth + snapSpaceX(rightMargin.getRight()));
>> 597: Pos alignment = getAlignment(r);
>> 598: positionInArea(r, snapPositionX(insideX + insideWidth -
>> rightWidth), middleY,
>
> possible negative result?
Same as with L558. Negative results are okay.
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java
> line 608:
>
>> 606: Pos alignment = getAlignment(c);
>> 607: layoutInArea(c, snapPositionX(insideX + leftWidth), middleY,
>> 608: snapSpaceX(insideWidth - leftWidth - rightWidth),
>
> possible negative result?
>
> I wonder if snapSpace* should clamp the return value since the space, unlike
> position, cannot be negative?
Negative margins are valid, so `snapSpace` can't clamp to zero.
Aside from that, the result can be negative here if the left and right
allocations together exceed the available inside width. This is preexisting
logic unrelated to snapping, but I need to think a little bit about whether
that should be prevented or not.
> modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderPaneTest.java
> line 1205:
>
>> 1203: Pane root = new Pane(borderpane);
>> 1204: stage = new Stage();
>> 1205: stage.renderScaleXProperty().bind(new
>> SimpleDoubleProperty(scaleX));
>
> binding is done with WeakReferences - should these properties be made
> instance fields instead to avoid them being collected during the test?
The observable passed into `target.bind(source)` is strongly referenced. Maybe
you're thinking of the other direction: the listener installed in `source` only
weakly references `target`. The code as it is used here is generally safe.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3872323152
PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3872323384
PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3872323847
PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3872324320
PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3872324559
PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3872325529
PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3872326014
PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3872326730
PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3872327471
PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3872322995
