Hi Martin,
I'm still unsure how this exactly is supposed to go wrong.
Whenever you dispatch a new event, you should always follow-up and check
if it was consumed after the dispatch finishes. If it is, then you mark
the triggering event also as consumed. If this is done consistently
everywhere, then the consumed flag should function as expected or not?
Code that does is simply wrong:
e -> {
node.fireEvent(new KeyEvent(...)); // can't check if it
was consumed!
// consume "e" or not?
}
The above is a bug, and needs to be fixed if it is present in FX.
So the way I see it, every time you fire a new event in response to
another, it is your responsibility to ensure that you check if the new
event was consumed, and if it was, to mark the initial event as consumed
as well. Following this everywhere should make the consumed flag work
as expected.
What am I missing?
--John
On 12/11/2024 19:14, Martin Fox wrote:
(Here’s hoping this e-mail makes it out to the list. If this is an ongoing
problem I might have to switch to using my gmail address.)
I’m writing up some notes to improve the documentation on event dispatch. There
seems to be only one rule governing dispatchers and that is “return null from
dispatchEvent to end dispatch”. Beyond that anything goes.
From what I can tell this PR will work if the event passed to the handler is
the original event or a copy created using copyFor() and the event returned
from dispatchEvent is either the same one that was passed to the handlers or a
copy created using copyFor(). These are all reasonable assumptions but there’s
nothing in the API that enforces them. In the worst case an event dispatcher
may receive an event, pass an entirely new event to one of its handlers, and
then return yet another entirely new event from dispatchEvent.
(In the codebase there are three classes that implement EventDispatcher and
thirteen that extend an existing EventDispatcher. My guess is that all of them
behave reasonably and copy their events using copyFor but I haven’t actually
cracked open the code to verify that.)
At this point I’ll resort to hand-waving and speculation. Perhaps this should
be divorced from events and reframed in terms of the dispatch process. Instead
of “do this if this event is not consumed” it’s “do this if the current
dispatch ends without consuming an event”. The unconsumed event handlers would
then be attached to the dispatch chain instead of the event. And that’s about
as far as I’ve thought this through.
Martin
On Nov 12, 2024, at 5:02 AM, John Hendrikx <john.hendr...@gmail.com> wrote:
I think `discardUnconsumedEventHandlers` is still a bit unpredictable, as it
would depend on handler order.
The user handler would need to be called *after* Skin/Behavior handlers for it
to be able to always discard any ifUnconsumed callbacks. If the Skin is
replaced, then it will re-register handlers and those will then be called
last...
Of course, if we change Behaviors to do their work in filters, then we could
get this to work properly.
--John
On 10/11/2024 08:12, Michael Strauß wrote:
In JavaFX, user code and skins share the same event system. Since
invocation order is fundamentally important for events, this leads to
a lot of problems when skins add event handlers to their controls.
Assume user code adds an event handler to a control, and _then_ sets a
skin that also adds an event handler for the same event. In this case,
the user-provided handler is invoked first. If the skin is set
_first_, the skin gets the first chance to handle the event (see also
https://bugs.openjdk.org/browse/JDK-8231245).
Prioritized event handlers might be a solution for this problem, but
they are quite difficult to get right. Instead, I think we can get
almost all of the benefits using a much simpler solution: unconsumed
event handlers.
We add a new method to the `Event` class:
<E extends Event> void ifUnconsumed(EventHandler<E> handler)
When an event filter or an event handler receives an event, it calls
the `ifUnconsumed` method with another event handler. Then, after both
phases of event delivery have completed, the list of unconsumed event
handlers associated with the event is invoked in sequence. Once an
unconsumed event handler consumes the event, further propagation is
stopped.
Skins and their behaviors would then always use the unconsumed form of
event handlers:
// inside a skin/behavior
control.addEventHandler(
KeyEvent.KEY_PRESSED,
e -> e.ifUnconsumed(event -> {
// act on the event
}));
This allows user code to see an event during both the capture and
bubble phases without any inteference of the skin or behavior. If user
code doesn't consume the event, the skin or behavior gets to act on
it.
In addition to that, we add a second new method to the `Event` class:
void discardUnconsumedEventHandlers()
Calling this method in an event handler allows user code to discard
any unconsumed event handlers that have been registered prior. Let's
consider an example where user code wants to prevent a control skin
from acting on the ENTER key event, but it also doesn't want to
consume the event. Adding an event handler to the control, and then
calling `discardUnconsumedEventHandlers()` inside of the event handler
achieves exactly that: it allows the ENTER key event to flow freely
through the control without interacting with it, since its unconsumed
event handlers will never be called.
Here is the PR for this proposal: https://github.com/openjdk/jfx/pull/1633
