On Sun, 4 Oct 2026 11:33:29 GMT, John Hendrikx <[email protected]> wrote:
>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManagerBase.java
>> line 245:
>>
>>> 243: ListenerListBase.callInvalidationListener(instance,
>>> listener);
>>> 244: }
>>> 245: finally {
>>
>> Suggestion:
>>
>> } finally {
>
> I've left these as the files I created myself all have a consistent style
> with these on the next line, fixed the other ones (missing space after `if`
> `for` -- I always mess this up, and in the files I only modified to keep
> style consistent within those)
ok
>> modules/javafx.base/src/main/java/javafx/beans/value/ObservableListValue.java
>> line 35:
>>
>>> 33: * @implNote
>>> 34: * The implementations of this interface in the JavaFX library do not
>>> provide all
>>> 35: * of the guarantees described by {@link ObservableValue} for change
>>> listeners: the
>>
>> In addition to the ones listed below, can you add that equal values may
>> trigger a notification? That guarantee from [ObservableValue lines
>> 97-99](https://github.com/hjohn/jfx/blob/871cbc31695403dd2f9bfaaaa5353478a0d8fb33/modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java#L97-L99)
>> doesn't hold. This also applies to the observable map and set value
>> interfaces.
>
> On the `equals` point, I think it's the same underlying issue as @mstr2
> raised above:
>
>> I think that invoking a change listener when only the contents of a list has
>> changed is a jarring defect. A change listener should only be invoked when
>> the list instance is changed; for its contents, we have ListChangeListener.
>> Now, I'm not sure if we can ever fix this, but maybe we shouldn't enshrine
>> this defect into specification with this PR. If we come to the conclusion
>> that it can't be changed, we can always specify this behavior later.
>
> I think this may be a longer discussion, as there are several issues with how
> change listeners work for these that deviate strongly from how they work for
> properties:
>
> | Case | Property / binding `ChangeListener` | `ObservableListValue`
> `ChangeListener` |
> |---|---|---|
> | `set(x)` where `x == old` (same instance) | no notification (`set` no-op) |
> no notification (`set` no-op) |
> | `set(x)` where `x != old` but `x.equals(old)` | **no notification** (the
> manager suppresses equal values) | **notification**, with `old.equals(new)`
> and `old != new` |
> | `set(x)` where `x != old` and `!x.equals(old)` | notification (`old`,
> `new`) | notification (`old`, `new`) |
> | contents change (same list instance) | n/a | **notification**, with `old ==
> new` (identical instance) |
>
> These are understandable choices, but it may have been better not to reuse
> the same `ChangeListener` interface given how they deviate (especially a
> reference change, but still being equal). I'd therefore hold off on
> specifying this behaviour for now, as @mstr2 suggested -- **however** I think
> it truly can't be fixed; using a change listener for lists to indicate
> reference changes is exactly what property change listeners **don't** do --
> so I think I still stand by my stance that they should be deprecated for the
> collection value types and one should use `InvalidationListener` +
> `getValue()` if they want the old `ChangeListener` behavior, or use the true
> API: `ListChangeListener`.
>
> Going a bit wider, this is worth getting right because change listeners (for
> properties) intentionally don't notify when `equals()` (which seems
> sensible), but that does open you up for "large object is replaced with
> another large but `equals()` object" and holding on to the old reference
> since a change listener doesn't get notified then. For `String`s this is
> already the case, but likely deemed acceptable since `String`s are often
> interned, but strictly speaking, a change listener that is keeping a r...
I'll leave it up to you then.
>> modules/javafx.base/src/main/java/javafx/collections/ObservableList.java
>> line 48:
>>
>>> 46: * guarantees described by {@link Observable} for their invalidation
>>> listeners: a listener
>>> 47: * that is removed while a notification is in progress may still be
>>> notified, and a nested
>>> 48: * notification notifies all listeners rather than only those that have
>>> already been notified.
>>
>> Question: you removed the Observable's guarantee about nested notifications,
>> so does the part about the nested notification still apply as an exception?
>> Same question applies to ObservableMap and ObservableSet.
>
> Yeah, I think I was too quick there -- the Observable should still mention
> that added/removed listeners will not participate in nested notifications
> (but with less technical wording, no mention of depth first) -- I clarified
> it in `Observable`, and I think that means we don't need to change anything
> for `ObservableList`+.
The change to Observable is good. In this class, you say "nested notification
notifies all listeners rather than only those that have already been notified."
which isn't quite the same as Observable's "A listener that is added while a
notification is in progress is not notified as part of that notification."
It might be fine as is, but I wanted to point out that slight difference.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4209589571
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4209613872
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4211602305
