RFR: 8328560: java/awt/event/MouseEvent/ClickDuringKeypress/ClickDuringKeypress.java imports Applet [v2]

[Phil Race]

On Wed, 20 Mar 2024 00:32:17 GMT, Alexander Zvegintsev <azvegint at openjdk.org> 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