RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]

[John Hendrikx]

On Mon, 10 Mar 2025 02:07:04 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> John Hendrikx has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix non-convergence logic one more time...
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java line 53:
> 
>> 51:  * within listener list. If possible use {@link ListenerManager}, as it has less
>> 52:  * storage requirements and is faster.
>> 53:  *
> 
> I suggest adding a line that says that this class is used by properties.

That doesn't seem like something you should be adding to the docs I think?

> modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java line 100:
> 
>> 98:      * @throws NullPointerException when listener is {@code null}
>> 99:      */
>> 100:     public void addListener(I instance, ChangeListener<? super T> listener) {
> 
> Same remarks from the invalidation listener method, but does `instance` not need a `null` check or does `getData` do that? It's not so clear.

This is basically handed off to individual implementations (`getData` is abstract), for example in `FloatBinding`:

    private static final ListenerManager<Number, FloatBinding> LISTENER_MANAGER = new ListenerManager<>() {
        @Override
        protected Object getData(FloatBinding instance) {
            return instance.listenerData;
        }

        @Override
        protected void setData(FloatBinding instance, Object data) {
            instance.listenerData = data;
        }
    };
    
I've updated the docs for the abstract methods to include that these should throw NPE when `instance` is `null`.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986834518
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986828001