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

8301302: Platform preferences API #1014

Closed
wants to merge 62 commits into from
Closed
Changes from 26 commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
2d28c85
Platform preferences implementation
mstr2 Jul 4, 2023
df03228
documentation
mstr2 Jul 18, 2023
ba26271
Removed platform-independent preference keys
mstr2 Jul 23, 2023
00e9eb7
doc changes
mstr2 Jul 23, 2023
dbad68d
Move Appearance enum to javafx.application
mstr2 Jul 23, 2023
e16491a
removed unused code
mstr2 Jul 24, 2023
2d1c7a9
Ensure that ColorProperty changes are atomic when observed
mstr2 Jul 24, 2023
0589078
doc changes
mstr2 Jul 24, 2023
f8a9c8a
Added test
mstr2 Jul 24, 2023
40e2f28
Removed application preferences implementation
mstr2 Sep 5, 2023
b100c6c
Addressed review comments
mstr2 Sep 5, 2023
4b9f75d
Revert Application changes
mstr2 Sep 5, 2023
27022f3
Added javadoc link to list of platform preferences
mstr2 Sep 5, 2023
02d6a2f
improve javadoc
mstr2 Sep 6, 2023
4172fa6
Update Eclipse .classpath file
mstr2 Sep 6, 2023
5081fda
Suppress empty change message in test application
mstr2 Sep 6, 2023
08a3692
Format arrays in test application
mstr2 Sep 6, 2023
2837e33
Merge branch 'master' into feature/platform-preferences
mstr2 Sep 6, 2023
9cfded0
Fire only a single invalidation event
mstr2 Sep 6, 2023
c8fd0e7
changes per review
mstr2 Sep 6, 2023
1eaea48
change test class name
mstr2 Sep 6, 2023
c736dee
Add signal handler for gtk-theme-name
mstr2 Sep 6, 2023
aae55f9
Remove javadocs
mstr2 Sep 6, 2023
cb81762
Handle key removals
mstr2 Sep 8, 2023
a893fa4
Merge branch 'master' into feature/platform-preferences
mstr2 Oct 29, 2023
8a5b213
changed parameter name
mstr2 Oct 29, 2023
7e826df
Javadoc change
mstr2 Oct 31, 2023
684f48b
formatting
mstr2 Oct 31, 2023
167d4e8
review changes
mstr2 Nov 1, 2023
ad74e39
changend bool comparison
mstr2 Nov 1, 2023
38177a8
review changes
mstr2 Nov 2, 2023
d012dec
removed extra newline
mstr2 Nov 3, 2023
9b607c6
Rename Appearance to ColorScheme
mstr2 Nov 10, 2023
6ec7e99
Doc changes
mstr2 Nov 17, 2023
3cc29e9
Add eager type checking for typed getters
mstr2 Nov 18, 2023
9a817d1
Remove Preferences.getPaint
mstr2 Nov 18, 2023
bb6071c
Replace HashMap with Map.ofEntries
mstr2 Nov 18, 2023
a3d0010
Support polymorphic values
mstr2 Nov 20, 2023
17b2b08
Use JLS 5.5.1 casting conversion rules
mstr2 Nov 24, 2023
9083d3e
flip S and T
mstr2 Nov 24, 2023
9adef99
address review comments
mstr2 Nov 24, 2023
57c8d0c
rename local variables
mstr2 Nov 24, 2023
bee1bae
typo
mstr2 Nov 24, 2023
89f1c8f
initialize field with NULL
mstr2 Nov 29, 2023
9cc3b4b
initialize field with NULL
mstr2 Nov 30, 2023
dbf24bf
check for pending exceptions in macOS PlatformSupport
mstr2 Dec 1, 2023
0572a36
check for pending exceptions in GTK PlatformSupport
mstr2 Dec 2, 2023
f291ea4
check for pending exceptions in Windows PlatformSupport
mstr2 Dec 2, 2023
a04a8c6
check for null return values of JNI methods
mstr2 Dec 4, 2023
0dff7a4
check return value of JNI functions
mstr2 Dec 4, 2023
6c37544
rename GLASS_CHECK_EXCEPTIONALLY_RETURN
mstr2 Dec 6, 2023
db393fc
null checking
mstr2 Dec 6, 2023
0759ddd
changed error condition check
mstr2 Dec 6, 2023
a2bd32d
consistently use NULL
mstr2 Dec 6, 2023
070eab5
swap message fields
mstr2 Dec 6, 2023
515395b
fixed bug in test application
mstr2 Dec 6, 2023
098ca17
fixed HighContrastScheme
mstr2 Dec 7, 2023
eac1d82
format arrays in test application, indicate added/removed prefs
mstr2 Dec 7, 2023
efdab27
renamed Windows.SPI.HighContrastOn to Windows.SPI.HighContrast
mstr2 Dec 7, 2023
20cccc5
query resource bundles for high-contrast schemes only on Windows
mstr2 Dec 7, 2023
6457503
javadoc
mstr2 Dec 7, 2023
3354782
removed unused import
mstr2 Dec 7, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/javafx.graphics/.classpath
Original file line number Diff line number Diff line change
@@ -30,7 +30,7 @@
<classpathentry combineaccessrules="false" kind="src" path="/base">
<attributes>
<attribute name="module" value="true"/>
<attribute name="add-exports" value="javafx.base/com.sun.javafx.property=javafx.graphics:javafx.base/test.util.memory=javafx.graphics"/>
<attribute name="add-exports" value="javafx.base/com.sun.javafx.property=javafx.graphics:javafx.base/test.javafx.collections=javafx.graphics:javafx.base/test.util.memory=javafx.graphics"/>
</attributes>
</classpathentry>
<classpathentry kind="con" path="org.eclipse.jdt.junit.JUNIT_CONTAINER/5">
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -84,8 +84,7 @@ public void handleOpenFilesAction(Application app, long time, String files[]) {
// currently used only on Mac OS X
public void handleQuitAction(Application app, long time) {
}
public boolean handleThemeChanged(String themeName) {
return false;
public void handlePreferencesChanged(Map<String, Object> preferences) {
}
}

@@ -259,12 +258,11 @@ protected void notifyWillResignActive() {
}
}

protected boolean notifyThemeChanged(String themeName) {
protected void notifyPreferencesChanged(Map<String, Object> preferences) {
EventHandler handler = getEventHandler();
if (handler != null) {
return handler.handleThemeChanged(themeName);
handler.handlePreferencesChanged(preferences);
}
return false;
}

protected void notifyDidResignActive() {
@@ -675,19 +673,6 @@ protected abstract FileChooserResult staticCommonDialogs_showFileChooser(Window
protected abstract int staticView_getMultiClickMaxX();
protected abstract int staticView_getMultiClickMaxY();

public String getHighContrastScheme(String themeName) {
return themeName;
}

/**
* Gets the Name of the currently active high contrast theme.
* If null, then high contrast is not enabled.
*/
public String getHighContrastTheme() {
checkEventThread();
return null;
}

protected boolean _supportsInputMethods() {
// Overridden in subclasses
return false;
@@ -768,4 +753,37 @@ public final Optional<Boolean> isKeyLocked(int keyCode) {
return Optional.empty();
}
}

/**
* Returns the current set of platform properties as a map of platform-specific keys to
* arbitrary values. Callers should assume that the returned map is immutable, while
* implementations should either return an immutable map, or give up ownership of the
* returned map.
*
* @return the current set of platform preferences
*/
public Map<String, Object> getPlatformPreferences() {
return Map.of();
}

/**
* Returns a map of platform-specific preference keys to well-known keys.
* <p>
* For example, the platform-specific key "Windows.UIColor.Foreground" is mapped to the
* well-known key "foregroundColor", which makes it easier to write shared code without
* depending on platform-specific details.
* <p>
* The following well-known keys are currently supported, which correspond to the names of color
* properties on the {@link com.sun.javafx.application.preferences.PreferenceProperties} class:
* <ul>
* <li>foregroundColor
* <li>backgroundColor
* <li>accentColor
* </ul>
*
* @return a map of platform-specific keys to well-known keys
*/
public Map<String, String> getWellKnownPlatformPreferenceKeys() {
return Map.of();
}
}
Original file line number Diff line number Diff line change
@@ -467,4 +467,14 @@ protected boolean _supportsInputMethods() {
@Override
protected native int _isKeyLocked(int keyCode);

@Override
public native Map<String, Object> getPlatformPreferences();

@Override
public Map<String, String> getWellKnownPlatformPreferenceKeys() {
return Map.of(
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO this should be returned from a private static final

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't understand. This is an overridden method, do you propose to introduce another private static final method, and getPlatformKeyMappings then calls out to this other method?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant that it is a constant, which I think would be good to extract. I think you may have mentioned it isn't called a whole lot, but recreating an immutable sometimes (large) map just seems unnecessary.

    private static final Map<String, String> MAPPINGS = Map.of(
        "GTK.theme_fg_color", "foregroundColor",
        "GTK.theme_bg_color", "backgroundColor"
    );

    @Override
    public Map<String, String> getPlatformKeyMappings() {
        return MAPPINGS;
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's only called once to initialize PlatformPreferences, so we should be fine.

"GTK.theme_fg_color", "foregroundColor",
"GTK.theme_bg_color", "backgroundColor"
);
}
}
Original file line number Diff line number Diff line change
@@ -36,6 +36,7 @@

import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Map;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;

@@ -388,4 +389,16 @@ public String getDataDirectory() {

@Override
protected native int _isKeyLocked(int keyCode);

@Override
public native Map<String, Object> getPlatformPreferences();

@Override
public Map<String, String> getWellKnownPlatformPreferenceKeys() {
return Map.of(
"macOS.NSColor.textColor", "foregroundColor",
"macOS.NSColor.textBackgroundColor", "backgroundColor",
"macOS.NSColor.controlAccentColor", "accentColor"
);
}
}
Original file line number Diff line number Diff line change
@@ -27,7 +27,6 @@
import com.sun.glass.ui.*;
import com.sun.glass.ui.CommonDialogs.ExtensionFilter;
import com.sun.glass.ui.CommonDialogs.FileChooserResult;
import com.sun.javafx.application.PlatformImpl;
import com.sun.prism.impl.PrismSettings;
import com.sun.javafx.tk.Toolkit;

@@ -36,12 +35,11 @@
import java.nio.IntBuffer;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.ResourceBundle;
import java.util.Map;

final class WinApplication extends Application implements InvokeLaterDispatcher.InvokeLaterSubmitter {

static float overrideUIScale;
private static final String BASE_NAME = "com/sun/glass/ui/win/themes";

private static boolean getBoolean(String propname, boolean defval, String description) {
String str = System.getProperty(propname);
@@ -341,17 +339,6 @@ public Pixels createPixels(int width, int height, IntBuffer data, float scalex,
}
}

@Override
public String getHighContrastScheme(String themeName) {
return PlatformImpl.HighContrastScheme.fromThemeName(ResourceBundle.getBundle(BASE_NAME)::getString, themeName);
}

private native String _getHighContrastTheme();
@Override public String getHighContrastTheme() {
checkEventThread();
return getHighContrastScheme(_getHighContrastTheme());
}

@Override
protected boolean _supportsInputMethods() {
return true;
@@ -380,4 +367,16 @@ public String getDataDirectory() {

@Override
protected native int _isKeyLocked(int keyCode);

@Override
public native Map<String, Object> getPlatformPreferences();

@Override
public Map<String, String> getWellKnownPlatformPreferenceKeys() {
return Map.of(
"Windows.UIColor.ForegroundColor", "foregroundColor",
"Windows.UIColor.BackgroundColor", "backgroundColor",
"Windows.UIColor.AccentColor", "accentColor"
);
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -27,6 +27,7 @@

import static com.sun.javafx.FXPermissions.CREATE_TRANSPARENT_WINDOW_PERMISSION;
import com.sun.javafx.PlatformUtil;
import com.sun.javafx.application.preferences.PlatformPreferences;
import com.sun.javafx.css.StyleManager;
import com.sun.javafx.tk.TKListener;
import com.sun.javafx.tk.TKStage;
@@ -1044,4 +1045,57 @@ private static boolean isSupportedImpl(ConditionalFeature feature) {
return Toolkit.getToolkit().isSupported(feature);
}
}

private static PlatformPreferences platformPreferences;

public static PlatformPreferences getPlatformPreferences() {
if (platformPreferences == null) {
throw new IllegalStateException("Toolkit not initialized");
}

return platformPreferences;
}

/**
* Called by Glass when the toolkit is initialized.
*
* @param wellKnownKeys a map of platform-specific keys to well-known keys
* @param preferences the initial set of platform preferences
*/
public static void initPreferences(Map<String, String> wellKnownKeys, Map<String, Object> preferences) {
platformPreferences = new PlatformPreferences(wellKnownKeys);
platformPreferences.update(preferences);
}

/**
* Called by Glass when one or several platform preferences have changed.
* <p>
* This method can be called on any thread. The supplied {@code preferences} map may
* include all platform preferences, or only the changed preferences. If a preference
* was removed, the corresponding key is mapped to {@code null}.
*
* @param preferences a map that includes the changed preferences
*/
public static void updatePreferences(Map<String, Object> preferences) {
if (isFxApplicationThread()) {
checkHighContrastThemeChanged(preferences);
platformPreferences.update(preferences);
} else {
// Make a defensive copy in case the caller of this method decides to re-use or
// modify its preferences map after the method returns. Don't use Map.copyOf
// because the preferences map may contain null values.
Map<String, Object> preferencesCopy = new HashMap<>(preferences);
runLater(() -> updatePreferences(preferencesCopy));
}
}

// This method will be removed when StyleThemes are added.
private static void checkHighContrastThemeChanged(Map<String, Object> preferences) {
if (preferences.get("Windows.SPI.HighContrastOn") == Boolean.TRUE) {
setAccessibilityTheme(preferences.get("Windows.SPI.HighContrastColorScheme") instanceof String s ? s : null);
} else {
setAccessibilityTheme(null);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this not be written more simply as

    String val = Boolean.TRUE.equals(preferences.get("Windows.SPI.HighContrastOn"))
              && preferences.get("Windows.SPI.HighContrastColorScheme") instanceof String s ? s : null;
    setAccessibilityTheme(val);

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't look simpler to me.

}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

package com.sun.javafx.application.preferences;

import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;

/**
* Contains information about a changed value.
*/
public record ChangedValue(Object oldValue, Object newValue) {

/**
* Returns a map that contains the new or changed mappings of {@code current} compared to {@code old}.
* A value has changed if {@link Objects#equals(Object, Object)} or {@link Arrays#equals(Object[], Object[])}
* returns {@code false} when invoked with the old and new value.
*
* @param old the old mappings
* @param current the current mappings
* @return a mapping of keys to changed values
*/
public static Map<String, ChangedValue> getEffectiveChanges(Map<String, Object> old, Map<String, Object> current) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this class does not handle addition of keys in current - should we explain this fact in this method and/or the class javadoc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't handle removals of keys in current, which is explained in the method documentation:

"Returns a map that contains the new or changed mappings of current compared to old.
A value has changed if Objects#equals or Arrays#equals returns false when invoked with the old and new value."

If you think this is not clear enough, I can try to improve the wording.

Copy link
Contributor

Choose a reason for hiding this comment

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

is it actually possible to have keys removed at runtime?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The set of preferences that is reported by a platform is hard-coded in the native platform implementation, and depends on the operating system version. It might indeed be the case that an OS upgrade/downgrade could result in a different set of reported properties, but can this happen while a JavaFX applications remains running?

In any case, even if that were to happen, the current behavior is that a preference that was once reported will not be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like I spoke too soon. Mappings can actually be removed at runtime, one such example is Windows.SPI.HighContrastColorScheme, which is only available if Windows.SPI.HighContrast is true.

This doesn't change the ChangedValue implementation, and there are tests that cover the key removal scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

if that's the case, isn't it possible to have the same key added at runtime? this scenario cannot be handled by the current code, as far as I can tell.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if I understand what you mean that the same key can be added at runtime. The protocol is as follows:

  1. The native toolkit reports either all preferences, or the changed preferences (or a mixture of both). If a report includes only changed preferences, this doesn't imply that unreported preferences (that were reported earlier) are removed.
  2. PlatformPreferences.update computes the difference between the currently known preferences, and the reported preferences. This is what ChangedValue.getEffectiveChanges does. A mapping that is not included in the reported preferences doesn't mean that it was removed; if the native toolkit wants to signal removal, it needs to map the respective key to null instead.
  3. If there are any effective changes (i.e. changes where the current value is actually different from the last known value), change notifications are fired. This includes additions (where new keys are seen for the first time), as well as removals (where a known key is now mapped to null).

This protocol gives native toolkits the option to choose how preferences are reported, some toolkits may always want to report all preferences, while other toolkits may only want to report the changed preferences. The burden of keeping track of the total set of preferences is placed on PlatformPreferences, not on the native toolkit. This allows us to implement the book-keeping logic in a single place, rather than in each toolkit individually.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for clarification. I misunderstood your earlier comment.

Map<String, ChangedValue> changed = null;

for (Map.Entry<String, Object> entry : current.entrySet()) {
Object newValue = entry.getValue();
Object oldValue = old.get(entry.getKey());
boolean equals;

if (oldValue instanceof Object[] oldArray && newValue instanceof Object[] newArray) {
equals = Arrays.equals(oldArray, newArray);
} else {
equals = Objects.equals(oldValue, newValue);
}

if (!equals) {
if (changed == null) {
changed = new HashMap<>();
}

changed.put(entry.getKey(), new ChangedValue(oldValue, newValue));
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect the code that handles added keys should be added here somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See my comment here.

return changed != null ? changed : Map.of();
}

}
Loading