On Thu, 7 Sep 2023 22:04:46 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> 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.
Do you a better idea? I guess it would make sense to rename it to something
more generic. Maybe ControlLayoutTest?
> 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?
Since no `Control` is overriding the `layoutChildren` method, I don't think it
will make any difference as of now
> 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)
sure.
> 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)
Will have a look, but it worked without so I did not bothered adding an epsilon
to the tests
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1322547208
PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1322546539
PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1322548432
PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1322550303
