On Tue, 31 Oct 2023 17:28:35 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 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<String, String> 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<String, > 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<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. 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 modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 66: > 64: private final List<MapChangeListener<? super String, Object>> > mapChangeListeners = new CopyOnWriteArrayList<>(); > 65: > 66: public PlatformPreferences(Map<String, String> 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 <T> Optional<T> getValue(String key, Class<T> 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<String, Object> 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<String, ChangedValue> changedEntries) > { Seems like it should be private. modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 214: > 212: > 213: void fireValueChangedEvent(Map<String, ChangedValue> changedEntries) > { > 214: for (var listener : invalidationListeners) { minor: IMHO, don't use `var` when it is not clear from the same line what it represents. modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 575: > 573: * > 574: * @return the {@code appearance} property > 575: */ Should this include `@defaultValue` ? modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 607: > 605: * The accent color. > 606: * <p> > 607: * If the platform does not report an accent color, this > property defaults to {@code #157EFB}. Perhaps include what that color represents (`vivid blue`)? 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. modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 621: > 619: * @param key the key > 620: * @throws NullPointerException if {@code key} is null > 621: * @throws IllegalArgumentException if the key is not mapped to > a {@code Integer} instance Suggestion: * @throws IllegalArgumentException if the key is not mapped to an {@code Integer} modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 688: > 686: * @param key the key > 687: * @param type the type of the value > 688: * @throws NullPointerException if {@code key} is null Suggestion: * @throws NullPointerException if {@code key}, or {@code type} is null Or my new favourite for this kind of thing: Suggestion: * @throws NullPointerException when any argument is null modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 689: > 687: * @param type the type of the value > 688: * @throws NullPointerException if {@code key} is null > 689: * @throws IllegalArgumentException if the key is not mapped to > a {@code type} instance Suggestion: * @throws IllegalArgumentException if the key is not mapped to a type {@code T} ------------- PR Review: https://git.openjdk.org/jfx/pull/1014#pullrequestreview-1707164742 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378164260 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378118623 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378124182 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378125544 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378130890 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378158956 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378160605 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378161236 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378133895 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378133314 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378136179 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378136573 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378138192 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378168481 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378169491 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378170933 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378172306 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378175004 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1378173803
