On Sat, 29 Jul 2023 00:12:45 GMT, Andy Goryachev <ango...@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(). I have a couple comments on the javadoc and one inline comment on the test. Also, I recommend adding a basic functional test of the new methods (I know that the same could be said of the others), meaning a test with in-range values to make sure that it does a floor (or ceil) as expected. modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 1808: > 1806: * If this region's snapToPixel property is true, returns a value > floored > 1807: * to the nearest pixel in the horizontal direction, else returns > the > 1808: * same value, for any non-negative value. For negative values > returns 0.0. I see this in the two new snap methods: > For negative values returns 0.0. Clamping to zero is not something any of the other snap methods do. It seems unexpected for a general purpose method such as this. If your use case really does require clamping to 0 (does it?), that should either be done by the caller, or it will need a name that implies clamping (e.g., has "clamp" or "positive" or similar in the name). So the first question to answer is whether this really needs to clamp to zero? Possibly worth noting is that this new method is similar to the existing private method `snapPortion`, but with a clamp to 0. I don't know if that might help inform the name (since it is non-public, it might not). 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. modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 1815: > 1813: * larger integers with exact precision beyond this limit. > 1814: * > 1815: * @since 22 Minor: our usual pattern is to put `@since` as the last tag. modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 1817: > 1815: * @since 22 > 1816: * @param value The value that needs to be snapped > 1817: * @return value either as passed, or floored or ceiled with scale, > based on snapToPixel property same comment here: "with scale" --> "to the nearest pixel". modules/javafx.graphics/src/test/java/test/javafx/scene/layout/RegionTest.java line 1289: > 1287: assertEquals(Double.POSITIVE_INFINITY, > region.snapInnerSpaceY(Double.MAX_VALUE), 0.0); > 1288: assertEquals(Double.NEGATIVE_INFINITY, > region.snapInnerSpaceX(-Double.MAX_VALUE), 0.0); > 1289: assertEquals(Double.NEGATIVE_INFINITY, > region.snapInnerSpaceY(-Double.MAX_VALUE), 0.0); Why is the expected value infinity here? ------------- PR Review: https://git.openjdk.org/jfx/pull/1190#pullrequestreview-1553287860 PR Review Comment: https://git.openjdk.org/jfx/pull/1190#discussion_r1278303962 PR Review Comment: https://git.openjdk.org/jfx/pull/1190#discussion_r1317925910 PR Review Comment: https://git.openjdk.org/jfx/pull/1190#discussion_r1317910608 PR Review Comment: https://git.openjdk.org/jfx/pull/1190#discussion_r1317927611 PR Review Comment: https://git.openjdk.org/jfx/pull/1190#discussion_r1317916678
