On Wed, 6 Sep 2023 23:29:35 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Introduces Region.snapInnerSpaceX/Y() methods for dealing with inner space >> (using Math.floor), see for instance >> [JDK-8299753](https://bugs.openjdk.org/browse/JDK-8299753), using existing >> methods Region.snapPortionX/Y(). > > modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line > 1808: > >> 1806: * If this region's snapToPixel property is true, then the value >> is either floored (positive values) or >> 1807: * ceiled (negative values) with a scale. When the absolute value >> of the given value >> 1808: * multiplied by the current scale is less than 10^15, then this >> method guarantees that: > > 1. This doesn't match the descriptions of the other snap methods. All of them > say "to the nearest pixel" without mentioning "the given scale" (which isn't > accurate anyway, since the scale isn't "given" here). I recommend doing the > same here. If we want to add something about the scale, then it should be > done for all of the snap methods, and in a follow-on issue. If we do this, I > would add it after the first sentence, and it would need to say that it is > the "render" scale, explaining that the scale is used so that it will be > rounded/floored/ceiled to the nearest screen pixel after taking scale into > account. > > 2. None of the other snap methods specify the guarantee you are making in > this method, so we shouldn't add it here. This is also something we could do > in a follow-on if needed. the javadoc was lifted almost verbatim from snapPortionX/Y (I did not find the words "given scale" though). Changing to be the same as snapSizeX/Y, although, technically speaking, that description is not correct either: the value in both sets of methods is ceiled/floored for positive values and floored/ceiled for negative values. We might reword both cases saying something like "rounded to the nearest pixel with larger/smaller value" which will capture the behavior exactly. What do you think? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1190#discussion_r1318847823
