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

7124313: [macosx] Swing Popups should overlap taskbar #9294

Closed
wants to merge 1 commit into from

Conversation

prsadhuk
Copy link
Contributor

@prsadhuk prsadhuk commented Jun 27, 2022

In macos, popups does not overlap taskbar unlike windows, as can be seen here in "Settings"
so we can omit this test from macOS run.

image


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-7124313: [macosx] Swing Popups should overlap taskbar

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9294

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 27, 2022

👋 Welcome back psadhukhan! 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 Jun 27, 2022
@openjdk
Copy link

openjdk bot commented Jun 27, 2022

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

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Jun 27, 2022
@mlbridge
Copy link

mlbridge bot commented Jun 27, 2022

Webrevs

@mrserb
Copy link
Member

mrserb commented Jun 27, 2022

An initial bug 6580930 said that the menu should be big enough to overlap the taskbar. I tried a simple example and it seems that if the popup is big enough(hundred of items) it will overlap taskbar. please double check that.

@prsadhuk
Copy link
Contributor Author

prsadhuk commented Jun 28, 2022

An initial bug 6580930 said that the menu should be big enough to overlap the taskbar. I tried a simple example and it seems that if the popup is big enough(hundred of items) it will overlap taskbar. please double check that.

It says for windows and linux only

In native applications for Windows, if a popup is big enough
it can overlap the taskbar (see the image attached)
the same behaiviour can be seen for GTK on *nix

For the existing testcase, it overlaps the taskbar in windows/linux but it does not for macos so I have excluded for macos.
I dont see any native macos popup overlap the taskbar..
if we have to put hundred of items to overlap the taskbar, it might be a bug in macos which needs to be dealt with separately but this testcase, as it is, is not applicable for macos.

Also, if we put hundred of items in popup , it fails in windows/linux so i wish not to tinker with this existing testcase

@mrserb
Copy link
Member

mrserb commented Jun 28, 2022

It says for windows and linux only

Are you sure? It has macOS specific code added as part of the https://bugs.openjdk.org/browse/JDK-8196096

Also, if we put hundred of items in popup , it fails in windows/linux so i wish not to tinker with this existing testcase

I think for the large menu it was fixed by the changed mentioned here So now the popup menu overlap the taskbar on macOS if it is big enough and will not be located under it.

@prsadhuk
Copy link
Contributor Author

Are you sure? It has macOS specific code added as part of the https://bugs.openjdk.org/browse/JDK-8196096

It deproblemlist it for windows. macos specific code was added to check for heavyweight poup in non-macos platform which we dont have for macos for which have existing bug JDK-7184956

@prsadhuk
Copy link
Contributor Author

I think for the large menu it was fixed by the changed mentioned here So now the popup menu overlap the taskbar on macOS if it is big enough and will not be located under it.

Dont think so. The comment was made on the same day this bug was filed so it was not working in macos even then.

@mrserb
Copy link
Member

mrserb commented Jun 28, 2022

Are you sure? It has macOS specific code added as part of the https://bugs.openjdk.org/browse/JDK-8196096

It deproblemlist it for windows. macos specific code was added to check for heavyweight poup in non-macos platform which we dont have for macos for which have existing bug JDK-7184956

That code was not added, it was excluded from executing on macOS. And as the reason for that excluded code the JDK-7184956 was referenced. But current failure is different. On my system it fails due to wrong "frame" location, it jumps from the "expected" location. In Swing that expected location is calculated by the JPopupMenu.adjustPopupLocationToFitScreen so we can mimic that logic in the test. Or we can test the large menu to check that the popup overlaps the dock.

@prsadhuk
Copy link
Contributor Author

Are you sure? It has macOS specific code added as part of the https://bugs.openjdk.org/browse/JDK-8196096

It deproblemlist it for windows. macos specific code was added to check for heavyweight poup in non-macos platform which we dont have for macos for which have existing bug JDK-7184956

That code was not added, it was excluded from executing on macOS. And as the reason for that excluded code the JDK-7184956 was referenced. But current failure is different. On my system it fails due to wrong "frame" location, it jumps from the "expected" location. In Swing that expected location is calculated by the JPopupMenu.adjustPopupLocationToFitScreen so we can mimic that logic in the test. Or we can test the large menu to check that the popup overlaps the dock.

I am not sure why we should tinker with existing testcase when the similar popupmenu with 7 items in "Settings" (as shown in bugfix description) does not overlap the taskbar in native.

@prrace Can we please ask Apple if this behaviour is a bug in Java that we dont overlap the taskbar for small popupmenu
or is it expected?

@mrserb
Copy link
Member

mrserb commented Jun 28, 2022

@prrace Can we please ask Apple if this behaviour is a bug in Java that we dont overlap the taskbar for small popupmenu or is it expected?

I do not think that this is an Apple bug, since we(our code) place that menu to the location similar to the native menu. But in java we still overlap it if the menu is big enough, it is just a different meaning of large menu from that bug.

@mrserb
Copy link
Member

mrserb commented Jun 28, 2022

Dont think so. The comment was made on the same day this bug was filed so it was not working in macos even then.

Seems it was related to that flag, but was fixed later: https://bugs.openjdk.org/browse/JDK-7154841

@prsadhuk
Copy link
Contributor Author

@prrace Can we please ask Apple if this behaviour is a bug in Java that we dont overlap the taskbar for small popupmenu or is it expected?

I do not think that this is an Apple bug, since we(our code) place that menu to the location similar to the native menu. But in java we still overlap it if the menu is big enough, it is just a different meaning of large menu from that bug.

I did not say it's an Apple bug..I am asking if Apple (or others?) thinks it's a Java bug
or is it expected behaviour similar to native
as what I am seeing in java, is the same as what native does for similar popup menu with similar number of items..

@prsadhuk
Copy link
Contributor Author

Dont think so. The comment was made on the same day this bug was filed so it was not working in macos even then.

Seems it was related to that flag, but was fixed later: https://bugs.openjdk.org/browse/JDK-7154841

That test works on macos for popup with many menu items and it overlaps the taskbar so why do we need another test (ie this test) to test the same on macos? I think we can omit this test from macos run as the other test test/jdk/javax/swing/JPopupMenu/7154841/bug7154841.java is testing the large popup behaviour in macos..

@mrserb
Copy link
Member

mrserb commented Jun 29, 2022

hat test works on macos for popup with many menu items and it overlaps the taskbar so why do we need another test (ie this test) to test the same on macos? I think we can omit this test from macos run as the other test test/jdk/javax/swing/JPopupMenu/7154841/bug7154841.java is testing the large popup behaviour in macos..

Since we found that fix, and the test that coverers the "large" use case the new test seems unnecessary.

@openjdk
Copy link

openjdk bot commented Jun 29, 2022

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

7124313: [macosx] Swing Popups should overlap taskbar

Reviewed-by: serb, dmarkov

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

  • b96ba19: 8289182: NMT: MemTracker::baseline should return void
  • 779b4e1: 8287001: Add warning message when fail to load hsdis libraries
  • 910053b: 8280235: Deprecated flag FlightRecorder missing from VMDeprecatedOptions test
  • 7b3bf97: 8289401: Add dump output to TestRawRSACipher.java
  • 86dc760: Merge
  • 1504804: 8289398: ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64 again
  • 9b7805e: 8289069: Very slow C1 arraycopy jcstress tests after JDK-8279886
  • c42b796: 8288058: Broken links on constant-values page
  • a814293: 8275784: Bogus warning generated for record with compact constructor
  • 6f9717b: 8288836: (fs) Files.writeString spec for IOException has "specified charset" when no charset is provided
  • ... and 175 more: https://git.openjdk.org/jdk/compare/fe807217a79753f84c00432e7451c17baa6645c5...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 Jun 29, 2022
@prsadhuk
Copy link
Contributor Author

prsadhuk commented Jul 7, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Jul 7, 2022

Going to push as commit 532a6ec.
Since your change was applied there have been 280 commits pushed to the master branch:

  • e05b2f2: 8289856: [PPC64] SIGSEGV in C2Compiler::init_c2_runtime() after JDK-8289060
  • cce77a7: 8289799: Build warning in methodData.cpp memset zero-length parameter
  • d1249aa: 8198668: MemoryPoolMBean/isUsageThresholdExceeded/isexceeded001/TestDescription.java still failing
  • a79ce4e: 8286941: Add mask IR for partial vector operations for ARM SVE
  • 569de45: 8289620: gtest/MetaspaceUtilsGtests.java failed with unexpected stats values
  • 403a9bc: 8289436: Make the redefine timer statistics more accurate
  • a40c17b: 8289775: Update java.lang.invoke.MethodHandle[s] to use snippets
  • 2a6ec88: Merge
  • 0526402: 8289477: Memory corruption with CPU_ALLOC, CPU_FREE on muslc
  • b3a0e48: 8289439: Clarify relationship between ThreadStart/ThreadEnd and can_support_virtual_threads capability
  • ... and 270 more: https://git.openjdk.org/jdk/compare/fe807217a79753f84c00432e7451c17baa6645c5...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jul 7, 2022
@openjdk openjdk bot closed this Jul 7, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 7, 2022
@openjdk
Copy link

openjdk bot commented Jul 7, 2022

@prsadhuk Pushed as commit 532a6ec.

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

@prsadhuk prsadhuk deleted the JDK-7124313 branch July 7, 2022 12:01
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
3 participants