On Thu, 15 Feb 2024 04:20:15 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> 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.
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Don't call into the native toolkit from Platform.getPreferences()
The code looks fine other than missing `doPrivileged` calls. I left a couple
other minor suggestions. I haven't tested it yet, but will do so next week.
Since we are defining a new system property a CSR might be in order (let me
think about it).
modules/javafx.graphics/src/main/java/com/sun/glass/ui/Application.java line
806:
> 804: /**
> 805: * Checks whether there are any problems with platform preferences
> detection,
> 806: * and emits a warning otherwise.
Suggestion: change "otherwise" to "if so" or similar (the current comment
indicates that a warning will be produced if there are no problems).
modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacApplication.java
line 466:
> 464: private static final String SUPPRESS_AWT_WARNING_PROPERTY =
> "javafx.preferences.suppressAppleAwtWarning";
> 465: private static final String AWT_APPEARANCE_PROPERTY =
> "apple.awt.application.appearance";
> 466: private boolean checkSystemAppearance =
> !Boolean.getBoolean(SUPPRESS_AWT_WARNING_PROPERTY);
This needs to be wrapped in a `doPrivileged` call, with a `@SuppressWarnings`
annotation, or it will fail to run with a security manager (which we still
support as of now, even though it is deprecated).
See, for example, `PlatformImpl.java` for the pattern to use.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacApplication.java
line 470:
> 468: @Override
> 469: public void checkPlatformPreferencesSupport() {
> 470: if (checkSystemAppearance &&
> "NSApplicationAWT".equals(applicationClassName)) {
Minor: consider using a static constant for the name of the AWT app class?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacApplication.java
line 473:
> 471: checkSystemAppearance = false;
> 472:
> 473: if
> (!"system".equals(System.getProperty(AWT_APPEARANCE_PROPERTY))) {
You need to wrap the call to `System.getProperty` in `doPrivileged (so you will
need to create a local var).
-------------
PR Review: https://git.openjdk.org/jfx/pull/1367#pullrequestreview-1886580420
PR Review Comment: https://git.openjdk.org/jfx/pull/1367#discussion_r1493344795
PR Review Comment: https://git.openjdk.org/jfx/pull/1367#discussion_r1493345874
PR Review Comment: https://git.openjdk.org/jfx/pull/1367#discussion_r1493346242
PR Review Comment: https://git.openjdk.org/jfx/pull/1367#discussion_r1493346477
