Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8266249: javax/swing/JPopupMenu/7156657/bug7156657.java fails on macOS #3844

Closed
wants to merge 4 commits into from

Conversation

azuev-java
Copy link
Member

@azuev-java azuev-java commented May 3, 2021

Fixed popup position taking into account its offset
Added a lot of screenshots taking to better understand failures should they happen down the line


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8266249: javax/swing/JPopupMenu/7156657/bug7156657.java fails on macOS

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3844/head:pull/3844
$ git checkout pull/3844

Update a local copy of the PR:
$ git checkout pull/3844
$ git pull https://git.openjdk.java.net/jdk pull/3844/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3844

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3844.diff

Take into account screen offset of the popup
Added saving of the screenshots in case of test failure
@bridgekeeper
Copy link

bridgekeeper bot commented May 3, 2021

👋 Welcome back kizune! 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 openjdk bot added the rfr Pull request is ready for review label May 3, 2021
@openjdk
Copy link

openjdk bot commented May 3, 2021

@azuev-java The following label will be automatically applied to this pull request:

  • swing

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.

@openjdk openjdk bot added the swing client-libs-dev@openjdk.org label May 3, 2021
@mlbridge
Copy link

mlbridge bot commented May 3, 2021

Webrevs

@pankaj-bansal
Copy link

As this is an intermittent failure (failed once in 40 iterations as mentioned in JBS), can you please run multiple iterations of the test in CI system without JTREG_RETRY_COUNT option to confirm there is no failure now?

@azuev-java
Copy link
Member Author

As this is an intermittent failure (failed once in 40 iterations as mentioned in JBS), can you please run multiple iterations of the test in CI system without JTREG_RETRY_COUNT option to confirm there is no failure now?

Added test run link to the bug.

Copy link
Member

@jayathirthrao jayathirthrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I had doubt whether we will have write access to test path to create new files in our CI systems, but it looks like we can. Verified it by making the test fail purposefully in our CI.

@openjdk
Copy link

openjdk bot commented May 7, 2021

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

8266249: javax/swing/JPopupMenu/7156657/bug7156657.java fails on macOS

Reviewed-by: jdv, pbansal, azvegint

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

  • 3af4efd: 8265291: Error in Javadoc for doAccessibleAction API in AccessibleJSlider class
  • be4f25b: 8266369: (se) Add wepoll based Selector
  • ff77ca8: 8266675: Optimize IntHashTable for encapsulation and ease of use
  • 04fad70: 8266765: [BACKOUT] JDK-8255493 Support for pre-generated java.lang.invoke classes in CDS dynamic archive
  • 0790e60: 8196743: jstatd doesn't see new Java processes inside Docker container
  • c6aa8f1: 8232644: bugs in serialized-form.html
  • b5b3119: 8266589: (fs) Improve performance of Files.copy() on macOS using copyfile(3)
  • 947d69d: 8265042: javadoc HTML files not generated for types nested in records
  • 946b0fe: 8266645: javac should not check for sealed supertypes in intersection types
  • 74fecc0: 8266503: [UL] Make Decorations safely copy-able and reduce their size
  • ... and 80 more: https://git.openjdk.java.net/jdk/compare/880c138b587e0902cd19c27a02baf41b57ac0bb0...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 openjdk bot added the ready Pull request is ready to be integrated label May 7, 2021
@prsadhuk
Copy link
Contributor

prsadhuk commented May 7, 2021

I'm not sure about this change...Maybe changing frame location from (0,0) at l192 to middle of screen though selLocationRelativeTo() may be enough to make the test more stable in CI systems.

@azuev-java
Copy link
Member Author

azuev-java commented May 7, 2021

I'm not sure about this change...Maybe changing frame location from (0,0) at l192 to middle of screen though selLocationRelativeTo() may be enough to make the test more stable in CI systems.

The problem here is not only in location but in the screenshot taking area outside of the popup and analyzing it. That is one of the reasons why test is unreliable - it analyzes the wrong place. And moving frame to the different position will not help either - the problem is not that system menubar overlaps the popup - it does not, the problem is that we are catching part of the menubar because we do not adjust for the offset it introduces to the screen coordinates. Here's the screenshot, note that area we are analyzing is one outlined with cyan borders. By not adjusting the location we just catching random stuff.
dev0scr0

@prsadhuk
Copy link
Contributor

prsadhuk commented May 8, 2021

I'm not sure about this change...Maybe changing frame location from (0,0) at l192 to middle of screen though selLocationRelativeTo() may be enough to make the test more stable in CI systems.

The problem here is not only in location but in the screenshot taking area outside of the popup and analyzing it. That is one of the reasons why test is unreliable - it analyzes the wrong place. And moving frame to the different position will not help either - the problem is not that system menubar overlaps the popup - it does not, the problem is that we are catching part of the menubar because we do not adjust for the offset it introduces to the screen coordinates. Here's the screenshot, note that area we are analyzing is one outlined with cyan borders. By not adjusting the location we just catching random stuff.
dev0scr0

OK...I will still urge to bring the frame to middle to be more safe.
BTW, greenFrame and redFrame screencapture are needed only in failure case so I guess those should be called if compare fails.
Also, dispose should be called in EDT same way we are doing a little bit afterwards..

@azuev-java
Copy link
Member Author

azuev-java commented May 8, 2021

I will still urge to bring the frame to middle to be more safe.

Ok, not that it makes any difference so there you go.

BTW, greenFrame and redFrame screencapture are needed only in failure case so I guess those should be called if compare fails.

Well, when comparison fails it is kind of too late to get "Before" and "After" screenshots, because "Before" is no longer there and "After" can also not be the same. And i'm saving these screenshots only if comparison fails so the performance impact is negligible.

Also, dispose should be called in EDT same way we are doing a little bit afterwards..

Ok, fixed.

@azuev-java
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this May 10, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 10, 2021
@openjdk
Copy link

openjdk bot commented May 10, 2021

@azuev-java Since your change was applied there have been 90 commits pushed to the master branch:

  • 3af4efd: 8265291: Error in Javadoc for doAccessibleAction API in AccessibleJSlider class
  • be4f25b: 8266369: (se) Add wepoll based Selector
  • ff77ca8: 8266675: Optimize IntHashTable for encapsulation and ease of use
  • 04fad70: 8266765: [BACKOUT] JDK-8255493 Support for pre-generated java.lang.invoke classes in CDS dynamic archive
  • 0790e60: 8196743: jstatd doesn't see new Java processes inside Docker container
  • c6aa8f1: 8232644: bugs in serialized-form.html
  • b5b3119: 8266589: (fs) Improve performance of Files.copy() on macOS using copyfile(3)
  • 947d69d: 8265042: javadoc HTML files not generated for types nested in records
  • 946b0fe: 8266645: javac should not check for sealed supertypes in intersection types
  • 74fecc0: 8266503: [UL] Make Decorations safely copy-able and reduce their size
  • ... and 80 more: https://git.openjdk.java.net/jdk/compare/880c138b587e0902cd19c27a02baf41b57ac0bb0...master

Your commit was automatically rebased without conflicts.

Pushed as commit 9b76955.

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

@@ -103,24 +109,56 @@ public void run() {
Rectangle popupRectangle = Util.invokeOnEDT(new Callable<Rectangle>() {
@Override
public Rectangle call() throws Exception {
return popupMenu.getBounds();
return new Rectangle(popupMenu.getLocationOnScreen(),
popupMenu.getSize());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that the getBounds method returns the wrong coordinate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per documentation - it returns coordinates related to the parent. Parent was a frameless window that we asked to go to 0,0 which means that parent coordinates == screen coordinates. With one exception - on Mac OS the menu bar is on the top and the window got shifted by the insets introduced by it. So, yes, in this case coordinates were incorrect. Or rather we used the wrong method to retrieve them.

@azuev-java azuev-java deleted the JDK-8266249 branch April 7, 2022 18:00
@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated swing client-libs-dev@openjdk.org
6 participants