RFR: 8301302: Platform preferences API [v18]

[Michael Strauß]

On Wed, 1 Nov 2023 13:59:07 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> 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/com/sun/javafx/application/PlatformImpl.java line 1098:
> 
>> 1096:         } else {
>> 1097:             setAccessibilityTheme(null);
>> 1098:         }
> 
> Can this not be written more simply as
> 
>     String val = Boolean.TRUE.equals(preferences.get("Windows.SPI.HighContrastOn"))
>               && preferences.get("Windows.SPI.HighContrastColorScheme") instanceof String s ? s : null;
>     setAccessibilityTheme(val);
> 
> ?

This doesn't look simpler to me.

> 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);
> 
> Why are these not `private`? I don't see them being used outside of this class.
> 
> Also, if possible, add some documentation on these fields so maintainers can understand better what each is for,

Done.

> modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 67:
> 
>> 65: 
>> 66:     public PlatformPreferences(Map<String, String> wellKnownKeys) {
>> 67:         this.wellKnownKeys = wellKnownKeys;
> 
> Is the argument map guaranteed to not change outside of this class, or is there a need for a defensive copy?

It doesn't change in any of the three platform toolkits, but I'm making a defensive copy anyway.

> modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 192:
> 
>> 190:     /**
>> 191:      * Updates this map of preferences with a new set of platform preferences.
>> 192:      * The specified preferences may include all available preferences, or only the changed preferences.
> 
> I would mention here that removed preferences are specified by being mapped to `null`, but that the resulting preferences (after update) cannot map to `null`.

Done.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1380659661
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1380661250
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1380661904
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1380662009