On Tue, 5 Sep 2023 19:30:30 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removed application preferences implementation
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PreferenceProperties.java
> line 118:
>
>> 116: }
>> 117:
>> 118: fireValueChangedEvent();
>
> It looks like this will fire `colorProperty.fireValueChangedEvent();` for all
> colors in `allColors`, even when none has changed.
> edit: I see what you did here. Contrary to it's name, `fireValueChangeEvent`
> does not always fire. Perhaps we could rename the method to
> fireValueChangeEventIfChanged/Maybe or some such?
I've renamed it to `fireValueChangedIfNecessary`.
> modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PreferenceProperties.java
> line 197:
>
>> 195:
>> 196: private void updateEffectiveValue() {
>> 197: // Choose the first non-null value in this order:
>> overrideValue, platformValue, defaultValue.
>
> are null default values allowed?
I can't think of a good reason why we would have preferences with a `null`
default value. The small set of convenience properties is meant as a baseline
that all JavaFX developers can rely on.
> tests/manual/events/PlatformPreferencesTest.java line 113:
>
>> 111: return "{\r\n\t" + entries + "\r\n}\r\n";
>> 112: }
>> 113:
>
> extra newline?
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316504358
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316505649
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1316506587
