On Fri, 17 Nov 2023 19:43:58 GMT, Kevin Rushforth <[email protected]> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Rename Appearance to ColorScheme
>
> modules/javafx.graphics/src/main/java/javafx/application/Platform.java line
> 630:
>
>> 628: * @throws NullPointerException if {@code key} is null
>> 629: * @throws IllegalArgumentException if a mapping exists, but
>> the key is not mapped to an {@code Integer}
>> 630: * @return the optional {@code Integer} to which the key is
>> mapped
>
> I presume that it returns `Optional.empty()` if the mapping is not present?
> This could lead to a situation where calling this method with a particular
> key will return an empty optional in some cases and throw an exception in
> others. Might it be better to specify that it will throw IAE if the type of
> the key is known to not be an Integer, whether or not there exists a mapping
> for that key? Alternatively, might it be better to always return an empty
> Optional unless there exists a mapping and the value is an integer? In the
> latter case, we would get rid of "IAE" entirely.
Alternative 2 (return empty value if the key maps to a different type) is
attractive because it doesn't require additional type validation for mappings
that are not present, and gets rid of the sometimes unexpected IAE.
However, there's a major downside: the preferences map is, first and foremost,
a `Map`. The typed getters are just a convenience to make it easier to work
with this map. When a native toolkit reports a key-value mapping, it will be
included in the map, and its value can be unconditionally retrieved with
`Map.get(String)`.
When we use a typed getter, it should return `Optional.empty()` exactly if
`get(String)` would return `null`. Returning an empty value when the wrong
typed getter is called (which is a bug) is unexpected and basically hides what
would be the equivalent of a `ClassCastException`. This rules out this approach.
Alternative 1 (always throw IAE if we call the wrong typed getter, whether or
not we have a mapping) is not unexpected, and does not violate the
`Optional.empty() <=> get(String)==null` invariant. This alternative is a bit
heavier on the implementation side, as we must now keep well-known lists of
key-type mappings around, and more importantly, keep them in sync with the
native toolkit.
I've implemented this alternative and updated the specification of the typed
getters similar to this:
/**
* Returns an optional {@code Integer} to which the specified key is mapped.
*
* @param key the key
* @throws NullPointerException if {@code key} is null
* @throws IllegalArgumentException if the key is not mappable to an {@code
Integer}
* @return the optional {@code Integer} to which the key is mapped
*/
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1398099493
