[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    ChangeListener documentation clarification
From:       John Hendrikx <john.hendrikx () gmail ! com>
Date:       2023-02-23 8:45:48
Message-ID: 6e985a78-f1e8-2222-690a-849415be861d () gmail ! com
[Download RAW message or body]

Hi list,

I've been busy trying to ensure that ChangeListeners received sensible 
old/new values at all times.  This is important as users may be relying 
on these values to be correct when doing calculations or creating nested 
bindings (where an old listener is removed based on the old value 
received, and a new listener is registered based on the new value 
received).

As you may know, currently  the values received by ChangeListeners are 
not what you'd expect when the value is modified within an ongoing 
notification (a nested change). This affects ChangeListeners registered 
*after* the listener that triggered the nested change.  These listeners 
receive incorrect old values, and duplicate new values.  It gets even 
more complicated when multiple listeners do this or when listeners are 
added/removed during the notification process (all of which is allowed).

I would like to update the ChangeListener javadoc to document what the 
user can expect when they register their listener.  The current 
documentation implies that:

- The listener is only called when a change occurs (implying new value 
received changed from last time)
- The above further implies old and new values are distinct as it must 
have changed
- The fact that it is called the "old value" implies that it will hold 
the value of the previously received new value and not some other value 
that is distinct from the received new value

If we can agree on the above, we may want to specify this contract a bit 
more explicitly.

Also I'd like to explicitly state that the received new value need not 
be the same as the value obtained from `ObservableValue#getValue` when 
another notification is still pending.

A further rule we should discuss is whether all ChangeListeners should 
receive notifications about **all** changes to a property. The current 
implementation will notify all listeners X times when there were X 
changes, but, when there are nested changes, will not actually send all 
the changes as a "new value" (it skips some of the values, and sends 
some twice).  I see a few options here:

1. All ChangeListeners receive notifications about all changes exactly 
as they occurred, regardless of nested changes made by listeners earlier 
in the chain of listeners.
2. ChangeListeners registered later in the chain of listeners will not 
see changes that were changed (vetoed) by earlier listeners. This 
implies that if there were X changes, later listeners may see X - Y 
changes, where Y is the number of nested changes made. This also implies 
there is a strict order in which listeners are registered and called.

I would be inclined to go with option 1 here, but I would like to hear 
what others think.  The current implementation in my PR 
(https://github.com/openjdk/jfx/pull/837) changes `ExpressionHelper` to 
follow the above contract, and uses option 1, where all change listeners 
receive all notifications exactly as they occurred.

The current implementations in JavaFX breaks the above contract in 
several ways:

1) Old value and new value can be the same instance. The collection 
properties (ListProperty) do this to avoid having to make a copy of the 
collection for a proper old value. I think this okay, but they need to 
document they are deviating here from the ChangeListener contract.  
Effectively, it is no longer a change listener, it only provides the 
current values.

2) New value received is the same new value as the previous call. This 
happens when an earlier listener makes a change, triggering a nested 
notification. When the nested notification completes, the parent 
notification continues by sending the same new value again to the later 
listeners.

3) Old value received is not the same as the new value of the previous 
call. This happens in the same situation as 2)

I'm hoping we can get this situation fixed as they may trigger subtle 
bugs in calculations involving old/new values. When manually tracking 
bindings for example it can result in listeners that are never removed 
(for incorrect old values) or listeners being registered multiple times 
(for duplicate new values).

Thanks for reading!

--John






[prev in list] [next in list] [prev in thread] [next in thread]