RFR: 8301302: Platform preferences API [v46]

[Nir Lisker]

On Thu, 7 Dec 2023 01:05:44 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:
> 
>   renamed Windows.SPI.HighContrastOn to Windows.SPI.HighContrast

There are classes such as `PlatformPreferences`, `PreferenceProperties`, and `ColorSchemeProperty` that are effectively singletons. Does it makes sense to just write them in a singleton pattern to avoid misuse?

modules/javafx.graphics/src/main/java/com/sun/javafx/application/WindowsHighContrastScheme.java line 74:

> 72:         if (themeName == null) {
> 73:             return null;
> 74:         }

This method is called only from `PlatformImpl` that already does the `null` check on the the string. In general, `null` checks should be done on the "outer most layer" and then all the inner layers can rely on the value being non-null.

Is this method expected to be called from other places as well? If not, the method can be made package visible.

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

> 77: 
> 78:     private final List<InvalidationListener> invalidationListeners = new CopyOnWriteArrayList<>();
> 79:     private final List<MapChangeListener<? super String, Object>> mapChangeListeners = new CopyOnWriteArrayList<>();

Can these be modified concurrently? What is the need for `CopyOnWriteArrayList`?

modules/javafx.graphics/src/main/java/javafx/application/ColorScheme.java line 35:

> 33:  * @since 22
> 34:  */
> 35: public enum ColorScheme {

Can there be future additions to this enum, or is its purpose to be limited to the current choices?

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

> 1: /*

I wonder if it's worth specifying the corresponding properties in JavaFX that match the ones in this class. For example, that the background property is used for `Region#backgroundProperty()`, and the foreground color is used for `Shape#fillProperty()` (or however `TextField` colors its text).

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

> 35: import javafx.scene.input.KeyCode;
> 36: import javafx.scene.paint.Color;
> 37: import javafx.scene.paint.Paint;

Unused import.

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

> 472:      * <p>
> 473:      * The following preferences are potentially available on the specified platforms:
> 474:      * <table id="preferences-table">

Long tables can benefit from alternating colored rows. This can be achieved with `<table class="striped">` I think.

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

> 570:      * @since 22
> 571:      */
> 572:     public interface Preferences extends ObservableMap<String, Object> {

Note that this will be the first case where a public API implementation of `ObservableMap` is not for a general use type, but for a specific use case. All previous implementations are:
`MapBinding, MapExpression, MapProperty, MapPropertyBase, ReadOnlyMapProperty, ReadOnlyMapPropertyBase, ReadOnlyMapWrapper, SimpleMapProperty`, and all are from the `base` module.

This might be fine, but consider composition here.

@kevinrushforth what do you think?

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

> 585: 
> 586:         /**
> 587:          * The color used for background regions.

Maybe "The color used for background **of** regions"?

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

> 587:          * The color used for background regions.
> 588:          * <p>
> 589:          * If the platform does not report a background color, this property defaults to {@code Color.WHITE}.

Is there value in writing this sentence on every property if they specify the `@defaultValue`?

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

> 602:          *
> 603:          * @return the {@code foregroundColor} property
> 604:          * @defaultValue {@code Color.BLACK}

Is `BLACK` a good default? From what I remember, because some devices have a difficulty with pure black, some dark gray color is used instead.

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

> 609: 
> 610:         /**
> 611:          * The accent color.

I think that this needs to explanation on what the accent color is.

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

PR Review: https://git.openjdk.org/jfx/pull/1014#pullrequestreview-1769250088
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418411093
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418434560
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418521512
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418571391
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418470429
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418495453
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418489681
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418540211
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418532876
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418584560
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418528568