On Thu, 2 Nov 2023 22:34:10 GMT, Nir Lisker <nlis...@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
