[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: JDK-8315569: Tests for the contract of SkinBase.layoutChildren(..) [v3]
From:       Andy Goryachev <angorya () openjdk ! org>
Date:       2023-09-22 17:22:21
Message-ID: LvQ5a98eE3qPNZgACJMF3-aTkEypOIfkaW30NHEbhe4=.6503b6e6-3b18-465f-97ea-381e91d2b98a () github ! com
[Download RAW message or body]

On Fri, 22 Sep 2023 10:11:12 GMT, Marius Hanl <mhanl@openjdk.org> wrote:

> > This PR adds a test that verifies the `SkinBase.layoutChildren(..)` method with \
> > different scales. While not explicitly documented, this method will receive the \
> > snapped and correctly calculated x, y, width and height values, so that children \
> > of the Control can be layouted correctly without requiring to do many more \
> > calculations regarding padding.
> 
> Marius Hanl has updated the pull request incrementally with one additional commit \
> since the last revision: 
> JDK-8315569: Improve test name and add 2.25 scale

looks good, with a few minor comments.  please let me know what you think.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlLayoutTest.java \
line 44:

> 42: import static org.junit.jupiter.api.Assertions.assertTrue;
> 43: 
> 44: class ControlLayoutTest {

1. should this class be public?
2. since no more tests are being planned here, perhaps we could rename it to be more \
specific (ControlLayoutTest still seems to be too generic to me), \
ControlLayoutChildrenContractTest or something like that? 3. could you please add a \
javadoc at the class level to explain what generate area this class tests?

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlLayoutTest.java \
line 69:

> 67:      */
> 68:     @ParameterizedTest
> 69:     @ValueSource(doubles = { 1.0, 1.25, 1.5, 1.75, 2.0, 2.25 })

also tried 0.333333, 0.00111, still looks good :-)

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlLayoutTest.java \
line 128:

> 126:         assertEquals(300, control.getHeight());
> 127:     }
> 128: 

extra newline?

-------------

PR Review: https://git.openjdk.org/jfx/pull/1229#pullrequestreview-1640365959
PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1334641544
PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1334645567
PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1334642702


[prev in list] [next in list] [prev in thread] [next in thread]