RFR: 8301302: Platform preferences API [v30]

[John Hendrikx]

On Fri, 24 Nov 2023 05:36:07 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 one additional commit since the last revision:
> 
>   flip S and T

Marked as reviewed by jhendrikx (Committer).

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

> 760:      *
> 761:      * @implSpec Implementations should either return an immutable map, or give up ownership
> 762:      *           of the returned map.

edit: I just noticed this isn't public API, so I guess it is less important:

---

I'm not much of a fan of the wording "should assume", the documentation should be clear what you get.

Stream#toList for example says something like:

> The returned List is unmodifiable; calls to any mutator method will always cause UnsupportedOperationException to be thrown. There are no guarantees on the implementation type or serializability of the returned List.

In our case here, if we don't want to straight up say it is always immutable, I would go with wording that says it is a snapshot and won't be updated.  I'd also remove the impl spec, as it has no choice to use a copy/immutable map now to adhere to the contract.

Suggestion:

     * Returns the current set of platform properties as a map of platform-specific keys to
     * arbitrary values. This is a snapshot, and won't be updated. There are no guarantees on 
     * the implementation type, modifiability or serializability of the returned {@code Map}.

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

> 768: 
> 769:     /**
> 770:      * Returns a map of platform-specific preference keys to well-known keys.

Not a fan of the wording "well-known keys".  They're platform independent keys as JavaFX defines them. Suggestions "JavaFX keys", "JavaFX standard keys", "FX keys, which are platform independent keys defined by JavaFX".

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

> 791:      * Returns a mapping of platform-specific keys to the types of their values.
> 792:      * Polymorphic types are supported by specifying the common base type; for example, a key can
> 793:      * be mapped to {@code Paint.class} to support any type of paint.

May want to search for mentions of `Paint` now that its removed, and use a different example here.

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

> 795:      * Implementors must keep this map in sync with the mappings reported by the native Glass toolkit.
> 796:      * If a native toolkit reports mappings for keys that are not contained in this map, the typed getters
> 797:      * in {@link javafx.application.Platform.Preferences} might not throw IllegalArgumentException as

Suggestion:

     * in {@link javafx.application.Platform.Preferences} might not throw {@code IllegalArgumentException} as

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

> 474:     @Override
> 475:     public Map<String, String> getPlatformKeyMappings() {
> 476:         return Map.of(

IMHO this should be returned from a `private static final`

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

> 483:     @Override
> 484:     public Map<String, Class<?>> getPlatformKeys() {
> 485:         return Map.ofEntries(

IMHO this should be returned from a `private static final`

Same for the other two `MacApplication`, `WinApplication`.

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

> 312: 
> 313:     // Assuming S is a class type:
> 314:     private boolean isClassConvertible(Class<?> S, Class<?> T) {

Locals here don't follow the Java naming conventions, which explains why the code is somewhat confusing to read.

Same for the other functions.

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

PR Review: https://git.openjdk.org/jfx/pull/1014#pullrequestreview-1747775381
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404259619
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404262597
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404241920
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404242642
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404267468
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404267556
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404272681