RFR: 8301302: Platform preferences API [v20]

[Michael Strauß]

On Thu, 2 Nov 2023 22:34:10 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> That's correct.
>> 
>>> can the value in preferences be `null`?
>> 
>> It could be `null` (signalling a removal), but then that's a no-op if the key wasn't present in `effectivePreferences`.
>
> So now I'm wondering why the need for the new type `ChangedValue` when you're already accumulating changes with `SimpleChange`.
> 
> The update method can be something like
> 
> 
>     public void update(Map<String, Object> preferences) {
>         var changes = new ArrayList<SimpleChange<String, Object>>();
>         preferences.forEach((key, newValue) -> {
>             var oldValue = effectivePreferences.get(key);
>             // oldValue can't be null, so no NPE
>             if (Objects.deepEquals(oldValue, newValue)) {
>                 return;
>             }
> 
>             var change = new SimpleChange<>(this);
>             if (oldValue != null && newValue == null) {
>                 change.setRemoved(key, oldValue);
>                 effectivePreferences.remove(key);
>             } else if (oldValue != null && newValue != null) {
>                 change.setPut(key, oldValue, newValue);
>                 effectivePreferences.put(key, newValue);
>             } else {
>                 change.setAdded(key, newValue);
>                 effectivePreferences.put(key, newValue);
>             }
>             changes.add(change);
>         });
> 
>         if (!changes.isEmpty()) {
>             properties.update(changes, platformKeyMappings);
>             fireValueChangedEvent(changes);
>         }
>     }
> 
> 
> with `fireValueChangedEvent` being
> 
>     private void fireValueChangedEvent(List<SimpleChange<String, Object>> changes) {
>         invalidationListeners.forEach(listener -> listener.invalidated(this));
>         changes.forEach(change -> mapChangeListeners.forEach(listener -> listener.onChanged(change)));
>     }
> 
> Saves the multiple iterations and the need for a new type.
> 
> Then `PreferenceProperties::update` can be adjusted:
> 
>     public void update(List<SimpleChange<String, Object>> changes, Map<String, String> platformKeyMappings) {
>         for (SimpleChange<String, Object> change : changes) {
>         ...
> 
> 
> I didn't test this, it's just an insight.

The main reason for the new record `ChangedValue` is to modularize the code for easier reuse. At the moment, only `PlatformPreferences` uses it, but a follow-up enhancement will introduce a second class `ApplicationPreferences`, which will also use it to compute the effective changes. At that point, we'd either have to duplicate some code, or factor it out anyway.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1380868731