On Thu, 7 Dec 2023 07:57:01 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> renamed Windows.SPI.HighContrastOn to Windows.SPI.HighContrast
>
> modules/javafx.graphics/src/main/java/javafx/application/ColorScheme.java
> line 35:
>
>> 33: * @since 22
>> 34: */
>> 35: public enum ColorScheme {
>
> Can there be future additions to this enum, or is its purpose to be limited
> to the current choices?
I suspect this is limited to the current choices, but it's a good question.
> modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 1:
>
>> 1: /*
>
> I wonder if it's worth specifying the corresponding properties in JavaFX that
> match the ones in this class. For example, that the background property is
> used for `Region#backgroundProperty()`, and the foreground color is used for
> `Shape#fillProperty()` (or however `TextField` colors its text).
With the exception of the high-contrast theme on Windows, JavaFX doesn't make
any use of these platform-specific properties. It's up to the application to do
that.
> modules/javafx.graphics/src/main/java/javafx/application/Platform.java line
> 474:
>
>> 472: * <p>
>> 473: * The following preferences are potentially available on the
>> specified platforms:
>> 474: * <table id="preferences-table">
>
> Long tables can benefit from alternating colored rows. This can be achieved
> with `<table class="striped">` I think.
That might be a good enhancement for other tables as well (GraphicsContext
comes to mind). I would recommend this as a follow-up.
> modules/javafx.graphics/src/main/java/javafx/application/Platform.java line
> 572:
>
>> 570: * @since 22
>> 571: */
>> 572: public interface Preferences extends ObservableMap<String, Object> {
>
> Note that this will be the first case where a public API implementation of
> `ObservableMap` is not for a general use type, but for a specific use case.
> All previous implementations are:
> `MapBinding, MapExpression, MapProperty, MapPropertyBase,
> ReadOnlyMapProperty, ReadOnlyMapPropertyBase, ReadOnlyMapWrapper,
> SimpleMapProperty`, and all are from the `base` module.
>
> This might be fine, but consider composition here.
>
> @kevinrushforth what do you think?
I think this is OK. Given what it is being used for, I'm satisfied with the
API. One thing we could consider is an `@implNote` indicating that applications
are not expected to implement this. This could be done as a follow-up.
> modules/javafx.graphics/src/main/java/javafx/application/Platform.java line
> 604:
>
>> 602: *
>> 603: * @return the {@code foregroundColor} property
>> 604: * @defaultValue {@code Color.BLACK}
>
> Is `BLACK` a good default? From what I remember, because some devices have a
> difficulty with pure black, some dark gray color is used instead.
I would expect this to be one of the properties that is set on all platforms
anyway, but if not, I think this `BLACK` is a reasonable default.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1419168545
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1419174768
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1419167654
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1419166082
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1419177332
