On Tue, 23 Jun 2026 09:45:14 GMT, Florian Kirmaier <[email protected]> wrote:
>> This PR fixes the computeSize methods not considering the gap in some cases. >> >> It also fixes a missed spot, in the previous PR for JDK-8092379. >> >> Each fix is covered by simple tests which fail before and work after the >> change. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Florian Kirmaier has updated the pull request incrementally with two > additional commits since the last revision: > > - JDK-8386590 > Removed some magic values. > - JDK-8386590 > Small code simplification. modules/javafx.graphics/src/test/java/test/javafx/scene/layout/GridPaneTest.java line 3175: > 3173: static class FixedAreaRegion extends Region { > 3174: final double SIDE = 100; > 3175: final double AREA = SIDE*SIDE; Suggestion: final double AREA = SIDE * SIDE; modules/javafx.graphics/src/test/java/test/javafx/scene/layout/GridPaneTest.java line 3176: > 3174: final double SIDE = 100; > 3175: final double AREA = SIDE*SIDE; > 3176: Orientation bias; Should also be `final` modules/javafx.graphics/src/test/java/test/javafx/scene/layout/GridPaneTest.java line 3196: > 3194: } > 3195: > 3196: void checkSize() { If we want to keep that, I would suggest that we rename it something like `assertSize` to make clear that this is an assertion modules/javafx.graphics/src/test/java/test/javafx/scene/layout/GridPaneTest.java line 3198: > 3196: void checkSize() { > 3197: // This should work when the Node is laid out properly. > 3198: assertEquals(AREA, getWidth() * getHeight(), 20); >From my testig, a `delta` of 10 was enough. Thats why I was a bit confused and >also suggested to use precise assertions instead. I understand you want to keep that for future tests, which is fine for me. But I'm a bit afraid that a high delta may masks some bugs where the test should fail but will pass because it fits into the delta of 20. Especially since v/hgap is usually set to 4 or 5, and we only have 2 cols/rows, a delta of 20 can be enough to e.g. hide the fact that a gap was not calculated in correctly or similar situations. Does that makes sense? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2187#discussion_r3459194615 PR Review Comment: https://git.openjdk.org/jfx/pull/2187#discussion_r3459197261 PR Review Comment: https://git.openjdk.org/jfx/pull/2187#discussion_r3459204365 PR Review Comment: https://git.openjdk.org/jfx/pull/2187#discussion_r3459231907
