On Thu, 7 Dec 2023 01:05:44 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: > > renamed Windows.SPI.HighContrastOn to Windows.SPI.HighContrast There are classes such as `PlatformPreferences`, `PreferenceProperties`, and `ColorSchemeProperty` that are effectively singletons. Does it makes sense to just write them in a singleton pattern to avoid misuse? modules/javafx.graphics/src/main/java/com/sun/javafx/application/WindowsHighContrastScheme.java line 74: > 72: if (themeName == null) { > 73: return null; > 74: } This method is called only from `PlatformImpl` that already does the `null` check on the the string. In general, `null` checks should be done on the "outer most layer" and then all the inner layers can rely on the value being non-null. Is this method expected to be called from other places as well? If not, the method can be made package visible. modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 79: > 77: > 78: private final List<InvalidationListener> invalidationListeners = new > CopyOnWriteArrayList<>(); > 79: private final List<MapChangeListener<? super String, Object>> > mapChangeListeners = new CopyOnWriteArrayList<>(); Can these be modified concurrently? What is the need for `CopyOnWriteArrayList`? 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? 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). modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 37: > 35: import javafx.scene.input.KeyCode; > 36: import javafx.scene.paint.Color; > 37: import javafx.scene.paint.Paint; Unused import. 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. 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? modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 587: > 585: > 586: /** > 587: * The color used for background regions. Maybe "The color used for background **of** regions"? modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 589: > 587: * The color used for background regions. > 588: * <p> > 589: * If the platform does not report a background color, this > property defaults to {@code Color.WHITE}. Is there value in writing this sentence on every property if they specify the `@defaultValue`? 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. modules/javafx.graphics/src/main/java/javafx/application/Platform.java line 611: > 609: > 610: /** > 611: * The accent color. I think that this needs to explanation on what the accent color is. ------------- PR Review: https://git.openjdk.org/jfx/pull/1014#pullrequestreview-1769250088 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418411093 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418434560 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418521512 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418571391 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418470429 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418495453 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418489681 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418540211 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418532876 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418584560 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1418528568
