On Sat, 2 Sep 2023 13:09:14 GMT, Marius Hanl <mh...@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: Set a min size

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

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

Do you plan to add more tests to this class?  The name does not specify which 
contract is being tested.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlContractTest.java
 line 46:

> 44: class ControlContractTest {
> 45: 
> 46:     private ControlStub control;

Do you think it's worth to run this test against all of the existing Controls?

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlContractTest.java
 line 67:

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

Could you please add 2.25 (I can see this one with the secondary monitor 
attached to my win11 box)

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlContractTest.java
 line 101:

> 99: 
> 100:                 double expectedWidth = 
> control.snapSizeX(control.getWidth()) - control.snappedLeftInset() - 
> control.snappedRightInset();
> 101:                 assertEquals(expectedWidth, w);

since we are performing floating point operations, we might accumulate small 
error.  
So this and 3 subsequent assertEquals() would need an EPSILON = 0.0000001 (an 
arbitrary value)

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1319158123
PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1319159680
PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1319150932
PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1319155511

Reply via email to