On Fri, 24 Nov 2023 05:36:07 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: > > flip S and T Marked as reviewed by jhendrikx (Committer). modules/javafx.graphics/src/main/java/com/sun/glass/ui/Application.java line 762: > 760: * > 761: * @implSpec Implementations should either return an immutable map, > or give up ownership > 762: * of the returned map. edit: I just noticed this isn't public API, so I guess it is less important: --- I'm not much of a fan of the wording "should assume", the documentation should be clear what you get. Stream#toList for example says something like: > The returned List is unmodifiable; calls to any mutator method will always > cause UnsupportedOperationException to be thrown. There are no guarantees on > the implementation type or serializability of the returned List. In our case here, if we don't want to straight up say it is always immutable, I would go with wording that says it is a snapshot and won't be updated. I'd also remove the impl spec, as it has no choice to use a copy/immutable map now to adhere to the contract. Suggestion: * Returns the current set of platform properties as a map of platform-specific keys to * arbitrary values. This is a snapshot, and won't be updated. There are no guarantees on * the implementation type, modifiability or serializability of the returned {@code Map}. modules/javafx.graphics/src/main/java/com/sun/glass/ui/Application.java line 770: > 768: > 769: /** > 770: * Returns a map of platform-specific preference keys to well-known > keys. Not a fan of the wording "well-known keys". They're platform independent keys as JavaFX defines them. Suggestions "JavaFX keys", "JavaFX standard keys", "FX keys, which are platform independent keys defined by JavaFX". modules/javafx.graphics/src/main/java/com/sun/glass/ui/Application.java line 793: > 791: * Returns a mapping of platform-specific keys to the types of their > values. > 792: * Polymorphic types are supported by specifying the common base > type; for example, a key can > 793: * be mapped to {@code Paint.class} to support any type of paint. May want to search for mentions of `Paint` now that its removed, and use a different example here. modules/javafx.graphics/src/main/java/com/sun/glass/ui/Application.java line 797: > 795: * Implementors must keep this map in sync with the mappings > reported by the native Glass toolkit. > 796: * If a native toolkit reports mappings for keys that are not > contained in this map, the typed getters > 797: * in {@link javafx.application.Platform.Preferences} might not > throw IllegalArgumentException as Suggestion: * in {@link javafx.application.Platform.Preferences} might not throw {@code IllegalArgumentException} as modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkApplication.java line 476: > 474: @Override > 475: public Map<String, String> getPlatformKeyMappings() { > 476: return Map.of( IMHO this should be returned from a `private static final` modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkApplication.java line 485: > 483: @Override > 484: public Map<String, Class<?>> getPlatformKeys() { > 485: return Map.ofEntries( IMHO this should be returned from a `private static final` Same for the other two `MacApplication`, `WinApplication`. modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 314: > 312: > 313: // Assuming S is a class type: > 314: private boolean isClassConvertible(Class<?> S, Class<?> T) { Locals here don't follow the Java naming conventions, which explains why the code is somewhat confusing to read. Same for the other functions. ------------- PR Review: https://git.openjdk.org/jfx/pull/1014#pullrequestreview-1747775381 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404259619 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404262597 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404241920 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404242642 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404267468 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404267556 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404272681
