RFR: 8305427: Write a test Check whether focus changes as expected when requestFocus is used to traverse randomly around the window [v5]

[Alexey Ivanov]

On Thu, 4 May 2023 09:56:18 GMT, Ravi Gupta <duke at openjdk.org> wrote:

>> This testcase Checking whether the Component becoming the Focus Owner and FocusEvent.FOCUS_GAINED will be received by the Component when the focus is requested on the component using requestFocus.
>> 
>> Testing:
>> Tested using Mach5(20 times per platform) in macos,linux and windows and got all pass.
>
> Ravi Gupta has updated the pull request incrementally with one additional commit since the last revision:
> 
>   code modified

Changes requested by aivanov (Reviewer).

test/jdk/java/awt/Focus/RequestFocusOwnerTest/RequestFocusOwnerTest.java line 58:

> 56:     private volatile static TextField textField;
> 57:     private volatile static Checkbox checkbox;
> 58:     private volatile static List list;

@DamonGuy also asked why these particular components were chosen. And it looks some of them are non-focusable.

test/jdk/java/awt/Focus/RequestFocusOwnerTest/RequestFocusOwnerTest.java line 71:

> 69:         new CountDownLatch(1);
> 70: 
> 71:     private volatile static FocusListener listener = new FocusListener() {

Why are all listeners volatile?
And why is this listener is just listener rather than a specific one as others?

test/jdk/java/awt/Focus/RequestFocusOwnerTest/RequestFocusOwnerTest.java line 97:

> 95:     };
> 96: 
> 97:     private volatile static FocusListener butonListener = new FocusListener() {

Suggestion:

    private volatile static FocusListener buttonListener = new FocusListener() {

test/jdk/java/awt/Focus/RequestFocusOwnerTest/RequestFocusOwnerTest.java line 170:

> 168:             });
> 169: 
> 170:             if (!button.isFocusOwner()) {

If you carefully call component methods on EDT, then `isFocusOwner` should also be called on EDT.

And before calling this method, you should still call `robot.waitForIdle()` — the `autoWaitForIdle` property works only when you call Robot API.

test/jdk/java/awt/Focus/RequestFocusOwnerTest/RequestFocusOwnerTest.java line 184:

> 182: 
> 183:             EventQueue.invokeAndWait(() -> {
> 184:                 checkbox.requestFocusInWindow();

@DamonGuy  asked the question:

> Question: The title and bug description mentions testing requestFocus but this test uses requestFocusInWindow. Are you actually testing the right thing here?

It's still unanswered. It's also unclear to me.

Are you testing `requestFocus` or `requestFocusInWindow`?

test/jdk/java/awt/Focus/RequestFocusOwnerTest/RequestFocusOwnerTest.java line 234:

> 232:                     + comp);
> 233:         }
> 234:         if (!comp.isFocusOwner()) {

The `isFocusOwner` method needs to be called on EDT. However, the latch synchronises the two threads, so the call to `isFocusOwner` on the main thread should return the same result.

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

PR Review: https://git.openjdk.org/jdk/pull/13293#pullrequestreview-1503857212
PR Review Comment: https://git.openjdk.org/jdk/pull/13293#discussion_r1245694846
PR Review Comment: https://git.openjdk.org/jdk/pull/13293#discussion_r1245678449
PR Review Comment: https://git.openjdk.org/jdk/pull/13293#discussion_r1245692361
PR Review Comment: https://git.openjdk.org/jdk/pull/13293#discussion_r1245683882
PR Review Comment: https://git.openjdk.org/jdk/pull/13293#discussion_r1245689251
PR Review Comment: https://git.openjdk.org/jdk/pull/13293#discussion_r1245691630