On Mon, 14 Oct 2024 06:33:52 GMT, Abhishek Kumar <abhis...@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
