jfx icon indicating copy to clipboard operation
jfx copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

8181084: JavaFX show big icons in system menu on macOS with Retina display

Open paul-court opened this issue 2 years ago • 11 comments

I hit on JDK-8181084 while making some changes to Scene Builder, so I decided to investigate.

Please note: I have not done any Objective-C or MacOS development before. So I would really like some feedback from someone else who knows this stuff better.

Anyway, after some googling, I discovered that MacOS uses points values for measurements and not pixels, so the actual fix for this issue was this block in GlassMenu.m:-

            if ((sx > 1) && (sy > 1) && (width > 1) && (height > 1)) {
                NSSize imgSize = {width / sx, height / sy};
                [image setSize: imgSize];
            }

The rest of the changes were needed in order to get the scaleX and scaleY values down from PixelUtils.java into GlassMenu.m.

Before this fix:-

Screenshot 2022-02-26 at 22 16 30

After this fix:-

Screenshot 2022-02-26 at 22 18 17

Progress

  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue
  • [ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

  • JDK-8181084: JavaFX show big icons in system menu on macOS with Retina display

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 743

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/743.diff

paul-court avatar Feb 27 '22 11:02 paul-court

:wave: Welcome back gargoyle! 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.

bridgekeeper[bot] avatar Feb 27 '22 11:02 bridgekeeper[bot]

Webrevs

mlbridge[bot] avatar Feb 27 '22 11:02 mlbridge[bot]

/reviewers 2

kevinrushforth avatar Feb 28 '22 15:02 kevinrushforth

@kevinrushforth The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

openjdk[bot] avatar Feb 28 '22 15:02 openjdk[bot]

Made a minor change to some variable names to be more consistent with how those values are referenced elsewhere and pulled in the latest upstream changes.

Can anyone help me out with pushing this little tweak over the finishing line?

paul-court avatar May 08 '22 06:05 paul-court

I'll look at this in more detail later. You will likely need some error checking when calling the JNI methods (even though they should not ever return an error), and you might want to cache the class and field IDs it in initIDs.

This will need a testing on all platforms, since the change to create a Pixel object using the scale is in shared code. Two things you could do to help with this.

  1. Enable GitHub actions runs in your personal fork of the jfx repo. You will need to push some change (and empty commit is fine) to trigger it, once you do. This will run the headless tests on all platforms, so is really just a "smoke test".
  2. Run the full set of tests on at least macOS by doing this:
gradle --continue --info -PFULL_TEST=true -PUSE_ROBOT=true test -x :web:test

kevinrushforth avatar May 12 '22 23:05 kevinrushforth

Hello again, been too busy with work for the last few months, but I'm hoping to push this over the line for JFX 20.

I've published my test app over at:- https://github.com/gargoyle/reimagined-octo-succotash with some instructions, and it all seems to work fine on my Linux desktop.

However, I cannot get JFX to build on my cleanly re-installed Mac running Big Sur (11.6.7) and openjdk 18.0.2, so I've hit a bit of a wall. :-(

The error is in building ccMacGlass and seems to be complaining about methods not returning NSString.

Just to close this NSString error bit off, this was because I only had XCode command line tools installed, and my request to install full XCode had quietly hung in the background without any kind of error. The same happened on a few retries, but I eventually fixed it by doing a "safe boot" and retrying the installation.

Building and testing is working on my Mac again now! :-)

paul-court avatar Jul 23 '22 10:07 paul-court

Thanks for all the feedback. I've updated as per your suggestions @jperedadnr and @kevinrushforth. Let me know how this looks to you - I'm learning a lot here :-)

I ran the tests as per your example and got 802 tests completes, 28 failed, 108 skipped, which is exactly the same as when I run them on the master branch. Is there something I should be doing on a Mac to get zero failures? (Although, it's a lot better than my Linux box which gets 200 failed!)

paul-court avatar Jul 25 '22 18:07 paul-court

@gargoyle To clarify my inline comments, the minimal change you need to make is:

  1. Change the tests of the four jField variables to remove the > 0 (so it becomes a boolean test for non-null).
  2. Change the tests of width and height to be > 0 (rather than > 1)

My other suggestions are optional.

kevinrushforth avatar Aug 03 '22 15:08 kevinrushforth

Thanks @kevinrushforth. Thinking about it, is there merit in changing the scalex and scaley checks to be != 0, as The main reason for checking at all is to prevent a division by zero. Although unlikely, could fractional values < 1 reach this code?

paul-court avatar Aug 04 '22 08:08 paul-court

Thinking about it, is there merit in changing the scalex and scaley checks to be != 0, as The main reason for checking at all is to prevent a division by zero. Although unlikely, could fractional values < 1 reach this code?

I doubt that a scale < 1 is possible on Mac, although we don't preclude it in other places in the code. Negative scales would be a problem, so a > 0 check seems fine.

kevinrushforth avatar Aug 04 '22 22:08 kevinrushforth

@gargoyle have you decided whether to change the scale tests to be > 0? Other than that pending question, the only required changes are the two I listed above.

kevinrushforth avatar Aug 16 '22 21:08 kevinrushforth

I'll leave the scalex/y checks as they are, otherwise it will require two more if checks in the block for a situation which is almost certainly not going to happen. I've made the other changes you suggested.

paul-court avatar Aug 17 '22 07:08 paul-court

@jperedadnr can you re-review (only minor change since you last approved it)?

kevinrushforth avatar Aug 17 '22 12:08 kevinrushforth

:warning: @gargoyle the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout JDK-8181084
$ git commit --author='Preferred Full Name <[email protected]>' --allow-empty -m 'Update full name'
$ git push

openjdk[bot] avatar Aug 17 '22 13:08 openjdk[bot]

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

8181084: JavaFX show big icons in system menu on macOS with Retina display

Reviewed-by: jpereda, kcr

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

  • eaddb0fbeeb99900636f9704758f6c004860ff9a: 8291908: VirtualFlow creates unneeded empty cells
  • 33534cb9e0524e91ed717b22fab667b284c52252: 8291630: Update attribution in webkit.md file
  • 38324a70c3054e75cb22865e7ffcc5375b62939d: 8291087: Wrong position of focus of screen reader on Windows with screen scale > 1
  • b4d86bdffc2dd89e3473884b4079092e7e2843d1: 8218826: TableRowSkinBase: horizontal layout broken if row has padding
  • 4c1f4c20b99dfc8c8016c305983dfea5e1b9d6f2: Merge
  • dd30402a5c22daf79b88dd35b42f32d0a0e28867: 8291589: Update copyright header for files modified in 2022
  • 779365f1af8dc837d7b6d292de5bd8dcd8947290: Merge
  • 5febacae1bf6776a31e151ef223739576dab67e6: 8291502: Mouse or touch presses on a non-focusable region don't clear the focusVisible flag of the current focus owner
  • 8fa89199e2d07763a3aaab3f3a7904c73457d4a0: 8291588: Update boot JDK to 18.0.2
  • 08ec9c8781a57b49a13a2b7febbe33172ebc1a5a: 8289605: Robot capture tests fail intermittently on Mac M1
  • ... and 7 more: https://git.openjdk.org/jfx/compare/9f636da27576b69f797d923599bd4ae4b4d37be8...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jperedadnr, @kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk[bot] avatar Aug 17 '22 13:08 openjdk[bot]

/integrate

paul-court avatar Aug 17 '22 18:08 paul-court

@gargoyle Your change (at version e9671f82ad8066a74408a8a264ab7288e21b63aa) is now ready to be sponsored by a Committer.

openjdk[bot] avatar Aug 17 '22 18:08 openjdk[bot]

/sponsor

kevinrushforth avatar Aug 17 '22 18:08 kevinrushforth

Going to push as commit 2618bf8aeef3c9d9d923576e8a610f5e9b2123f1. Since your change was applied there have been 17 commits pushed to the master branch:

  • eaddb0fbeeb99900636f9704758f6c004860ff9a: 8291908: VirtualFlow creates unneeded empty cells
  • 33534cb9e0524e91ed717b22fab667b284c52252: 8291630: Update attribution in webkit.md file
  • 38324a70c3054e75cb22865e7ffcc5375b62939d: 8291087: Wrong position of focus of screen reader on Windows with screen scale > 1
  • b4d86bdffc2dd89e3473884b4079092e7e2843d1: 8218826: TableRowSkinBase: horizontal layout broken if row has padding
  • 4c1f4c20b99dfc8c8016c305983dfea5e1b9d6f2: Merge
  • dd30402a5c22daf79b88dd35b42f32d0a0e28867: 8291589: Update copyright header for files modified in 2022
  • 779365f1af8dc837d7b6d292de5bd8dcd8947290: Merge
  • 5febacae1bf6776a31e151ef223739576dab67e6: 8291502: Mouse or touch presses on a non-focusable region don't clear the focusVisible flag of the current focus owner
  • 8fa89199e2d07763a3aaab3f3a7904c73457d4a0: 8291588: Update boot JDK to 18.0.2
  • 08ec9c8781a57b49a13a2b7febbe33172ebc1a5a: 8289605: Robot capture tests fail intermittently on Mac M1
  • ... and 7 more: https://git.openjdk.org/jfx/compare/9f636da27576b69f797d923599bd4ae4b4d37be8...master

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Aug 17 '22 18:08 openjdk[bot]

@kevinrushforth @gargoyle Pushed as commit 2618bf8aeef3c9d9d923576e8a610f5e9b2123f1.

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

openjdk[bot] avatar Aug 17 '22 18:08 openjdk[bot]