Re: RFR: 8301302: Platform preferences API [v18]
On Wed, 1 Nov 2023 13:59:07 GMT, Nir Lisker 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 effectivePreferences = new HashMap<>();
>> 60: final Map 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 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
Re: RFR: 8301302: Platform preferences API [v18]
On Tue, 31 Oct 2023 17:28:35 GMT, Michael Strauß 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/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);
?
modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java
line 61:
> 59: final Map effectivePreferences = new HashMap<>();
> 60: final Map 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,
modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java
line 67:
> 65:
> 66: public PlatformPreferences(Map 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?
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`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378839517
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378817922
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1380059912
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1380097328
Re: RFR: 8301302: Platform preferences API [v18]
On Wed, 1 Nov 2023 08:24:10 GMT, John Hendrikx wrote: >> This might not be an issue, considering that the Boolean(String) constructor >> is terminally deprecated anyway. > > It might not be, or it may become a bigger issue than it is now. Why take > the risk? The rule in Java is always the same, objects are compared with > `equals` unless you specifically need reference equality. > > You don't do this with other primitive wrappers either (because you are > probably aware of things like the integer cache, which is just an > implementation detail and no guarantee). Same for these booleans, you're > relying on implementation details for this to work correctly. I agree with John: `if (Boolean.TRUE.equals(` is much better (maintaintability, correctness) - PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378908269
Re: RFR: 8301302: Platform preferences API [v18]
On Wed, 1 Nov 2023 03:25:50 GMT, Michael Strauß wrote: >> modules/javafx.graphics/src/main/java/javafx/application/Platform.java line >> 636: >> >>> 634: * if no mapping exists for the specified key >>> 635: */ >>> 636: Optional 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`? > > 1. There is no `Double` value now, but there might be in the future. For > exampe, the API may expose system-provided double-click times, or it may > expose information about the dimensions of system decorations. As for > `Paint`, it might be conceivable that a platform would expose color gradients. > > 2. The list is only fully known for the three listed platforms, but it's > unknown for other platforms. Using an enum key instead of a string key is > certainly possible, but then we'd be hard-coding platform-specific constants > into the core of JavaFX. You might argue that providing a list of supported > keys isn't really all that different from using a language feature like enums > to represent the constants. Using platform-specific preferences requires a > deep understanding of the platform in any case, and is not something > application developers should rely on regularly. Instead, they should be > using the common subset of well-known preferences, which is easily accessible > via the properties on the `Platform` class. Alright, my assumption that the listed keys are complete is wrong then. I thought that "The following list contains all preferences that are potentially available on the specified platforms" was a good enough guarantee. - PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378771358
Re: RFR: 8301302: Platform preferences API [v18]
On Wed, 1 Nov 2023 02:28:15 GMT, Michael Strauß wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java
>> line 52:
>>
>>> 50: * by calling the {@link #update(Map)} method.
>>> 51: */
>>> 52: public class PlatformPreferences extends AbstractMap
>>> implements Platform.Preferences {
>>
>> Is there a need for this class to be public? It seems to me that
>> `Platform.Preferences` is public, and that in order to get the platform
>> preferences you call `Platform.getPreferences()`, which returns the
>> interface.
>>
>> Otherwise, you need to document all `public` methods (not from the
>> interface), including the constructor.
>
> `PlatformPreferences` is in the `com.sun.javafx.application.preferences`
> package, but must be accessible from the `com.sun.javafx.application`
> package. That's why it needs to be public. The class doesn't have
> undocumented public methods (aside from the constructor), but then again this
> class is also not API.
Sorry, I missed that it was in `com.sun.*`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378516914
Re: RFR: 8301302: Platform preferences API [v18]
On Wed, 1 Nov 2023 02:16:23 GMT, Michael Strauß wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/application/PlatformImpl.java
>> line 1094:
>>
>>> 1092: // This method will be removed when StyleThemes are added.
>>> 1093: private static void checkHighContrastThemeChanged(Map>> Object> preferences) {
>>> 1094: if (preferences.get("Windows.SPI.HighContrastOn") ==
>>> Boolean.TRUE) {
>>
>> It's better to not use reference equality here, as `new Boolean("true") !=
>> Boolean.TRUE`.
>> Suggestion:
>>
>> if
>> (Boolean.TRUE.equals(preferences.get("Windows.SPI.HighContrastOn")) {
>
> This might not be an issue, considering that the Boolean(String) constructor
> is terminally deprecated anyway.
It might not be, or it may become a bigger issue than it is now. Why take the
risk? The rule in Java is always the same, objects are compared with `equals`
unless you specifically need reference equality.
You don't do this with other primitive wrappers either (because you are
probably aware of things like the integer cache, which is just an
implementation detail and no guarantee). Same for these booleans, you're
relying on implementation details for this to work correctly.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378512740
Re: RFR: 8301302: Platform preferences API [v18]
On Tue, 31 Oct 2023 22:33:00 GMT, Nir Lisker 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/javafx/application/Platform.java line > 636: > >> 634: * if no mapping exists for the specified key >> 635: */ >> 636: Optional 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`? 1. There is no `Double` value now, but there might be in the future. For exampe, the API may expose system-provided double-click times, or it may expose information about the dimensions of system decorations. As for `Paint`, it might be conceivable that a platform would expose color gradients. 2. The list is only fully known for the three listed platforms, but it's unknown for other platforms. Using an enum key instead of a string key is certainly possible, but then we'd be hard-coding platform-specific constants into the core of JavaFX. You might argue that providing a list of supported keys isn't really all that different from using a language feature like enums to represent the constants. Using platform-specific preferences requires a deep understanding of the platform in any case, and is not something application developers should rely on regularly. Instead, they should be using the common subset of well-known preferences, which is easily accessible via the properties on the `Platform` class. - PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378356218
Re: RFR: 8301302: Platform preferences API [v18]
On Tue, 31 Oct 2023 21:04:15 GMT, John Hendrikx 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/glass/ui/Application.java line
> 786:
>
>> 784: * @return a map of platform-specific keys to well-known keys
>> 785: */
>> 786: public Map getWellKnownPlatformPreferenceKeys() {
>
> Not really liking the name. Aren't these keys that are mapped to FX keys?
> Perhaps `getPlatformSpecificMappings`
I chose `getPlatformKeyMappings` to put more emphasis on the fact that this is
a mapping of keys to keys.
> modules/javafx.graphics/src/main/java/javafx/application/Platform.java line
> 575:
>
>> 573: *
>> 574: * @return the {@code appearance} property
>> 575: */
>
> Should this include `@defaultValue` ?
Yes.
> modules/javafx.graphics/src/main/java/javafx/application/Platform.java line
> 607:
>
>> 605: * The accent color.
>> 606: *
>> 607: * If the platform does not report an accent color, this
>> property defaults to {@code #157EFB}.
>
> Perhaps include what that color represents (`vivid blue`)?
Done.
> modules/javafx.graphics/src/main/java/javafx/application/Platform.java line
> 617:
>
>> 615:
>> 616: /**
>> 617: * Returns the {@code Integer} instance to which the specified
>> key is mapped.
>
> I think the wording of all of these should be modified slightly. No integer
> instance is returned. When dealing with optional I usually word it as:
>
> Returns an optional {@code Integer} to which ...
>
> ... and I leave out the `Optional.empty` part.
>
> I'd also leave out all the `instance` suffixes.
I reworded all of these methods, but added a clarification that the
`IllegalArgumentException` is only thrown if a mapping exists, but the value is
wrong.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378350039
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378350135
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378348626
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378349137
Re: RFR: 8301302: Platform preferences API [v18]
On Tue, 31 Oct 2023 22:31:11 GMT, Nir Lisker 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/javafx/application/Platform.java line
> 459:
>
>> 457: * Contains UI preferences of the current platform.
>> 458: *
>> 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.
Done.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378345734
Re: RFR: 8301302: Platform preferences API [v18]
On Tue, 31 Oct 2023 21:05:23 GMT, Nir Lisker 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/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. Added the `@see` tag. There will be other uses coming in the future (window decorations and style themes). > 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. Almost no public enumerations in JavaFX use a semicolon after the last constant (not considering those that need it because they have additional methods). Some enumerations do use a comma, but many others don't. - PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378343052 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378342416
Re: RFR: 8301302: Platform preferences API [v18]
On Tue, 31 Oct 2023 20:58:55 GMT, John Hendrikx 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/preferences/PlatformPreferences.java
> line 52:
>
>> 50: * by calling the {@link #update(Map)} method.
>> 51: */
>> 52: public class PlatformPreferences extends AbstractMap
>> implements Platform.Preferences {
>
> Is there a need for this class to be public? It seems to me that
> `Platform.Preferences` is public, and that in order to get the platform
> preferences you call `Platform.getPreferences()`, which returns the interface.
>
> Otherwise, you need to document all `public` methods (not from the
> interface), including the constructor.
`PlatformPreferences` is in the `com.sun.javafx.application.preferences`
package, but must be accessible from the `com.sun.javafx.application` package.
That's why it needs to be public. The class doesn't have undocumented public
methods (aside from the constructor), but then again this class is also not API.
> modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java
> line 61:
>
>> 59: final Map effectivePreferences = new HashMap<>();
>> 60: final Map unmodifiableEffectivePreferences =
>> Collections.unmodifiableMap(effectivePreferences);
>> 61: final PreferenceProperties properties = new
>> PreferenceProperties(this);
>
> minor: `this` escapes here before object is fully constructed
Yes, but `PreferenceProperties` doesn't do anything with `this`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378332197
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378333037
Re: RFR: 8301302: Platform preferences API [v18]
On Tue, 31 Oct 2023 20:19:42 GMT, John Hendrikx 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 1094:
>
>> 1092: // This method will be removed when StyleThemes are added.
>> 1093: private static void checkHighContrastThemeChanged(Map> Object> preferences) {
>> 1094: if (preferences.get("Windows.SPI.HighContrastOn") ==
>> Boolean.TRUE) {
>
> It's better to not use reference equality here, as `new Boolean("true") !=
> Boolean.TRUE`.
> Suggestion:
>
> if
> (Boolean.TRUE.equals(preferences.get("Windows.SPI.HighContrastOn")) {
This might not be an issue, considering that the Boolean(String) constructor is
terminally deprecated anyway.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378327099
Re: RFR: 8301302: Platform preferences API [v18]
On Tue, 31 Oct 2023 20:13:11 GMT, John Hendrikx 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/glass/ui/gtk/GtkApplication.java > line 479: > >> 477: "GTK.theme_bg_color", "backgroundColor" >> 478: ); >> 479: } > > Not sure how often these are called, but since the map returned is immutable, > you could return the same map each time. Same applies for the other 2 > application classes. It's only called once when QuantumToolkit is initialized, so there's no need to do that. - PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378304357
Re: RFR: 8301302: Platform preferences API [v18]
On Tue, 31 Oct 2023 20:35:47 GMT, John Hendrikx 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/preferences/PlatformPreferences.java
> line 214:
>
>> 212:
>> 213: void fireValueChangedEvent(Map
>> changedEntries) {
>> 214: for (var listener : invalidationListeners) {
>
> minor: IMHO, don't use `var` when it is not clear from the same line what it
> represents.
Can just use `invalidationListeners.forEach(listener ->
listener.invalidated(this));` here.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378270348
Re: RFR: 8301302: Platform preferences API [v18]
On Tue, 31 Oct 2023 17:28:35 GMT, Michael Strauß 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: *
> 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 {
> 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 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
Re: RFR: 8301302: Platform preferences API [v18]
On Tue, 31 Oct 2023 17:28:35 GMT, Michael Strauß 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
Partial review, didn't look closely at the non-Java code.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/Application.java line
786:
> 784: * @return a map of platform-specific keys to well-known keys
> 785: */
> 786: public Map getWellKnownPlatformPreferenceKeys() {
Not really liking the name. Aren't these keys that are mapped to FX keys?
Perhaps `getPlatformSpecificMappings`
modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkApplication.java
line 479:
> 477: "GTK.theme_bg_color", "backgroundColor"
> 478: );
> 479: }
Not sure how often these are called, but since the map returned is immutable,
you could return the same map each time. Same applies for the other 2
application classes.
modules/javafx.graphics/src/main/java/com/sun/javafx/application/PlatformImpl.java
line 1094:
> 1092: // This method will be removed when StyleThemes are added.
> 1093: private static void checkHighContrastThemeChanged(Map Object> preferences) {
> 1094: if (preferences.get("Windows.SPI.HighContrastOn") ==
> Boolean.TRUE) {
It's better to not use reference equality here, as `new Boolean("true") !=
Boolean.TRUE`.
Suggestion:
if (Boolean.TRUE.equals(preferences.get("Windows.SPI.HighContrastOn")) {
modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/ChangedValue.java
line 36:
> 34: * Contains information about a changed value.
> 35: */
> 36: public record ChangedValue(Object oldValue, Object newValue) {
Javadoc is incomplete here, missing `@param`s.
modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/ChangedValue.java
line 59:
> 57: } else {
> 58: equals = Objects.equals(oldValue, newValue);
> 59: }
`deepEquals` does what you do here.
Suggestion:
boolean equals = Objects.deepEquals(newValue, oldValue);
modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java
line 52:
> 50: * by calling the {@link #update(Map)} method.
> 51: */
> 52: public class PlatformPreferences extends AbstractMap
> implements Platform.Preferences {
Is there a need for this class to be public? It seems to me that
`Platform.Preferences` is public, and that in order to get the platform
preferences you call `Platform.getPreferences()`, which returns the interface.
Otherwise, you need to document all `public` methods (not from the interface),
including the constructor.
modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java
line 61:
> 59: final Map effectivePreferences = new HashMap<>();
> 60: final Map unmodifiableEffectivePreferences =
> Collections.unmodifiableMap(effectivePreferences);
> 61: final PreferenceProperties properties = new
> PreferenceProperties(this);
minor: `this` escapes here before object is fully constructed
modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java
line 66:
> 64: private final List>
> mapChangeListeners = new CopyOnWriteArrayList<>();
> 65:
> 66: public PlatformPreferences(Map wellKnownKeys) {
javadoc missing on public API method (see above, does this need to be public?)
modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java
line 101:
> 99:
> 100: @Override
> 101: @SuppressWarnings("unchecked")
minor: You could extract the unchecked cast to a variable, and only mark that
as unchecked.
modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java
line 104:
> 102: public Optional getValue(String key, Class type) {
> 103: Objects.requireNonNull(key, "key cannot be null");
> 104: Objects.requireNonNull(key, "type cannot be null");
Suggestion:
Objects.requireNonNull(type, "type cannot be null");
modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java
line 196:
> 194: * @param preferences the new preference mappings
> 195: */
> 196: public void update(Map preferences) {
Should specify that it throws NPE when given `null`
modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java
line 213:
> 211: }
> 212:
> 213: void fireValueChangedEvent(Map changedEntries)
> {
Seems like it should be private.
modules/javafx.graphics/src/main/java/com/sun/javafx/
Re: RFR: 8301302: Platform preferences API [v18]
> 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 - Changes: - all: https://git.openjdk.org/jfx/pull/1014/files - new: https://git.openjdk.org/jfx/pull/1014/files/8a5b2135..684f48bb Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1014&range=17 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1014&range=16-17 Stats: 5 lines in 1 file changed: 2 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1014.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1014/head:pull/1014 PR: https://git.openjdk.org/jfx/pull/1014
