On Wed, 19 Aug 2026 14:21:45 GMT, Michael Strauß <[email protected]> wrote:
> StackPane has the following defects: > > 1. All measurement methods use raw insets rather than `snappedTopInset()`, > `snappedLeftInset()`, etc. > 2. `layoutChildren()` also uses raw insets and raw subtraction for the > content rectangle. > 3. Calculated totals and spans are not re-snapped after addition or > subtraction. > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Changes requested by angorya (Reviewer). modules/javafx.graphics/src/main/java/javafx/scene/layout/StackPane.java line 355: > 353: > 354: private double snapContentWidth(double width) { > 355: return width < 0 ? -1 : snapSpaceX(snapSpaceX(width) - > snappedLeftInset() - snappedRightInset()); is it possible for `snapSpaceX(snapSpaceX(width) - snappedLeftInset() - snappedRightInset()` to become negative? modules/javafx.graphics/src/main/java/javafx/scene/layout/StackPane.java line 369: > 367: return snapSizeX(boundedSize( > 368: child.minWidth(-1), > 369: adjustWidthByMargin(areaWidth, getMargin(child)), adjustWidthByMargin() does not snap the return value - should it? modules/javafx.graphics/src/test/java/test/javafx/scene/layout/StackPaneTest.java line 601: > 599: > 600: @ParameterizedTest > 601: @ValueSource(doubles = {1, 1.25, 1.5, 1.75, 2}) we should include 2.25 (everywhere) - I can see this option on my win11 laptop. and maybe use a `allScales()` with the `@MethodSource` annotation to supply parameters (instead of repeating the same list over and over) modules/javafx.graphics/src/test/java/test/javafx/scene/layout/StackPaneTest.java line 616: > 614: stack.snappedTopInset() + stack.snapSizeY(100) + > stack.snappedBottomInset()); > 615: > 616: assertEquals(expectedWidth, stack.minWidth(-1), 1e-10); it might be better to declare a constant (`EPSILON`?) modules/javafx.graphics/src/test/java/test/javafx/scene/layout/StackPaneTest.java line 658: > 656: assertEquals(9.6, child.getLayoutY(), 1e-10); > 657: assertEquals(101, child.getLayoutBounds().getWidth(), 1e-10); > 658: assertEquals(101, child.getLayoutBounds().getHeight(), 1e-10); should some fractional values be used instead of round numbers (here and elsewhere)? maybe chosen such that these tests fails in master and passed with the fix? (this test is currently passes in master) modules/javafx.graphics/src/test/java/test/javafx/scene/layout/StackPaneTest.java line 702: > 700: stage.setRenderScaleX(renderScale); > 701: stage.setRenderScaleY(renderScale); > 702: stage.setScene(new Scene(root)); I see in SnappingTest:120 the scale is bound (probably because it might change in stage.show(). Should we do the same here? I've added IO.println(renderScale + " " + stage.getRenderScaleX()); after L703 and got this: 1.0 1.0 1.25 1.0 1.5 1.0 1.75 1.0 2.0 1.0 looks like the tests are not testing (that might explain the fact that some tests are passing in master). ------------- PR Review: https://git.openjdk.org/jfx/pull/2266#pullrequestreview-4977385216 PR Review Comment: https://git.openjdk.org/jfx/pull/2266#discussion_r3824325056 PR Review Comment: https://git.openjdk.org/jfx/pull/2266#discussion_r3824314538 PR Review Comment: https://git.openjdk.org/jfx/pull/2266#discussion_r3817260333 PR Review Comment: https://git.openjdk.org/jfx/pull/2266#discussion_r3817276756 PR Review Comment: https://git.openjdk.org/jfx/pull/2266#discussion_r3824607544 PR Review Comment: https://git.openjdk.org/jfx/pull/2266#discussion_r3824530916
