RFR: 8301302: Platform preferences API [v18]

John Hendrikx jhendrikx at openjdk.org
Tue Oct 31 21:51:05 UTC 2023 

On Tue, 31 Oct 2023 17:28:35 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> Please read [this document](https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548) for an introduction to the Platform Preferences API, and how it interacts with the proposed style theme and stage appearance features.
>
> Michael Strauß has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - formatting
>  - Javadoc change

Partial review, didn't look closely at the non-Java code.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/Application.java line 786:

> 784:      * @return a map of platform-specific keys to well-known keys
> 785:      */
> 786:     public Map<String, String> getWellKnownPlatformPreferenceKeys() {

Not really liking the name.  Aren't these keys that are mapped to FX keys?  Perhaps `getPlatformSpecificMappings`

modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkApplication.java line 479:

> 477:             "GTK.theme_bg_color", "backgroundColor"
> 478:         );
> 479:     }

Not sure how often these are called, but since the map returned is immutable, you could return the same map each time.  Same applies for the other 2 application classes.

modules/javafx.graphics/src/main/java/com/sun/javafx/application/PlatformImpl.java line 1094:

> 1092:     // This method will be removed when StyleThemes are added.
> 1093:     private static void checkHighContrastThemeChanged(Map<String, Object> preferences) {
> 1094:         if (preferences.get("Windows.SPI.HighContrastOn") == Boolean.TRUE) {

It's better to not use reference equality here, as `new Boolean("true") != Boolean.TRUE`.
Suggestion:

        if (Boolean.TRUE.equals(preferences.get("Windows.SPI.HighContrastOn")) {

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/ChangedValue.java line 36:

> 34:  * Contains information about a changed value.
> 35:  */
> 36: public record ChangedValue(Object oldValue, Object newValue) {

Javadoc is incomplete here, missing `@param`s.

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/ChangedValue.java line 59:

> 57:             } else {
> 58:                 equals = Objects.equals(oldValue, newValue);
> 59:             }

`deepEquals` does what you do here.
Suggestion:

            boolean equals = Objects.deepEquals(newValue, oldValue);

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 52:

> 50:  * by calling the {@link #update(Map)} method.
> 51:  */
> 52: public class PlatformPreferences extends AbstractMap<String, Object> implements Platform.Preferences {

Is there a need for this class to be public?  It seems to me that `Platform.Preferences` is public, and that in order to get the platform preferences you call `Platform.getPreferences()`, which returns the interface.

Otherwise, you need to document all `public` methods (not from the interface), including the constructor.

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 61:

> 59:     final Map<String, Object> effectivePreferences = new HashMap<>();
> 60:     final Map<String, Object> unmodifiableEffectivePreferences = Collections.unmodifiableMap(effectivePreferences);
> 61:     final PreferenceProperties properties = new PreferenceProperties(this);

minor: `this` escapes here before object is fully constructed

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 66:

> 64:     private final List<MapChangeListener<? super String, Object>> mapChangeListeners = new CopyOnWriteArrayList<>();
> 65: 
> 66:     public PlatformPreferences(Map<String, String> wellKnownKeys) {

javadoc missing on public API method (see above, does this need to be public?)

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 101:

> 99: 
> 100:     @Override
> 101:     @SuppressWarnings("unchecked")

minor: You could extract the unchecked cast to a variable, and only mark that as unchecked.

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 104:

> 102:     public <T> Optional<T> getValue(String key, Class<T> type) {
> 103:         Objects.requireNonNull(key, "key cannot be null");
> 104:         Objects.requireNonNull(key, "type cannot be null");

Suggestion:

        Objects.requireNonNull(type, "type cannot be null");

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 196:

> 194:      * @param preferences the new preference mappings
> 195:      */
> 196:     public void update(Map<String, Object> preferences) {

Should specify that it throws NPE when given `null`

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 213:

> 211:     }
> 212: 
> 213:     void fireValueChangedEvent(Map<String, ChangedValue> changedEntries) {

Seems like it should be private.

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 214:

> 212: 
> 213:     void fireValueChangedEvent(Map<String, ChangedValue> changedEntries) {
> 214:         for (var listener : invalidationListeners) {

minor: IMHO, don't use `var` when it is not clear from the same line what it represents.

modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 575:

> 573:          *
> 574:          * @return the {@code appearance} property
> 575:          */

Should this include `@defaultValue` ?

modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 607:

> 605:          * The accent color.
> 606:          * <p>
> 607:          * If the platform does not report an accent color, this property defaults to {@code #157EFB}.

Perhaps include what that color represents (`vivid blue`)?

modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 617:

> 615: 
> 616:         /**
> 617:          * Returns the {@code Integer} instance to which the specified key is mapped.

I think the wording of all of these should be modified slightly.  No integer instance is returned.  When dealing with optional I usually word it as:

     Returns an optional {@code Integer} to which ...

... and I leave out the `Optional.empty` part.

I'd also leave out all the `instance` suffixes.

modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 621:

> 619:          * @param key the key
> 620:          * @throws NullPointerException if {@code key} is null
> 621:          * @throws IllegalArgumentException if the key is not mapped to a {@code Integer} instance

Suggestion:

         * @throws IllegalArgumentException if the key is not mapped to an {@code Integer}

modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 688:

> 686:          * @param key the key
> 687:          * @param type the type of the value
> 688:          * @throws NullPointerException if {@code key} is null

Suggestion:

         * @throws NullPointerException if {@code key}, or {@code type} is null

Or my new favourite for this kind of thing:
Suggestion:

         * @throws NullPointerException when any argument is null

modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 689:

> 687:          * @param type the type of the value
> 688:          * @throws NullPointerException if {@code key} is null
> 689:          * @throws IllegalArgumentException if the key is not mapped to a {@code type} instance

Suggestion:

         * @throws IllegalArgumentException if the key is not mapped to a type {@code T}

-------------

PR Review: https://git.openjdk.org/jfx/pull/1014#pullrequestreview-1707164742
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378164260
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378118623
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378124182
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378125544
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378130890
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378158956
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378160605
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378161236
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378133895
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378133314
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378136179
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378136573
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378138192
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378168481
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378169491
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378170933
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378172306
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378175004
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378173803

More information about the openjfx-dev mailing list