On Tue, 31 Oct 2023 20:58:55 GMT, John Hendrikx <jhendr...@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/preferences/PlatformPreferences.java
> line 52:
>
>> 50: * by calling the {@link #update(Map)} method.
>> 51: */
>> 52: public class PlatformPreferences extends AbstractMap<String, Object>
>> 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<String, Object> effectivePreferences = new HashMap<>();
>> 60: final Map<String, Object> 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
