RFR: 8341982: Simplify JButton/bug4323121.java

[Alexey Ivanov]

On Mon, 14 Oct 2024 06:33:52 GMT, Abhishek Kumar <abhiscxk at openjdk.org> wrote:

>> The test `javax/swing/JButton/bug4323121.java` contains lots of unused methods.
>> 
>> I removed all the unused methods by extending `MouseAdapter`.
>> 
>> I use `CountDownLatch` to synchronise actions in the test.
>> 
>> The test still verifies `button.getModel().isArmed()` doesn't always return `true` for classes which extend `JButton`. I verified the updated test fails in 1.3.0 and passes in 1.4.0, so the test still reproduces the original problem.
>
> test/jdk/javax/swing/JButton/bug4323121.java line 58:
> 
>> 56:     private static final CountDownLatch mouseEntered = new CountDownLatch(1);
>> 57: 
>> 58:     // Thread-safe by using the mouseEntered latch
> 
> Comment may be put before `private static final CountDownLatch mouseEntered = new CountDownLatch(1);`

Why?

This comment is meant for the `modelArmed` flag. The way it's used now is thread-safe because of using `mouseEntered`, specifically a value is written to the flag before `mouseEntered.countDown()` is called, and its value is read after `mouseEntered.await` returns.

If any of the above is modified, the `modelArmed` flag may become not thread-safe and require some kind of synchronisation.

> test/jdk/javax/swing/JButton/bug4323121.java line 69:
> 
>> 67:             SwingUtilities.invokeAndWait(() -> {
>> 68:                 button = new TestButton("gotcha");
>> 69:                 button.addMouseMotionListener(eventHandler);
> 
> Don't think it is required anymore to add `MouseMotionListener`.

I just kept it because it was there. When the event handler extends `MouseAdapter`, there are no additional methods to override.

However, you're right, `MouseMotionListener` is not needed.

> test/jdk/javax/swing/JButton/bug4323121.java line 70:
> 
>> 68:                 button = new TestButton("gotcha");
>> 69:                 button.addMouseMotionListener(eventHandler);
>> 70:                 button.addMouseListener(eventHandler);
> 
> You may get rid of `eventHandler` object (provided MouseMotionListener is not required) by adding the mouse listener similar to WindowFocusListener where you used WindowAdapter class and override WindowGainedFocus method. 
>  
>             button.addMouseListener(new MouseAdapter() {
>                     @Override
>                     public void mouseEntered(MouseEvent e) {
>                         if (button.getModel().isArmed()) {
>                             modelArmed = true;
>                         }
>                         mouseEntered.countDown();
>                     }
>                 });

Yes, it's an option.

I used the same approach in #21474 where the test object serves as an event handler. This avoids anonymous classes which add indentation.

> test/jdk/javax/swing/JButton/bug4323121.java line 88:
> 
>> 86:             });
>> 87: 
>> 88:             if (!windowGainedFocus.await(1, SECONDS)) {
> 
> Should we add a small delay before checking?

It *has* a delay of 1 second: if the event is not delivered within 1 second, the test will fail; it the event is delivered, the test continues.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21475#discussion_r1799573403
PR Review Comment: https://git.openjdk.org/jdk/pull/21475#discussion_r1799680813
PR Review Comment: https://git.openjdk.org/jdk/pull/21475#discussion_r1799686149
PR Review Comment: https://git.openjdk.org/jdk/pull/21475#discussion_r1799687977