RFR: 8301302: Platform preferences API [v5]

Michael Strauß mstrauss at openjdk.org
Tue Sep 5 23:26:50 UTC 2023


On Tue, 5 Sep 2023 19:30:30 GMT, Andy Goryachev <angorya at 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


More information about the openjfx-dev mailing list