On Tue, 5 Sep 2023 00:55:44 GMT, Michael Strauß <mstra...@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
