[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8301302: Platform preferences API [v18]
From:       Nir Lisker <nlisker () openjdk ! org>
Date:       2023-10-31 23:03:40
Message-ID: gSN-cooWyqrPgj18NYTWW8nMW_b0HeEepB0yGP4f1o4=.997bbda5-878c-48eb-b986-cb707904e706 () github ! com
[Download RAW message or body]

On Tue, 31 Oct 2023 17:28:35 GMT, Michael Strauß <mstrauss@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

modules/javafx.graphics/src/main/java/javafx/application/Appearance.java line 31:

> 29:  * Defines the appearance of the user interface.
> 30:  *
> 31:  * @since 22

I would add an `@see javafx.application.Platform.Preferences#appearanceProperty()` \
tag (if I got the syntax right) because it's not clear how and where to use this \
class from the description.

Can there be other uses for this enum outside of the current one in the above \
property? If so, it should be documented.

modules/javafx.graphics/src/main/java/javafx/application/Appearance.java line 44:

> 42:      */
> 43:     DARK
> 44: 

Minor:

I was told once that JavaFX uses a `;` after the last enum element.

Also no need for the extra empty line.

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

> 457:      * Contains UI preferences of the current platform.
> 458:      * <p>
> 459:      * {@code Preferences} extends {@link ObservableMap} to expose platform \
> preferences as key-value pairs.

I would mentioned here (like in `getPreferences`) that this map is unmodifiable to \
the user, and that changes by the underlying platform can be tracked by listening on \
this map.

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

> 565:      */
> 566:     public interface Preferences extends ObservableMap<String, Object> {
> 567:         /**

Missing empty line.

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

> 634:          *         if no mapping exists for the specified key
> 635:          */
> 636:         Optional<Double> getDouble(String key);

I'm a bit confused about this and similar methods. Several points:

1. There is no value that is a `Double`, and also no `Paint`, so I'm not sure what \
these are for considering that list gives all possible valid entries.

2. If the list of keys in the table is fully known, wouldn't an enum make more sense \
and be more safe than of a `String`?

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378165295
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378158983
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378222691
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378235234
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378223632


[prev in list] [next in list] [prev in thread] [next in thread]