On Fri, 4 Sep 2026 12:51:43 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 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 five additional > commits since the last revision: > > - review comments > - Merge branch 'master' into fixes/snapping-region > - more render scales > - fixes > - tests Looks good, just a few minor things you may want to take a look at modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 279: > 277: * margin adjustment and does not choose a fitting policy for a > potentially unsnapped input height. > 278: * A caller that chooses to consume the result as a pixel-aligned > allocated content span is responsible > 279: * for applying the appropriate snapping operation, usually {@link > #snapSpaceY(double)}. should that be `snapSizeY` (or drop it completely like in `adjustWidthByMargin` that doesn't mention this part)? modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 2213: > 2211: double scaleY = getSnapScaleY(); > 2212: double snappedLeft = margin != null ? > snapSpaceX(margin.getLeft(), snap) : 0; > 2213: double snappedRight = margin != null ? > snapSpaceX(margin.getRight(), snap) : 0; Here you used the specific function (snapSpaceX) where I think everywhere else you've been using the `snapSpace` 3 parameter variant modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 2944: > 2942: } else { > 2943: snappedX = rawX; > 2944: snappedY = rawY; Is this inconsistent with what the `snapped` prefix means? ------------- PR Review: https://git.openjdk.org/jfx/pull/2278#pullrequestreview-5120643103 PR Review Comment: https://git.openjdk.org/jfx/pull/2278#discussion_r3940127608 PR Review Comment: https://git.openjdk.org/jfx/pull/2278#discussion_r3940105545 PR Review Comment: https://git.openjdk.org/jfx/pull/2278#discussion_r3940120897
