I've done some preliminary testing on the limits of what's possible.
Disallowing nested changes broke about 50 tests. Changing the order of
listener events broke 9 tests (I reversed the order, see this diff for
what I did:
https://github.com/hjohn/jfx/commit/c7588c49d42257f75627f15ca7ed01147d5e5e3d).
I investigated the tests that broke when the ordering changed. They're
all Tree/Table/ListView tests. The tests breaking all seem to interact
with the selection and focus model, so there might be some kind of
ordering dependency in there (list of the 9 tests that broke added at
end of this post).
Disallowing nested emissions broke far more, Spinner, ComboBox,
ColorPicker, Scroll related things, TableCell. I didn't run systemTests
or web tests, so there might be even more that breaks.
Further comments inline:
On 10/07/2022 20:11, Nir Lisker wrote:
There was a previous implementation discussion that touched on some of
the points here in https://github.com/openjdk/jfx/pull/108
I think there are a few (semi-)independent points here:
1. Order of listeners: as John said, there is no specification of the
order of listeners, but the add/removeListener methods imply that
there is some sort of order by saying that the "first occurrence" will
be removed.
2. Dispatch order: the docs don't specify (from what I could find) the
order in which listeners receive events. That is, if listener1 and
listener2 are registered in that order, I didn't find anything that
specifies that they will be notified in this order, although It makes
sense that they are. From the API perspective, there is no
`getListeners()` method or the like that allows me to see the
order, so I have to "remember" that order somehow, and that's an
indication that the order is actually not important.
Furthermore, the order is separate for invalidation listeners and
change listeners: first all invalidation listeners are notified, then
all change listeners, even if all the change listeners were
registered before. This alone can be surprising.
3. Concurrent modification of listeners: if a listener is added or
removed from another thread (while an event is being sent through the
listener list or otherwise), what should happen.
4. Nested events (relevant only for change listeners I think): if a
value is changed inside a listener, what should happen. The docs in
ChangeListener#changed mention that it is "bad practice", but don't
prohibit it.
Thanks, good summary there.
I have some thoughts, but I haven't constructed an opinion yet:
1+2. It should be tested how much practical maneuvering room we have
here first. If we don't keep an ordered collection of listeners, or
don't dispatch the event to them in that order, or even if we dispatch
first to all change listeners and then to all invalidation listeners,
what breaks? This will give us an idea of our options.
3. I agree with Michael that as long as there is no mention of thread
safety in any of the relevant classes (observables and listeners), the
behavior should be unspecified, exactly as it is in core JDK classes
that don't specify anything. We can revisit this when the GC part of
the story is on the move, as John said.
4. I don't think we can restrict nested changes at this point
considering the docs, and I remember some classes in JavaFX doing this
(I think it was in the animation package). However, I propose to look
at another approach: The event dispatcher will use an iterator, and
whatever changes happen during iteration will reflect on it (if they
are relevant). It is not guaranteed that all listeners that were
registered *at the time of the event* will receive it, but only those
that are registered in time for a dispatch. It removes the hurdles of
locking or maintaining a queue. Also, if I have a listener1 whose job
is to remove a listener2 when an event is received, it's very probable
that I don't want listener2 to react to the event (unless it wasn't
registered before listener1, in which case "too late", but see also
points 1+2).
If this approach is viable, it should also be tested practically.
As for the last point, I don't think we should change a current event
emission. When an event occurs, I think every one that was registered
should receive it (which is currently ensured by making a copy). Doing
it differently (allowing the list to change during emission of a single
event) would actually introduce ordering effects and would be a much
harder behavior to document.
As for it removing hurdles, I think a copy free approach is already
possible *if* we specify that changes are historical (ie. everyone at
the time of registration receives the exact same change event,
regardless of nested value changes or nested listener list changes).
If changes are historical, the implementation probably must delay nested
value changes to adhere to the specification.
If we further specify (or implement in this case) that for a single
property, no more than a single emission can run concurrently (this
seems reasonable IMHO, especially given the fact that concurrent
emissions probably hardly happen currently as it would break too much
things in unexpected ways), then the implementation can buffer any list
modifications made.
All the above taken together will allow a copy free use of listener
list. A lock is still needed to detect a nested emission, when to buffer
changes and to prevent emissions on another thread.
--John
ListViewTest > ensureSelectionIsCorrectWhenItemsChange FAILED
java.lang.AssertionError: expected:<0> but was:<-1>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:647)
at org.junit.Assert.assertEquals(Assert.java:633)
at
test.javafx.scene.control.ListViewTest.ensureSelectionIsCorrectWhenItemsChange(ListViewTest.java:337)
TableViewTest > ensureSelectionIsCorrectWhenItemsChange FAILED
java.lang.AssertionError: expected:<0> but was:<-1>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:647)
at org.junit.Assert.assertEquals(Assert.java:633)
at
test.javafx.scene.control.TableViewTest.ensureSelectionIsCorrectWhenItemsChange(TableViewTest.java:351)
TreeTableViewTest >
test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at
test.javafx.scene.control.TreeTableViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeTableViewTest.java:2315)
TreeTableViewTest >
test_rt27180_expandBranch_laterSiblingAndChildrenSelected FAILED
java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at
test.javafx.scene.control.TreeTableViewTest.test_rt27180_expandBranch_laterSiblingAndChildrenSelected(TreeTableViewTest.java:2340)
TreeTableViewTest > test_rt27185 FAILED
java.lang.AssertionError: expected:<TreeItem [ value: Mike Graham
]> but was:<null>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:120)
at org.junit.Assert.assertEquals(Assert.java:146)
at
test.javafx.scene.control.TreeTableViewTest.test_rt27185(TreeTableViewTest.java:1569)
TreeViewTest >
test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at
test.javafx.scene.control.TreeViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeViewTest.java:1018)
TreeViewTest >
test_rt27180_collapseBranch_laterSiblingAndChildrenSelected FAILED
java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at
test.javafx.scene.control.TreeViewTest.test_rt27180_collapseBranch_laterSiblingAndChildrenSelected(TreeViewTest.java:999)
TreeViewTest > test_rt27180_expandBranch_laterSiblingAndChildrenSelected
FAILED
java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at
test.javafx.scene.control.TreeViewTest.test_rt27180_expandBranch_laterSiblingAndChildrenSelected(TreeViewTest.java:1043)
TreeViewTest > test_rt27185 FAILED
java.lang.AssertionError: expected:<TreeItem [ value: Mike Graham
]> but was:<null>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:120)
at org.junit.Assert.assertEquals(Assert.java:146)
at
test.javafx.scene.control.TreeViewTest.test_rt27185(TreeViewTest.java:630)
