On Thu, 7 Dec 2023 05:49:07 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/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.
The method now returns `NONE` when another constant doesn't apply. I've removed
the `public` modifier as you've suggested.
> 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`?
It's to prevent `ConcurrentModificationException` if a listener implementation
adds or removes itself (or another listener).
> 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.
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1419304904
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1419307136
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1419308447
