RFR: 8301302: Platform preferences API [v46]

[Kevin Rushforth]

On Thu, 7 Dec 2023 07:57:01 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Michael Strauß has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   renamed Windows.SPI.HighContrastOn to Windows.SPI.HighContrast
>
> 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?

I suspect this is limited to the current choices, but it's a good question.

> 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).

With the exception of the high-contrast theme on Windows, JavaFX doesn't make any use of these platform-specific properties. It's up to the application to do that.

> 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.

That might be a good enhancement for other tables as well (GraphicsContext comes to mind). I would recommend this as a follow-up.

> 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?

I think this is OK. Given what it is being used for, I'm satisfied with the API. One thing we could consider is an `@implNote` indicating that applications are not expected to implement this. This could be done as a follow-up.

> 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.

I would expect this to be one of the properties that is set on all platforms anyway, but if not, I think this `BLACK` is a reasonable default.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1419168545
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1419174768
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1419167654
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1419166082
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1419177332