On Thu, 2 Nov 2023 19:11:39 GMT, Michael Strauß <mstra...@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
