On Sat, 5 Sep 2026 11:42:14 GMT, Michael Strauß <[email protected]> wrote:
>> This PR is an audit of several snapping-related methods in `Region`, along >> with some bug fixes. >> >> I've added a helper method `snapAligned()`, which does the exact same thing >> as `snapSpace()`, but clearly states that the author knows that the value is >> already pixel-aligned. In addition, I've renamed several local variables >> around a "rawFoo" and "snappedFoo" naming scheme, so as to make it easier to >> see what's what. >> >> This PR should probably be integrated before the other layout container PRs, >> because those use the `Region` layout methods. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > review comments Changes look good, I do have some nitpicks. I did run the tests with coverage and it seems like the branch for non resizable children is not covered by any `Region` test yet. See the screenshot below, in `layoutInArea`: <img width="1369" height="805" alt="Image" src="https://github.com/user-attachments/assets/58e9816d-9950-4098-a24b-42172d9f8b8b"; /> modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 1992: > 1990: double alt = -1; > 1991: > 1992: if (availableHeight != -1 && child.isResizable() && > child.getContentBias() == Orientation.VERTICAL) { // width depends on height Minor, preexisting: Inline comments always are bit weird, especially here where it is very much on the right. Maybe just move the comment `// width depends on height` above the method? (There is another one below as well) modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 2060: > 2058: if (baseline == BASELINE_OFFSET_SAME_AS_HEIGHT) { > 2059: double snappedChildMinHeight = > snapSize(child.minHeight(alt), snap, scaleY); > 2060: double snappedAbove = snapAligned(snappedTop + > snappedBottom + snappedChildMinHeight, snap, scaleY); `snappedAbove` sound a bit weird, maybe another name better fits the purpose? modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 2286: > 2284: return snapSize(snappedAbove + maxBaselineComplement, > snap, scaleY); > 2285: } else { > 2286: double margins = snapAligned(snappedTop + > snappedBottom, snap, scaleY); Minor: Should probably also be named `snappedMargins`, at least thats how it is usually named on the other methods modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 2361: > 2359: double right = margin != null ? snapSpace(margin.getRight(), > snapToPixel, snapScale) : 0; > 2360: > 2361: return snapAligned(snapSize(width, snapToPixel, snapScale) - > left - right, snapToPixel, snapScale); Maybe save the result `snapSize` into a `snappedWidth` variable so it is easier to parse (readability)? modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 2365: > 2363: > 2364: /* > 2365: * Removes the given Margin (if any) from a height which still > includes margins Very Minor: Shouldn't `Margin` here be written lower case, `margin`? Looks a bit weird because later in the sentence, it is written as `margins` modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 2860: > 2858: Insets margin, boolean fillWidth, > boolean fillHeight, > 2859: HPos halignment, VPos valignment, > boolean isSnapToPixel) { > 2860: // Very minor: This line can be removed, empty comment line modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 2868: > 2866: // areaBaselineOffset-rawBaseline or > areaHeight-areaBaselineOffset as space. > 2867: // margins: Snap each margin independently as > space. > 2868: // also empty comment line modules/javafx.graphics/src/test/java/test/javafx/scene/layout/RegionTest.java line 1569: > 1567: > 1568: assertEquals(10, RegionShim.computeChildMaxAreaWidth( > 1569: pane, child, -1, Insets.EMPTY, 50, true)); Minor: The assertion will look better when `RegionShim.computeChildMaxAreaWidth` is assigned to a variable before, maybe `var width = ...` modules/javafx.graphics/src/test/java/test/javafx/scene/layout/RegionTest.java line 1686: > 1684: > 1685: assertEquals(10, RegionShim.computeChildMaxAreaHeight( > 1686: pane, child, -1, Insets.EMPTY, 50, true)); Here as well. modules/javafx.graphics/src/test/java/test/javafx/scene/layout/RegionTest.java line 2538: > 2536: } > 2537: > 2538: private static Stage showAtScale(Pane root, double scaleX, double > scaleY) { This is more a `createAtScale` method, as wedo not `show` the `Stage` ------------- Changes requested by mhanl (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/2278#pullrequestreview-5126359841 PR Review Comment: https://git.openjdk.org/jfx/pull/2278#discussion_r3945040235 PR Review Comment: https://git.openjdk.org/jfx/pull/2278#discussion_r3945052889 PR Review Comment: https://git.openjdk.org/jfx/pull/2278#discussion_r3945055381 PR Review Comment: https://git.openjdk.org/jfx/pull/2278#discussion_r3945061432 PR Review Comment: https://git.openjdk.org/jfx/pull/2278#discussion_r3945034037 PR Review Comment: https://git.openjdk.org/jfx/pull/2278#discussion_r3945034949 PR Review Comment: https://git.openjdk.org/jfx/pull/2278#discussion_r3945035830 PR Review Comment: https://git.openjdk.org/jfx/pull/2278#discussion_r3945014709 PR Review Comment: https://git.openjdk.org/jfx/pull/2278#discussion_r3945015160 PR Review Comment: https://git.openjdk.org/jfx/pull/2278#discussion_r3945065240
