RFR: 8301302: Platform preferences API [v20]

[Nir Lisker]

On Thu, 2 Nov 2023 19:11:39 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 198:
>> 
>>> 196:      * @throws NullPointerException if {@code preferences} is {@code null}
>>> 197:      */
>>> 198:     public void update(Map<String, Object> preferences) {
>> 
>> I want to make sure I understand the exact details of the update rules. Is this correct?
>> 
>> For each key in `preferences`:
>> * if it exists in `effectivePreferences`:
>>     * if the values are the same, there is no change (`preferences` contains all mapping and not just the changes)
>>     * if the value in `preferences` is `null`, then this is a removal operation (because the value of `effectivePreferences` can't be `null`)
>>     * if the value in `preferences` is not `null` and not the same, then this is an update operation
>> * if it doesn't exist in `effectivePreferences`:
>>     * if the value in `preferences` is not `null`, then this is an add operation
>>     * can the value in `preferences` be `null`?
>
> 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.

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

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