On Thu, 23 Oct 2025 19:25:58 GMT, Andy Goryachev <[email protected]> wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains eight additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into feature/bulk-listeners
>> - remove unused variable
>> - Don't repeatedly call backingSet.size()
>> - Separate code paths for Change/IterableChange
>> - Use MapListenerHelper in PlatformPreferences to support bulk change
>> notifications
>> - Factor out IterableSetChange/IterableMapChange implementations
>> - add tests, documentation
>> - Implementation of bulk change listeners for ObservableSet and
>> ObservableMap
>
> modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableMapWrapper.java
> line 148:
>
>> 146: change.nextAdded(key, newValue);
>> 147: } else {
>> 148: V oldValue = backingMap.get(key);
>
> `containsKey()` followed by a `get()` - do you think it's possible to
> optimize this to avoid looking up twice? maybe use `get() == null` ?
This is possible if we know that the map doesn't contain null mappings. But
since `ObservableMapWrapper` is a wrapper around an arbitray `Map`, we need to
account for null mappings. As a consequence of that, we don't know if
`map.get(key) == null` means that no mapping is present, or if a mapping is
present but its value is `null`.
> modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableMapWrapper.java
> line 795:
>
>> 793: }
>> 794:
>> 795: private class SimpleChange extends MapChangeListener.Change<K,V> {
>
> could this and `BulkChange` classes made static?
Yes, I can do that for the existing `Change` implementations, but it's not
directly related to bulk change notifications. Maybe another PR would be better.
> modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSetWrapper.java
> line 532:
>
>> 530: @Override
>> 531: public void clear() {
>> 532: if (backingSet.size() > 1) {
>
> minor suggestion:
>
> switch(backing.size()) {
> case 0:
> break;
> case 1:
> ..
> default:
> ..
> }
>
> though the compiler is probably going to optimize the second call to `size()`
> out.
I've factored out the repeated call to `backingSet.size()`, and reordered the
branches such that == 1 comes before > 1. This reads more cleanly now, and I
think is also a bit better than a switch block.
> modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
> line 166:
>
>> 164:
>> Thread.currentThread().getUncaughtExceptionHandler().uncaughtException(Thread.currentThread(),
>> e);
>> 165: }
>> 166: } while (change instanceof IterableSetChange<? extends E> c
>> && c.nextChange());
>
> this is also minor: we are doing instanceof in a loop, perhaps we just need
> to add a second code path:
>
> if(change instanceof IterableSetChange c) {
> try {...}
> } else {
> try {..}
Yes, changed this here and also in `MapListenerHelper`.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457888231
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457888283
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457895737
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457897995
