8341978: Improve JButton/bug4490179.java #21474

aivanov-jdk · 2024-10-11T16:54:20Z

At first, I noticed bug4490179.java didn't release mouse button 1, which could affect the following tests.

I started looking at the test and updated it to use CountDownLatch instead of delays.

@azvegint handled releasing mouse button 1 in #19339 (comment).

The updated version first verifies mouseButton3Released event is received and ensures actionPerformed is not received at this time.

Then mouse button is released; this should trigger two events: mouseButton1Released and actionPerformed. It is verified that these events are received.

I kept debugging prints, they help understanding which actions lead to which events.

The updated test reproduces the original, it is easily verified using Java 8; the updated test is stable in CI.

The javax/swing/JButton/bug4490179.java test is also modified in #20861, @mrserb, could you take a look? I was going to suggested factoring out the changes to this test from #20861, yet I modified it myself. I realised it when I started creating the PR.

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-8341978: Improve JButton/bug4490179.java (Bug - P4)

Reviewers

Alexander Zvegintsev (@azvegint - Reviewer)
@abdelhak-zaaim (no known openjdk.org user name / role)
Alisen Chung (@alisenchung - Committer)
Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21474

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-11T16:55:09Z

👋 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-11T16:55:32Z

@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:

8341978: Improve JButton/bug4490179.java

Use latch to synchronize actions in the test.
Verify mouseButton3Released does not trigger actionPerformed
while mouse button 1 is pressed.
Ensure mouse button 1 is released.
Verify releasing mouse button 1 triggers actionPerformed.

Reviewed-by: azvegint, achung, serb

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 65 new commits pushed to the master branch:

3da6890: 8338751: ConfigureNotify behavior has changed in KWin 6.2
4e703b2: 8340140: Open some dialog awt tests 3
b4ab290: 8331959: Update PKCS#11 Cryptographic Token Interface to v3.1
2853852: 8342332: [JVMCI] Export CompilerToVM::Data::dtanh
ed68096: 8341862: PPC64: C1 unwind_handler fails to unlock synchronized methods with LM_MONITOR
d4f0ba7: 8342439: Build failure after 8338023
44151f4: 8342145: File libCreationTimeHelper.c compile fails on Alpine
709914f: 8338023: Support two vector selectFrom API
c34fb2c: 8338883: Show warning when CreateCoredumpOnCrash set, but core dump will not happen
7625b29: 8329597: C2: Intrinsify Reference.clear
... and 55 more: https://git.openjdk.org/jdk/compare/7276a1bec0d90f63e9e433fdcdfd6564b70dc9bb...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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-11T16:56:06Z

@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.

aivanov-jdk · 2024-10-11T16:57:16Z

/summary
Use latch to synchronize actions in the test.
Verify mouseButton3Released does not trigger actionPerformed
while mouse button 1 is pressed.
Ensure mouse button 1 is released.
Verify releasing mouse button 1 triggers actionPerformed.

openjdk · 2024-10-11T16:58:07Z

@aivanov-jdk Setting summary to:

Use latch to synchronize actions in the test.
Verify mouseButton3Released does not trigger actionPerformed
while mouse button 1 is pressed.
Ensure mouse button 1 is released.
Verify releasing mouse button 1 triggers actionPerformed.

mlbridge · 2024-10-11T17:01:41Z

Webrevs

00: Full (3cfb7d5f)

alisenchung

just some formatting comments, test is working fine for me on macos

alisenchung · 2024-10-15T22:39:09Z

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

+
+    private static volatile Point buttonCenter;
+
+    private static final CountDownLatch windowGainedFocus = new CountDownLatch(1);


line here is too long and probably should be put in 2 lines

I don't think it should, it's 86 column long, it nearly fits the 80-column limit; I don't think wrapping the line improves readability or clarity.

alisenchung · 2024-10-15T22:39:39Z

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

+                robot.waitForIdle();
+
+                if (actionPerformed.await(100, MILLISECONDS)) {
+                    throw new RuntimeException("Action event triggered by releasing button 3");


line here also should be put into 2 lines

I'm reluctant about changing it… The PR is approved now. And the line still fits 100-column limit.

Yet here it wrapping the line has its value, the button number is at the end of the line.

I talked to @azvegint and we decided it's good enough. Wrapping the lines would require re-review.

aivanov-jdk · 2024-10-18T11:02:27Z

/integrate

openjdk · 2024-10-18T11:03:03Z

Going to push as commit 8174cbd.
Since your change was applied there have been 100 commits pushed to the master branch:

9201e9f: 8342409: [s390x] C1 unwind_handler fails to unlock synchronized methods with LM_MONITOR
0963b9e: 8341664: ReferenceClassDescImpl cache internalName
c51a086: 8339694: ciTypeFlow does not correctly handle unresolved constant dynamic of array type
7f4ed50: 8341020: Error handler crashes when Metaspace is not fully initialized
f50bd0d: 8341513: Remove the unused thread_type field from OSThread
7a16906: 8341134: Deprecate for removal the jrunscript tool
ffe6091: 8173970: jar tool should have a way to extract to a directory
2b03dbd: 8311530: Deprecate jdk.jsobject module for removal
f7e8f50: 8341292: Open some TextArea awt tests 3
fe83b7d: 8339341: SurfaceManager cacheMap retains strong references
... and 90 more: https://git.openjdk.org/jdk/compare/7276a1bec0d90f63e9e433fdcdfd6564b70dc9bb...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-18T11:03:13Z

@aivanov-jdk Pushed as commit 8174cbd.

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

8341978: Improve JButton/bug4490179.java

3cfb7d5

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

azvegint approved these changes Oct 11, 2024

View reviewed changes

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

abdelhak-zaaim approved these changes Oct 12, 2024

View reviewed changes

aivanov-jdk mentioned this pull request Oct 14, 2024

8341982: Simplify JButton/bug4323121.java #21475

Open

3 tasks

alisenchung approved these changes Oct 15, 2024

View reviewed changes

mrserb approved these changes Oct 17, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 18, 2024

openjdk bot closed this Oct 18, 2024

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

aivanov-jdk deleted the 8341978-JButton/bug4490179.java branch October 18, 2024 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8341978: Improve JButton/bug4490179.java #21474

8341978: Improve JButton/bug4490179.java #21474

aivanov-jdk commented Oct 11, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 11, 2024

openjdk bot commented Oct 11, 2024 •

edited

Loading

openjdk bot commented Oct 11, 2024

aivanov-jdk commented Oct 11, 2024

openjdk bot commented Oct 11, 2024

mlbridge bot commented Oct 11, 2024

alisenchung left a comment

alisenchung Oct 15, 2024

aivanov-jdk Oct 17, 2024 •

edited

Loading

alisenchung Oct 15, 2024

aivanov-jdk Oct 17, 2024

aivanov-jdk Oct 18, 2024

aivanov-jdk commented Oct 18, 2024

openjdk bot commented Oct 18, 2024

openjdk bot commented Oct 18, 2024


		private static volatile Point buttonCenter;

		private static final CountDownLatch windowGainedFocus = new CountDownLatch(1);

8341978: Improve JButton/bug4490179.java #21474

8341978: Improve JButton/bug4490179.java #21474

Conversation

aivanov-jdk commented Oct 11, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 11, 2024

openjdk bot commented Oct 11, 2024 • edited Loading

openjdk bot commented Oct 11, 2024

aivanov-jdk commented Oct 11, 2024

openjdk bot commented Oct 11, 2024

mlbridge bot commented Oct 11, 2024

Webrevs

alisenchung left a comment

Choose a reason for hiding this comment

alisenchung Oct 15, 2024

Choose a reason for hiding this comment

aivanov-jdk Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

alisenchung Oct 15, 2024

Choose a reason for hiding this comment

aivanov-jdk Oct 17, 2024

Choose a reason for hiding this comment

aivanov-jdk Oct 18, 2024

Choose a reason for hiding this comment

aivanov-jdk commented Oct 18, 2024

openjdk bot commented Oct 18, 2024

openjdk bot commented Oct 18, 2024

aivanov-jdk commented Oct 11, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 11, 2024 •

edited

Loading

aivanov-jdk Oct 17, 2024 •

edited

Loading