This provides and uses a new implementation of ExpressionHelper, called ListenerManager with improved semantics.

See also #837 for a previous attempt which instead of triggering nested emissions immediately (like this PR and ExpressionHelper) would wait until the current emission finishes and then start a new (non-nested) emission.

Behavior

Listener...	ExpressionHelper	ListenerManager
Invocation Order	In order they were registered, invalidation listeners always before change listeners	(unchanged)
Removal during Notification	All listeners present when notification started are notified, but excluded for any nested changes	Listeners are removed immediately regardless of nesting
Addition during Notification	Only listeners present when notification started are notified, but included for any nested changes	New listeners are never called during the current notification regardless of nesting

Nested notifications:

	ExpressionHelper	ListenerManager
Type	Depth first (call stack increases for each nested level)	(same)
# of Calls	Listeners * Depth (using incorrect old values)	Collapses nested changes, skipping non-changes
Vetoing Possible?	No	Yes
Old Value correctness	Only for listeners called before listeners making nested changes	Always

Performance

Listener	ExpressionHelper	ListenerManager
Addition	Array based, append in empty slot, resize as needed	(same)
Removal	Array based, shift array, resize as needed	(same)
Addition during notification	Array is copied, removing collected WeakListeners in the process	Appended when notification finishes
Removal during notification	As above	Entry is `null`ed (to avoid moving elements in array that is being iterated)
Notification completion with changes	-	Null entries (and collected WeakListeners) are removed
Notifying Invalidation Listeners	1 ns each	(same)
Notifying Change Listeners	1 ns each (*)	2-3 ns each

(*) a simple for loop is close to optimal, but unfortunately does not provide correct old values

Memory Use

Does not include alignment, and assumes a 32-bit VM or one that is using compressed oops.

Listener	ExpressionHelper	ListenerManager	OldValueCaching ListenerManager
No Listeners	none	none	none
Single InvalidationListener	16 bytes overhead	none	none
Single ChangeListener	20 bytes overhead	none	16 bytes overhead
Multiple listeners	57 + 4 per listener (excluding unused slots)	57 + 4 per listener (excluding unused slots)	61 + 4 per listener (excluding unused slots)

About nested changes

Nested changes are simply changes that are made to a property that is currently in the process of notifying its listeners. This all occurs on the same thread, and a nested change is nothing more than the same property being modified, triggering its listeners again deeper in the call stack with another notification, while higher up the call stack a notification is still being handled:

       (top of stack)
       fireValueChangedEvent (property A)  <-- nested notification
       setValue (property A)  <-- listener modifies property A
       changed (Listener 1)  <-- a listener called by original notification
       fireValueChangedEvent (property A)  <-- original notification

How do nested changes look?

Let's say we have three listeners, where the middle listener changes values to uppercase. When changing a property with the initial value "A" to a lowercase "b" the listeners would see the following events:

ExpressionHelper

Nesting Level	Time	Listener 1	Listener 2	Listener 3	Comment
0	T1	A -> b
0	T2		A -> b		Value is changed to B
1	T3	b -> B			A nested loop started deeper on the call stack
1	T4		b -> B
1	T5			b -> B
0	T6			A -> B	Top level loop is resumed

Note how the values received by the 3rd listener are non-sensical. It receives two changes both of which changes to B from old values that are out of order.

ListenerManager (new)

This how ListenerManager sends out these events:

Nesting Level	Time	Listener 1	Listener 2	Listener 3	Comment
0	T1	A -> b
0	T2		A -> b		Value is changed to B
1	T3	b -> B			A nested loop started deeper on the call stack
1	T4		b -> B		The nested loop is terminated early at this point
0	T5			A -> B	Top level loop is resumed

Note how the 3rd listener now receives an event that reflects what actually happened from its perspective. Also note that less events overall needed to be send out.

About Invocation Order

A lot of code depends on the fact that an earlier registered listener of the same type is called before a later registered later of the same type. For listeners of different types it is a bit less clear. What is clear however is that invalidation and change listeners are defined by separate interfaces. Mixing their invocations (to conserve registration order) would not make sense. Historically, invalidation listeners are called before change listeners. No doubt, code will be (unknowingly) relying on this in today's JavaFX applications so changing this is not recommended. Perhaps there is reason to say that invalidation listeners should be called first as they're defined by the super interface of ObservableValue which introduces change listeners.

About Listener Add/Remove performance

Many discussions have happened in the past to improve the performance of removing listeners, ranging from using maps to ordered data structures with better remove performance. Often these solutions would subtly change the notification order, or increase the overhead per listener significantly.

But these discussions never really looked at the other consequences of having tens of thousands of listeners. Even if listeners could be removed in something approaching O(1) time (additions are already O(1) and so are not the issue), what about the performance of notifying that many listeners? That will still be O(n), and so even if JavaFX could handle addition and removal of that many listeners comfortably, actually using a property with that many listeners is still impossible as it would block the FX thread for far too long when sending out that many notifications.

Therefore, I'm of the opinion that "fixing" this "problem" is pointless. Instead, having that many listeners should be considered a design flaw in the application. A solution that registers only a single listener that updates a shared model may be much more appropriate.

About Old Value Correctness

...and why it is important.

A change listener provides a callback that gives the old and the new value. One may reasonably expect that these values are never the same, and one may reasonably expect that the given old value is the same as the previous invocation's new value (if there was a previous invocation).

In JavaFX, many change listeners are used for important tasks, ranging from reverting changes in order to veto something, to registering and unregistering listeners on properties. Many of those change listeners do not care about the old value, but there are a significant number that use it and rely on it being correct. A common example is the registering of a listener on the "new" value, and removing the same listener from the "old" value in order to maintain a link to some other property that changes location:

    (obs, old, current) -> {
          if (old != null) {
               old.removeListener(x);
          } 
          if (current != null) {
               current.addListener(x);
          }
    }

The above code looks bug free, and it would be if the provided old values are always correct. Unfortunately, this does not have to be the case. When a nested change is made (which can be made by a user registered listener on the same property), ExpressionHelper makes no effort to ensure that for all registered listener the received old and new values make sense. This leads to listeners being notified twice with the same "new" value for example, but with a different old value. Imagine the above listener receives the following change events:

      scene1 becomes scene3
      scene2 becomes scene3

The above code would remove its listeners from scene1 and scene2, and register two listeners on scene3. This leads to the listener being called twice when something changes. When later the scene changes to scene4, it receives:

      scene3 becomes scene4

Because it registered its listener twice on scene3, and only removes one of them, it now has listeners on both scene3 and scene4.

Clearly it is incredibly important that changes make sense, or even simple code that looks innocuous becomes problematic.

The PR

The ListenerManager differs from ExpressionHelper in the following ways:

Provides correct old/new values to ChangeListeners under all circumstances
Unnecessary change events are never sent
Single invalidation or change listeners are inlined directly into the observable class (in other words, properties with only a single listener don't take up any extra space at all)
Performance is slightly worse when calling change listeners (but remember that ExpressionHelper is not following the contract).
Removed listeners are never called after being removed (even if they were part of the initial list when the notification triggered)
Added listeners are only called when a new non-nested (top level) notification starts
Locking and maintaining the listener list works a bit differently -- the main takeaway is that the list indices remain the same when modified during nested modifications, which allows using the same list no matter how deep the nesting
No reference is stored to the ObservableValue and no copy is kept of the current value
Memory use when there is more than 1 listener should be similar, and better when not
Although complicated, the code is I think much better separated, and more focused on accomplishing a single task:
- ListenerManager manages the listener storage in property classes, and the transformation between the listener variants (it either uses listeners directly, or uses a ListenerList when there are multiple listeners).
- ListenerListBase handles the locking and compacting of its listener lists.
- ListenerList which extends ListenerListBase is only concerned with the recursion algorithm for notification.
- ArrayManager handles resizing and reallocating of arrays.
- There are variants of ListenerList and ListenerManager which can cache their old values when its not possible to supply these (this has a cost, and is what ExpressionHelper does by default).

The notification mechanism deals with nested notifications by tracking how many listeners were notified already before the nested notification occurs. For example, if 5 listeners were notified, and then listener 5 makes a nested change, then in that nested change only the first 5 listeners are notified again (if they still exist). The nested loop is then terminated early, at which point the top level loop resumes: it continues where it left of and notifies listener 6 and so on. This ensures that all notifications are always correct, and that listeners that "veto" changes can effectively block later listeners from seeing those at all.

For example, if the first listener always uppercases any received values, then any succeeding listeners will always see only uppercase values. The first listener receives two notifications (X -> a and a -> A), and the second receives only X -> A. Contrast this with the old ExpressionHelper, which sends odd notifications to the second listener (a -> A and X -> A, in that order).

Unfortunately, due to a somewhat weird design choice in the PropertyBase classes, the strategy of not having to cache the "current value" (or old value) can't be used (it can only be used for Bindings for now). So there is another variant of this helper, called OldValueCachingListenerHelper, with some slight differences:

Has an extra field for storing the old value when there are any ChangeListeners active
Can't store a ChangeListener inline; a minimal wrapper is needed to track the old value (ExpressionHelper does the same)

Progress

[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue
[ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs (Enhancement - P2)

Reviewers

Michael Strauß (@mstr2 - Reviewer) 🔄 Re-review required (review applies to 139c2aa4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1081/head:pull/1081
$ git checkout pull/1081

Update a local copy of the PR:
$ git checkout pull/1081
$ git pull https://git.openjdk.org/jfx.git pull/1081/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1081

View PR using the GUI difftool:
$ git pr show -t 1081

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1081.diff

Using Webrev

Link to Webrev Comment

Apr 04 '23 15:04 hjohn

:wave: Welcome back jhendrikx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Apr 04 '23 15:04 bridgekeeper[bot]

John and I discussed this off-list. I will write a short review of this change.

Behavior

The solution implements has the following behaviors, which are compared to the current (partially-flawed) ones:

Listeners are invoked in the order they were registered, with invalidation listeners being invoked before change listeners. This is also the current behavior. The behavior of invoking all listeners according to the order of registrations will be investigated.
Listeners that are removed during event propagation will be removed immediately, and will not receive the event (if they hadn't already). This differs from the current behavior of removing the listeners only after the event finished (buggy implementation).
Listeners that are added during event propagation will be effectively added after the event finishes, and will not receive the event during which they were added. This is also the current behavior (buggy implementation). The behavior of adding the listeners immediately, as is done with removal, will be investigated.
Nested events are invoked "depth-first", meaning that the parent event propagation is halted until the nested event finishes (see below). This differs from the current behavior that takes the "breadth-first" approach - each event finishes before the nested one starts (buggy implementation).
Nested events are only handled by listeners who received the parent event already so that they can react to the new change. Listeners that did not receive the parent event will only get a single (updated) event so that they don't react to irrelevant values. This allows vetoing, This differs from the current behavior that sends all events to all listeners (buggy implementation).

Examples

Suppose 5 change listeners are registered when an event starts. Removal during an event

L1 gets the event
L2 gets the event and removes L4
L3 gets the event and removes L2 (L2 already got the event)
L4 does not get the event (removed by L2)
L5 gets the event
final listeners: L1, L3, L5

Addition during an event

L1 gets the event
L2 gets the event and adds L6
L3-L5 get the event
L6 does not get the event (added by L2)
final listeners: L1 - L6

Nested event (value change during an event)

The observable value changes from 0 to 1
L1 gets 0->1
L2 gets 0->1
L3 gets 0->1 and sets the value to 2 (vetoing)
L1-L3 get 1->2 (nested event - listeners can react to the new change)
L4-L5 get 0->2 (parent event continues with the updated value)

Recursive change (see https://continuously.dev/blog/2015/02/10/val-a-better-observablevalue.html) The code

IntegerProperty p = new SimpleIntegerProperty(0);
// L1
p.addListener((obs, old, val) -> {
    if (val.intValue() > 0) {
        p.set(val.intValue() - 1);
    }
});
// L2
p.addListener((obs, old, val) -> System.out.println(old + " -> " + val));
p.set(2);

will trigger

L1 0->2 (set 1)
L1 2->1 (set 0)
L1 2->0
L2 is not triggered because the updated event is 0->0
Nothing is printed

instead of the current behavior that will print

1->0
2->0
0->0

Equality check

Change events require a comparison method for the old and new value. The 2 candidates are reference equality (==) and object equality (Objects#equals). There is some inconsistency in JavaFX about how this equality check is made (it is made in a few places on a few different types). It makes sense to do == with primitive types, and equals with String and the primitive wrappers. For other types, it depends on their characteristics. The "safer" option is == because a change that is triggered by != can then be tested for !oldValue.equals(newValue) in the listener and be vetoed; the opposite is not possible. This might mean that the user will have to give the comparison method that is desired.

Currently, == is used except for String. The behavior is preserved in this change, but will be investigated further in order to allows for more sensible change events.

Performance

Performance both in memory and speed is either equal or slightly worse than the current one. This is because the current behavior is wrong and fixing it entails more complications. In practice, the difference should be small. Registering many listeners on the same observable is not recommended and has caused issues in the past as well. Performance is a WIP and benchmarks will be posted later.

Apr 13 '23 15:04 nlisker

John and I discussed this off-list. I will write a short review of this change.

I have some small corrections I think.

Nested events are invoked "depth-first", meaning that the parent event propagation is halted until the nested event finishes (see below). This differs from the current behavior that takes the "breadth-first" approach - each event finishes before the nested one starts (buggy implementation).

Current behavior in ExpressionHelper is also depth first.

Equality check

Change events require a comparison method for the old and new value. The 2 candidates are reference equality (==) and object equality (Objects#equals). There is some inconsistency in JavaFX about how this equality check is made (it is made in a few places on a few different types). It makes sense to do == with primitive types, and equals with String and the primitive wrappers. For other types, it depends on their characteristics. The "safer" option is == because a change that is triggered by != can then be tested for !oldValue.equals(newValue) in the listener and be vetoed; the opposite is not possible. This might mean that the user will have to give the comparison method that is desired.

Currently, == is used except for String. The behavior is preserved in this change, but will be investigated further in order to allows for more sensible change events.

Just to add here, there are actually two checks involved. When you "set" the value on a property, all properties do a reference equality check (apart from String) to determine whether to fire their listeners. This means that InvalidationListeners always fire in these circumstances. Change listeners however are protected by a 2nd check that is part of ExpressionHelper. This check uses equals for all property types. This means that the behavior of an InvalidationListener + get is sometimes subtly different from using a ChangeListener.

When looking only at change listeners, this behavior makes sense for any type that is either primitive, immutable or mutable without overriding equals. For types that are mutable and override equals, the odd situation can occur that no change fires because the two instances are equal, but that the instance reference did change. When such a type is mutable, any further mutations could be missed. Simple example:

  List a = new ArrayList<>();
  List b = a.clone();
  ObjectProperty<List> prop = new SimpleObjectProperty<>(a);
  ObjectProperty<List> copy = new SimpleObjectProperty<>(a);

  // keep properties in sync:
  prop.addListener((obs, o, n) -> copy.set(n));
  prop.get().equals(copy.get());  // true :-)

  // change first property:
  prop.set(b);   // no change fired, they're equals!
  
  b.add("Hello");

  prop.get().equals(copy.get());  // false, as prop has reference B, while copy has reference A still...

It doesn't happen too often that properties are used with a type that is mutable with its own equals implementation, so this usually works correctly; for cases where you do want to use a mutable type with its own equals in an ObjectProperty though, I think having a variant of ObjectProperty with a reference equality check for its change listeners call may be sufficient.

Performance

Performance both in memory and speed is either equal or slightly worse than the current one. This is because the current behavior is wrong and fixing it entails more complications. In practice, the difference should be small. Registering many listeners on the same observable is not recommended and has caused issues in the past as well. Performance is a WIP and benchmarks will be posted later.

The implementation is able to avoid using a wrapper for single invalidation/change listeners, which improves memory use a bit for the second most common state properties are in (having 1 listener only -- the most common state being having no listeners at all).

As for adding/removing many listeners, I've have changed my stance on this and I don't think we should cater to situations that can have 10.000's of listeners -- even if add/remove performance was much improved, that won't make notifying such high amounts of listeners any better. Having such high amounts of listeners on a single property is a sign that something is wrong with the design, and even if it were to perform reasonably (which it won't due to the sheer amount of listener calls), it would be better to investigate how to avoid adding so many listeners, perhaps by adding a single listener and distributing the requested information more directly (via a shared model for example).

This implementation will have similar performance when it comes to adding/removing listeners as the current implementation. It grows and shrinks the listener list on demand, with the major difference being that when the list is locked (due to an ongoing notification) it will avoid modifying the lists in such a way that indices of current listeners change (removals are nulled out). After the list unlocks, the list is compacted if there were any listener additions/removals, and nulls (and weak listeners) are removed at that time. For this reason it may win out in some cases where listeners are added/removed during notifications, as it does not need to make copies of the listener list, but that is going to be a very rare occurrence.

Apr 13 '23 17:04 hjohn

Webrevs

Apr 15 '23 19:04 mlbridge[bot]

Benchmarks

Here are are some performance tests. The main take away is that invalidation performs roughly the same as before, while change listener notifications, which have acquired some more complexity, are about twice as slow. Also worth nothing is that the methods timed are generally so fast that a branch mispredication can have a big impact on the absolute performance. For example, when I re-order the method that "switches" on the data type (ListenerList, InvalidationListener or ChangeListener) I can get slightly better performance for some of these, losing performance somewhere else. I've picked a balance for now that makes the code easiest to read.

The branch mispredication is also the reason that a single ChangeListener performs about as good as a ListenerList with up to 3 listeners (but of course, the single listener case consumes less memory).

                                                                           ExpressionHelper     ListenerManager
Benchmark                         (inv_chg_ListenerCounts)  Mode  Cnt   Score    Error  Units   Score   Error  Units
FireValueChanged.binding                             0,  0  avgt   10   2.247 ±  0.022  ns/op   2.335 ± 0.021  ns/op
FireValueChanged.binding                             0,  1  avgt   10   8.858 ±  0.136  ns/op  17.715 ± 0.092  ns/op
FireValueChanged.binding                             1,  0  avgt   10   2.551 ±  0.080  ns/op   2.267 ± 0.028  ns/op
FireValueChanged.binding                             0,  2  avgt   10  10.278 ±  0.148  ns/op  12.049 ± 0.103  ns/op
FireValueChanged.binding                             2,  0  avgt   10   4.831 ±  0.064  ns/op   5.560 ± 0.104  ns/op
FireValueChanged.binding                             2,  2  avgt   10  11.517 ±  0.114  ns/op  14.409 ± 0.511  ns/op
FireValueChanged.binding                             0,  3  avgt   10  11.301 ±  0.096  ns/op  13.756 ± 0.308  ns/op
FireValueChanged.binding                             3,  0  avgt   10   6.411 ±  0.246  ns/op   6.822 ± 0.105  ns/op
FireValueChanged.binding                             3,  3  avgt   10  13.410 ±  0.151  ns/op  16.564 ± 0.305  ns/op
FireValueChanged.binding                             0, 20  avgt   10  17.952 ±  0.182  ns/op  36.591 ± 0.344  ns/op
FireValueChanged.binding                            20,  0  avgt   10  11.817 ±  0.105  ns/op  12.869 ± 0.115  ns/op
FireValueChanged.binding                            20, 20  avgt   10  26.356 ±  0.344  ns/op  50.574 ± 2.497  ns/op
FireValueChanged.longBinding                         0,  0  avgt   10   1.337 ±  0.029  ns/op   2.221 ± 0.022  ns/op
FireValueChanged.longBinding                         0,  1  avgt   10   5.707 ±  0.084  ns/op  14.803 ± 0.051  ns/op
FireValueChanged.longBinding                         1,  0  avgt   10   1.408 ±  0.011  ns/op   2.355 ± 0.029  ns/op
FireValueChanged.longBinding                         0,  2  avgt   10   7.890 ±  0.080  ns/op  13.723 ± 0.329  ns/op
FireValueChanged.longBinding                         2,  0  avgt   10   4.082 ±  0.054  ns/op   5.419 ± 0.107  ns/op
FireValueChanged.longBinding                         2,  2  avgt   10   8.306 ±  0.133  ns/op  13.915 ± 0.206  ns/op
FireValueChanged.longBinding                         0,  3  avgt   10   8.695 ±  0.118  ns/op  16.345 ± 0.172  ns/op
FireValueChanged.longBinding                         3,  0  avgt   10   4.379 ±  0.063  ns/op   6.642 ± 0.155  ns/op
FireValueChanged.longBinding                         3,  3  avgt   10  10.434 ±  0.242  ns/op  16.890 ± 0.188  ns/op
FireValueChanged.longBinding                         0, 20  avgt   10  14.948 ±  0.172  ns/op  57.031 ± 0.631  ns/op
FireValueChanged.longBinding                        20,  0  avgt   10  11.345 ±  0.106  ns/op  12.995 ± 0.162  ns/op
FireValueChanged.longBinding                        20, 20  avgt   10  24.327 ±  0.606  ns/op  64.793 ± 0.903  ns/op
FireValueChanged.property                            0,  0  avgt   10   3.645 ±  0.101  ns/op   3.654 ± 0.053  ns/op
FireValueChanged.property                            0,  1  avgt   10   8.631 ±  0.131  ns/op  19.572 ± 0.224  ns/op
FireValueChanged.property                            1,  0  avgt   10   4.208 ±  0.041  ns/op   3.742 ± 0.066  ns/op
FireValueChanged.property                            0,  2  avgt   10  11.540 ±  0.345  ns/op  15.681 ± 0.240  ns/op
FireValueChanged.property                            2,  0  avgt   10   6.447 ±  0.231  ns/op   7.544 ± 0.165  ns/op
FireValueChanged.property                            2,  2  avgt   10  13.335 ±  0.267  ns/op  15.447 ± 0.365  ns/op
FireValueChanged.property                            0,  3  avgt   10  12.325 ±  0.237  ns/op  17.656 ± 0.488  ns/op
FireValueChanged.property                            3,  0  avgt   10   7.334 ±  0.087  ns/op   8.295 ± 0.151  ns/op
FireValueChanged.property                            3,  3  avgt   10  15.640 ±  0.264  ns/op  18.467 ± 0.489  ns/op
FireValueChanged.property                            0, 20  avgt   10  18.696 ±  0.346  ns/op  51.939 ± 0.965  ns/op
FireValueChanged.property                           20,  0  avgt   10  13.002 ±  0.163  ns/op  14.507 ± 0.334  ns/op
FireValueChanged.property                           20, 20  avgt   10  27.254 ±  0.328  ns/op  48.335 ± 0.916  ns/op

Apr 16 '23 19:04 hjohn

@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

May 15 '23 13:05 bridgekeeper[bot]

@hjohn this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout feature/nested-emission-with-correct-old-values
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Jun 09 '23 01:06 openjdk[bot]

@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Jul 07 '23 13:07 bridgekeeper[bot]

@nlisker I think I addressed all the issues, do you think it can move forward?

Jul 07 '23 13:07 hjohn

@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Aug 22 '23 02:08 bridgekeeper[bot]

Keep it open

Aug 22 '23 06:08 hjohn

@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Sep 19 '23 09:09 bridgekeeper[bot]

Keep it open

Sep 19 '23 12:09 hjohn

@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Nov 14 '23 11:11 bridgekeeper[bot]

@hjohn This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

Dec 12 '23 17:12 bridgekeeper[bot]

I completely forgot about this PR, but it looks very interesting, especially about the nested events. If helpful, I can test this soon in a bigger application, especially for any regressions. And of course also review the code, but need more time for that.

Feb 08 '24 21:02 Maran23

It's the first item on my review list for FX 23. Hope to get the time for the review queue in 1-2 weeks.

Feb 08 '24 21:02 nlisker

I completely forgot about this PR, but it looks very interesting, especially about the nested events. If helpful, I can test this soon in a bigger application, especially for any regressions. And of course also review the code, but need more time for that.

@Maran23 both will be appreciated; I've tested this with my own FX applications for a while, and haven't encountered anything unexpected.

Feb 10 '24 17:02 hjohn

/open

Feb 10 '24 17:02 hjohn

@hjohn This pull request is now open

Feb 10 '24 17:02 openjdk[bot]

Tested with a big application, did not find any regression. All listeners still work as expected, tested especially a lot of 'if something is selected, this button should be enabled/disabled' and the like. I did not checked the code yet, just a little bit. One question: Should I test something special/do you see a case which could cause a problem here?

Mar 08 '24 18:03 Maran23

Tested with a big application, did not find any regression. All listeners still work as expected, tested especially a lot of 'if something is selected, this button should be enabled/disabled' and the like. I did not checked the code yet, just a little bit. One question: Should I test something special/do you see a case which could cause a problem here?

I think testing it with a big application is excellent (I do the same here with my own large application), and I'm happy to hear you did not spot any regressions!

This change does change (unspecified) behavior a little bit, although I think well within the documented contract of how change listeners operate (ie. applications should not be relying on the unspecified behavior).

It will be hard to notice any change directly, as the changes are all related to advanced usage of listeners (adding/removing listeners during listener callbacks) or nested changes (the same property gets changed during a callback, directly or indirectly).

During a listener callback, you remove a listener that is currently being notified (ie. yourself, or any listener that may have triggered your callback that is further up the call stack)

Before this change, there is a chance that such a removed listener may still be called a few more times, despite it being removed in nested cases; its doubtful anyone is relying on that behavior, and (IMHO) these extra callbacks are more likely to break things, because things have been cleaned up (who assumes that their listener might still get called after removal?)

During a listener callback, you add a listener that is currently being notified (this can't be yourself, but it could be a listener on a property that triggered your callback that is further up the stack)

Before this change, this newly added listener may start participating in the current nested listener notification (ie. it would start participating halfway during a chain of nested changes). After this change, such a newly added listener would only be notified on the next top level change. It's unlikely anyone is relying on this behavior.

During a listener callback, you change the value the property it is attached to (or any property that may have led to your callback being called further up the callstack).

The most "common" scenario for this is veto-ing changes. For example, a property that can't be empty may get set to a non-empty value (or its original value) in a listener, triggering another notification. Your listener would not notice any differences (it would get called immediately with the change that you just did), but any listeners added after yours may not "see" the empty value because it was changed to something else before they were called. They will only see the new value (and the corresponding correct old value since last they were called) -- they may also be not called at all if the value was changed back to its original, and so they would be completely unaware of the temporary change.

Mar 09 '24 05:03 hjohn

❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.

Mar 13 '24 20:03 openjdk[bot]

ListenerManager is an obvious improvement, as it fixes incorrect behavior and allows listeners to veto changes. However, the behavior of ListenerManager is also an implementation detail and not documented anywhere. This leads me to the following questions:

How will users know that they can now do all of the things that were previously broken? Do we need a specification for what is allowed and what's not allowed?
Should this behavior be required for all valid ObservableValue implementations? (This would render many existing implementations defective.)
If ObservableValue implementations are not required to replicate the ListenerManager behavior, we should probably make it easily discoverable whether any particular implementation (most of them are properties) supports nested changes/vetoing. In most of the public API, there's no obvious way to see (without looking at the source code) whether a property implementation extends one of the *PropertyBase classes.

Mar 25 '24 13:03 mstr2

ListenerManager is an obvious improvement, as it fixes incorrect behavior and allows listeners to veto changes. However, the behavior of ListenerManager is also an implementation detail and not documented anywhere. This leads me to the following questions:

How will users know that they can now do all of the things that were previously broken? Do we need a specification for what is allowed and what's not allowed?

Currently the specification is vague enough that there's a lot of wiggle room. For example, we don't specify whether invalidation listeners are called before change listeners, yet a lot of code will be relying on that unknowingly. We also don't specify whether successive change listener calls should always be a change (ie. never get A -> A), or that it should match with what the previous change reported (ie. if called with ? -> B, then the next call must be B -> ?).

IMHO we should though. I would specify for example that:

Invalidation listeners are called before Change listeners (reason: invalidation listeners are a lower level concept defined in a higher level interface). They definitely should not be mixed (they're defined by two different interfaces).
Change listeners should (obviously as this MR fixes this) guarantee the old value makes sense:
- Old value will be equal to previous new value (essential for patterns that use the old value to unregister a related listener)
- Never called when old value equals new value (it's not a change then) -- this allows vetoing, and generally saves unnecessary calls

We should probably also specify the order of calls (as code will again unknowingly be relying on this already):

A listener registered after a listener of the same type will always be called after earlier registered listeners (code relies on this in various ways, even in FX itself)
Listeners of different types follow a fixed order: invalidation first, changes second (code relies on this already)
The behavior of ObservableValues that contain mutable values (ie. lists/sets/maps/atomics) will be undefined if those values are mutated while held by an observable (same as when you mutate keys that are currently part of a Set).

We can also specify some behavior with regards to when an event can be received when adding and removing listeners, although I think that's less of an issue.

Should this behavior be required for all valid ObservableValue implementations? (This would render many existing implementations defective.)

It's hard to require anything in an interface, but I think the interface should specify this regardless. Just look at an interface like Set that requires a specific way of implementing hashCode. You can violate it easily, but you will suffer the consequences when comparing sets of different types. Same with custom implementations of ObservableValue. You take a risk when using some unvetted 3rd party implementation.

At a minimum all implementations in JavaFX should follow the specification. This will likely cover most implementations of ObservableValue, leaving only a few custom implementations that are not 100% spec compliant (note: a lot of the problems only occur with nested changes, which occur only in complicated code that triggers a cascade of changes, usually layout/skin/css related).

A problem there are the Set/List/Map ObservableValue implementations. They are not observable values, they are observable collections that deserve their own interface. Invalidation listeners are fine, but value listeners make no sense. I've looked into these before, and all I can say is that they take great liberties with what is considered a "change" (ignoring even the most basic specifications). I'd recommend deprecating the observable value parts of these, and moving users towards either invalidation or the collection specific change listeners.

If ObservableValue implementations are not required to replicate the ListenerManager behavior, we should probably make it easily discoverable whether any particular implementation (most of them are properties) supports nested changes/vetoing. In most of the public API, there's no obvious way to see (without looking at the source code) whether a property implementation extends one of the *PropertyBase classes.

I think if the implementation is in javafx.* it should be correct. Anyone can violate any interface (just look at custom collection implementations which often fail to follow the spec). We could provide a more lenient abstract base class or helper to make it easier to conform to the spec.

Mar 25 '24 21:03 hjohn

@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Apr 22 '24 23:04 bridgekeeper[bot]

@hjohn This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

May 31 '24 16:05 bridgekeeper[bot]

/open

Jun 16 '24 21:06 hjohn

@hjohn This pull request is now open

Jun 16 '24 21:06 openjdk[bot]

@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Jul 15 '24 00:07 bridgekeeper[bot]

jfx jfx copied to clipboard

8290310: ChangeListener events are incorrect or misleading when a nested change occurs

Behavior

Nested notifications:

Performance

Memory Use

About nested changes

How do nested changes look?

ExpressionHelper

ListenerManager (new)

About Invocation Order

About Listener Add/Remove performance

About Old Value Correctness

The PR

Progress

Issue

Reviewers

Reviewing

Behavior

Examples

Equality check

Performance

Equality check

Performance

Webrevs

Benchmarks

jfx
jfx copied to clipboard