8341982: Simplify JButton/bug4323121.java #21475

aivanov-jdk · 2024-10-11T18:20:52Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8341982: Simplify JButton/bug4323121.java (Bug - P4)

Reviewers

Harshitha Onkar (@honkar-jdk - Reviewer) Review applies to 8759e936
Damon Nguyen (@DamonGuy - Committer) Review applies to 128d7bef
Alisen Chung (@alisenchung - Committer) Review applies to 128d7bef
Abhishek Kumar (@kumarabhi006 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21475/head:pull/21475
$ git checkout pull/21475

Update a local copy of the PR:
$ git checkout pull/21475
$ git pull https://git.openjdk.org/jdk.git pull/21475/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21475

View PR using the GUI difftool:
$ git pr show -t 21475

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21475.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-11T18:21:26Z

👋 Welcome back aivanov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-10-11T18:21:53Z

@aivanov-jdk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8341982: Simplify JButton/bug4323121.java

Reviewed-by: abhiscxk, honkar, dnguyen, achung

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-10-11T18:22:33Z

@aivanov-jdk The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-10-11T18:25:40Z

Webrevs

test/jdk/javax/swing/JButton/bug4323121.java

DamonGuy

Test looks good still. Changes made do clean up the look of the test's code a lot.

kumarabhi006 · 2024-10-18T12:02:49Z

test/jdk/javax/swing/JButton/bug4323121.java

+    private static final CountDownLatch mouseEntered = new CountDownLatch(1);
+
+    // Usage of this flag is thread-safe because of using the mouseEntered latch
+    private static boolean modelArmed;


should it be volatile?

No, it shouldn't: “this flag is thread-safe because of using the mouseEntered latch”.

test/jdk/javax/swing/JButton/bug4323121.java

Co-authored-by: Abhishek Kumar <abhishek.cx.kumar@oracle.com>

kumarabhi006

Verified the test and look good to me.

mrserb · 2024-10-18T16:36:59Z

test/jdk/javax/swing/JButton/bug4323121.java

+                throw new RuntimeException("Window didn't gain focus");
+            }
            robot.waitForIdle();
-            robot.delay(1000);


The new code probably should follow the same pattern, flush all events/commands to native by the waitForIdle. and then wait response from OS for 1_or_less second.

I reversed the order because window may gain focus before all the events are processed.

To ensure all the events in the queue are handled, I added robot.waitForIdle().

The next action in the test is to get location of the button, which can be done safely since the frame must be displaying on the screen because it has already received windowGainedFocus event.

For this reason, there's no need for robot.delay(1000).

This test follows the same pattern that I used in #21474:

https://github.com/openjdk/jdk/pull/21474/files#diff-2a8cee50408407d7e24de4cb85f0921cfc5db7c215d3b29cf101a4050323e9a9R92

To ensure all the events in the queue are handled, I added robot.waitForIdle().

This will flush events which were posted after the windows gained focus, but before the patch the test flushed all events (including event to show the window which might be asynchronous). My point is that this operation might be longer that 1 second(one of the reason we have all that deleyas). So we should waitForIdle first, then wait via delay or other sync_ops.

This will flush all the pending events in the queue, which includes events that were posted before the windowGainedFocus latch is released as well as events are posted afterwards.

There's no difference whether I place waitForIdle before or after.

If I place robot.waitForIdle before windowGainedFocus.await, then await immediately returns, that is the delay added by await is zero.

If I place robot.waitForIdle after windowGainedFocus.await, the main thread stops until the window gained focus event is delivered, after which robot.waitForIdle flushes the remaining events in the queue.

In both cases, there's a slight possibility windowGainedFocus is never triggered if the window didn't get focus for whatever reason…

On my Windows 11 laptop, the average time (over 20 runs of the bug4323121 test)

for windowGainedFocus.await: 12 ms

for robot.waitForIdle: 36 ms

or cumulatively: 48 ms

If I change the order of calls,

for robot.waitForIdle: 51 ms

for windowGainedFocus.await: 0 ms

or cumulatively: 51 ms

Both versions align well — robot.waitForIdle takes about 50 ms. (The version where waitForIdle follows await is slightly quicker because await does nothing at all whereas waitForIdle posts events from its own copy of the queue for handling.

If there's no difference, I see no reason to complicate the code.

@mrserb Are any more concerns with the updated code in JButton/bug4323121.java?

@mrserb Are any more concerns with the updated code in JButton/bug4323121.java?

@mrserb Is there anything else you've got concerns about? If not, I'll integrate the PR.

honkar-jdk

LGTM

test/jdk/javax/swing/JButton/bug4323121.java

honkar-jdk · 2024-10-18T19:16:54Z

test/jdk/javax/swing/JButton/bug4323121.java

-        public void mouseClicked(MouseEvent e) {
        }
    }
+


Extra newline

I don't get where you wanted to add a blank line…

test/jdk/javax/swing/JButton/bug4323121.java

kumarabhi006

LGTM.

aivanov-jdk · 2024-12-04T13:28:45Z

/integrate

openjdk · 2024-12-04T13:29:54Z

Going to push as commit 6fa5cea.
Since your change was applied there have been 565 commits pushed to the master branch:

e13206d: 8345065: Cleanup DomainCombiner, SubjectDomainCombiner, Subject, and PrivilegedAction specifications
4000e92: 8343704: Bad GC parallelism with processing Cleaner queues
0c7451a: 8332686: InetAddress.ofLiteral can throw StringIndexOutOfBoundsException
56d315d: 8343540: Report preview error for inherited effectively-preview methods
994504c: 8329351: Add runtime/Monitor/TestRecursiveLocking.java for recursive Java monitor stress testing
3d49665: 8345286: Remove use of SecurityManager API from misc areas
38927fc: 8343213: TEST_BUG: [Graal] java/lang/ref/Basic.java fails
cf1eb58: 8344935: [ubsan]: javaThread.hpp:1241:52: runtime error: load of value 9831830, which is not a valid value for type 'freeze_result'
943aa03: 8345404: [ppc64le] ProblemList TestAllocOutOfMemory.java#large
9e2b66f: 8345178: RISC-V: Add gtests for narrow cmpxchg
... and 555 more: https://git.openjdk.org/jdk/compare/c0e6c3b93c0d21debc538e0135805c2957053108...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-04T13:30:01Z

@aivanov-jdk Pushed as commit 6fa5cea.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8341982: Simplify JButton/bug4323121.java

25c84b6

openjdk bot added the rfr Pull request is ready for review label Oct 11, 2024

openjdk bot added the client client-libs-dev@openjdk.org label Oct 11, 2024

kumarabhi006 reviewed Oct 14, 2024

View reviewed changes

aivanov-jdk added 2 commits October 14, 2024 16:28

Remove MouseMotionListener; get rid of eventHandler variable

5d1eeb6

Clarify thread-safety of the modelArmed flag

128d7be

honkar-jdk approved these changes Oct 15, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 15, 2024

DamonGuy approved these changes Oct 15, 2024

View reviewed changes

alisenchung approved these changes Oct 15, 2024

View reviewed changes

aivanov-jdk added 2 commits October 18, 2024 12:34

Inline mouseEntered listener for the button

25508f7

Amend indentation in the finally block

f32b1f5

openjdk bot removed the ready Pull request is ready to be integrated label Oct 18, 2024

aivanov-jdk requested a review from honkar-jdk October 18, 2024 11:40

kumarabhi006 reviewed Oct 18, 2024

View reviewed changes

Remove space before comma

8759e93

Co-authored-by: Abhishek Kumar <abhishek.cx.kumar@oracle.com>

kumarabhi006 approved these changes Oct 18, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 18, 2024

mrserb reviewed Oct 18, 2024

View reviewed changes

honkar-jdk approved these changes Oct 18, 2024

View reviewed changes

Use JButton directly instead of TestButton subclass

8e989b2

openjdk bot removed the ready Pull request is ready to be integrated label Oct 21, 2024

kumarabhi006 reviewed Oct 21, 2024

View reviewed changes

test/jdk/javax/swing/JButton/bug4323121.java Show resolved Hide resolved

test/jdk/javax/swing/JButton/bug4323121.java Outdated Show resolved Hide resolved

aivanov-jdk added 2 commits October 21, 2024 19:42

Get rid of windowGainedFocus latch

d16918a

Update summary and error message

d61e433

kumarabhi006 approved these changes Oct 22, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 22, 2024

Merge master

3558f0c

openjdk bot added the integrated Pull request has been integrated label Dec 4, 2024

openjdk bot closed this Dec 4, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 4, 2024

aivanov-jdk deleted the 8341982-JButton/bug4323121.java branch December 4, 2024 13:30

aivanov-jdk mentioned this pull request Jan 24, 2025

8342096: Popup menus that request focus are not shown on Linux with Wayland #22729

Closed

3 tasks

8341982: Simplify JButton/bug4323121.java #21475

8341982: Simplify JButton/bug4323121.java #21475

Uh oh!

Conversation

aivanov-jdk commented Oct 11, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 11, 2024

Uh oh!

openjdk bot commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 11, 2024

Uh oh!

mlbridge bot commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DamonGuy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kumarabhi006 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

honkar-jdk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kumarabhi006 left a comment

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk commented Dec 4, 2024

Uh oh!

openjdk bot commented Dec 4, 2024

Uh oh!

openjdk bot commented Dec 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

aivanov-jdk commented Oct 11, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 11, 2024 •

edited

Loading

mlbridge bot commented Oct 11, 2024 •

edited

Loading