On Wed, 1 Nov 2023 13:59:07 GMT, Nir Lisker <nlis...@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
