On Thu, 4 May 2023 09:56:18 GMT, Ravi Gupta <[email protected]> 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
