jfx icon indicating copy to clipboard operation
jfx copied to clipboard

8325900: Emit a warning on macOS if AWT has set the NSAppearance

Open mstr2 opened this issue 1 year ago • 7 comments

Platform preferences detection doesn't pick up effective macOS system preferences if AWT owns the NSApplication and has set its NSAppearance to a fixed value.

The workaround is to set the system property "apple.awt.application.appearance=system".

If this property is not set, the following warning will be emitted if a JavaFX application attempts to use the platform preferences API:

WARNING: Reported preferences may not reflect the macOS system preferences unless the sytem
property apple.awt.application.appearance=system is set. This warning can be disabled by
setting javafx.preferences.suppressAppleAwtWarning=true.

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-8325900: Emit a warning on macOS if AWT has set the NSAppearance (Enhancement - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1367

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

Using diff file

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

Webrev

Link to Webrev Comment

mstr2 avatar Feb 14 '24 21:02 mstr2

:wave: Welcome back mstrauss! 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 14 '24 21:02 bridgekeeper[bot]

Webrevs

mlbridge[bot] avatar Feb 14 '24 21:02 mlbridge[bot]

I'll take a closer look tomorrow or Friday. This might need to be refactored a bit so we don't call directly into glass from javafx.application.Platform.

/reviewers 2

kevinrushforth avatar Feb 14 '24 22:02 kevinrushforth

@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

openjdk[bot] avatar Feb 14 '24 22:02 openjdk[bot]

Platform preferences detection doesn't pick up effective macOS system preferences if AWT owns the NSApplication and has set its NSAppearance to a fixed value.

What about SWT ? It looks like SWT also supports dark mode since version 4.12 [1][2].

[1] : https://eclipse.dev/eclipse/news/4.12/platform_isv.php#dark-theme-mac [2] : https://bugs.eclipse.org/bugs/show_bug.cgi?id=540357

AnirvanSarkar avatar Feb 19 '24 17:02 AnirvanSarkar

What about SWT ? It looks like SWT also supports dark mode since version 4.12 [1][2].

[1] : https://eclipse.dev/eclipse/news/4.12/platform_isv.php#dark-theme-mac [2] : https://bugs.eclipse.org/bugs/show_bug.cgi?id=540357

I haven’t tested this in combination with SWT, have you?

mstr2 avatar Feb 23 '24 22:02 mstr2

What about SWT ? It looks like SWT also supports dark mode since version 4.12 [1][2]. [1] : https://eclipse.dev/eclipse/news/4.12/platform_isv.php#dark-theme-mac [2] : https://bugs.eclipse.org/bugs/show_bug.cgi?id=540357

I haven’t tested this in combination with SWT, have you?

I just did and it works as expected (so no special-casing of an SWT application is needed).

kevinrushforth avatar Feb 24 '24 00:02 kevinrushforth

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

8325900: Emit a warning on macOS if AWT has set the NSAppearance

Reviewed-by: kcr, mfox

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

  • 0d2ad0e0e6a8ab29e73e77ddbdbe4b6c4d8ab147: 8273349: Check uses of Stream::peek in controls and replace as needed
  • bf237329bf2d95bfdf248afc30d6df47d470d99a: 8316372: Monkey Tester Application Part 3
  • 7a4d2976a245053399ad760930c0e6fb13a67637: 8328754: Fix missing @Overrides in test
  • 9ca8e51ed3cae9d6381c42fd6e39316150b40cbc: 8328811: Fix missing @Overrides in demos
  • b3f5a7896830ae3fa9abf2c684f6b9279f4b926b: 8307980: Rotate Transformation never invalidates inverseCache
  • 6adbcffafd15f9f771c09afb03649e83e9e0b02a: 8328750: [TestBug] Improve Stub Font Support
  • 0541f37179ff4a672a40f3c4976e6019b8ecf7c2: 8306322: JDK8130122Test fails intermittently
  • 5d886f82260ee508c0da2dfee5d3ace1a199a675: 8267565: Support "@3x" and greater high-density image naming convention
  • 4beeb89f864ccf1424db36c9739a7f6999adeecc: 8325075: Enable -Werror for javadoc to fail on any warnings
  • 93e3bb2ce8c756f84ca23dc2e96b37c20936c1df: 8328751: Fix missing @Overrides in modules except javafx.web
  • ... and 34 more: https://git.openjdk.org/jfx/compare/de0255d6b6da52a7af367319f883afb08de5182a...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[bot] avatar Mar 13 '24 20:03 openjdk[bot]

Pending a second reviewer.

@beldenfox or @prsadhuk Would one of you be able to review?

kevinrushforth avatar Mar 26 '24 18:03 kevinrushforth

I can review this.

beldenfox avatar Mar 27 '24 16:03 beldenfox

/integrate

mstr2 avatar Mar 29 '24 19:03 mstr2

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

  • 0d2ad0e0e6a8ab29e73e77ddbdbe4b6c4d8ab147: 8273349: Check uses of Stream::peek in controls and replace as needed
  • bf237329bf2d95bfdf248afc30d6df47d470d99a: 8316372: Monkey Tester Application Part 3
  • 7a4d2976a245053399ad760930c0e6fb13a67637: 8328754: Fix missing @Overrides in test
  • 9ca8e51ed3cae9d6381c42fd6e39316150b40cbc: 8328811: Fix missing @Overrides in demos
  • b3f5a7896830ae3fa9abf2c684f6b9279f4b926b: 8307980: Rotate Transformation never invalidates inverseCache
  • 6adbcffafd15f9f771c09afb03649e83e9e0b02a: 8328750: [TestBug] Improve Stub Font Support
  • 0541f37179ff4a672a40f3c4976e6019b8ecf7c2: 8306322: JDK8130122Test fails intermittently
  • 5d886f82260ee508c0da2dfee5d3ace1a199a675: 8267565: Support "@3x" and greater high-density image naming convention
  • 4beeb89f864ccf1424db36c9739a7f6999adeecc: 8325075: Enable -Werror for javadoc to fail on any warnings
  • 93e3bb2ce8c756f84ca23dc2e96b37c20936c1df: 8328751: Fix missing @Overrides in modules except javafx.web
  • ... and 34 more: https://git.openjdk.org/jfx/compare/de0255d6b6da52a7af367319f883afb08de5182a...master

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Mar 29 '24 19:03 openjdk[bot]

@mstr2 Pushed as commit eca323547ec0e84b40bebb213350b6cea5385904.

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

openjdk[bot] avatar Mar 29 '24 19:03 openjdk[bot]