RFR: 8301302: Platform preferences API [v5]

Andy Goryachev angorya at openjdk.org
Tue Sep 5 20:02:14 UTC 2023 

On Tue, 5 Sep 2023 00:55:44 GMT, Michael Strauß <mstrauss at openjdk.org> 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 one additional commit since the last revision:
> 
>   Removed application preferences implementation

modules/javafx.graphics/src/main/java/com/sun/glass/ui/Application.java line 762:

> 760:      */
> 761:     public Map<String, Object> getPlatformPreferences() {
> 762:         return Map.of();

Should javadoc explicitly proclaim that the map is immutable?

modules/javafx.graphics/src/main/java/com/sun/javafx/application/PlatformImpl.java line 1081:

> 1079:     public static void updatePreferences(Map<String, Object> preferences) {
> 1080:         if (isFxApplicationThread()) {
> 1081:             checkHighContrastThemeChanged(preferences);

is this needed in this preferences-only PR?

modules/javafx.graphics/src/main/java/com/sun/javafx/application/PlatformImpl.java line 1093:

> 1091: 
> 1092:     // This method will be removed when StyleThemes are added.
> 1093:     private static void checkHighContrastThemeChanged(Map<String, Object> preferences) {

is this needed?

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/ChangedValue.java line 47:

> 45:      * @return a mapping of keys to changed values
> 46:      */
> 47:     public static Map<String, ChangedValue> getEffectiveChanges(Map<String, Object> old, Map<String, Object> current) {

this class does not handle addition of keys in `current` - should we explain this fact in this method and/or the class javadoc?

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 112:

> 110:         }
> 111: 
> 112:         throw new IllegalArgumentException(

should this behavior be documented?
there is no mention of an exception in case of type mismatch in the base class.

also, do we want to have some kind of trivial conversion implemented such as int -> long -> double ?  
or not?

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PreferenceProperties.java line 118:

> 116:         }
> 117: 
> 118:         fireValueChangedEvent();

It looks like this will fire `colorProperty.fireValueChangedEvent();` for all colors in `allColors`, even when none has changed.
edit: I see what you did here.  Contrary to it's name, `fireValueChangeEvent` does not always fire.  Perhaps we could rename the method to fireValueChangeEventIfChanged/Maybe or some such?

modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PreferenceProperties.java line 197:

> 195: 
> 196:         private void updateEffectiveValue() {
> 197:             // Choose the first non-null value in this order: overrideValue, platformValue, defaultValue.

are null default values allowed?

modules/javafx.graphics/src/main/java/javafx/application/Application.java line 35:

> 33: import javafx.css.Stylesheet;
> 34: import javafx.scene.Scene;
> 35: import javafx.scene.paint.Color;

strictly speaking, this file should be unchanged.

modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 448:

> 446:      * by JavaFX when the operating system reports that a platform preference has changed.
> 447:      *
> 448:      * @return the {@code Preferences} instance

minor: make it a link or add a line with a link to the comment itself saying something like
`Please refer to { @ link Preferences } javadoc for a list of expected preferences.`

modules/javafx.graphics/src/test/java/test/com/sun/javafx/application/preferences/PlatformPreferencesTest.java line 41:

> 39: 
> 40: import static org.junit.jupiter.api.Assertions.*;
> 41: import static test.javafx.collections.MockMapObserver.Tuple.tup;

this generates 9 errors in eclipse, starting with

Description	Resource	Type
The type test.javafx.collections.MockMapObserver.Tuple.tup is not accessible	PlatformPreferencesTest.java	Java Problem

the following  change to graphics/.classpath should fix it (inc. complete file):


<?xml version="1.0" encoding="UTF-8"?>
<classpath>
	<classpathentry kind="src" path="src/main/java"/>
	<classpathentry kind="src" path="build/gensrc/jsl-prism"/>
	<classpathentry kind="src" path="build/gensrc/jsl-decora"/>
	<classpathentry kind="src" path="build/hlsl/Decora"/>
	<classpathentry kind="src" path="build/hlsl/Prism"/>
	<classpathentry kind="src" output="testbin" path="src/shims/java">
		<attributes>
			<attribute name="test" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="src" output="testbin" path="src/test/java">
		<attributes>
			<attribute name="test" value="true"/>
			<attribute name="optional" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="src" path="src/main/resources">
		<attributes>
			<attribute name="optional" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="src" output="testbin" path="src/test/resources">
		<attributes>
			<attribute name="test" value="true"/>
			<attribute name="optional" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry combineaccessrules="false" kind="src" path="/base">
		<attributes>
			<attribute name="module" value="true"/>
			<attribute name="add-exports" value="javafx.base/com.sun.javafx.property=javafx.graphics:javafx.base/test.javafx.collections=javafx.graphics:javafx.base/test.util.memory=javafx.graphics"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="con" path="org.eclipse.jdt.junit.JUNIT_CONTAINER/5">
		<attributes>
			<attribute name="test" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER">
		<attributes>
			<attribute name="module" value="true"/>
			<attribute name="add-exports" value="java.base/sun.security.util=javafx.graphics"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="output" path="bin"/>
</classpath>

tests/manual/events/PlatformPreferencesTest.java line 113:

> 111:         return "{\r\n\t" + entries + "\r\n}\r\n";
> 112:     }
> 113: 

extra newline?

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316296127
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316302729
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316302948
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316305695
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316310131
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316313168
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316320186
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316322553
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316325117
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316336107
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316337291

More information about the openjfx-dev mailing list