On Fri, 10 Nov 2023 21:23:54 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: > > Rename Appearance to ColorScheme Here is my feedback on the latest API specification. I left a few inline comments along with a couple general comments below. I like the name change of Appearance to ColorScheme. I see that Joe made this comment in the CSR: > As a stylistic point, it would also be acceptable to have a general "Throws > NPE on null argument unless otherwise specified" disclaimer at the type level > as opposed to repeating that for every method. I'll leave it up to you to decide whether you want to do this. modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 469: > 467: * the untyped {@link #get} method. > 468: * <p> > 469: * The preferences that are reported by the platform may be > dependent on the operating system version, They also might be dependent on some mode of operation of the platform. Do you think that's worth calling out? modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 578: > 576: * from the perceptual brightness of {@link > #backgroundColorProperty() backgroundColor} in relation > 577: * to {@link #foregroundColorProperty() foregroundColor} and > defaults to {@link ColorScheme#LIGHT} > 578: * if the platform does not report color preferences. Do we want to allow for the possibility of a platform reporting the color scheme directly (rather than specifying that it is always derived from the relative brightness of the foreground and background colors)? I can't think of a need off hand. 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. modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 632: > 630: * @return the optional {@code Integer} to which the key is > mapped > 631: */ > 632: Optional<Integer> getInteger(String key); Did you consider using `OptionalInt` instead of `Optional<Integer>`? What you chose seems more consistent (e.g., there is no `OptionalBoolean` class, so `getBoolean` has to return an `Optional<Boolean>`), so I wouldn't advocate changing it. ------------- PR Review: https://git.openjdk.org/jfx/pull/1014#pullrequestreview-1737695354 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1397751899 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1397771608 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1397796039 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1397800252
