Default event handlers

[mstr2]

Implementation of default event handlers.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2011

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

Using diff file

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

[bridgekeeper]

👋 Welcome back mstrauss! 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.

[openjdk]

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

[hjohn]

Why did you choose to not simply register the default handlers on the event itself? If cloning of the list is a concern, then you may choose to not modify the list when a handler is added, but instead replace it with a new list. The clone (which will likely happen far more often) can then simply copy the pointer instead of the contents.

It seems to me that the locking mechanism, and the other restrictions can then simply be dropped. At the end of bubbling, you just ask for the list and proceed with processing these handlers.

[mstr2]

As an event can be transformed as it travels through the dispatch chain, we can't effectively enforce that it only ever be cloned or copyFor'ed so that its internal fields carry over to the next event instance. Attaching the default handlers to the invocation instead of the event seems to be the safer choice here.

The locking mechanism is also intended to alert users that they're using the API incorrectly, as it is explicitly intended that ifUnconsumed() is only ever called from the event handler of that particular event.

[hjohn]

By transformed you mean converted to a different event type?

I feel the use of the accessor pattern here is overkill. It makes a pretty neutral class representing events permanently attached to FX framework internals, for IMHO a minor benefit.

I see in the test this the case we're talking about:

@Test
void cannotAddDefaultHandlerAfterDeliveryIsComplete() {
    target2.addEventFilter(EmptyEvent.ANY, e -> {
        e.ifUnconsumed(e2 -> {
            // This call will fail with IllegalStateException:
            e2.ifUnconsumed(_ -> {});
        });
    });

    assertThrows(IllegalStateException.class, () -> EventUtil.fireEvent(target2, new EmptyEvent()));
}

I have the impression that you could probably prevent that in EventDispatchContext. When you start iterating over defaultHandlers you could set a flag just before to "lock" it. Then in addDefaultHandler you could only allow adding more handlers if we're not already doing callbacks on the list of default handlers?

[mstr2]

By transformed you mean converted to a different event type?

Yes, for example as it is done with DirectEvent and RedirectedEvent.

I feel the use of the accessor pattern here is overkill. It makes a pretty neutral class representing events permanently attached to FX framework internals, for IMHO a minor benefit.

You won't be able to use this class without at least bringing in javafx.base, and in that case you already have all the internals. What's the concern here? Do you imagine javafx.event.Event class be useful outside of JavaFX?

In any case, I've changed the implementation to not require an accessor for Event. The currently dispatched event will instead be tracked in EventDispatchContext, where we can check whether any of its methods is called with an unexpected event instance.

[hjohn]

Thanks, the refactor looks a lot cleaner and easier to reason about, with less mutable state on event itself that's magically accessible.

[mstr2]

I've added registration methods for default handlers to EventTarget. These are mostly for convenience, but there are some advantages:

1. Using addDefaultEventHandler() is a drop-in replacement for addEventHandler(), with the same semantics for registration and unregistration. Existing event handlers don't need to be changed. This is a good option for code that always defers event handling to the default phase (like skins).
2. On the other hand, Event.ifUnconsumed() allows finer-grained control: users can inspect the event before deciding to defer event handling to the default phase.