On Fri, 24 Nov 2023 11:41:30 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> flip S and T
>
> 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".
I changed it to the wording "platform-independent keys".
> 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.
I think the example is probably one of the most practically relevant use cases,
even though right now there aren't any mappings with a declared type of `Paint`.
> 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`
I don't understand. This is an overridden method, do you propose to introduce
another `private static final` method, and `getPlatformKeyMappings` then calls
out to this other method?
> 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.
Changed.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404572373
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404573061
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404570744
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1404572002
