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

8252015: [macos11] java.awt.TrayIcon requires updates for template images #481

Closed

Conversation

@petermz
Copy link

@petermz petermz commented Oct 2, 2020

Issue

JDK-8252015: [macos11] java.awt.TrayIcon requires updates for template images

Problem

According to Apple's human interface guidelines, developers should use template images for tray icons. This way icons look good when desktop theme or wallpaper change. On the API level, this means setting the NSImage::isTemplate flag. Currently there's no way in Java to set this flag. Even if one uses a template (black and transparent pixels only) image, MacOS will treat it as regular image. As a result, the image will not be visible when using dark theme.

Solution:

  • Add a new system property, java.awt.enableTemplateImages apple.awt.enableTemplateImages, to indicate that template images are being used by tray icons.When this property is set, all tray icon images are treated as templates. It's the developers' responsibility to ensure their images are indeed templates. This property allows us to avoid new API and accidental behavior changes
  • Value of this property is passed from Java into NSImage::setTemplate
  • We need the tray icon image rendered by MacOS, so we use a button (rather than a view) to host the image

To do after CSR approval:

  • Rename property apple.awt.enableTemplateImages
  • Add TrayIcon javadoc, use @systemProperty tag there

Progress

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

Issue

  • JDK-8252015: [macos11] java.awt.TrayIcon requires updates for template images

Reviewers

Contributors

  • Tres Finocchiaro <tres.finocchiaro@gmail.com>
  • Peter Zhelezniakov <peterz@openjdk.org>

Download

$ git fetch https://git.openjdk.java.net/jdk pull/481/head:pull/481
$ git checkout pull/481

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 2, 2020

👋 Welcome back peterz! 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 label Oct 2, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 2, 2020

@petermz The following label will be automatically applied to this pull request:

  • awt

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 awt label Oct 2, 2020
@petermz
Copy link
Author

@petermz petermz commented Oct 2, 2020

/contributor add @tresf

@petermz
Copy link
Author

@petermz petermz commented Oct 2, 2020

/contributor add @petermz

@openjdk
Copy link

@openjdk openjdk bot commented Oct 2, 2020

@petermz Could not parse @tresf as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

@openjdk
Copy link

@openjdk openjdk bot commented Oct 2, 2020

@petermz
Contributor Peter Zhelezniakov <peterz@openjdk.org> successfully added.

@petermz
Copy link
Author

@petermz petermz commented Oct 2, 2020

/contributor add Tres Finocchiaro tres.finocchiaro@gmail.com

@openjdk
Copy link

@openjdk openjdk bot commented Oct 2, 2020

@petermz
Contributor Tres Finocchiaro <tres.finocchiaro@gmail.com> successfully added.

@petermz
Copy link
Author

@petermz petermz commented Oct 2, 2020

/label add awt, macosx, trayicon

@openjdk
Copy link

@openjdk openjdk bot commented Oct 2, 2020

@petermz The label macosx is not a valid label. These labels are valid:

  • serviceability
  • hotspot
  • sound
  • hotspot-compiler
  • kulla
  • i18n
  • shenandoah
  • jdk
  • javadoc
  • 2d
  • security
  • swing
  • hotspot-runtime
  • jmx
  • build
  • nio
  • beans
  • core-libs
  • compiler
  • net
  • hotspot-gc
  • hotspot-jfr
  • awt

@petermz
Copy link
Author

@petermz petermz commented Oct 2, 2020

/test builds

@openjdk
Copy link

@openjdk openjdk bot commented Oct 2, 2020

@petermz you need to get approval to run the tests in builds for commits up until 8e38376

@openjdk openjdk bot added the test-request label Oct 2, 2020
@petermz
Copy link
Author

@petermz petermz commented Oct 2, 2020

/test tier1

@openjdk
Copy link

@openjdk openjdk bot commented Oct 2, 2020

@petermz you need to get approval to run the tests in tier1 for commits up until 8e38376

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 2, 2020

Webrevs

@petermz
Copy link
Author

@petermz petermz commented Oct 2, 2020

/contributor remove @petermz

@openjdk
Copy link

@openjdk openjdk bot commented Oct 2, 2020

@petermz
Contributor Peter Zhelezniakov <peterz@openjdk.org> successfully removed.

@petermz
Copy link
Author

@petermz petermz commented Oct 2, 2020

/contributor add @petermz

@openjdk
Copy link

@openjdk openjdk bot commented Oct 2, 2020

@petermz
Contributor Peter Zhelezniakov <peterz@openjdk.org> successfully added.

@mrserb
Copy link
Member

@mrserb mrserb commented Oct 5, 2020

Hi, Peter. You are welcome!
Just an initial question about the template images, can you please provide some steps to reproduce this bug and so it will be possible to check that the fix solves it.

@petermz
Copy link
Author

@petermz petermz commented Oct 6, 2020

Hi Sergey, and thanks for looking into this!

Try the attached jar, it's a modified version of TrayIconDemo using a template icon taken from Wikimedia Commons. Without the fix, or without -Dsun.awt.enableTemplateImages, the icon disappears if you switch to Dark theme. With the fix and the property set, the system repaints it in white.

TrayIconDemo.jar.zip

@mrserb
Copy link
Member

@mrserb mrserb commented Oct 28, 2020

/csr

@openjdk openjdk bot added the csr label Oct 28, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 28, 2020

@mrserb has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@petermz please create a CSR request and add link to it in JDK-8252015. This pull request cannot be integrated until the CSR request is approved.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 26, 2020

@petermz This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@tresf
Copy link
Contributor

@tresf tresf commented Nov 26, 2020

simply add a new comment to the pull request

✔️

@petermz
Copy link
Author

@petermz petermz commented Nov 26, 2020

Filed CSR request

@@ -83,17 +99,23 @@ -(void) dealloc {
// the item's view to nil: it can lead to a crash in some scenarios.
Copy link
Member

@mrserb mrserb Nov 30, 2020

Choose a reason for hiding this comment

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

This comment seems outdated, we either need to nullify the menu delegate or follow the old approach and update the comment on why the menuDelegate is not set to nil.

@@ -194,6 +194,8 @@ public void updateImage() {
}

void updateNativeImage(Image image) {
boolean imageTemplate = Boolean.parseBoolean(System.getProperty("sun.awt.enableTemplateImages", "false"));
Copy link
Member

@mrserb mrserb Nov 30, 2020

Choose a reason for hiding this comment

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

"sun.awt.enableTemplateImages" -> "apple.awt.enableTemplateImages"

Copy link
Member

@mrserb mrserb Nov 30, 2020

Choose a reason for hiding this comment

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

The impl block in the java.awt.TrayIcon class?

@mrserb
Copy link
Member

@mrserb mrserb commented Nov 30, 2020

Please check the tests in the .../test/jdk/java/awt/TrayIcon/ folder it looks like some of them fail after the fix:
java/awt/TrayIcon/ActionCommand/ActionCommand.java
java/awt/TrayIcon/ActionEventMask/ActionEventMask.java
java/awt/TrayIcon/MouseEventMask/MouseEventMaskTest.java
java/awt/TrayIcon/SecurityCheck/FunctionalityCheck/FunctionalityCheck.java
java/awt/TrayIcon/TrayIconEventModifiers/TrayIconEventModifiersTest.java
java/awt/TrayIcon/TrayIconEvents/TrayIconEventsTest.java
java/awt/TrayIcon/TrayIconMouseTest/TrayIconMouseTest.java
java/awt/TrayIcon/TrayIconPopup/TrayIconPopupTest.java

@petermz
Copy link
Author

@petermz petermz commented Dec 4, 2020

The icon delegate was not listening for mouse events -- thanks for catching this. I've restored the AWTTrayIconView thing (so that it can handle mouse events) and made it inherit from NSStatusBarButton (so that icon painting is delegated to MacOS). The fix is less verbose now. I've also renamed the property apple.awt.enableTemplateImages and added an @implnote as discussed in the CSR.

Can we consider the CSR approved btw?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 4, 2020

@petermz no, the CSR is not in the Approved state. Once it is, the Skara bot will recognize this and remove the csr label such that it will not longer be an Integration Blocker.

@mrserb
Copy link
Member

@mrserb mrserb commented Dec 19, 2020

The new version of the changes looks much better, the test run is in progress.

mrserb
mrserb approved these changes Jan 3, 2021
@voitylov
Copy link

@voitylov voitylov commented Jan 3, 2021

@mrserb thank you for the review of the issue. Could you review the associated JDK-8255597 CSR as well?

@openjdk openjdk bot removed the csr label Jan 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 11, 2021

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

8252015: [macos11] java.awt.TrayIcon requires updates for template images

Co-authored-by: Tres Finocchiaro <tres.finocchiaro@gmail.com>
Co-authored-by: Peter Zhelezniakov <peterz@openjdk.org>
Reviewed-by: 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 1475 new commits pushed to the master branch:

  • ac2dee5: 8259539: JDK-8255711 broke trap messages
  • 4697cfa: 8259576: Misplaced curly brace in Matcher::find_shared_post_visit
  • a6ab9e4: 8258576: Try to get zerobased CCS if heap is above 32 and CDS is disabled
  • a3561ae: 8258243: C2: assert failed ("Bad derived pointer") with -XX:+VerifyRegisterAllocator
  • 4663704: 8259583: Remove unused decode_env::_codeBuffer
  • 9d4e84f: 8259565: Zero: compiler/runtime/criticalnatives fails because CriticalJNINatives is not supported
  • 98ccfbf: 8255710: Opensource unit/regression tests for CMM
  • 61c5b95: 7194219: java/awt/Component/UpdatingBootTime/UpdatingBootTime.html fails on Linux
  • 77f6290: 8258254: Move PtrQueue flush to PtrQueueSet subclasses
  • 2255ab7: 8258810: Improve enum traits
  • ... and 1465 more: https://git.openjdk.java.net/jdk/compare/4185ed3290fa96bd58781082fb777dc75c52bec8...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 (@mrserb) 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 openjdk bot added the ready label Jan 11, 2021
@petermz
Copy link
Author

@petermz petermz commented Jan 12, 2021

/integrate

@openjdk openjdk bot added the sponsor label Jan 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 12, 2021

@petermz
Your change (at version 3027d82) is now ready to be sponsored by a Committer.

@AlexanderScherbatiy
Copy link

@AlexanderScherbatiy AlexanderScherbatiy commented Jan 12, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Jan 12, 2021

@AlexanderScherbatiy @petermz Since your change was applied there have been 1475 commits pushed to the master branch:

  • ac2dee5: 8259539: JDK-8255711 broke trap messages
  • 4697cfa: 8259576: Misplaced curly brace in Matcher::find_shared_post_visit
  • a6ab9e4: 8258576: Try to get zerobased CCS if heap is above 32 and CDS is disabled
  • a3561ae: 8258243: C2: assert failed ("Bad derived pointer") with -XX:+VerifyRegisterAllocator
  • 4663704: 8259583: Remove unused decode_env::_codeBuffer
  • 9d4e84f: 8259565: Zero: compiler/runtime/criticalnatives fails because CriticalJNINatives is not supported
  • 98ccfbf: 8255710: Opensource unit/regression tests for CMM
  • 61c5b95: 7194219: java/awt/Component/UpdatingBootTime/UpdatingBootTime.html fails on Linux
  • 77f6290: 8258254: Move PtrQueue flush to PtrQueueSet subclasses
  • 2255ab7: 8258810: Improve enum traits
  • ... and 1465 more: https://git.openjdk.java.net/jdk/compare/4185ed3290fa96bd58781082fb777dc75c52bec8...master

Your commit was automatically rebased without conflicts.

Pushed as commit 400dc76.

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

@tresf
Copy link
Contributor

@tresf tresf commented Jan 19, 2021

@petermz do you mind editing the original bug report PR description, incase this bug report PR is referenced in the future:

- Add a new system property, `java.awt.enableTemplateImages`, ...
+ Add a new system property, `apple.awt.enableTemplateImages`, ...

@petermz
Copy link
Author

@petermz petermz commented Jan 20, 2021

@tresf
Copy link
Contributor

@tresf tresf commented Jan 20, 2021

Sorry, I meant this PR's description, sorry for the confusion.

@petermz
Copy link
Author

@petermz petermz commented Jan 20, 2021

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants