On Wed, 20 Mar 2024 00:32:17 GMT, Alexander Zvegintsev <[email protected]>
wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8328560
>
> test/jdk/java/awt/event/MouseEvent/ClickDuringKeypress/ClickDuringKeypress.java
> line 25:
>
>> 23:
>> 24: /*
>> 25: @test 1.2 98/08/05
>
> Now we don't have `@test` at all.
oops, fixed
> test/jdk/java/awt/event/MouseEvent/ClickDuringKeypress/ClickDuringKeypress.java
> line 29:
>
>> 27: @summary Tests that clicking mouse and pressing keys generates correct
>> amount of click-counts
>> 28: @run main ClickDuringKeypress
>> 29: */
>
> We usually move this block just before a class declaration, for better
> readability.
ok
> test/jdk/java/awt/event/MouseEvent/ClickDuringKeypress/ClickDuringKeypress.java
> line 53:
>
>> 51: static volatile Frame frame;
>> 52: static volatile Robot robot;
>> 53: static volatile ClickDuringKeypress clicker;
>
> Suggestion:
>
> static final ClickDuringKeypress clicker = new ClickDuringKeypress();
>
> `clicker` can be made final
ok
> test/jdk/java/awt/event/MouseEvent/ClickDuringKeypress/ClickDuringKeypress.java
> line 61:
>
>> 59: robot = new Robot();
>> 60: robot.mouseMove(200, 200);
>> 61: EventQueue.invokeAndWait(frame::show);
>
> Suggestion:
>
> EventQueue.invokeAndWait(() -> frame.setVisible(true));
>
>
> `show` is deprecated.
yeah I know, I changed this as you suggested but
(1) I like the :: syntax, (2) I never thought it was necessary to deprecate
show()
> test/jdk/java/awt/event/MouseEvent/ClickDuringKeypress/ClickDuringKeypress.java
> line 96:
>
>> 94: throw new RuntimeException("Not Activated. Test fails");
>> 95: }
>> 96: }
>
> Do we really need this synchronization?
> Shouldn't the standard
>
> robot.waitForIdle();
> robot.delay(1000);
>
> be enough?
OK .. changed .. but I'm now a long way from removing an un-used import as a
lot of this test is now quite different than before.
> test/jdk/java/awt/event/MouseEvent/ClickDuringKeypress/ClickDuringKeypress.java
> line 111:
>
>> 109: for (int i = 0; i < CLICKCOUNT / 2; i++) {
>> 110: robot.mousePress(InputEvent.BUTTON1_DOWN_MASK);
>> 111: robot.delay(10);
>
> Here and after
> Suggestion:
>
>
>
> We have already called `robot.setAutoDelay(DOUBLE_CLICK_AUTO_DELAY);` before.
> We can set `DOUBLE_CLICK_AUTO_DELAY` to 20 and remove the multiple
> `robot.delay(10)` calls.
>
> Probably we also want to add `setAutoWaitForIdle(true)`
ok
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18386#discussion_r1532506155
PR Review Comment: https://git.openjdk.org/jdk/pull/18386#discussion_r1532510812
PR Review Comment: https://git.openjdk.org/jdk/pull/18386#discussion_r1532510509
PR Review Comment: https://git.openjdk.org/jdk/pull/18386#discussion_r1532508651
PR Review Comment: https://git.openjdk.org/jdk/pull/18386#discussion_r1532509878
PR Review Comment: https://git.openjdk.org/jdk/pull/18386#discussion_r1532510220
