Promote addEventHandler/removeEventHandler to EventTarget interface

[Michael Strauß]

I've created a draft PR with the proposed changes:
https://github.com/openjdk/jfx/pull/1090

Adding event handler management methods to `EventTarget` greatly
increases the usefulness of the event system for the reasons John and
I mentioned earlier.
I want to circle back to the compatibility issues that arise from the
proposed change:

In general, adding a default-implemented method to an existing
interface is not in itself an incompatible change. However,
compatibility issues can arise when the new interface method doesn't
override, but clash with the erasure of a preexisting method on an
implementation of that interface.
This is exactly the issue we are facing, but the circumstances in
which this matters are extremely limited:

The affected classes are Menu, MenuItem, TreeColumnBase and TreeItem.

All of these classes provide a version of `addEventHandler` and
`removeEventHandler`, but in contast to all other implementations, the
signature of the method is slightly different.
While the usual signature is the following:
    void addEventHandler(EventType<T>, EventHandler<? super T>),
the mentioned classes instead declare a method with this signature:
    void addEventHandler(EventType<T>, EventHandler<T>).

Note that the event handler is parameterized as <T>, not <? super T>.
This is most likely an oversight, since there doesn't seem to be a
justification to restrict event handlers to exactly T.

Changing the method signature to conform to the correct version <?
super T> is a binary-compatible change, since the erasure of both
methods remains the same.
However, it is not a source-compatible change. But the ways in which
this source incompatibility manifests are very limited:

Callers of the modified API are not affected, since T <= ? super T.
This probably accounts for the vast majority of cases.
Overriding implementations will fail to compile without changing the
method signature.

To summarize, here are the two cases where the proposed changes cause
compatibility issuse:

1. Existing `EventTarget` implementations that add a non-overriding,
but erasure-equal `addEventHandler` method.
2. Subclasses of Menu, MenuItem, TreeColumnBase, or TreeItem that
override the `addEventHandler` method.

The first case is the kind of risk that always comes with new
default-implemented interface methods. In this case, I think this is
where most of real-world compatibility problems could arise.
The second case doesn't seem relevant to me, because I don't see a
good reason why a subclass of Menu, MenuItem, TreeColumnBase, or
TreeView would override the `addEventHandler`, ... methods.

In the end, as with all additions to existing interfaces, we need to
carefully consider the benefits of the enhancement against potential
risks.
In this case, I think the risks are quite low, while the benefits are
very significant.


On Tue, Mar 7, 2023 at 3:35 PM Kevin Rushforth
<kevin.rushforth at oracle.com> wrote:
>
> An incompatible change in a fundamental interface such as EventTarget is
> not something we would want to do, so any proposed changes will need to
> retain compatibility (both binary and source). At a minimum, this means
> that any newly added methods would need to have a default
> implementation, and we would need to look closely at other aspects of
> compatibility.
>
> -- Kevin
>
>
> On 3/7/2023 1:24 AM, John Hendrikx wrote:
> > Hi Michael,
> >
> > Did you file a JIRA issue for this one?
> >
> > I've recently also been doing some rewriting to use the Event system
> > more.  I'm removing custom Scene walking code (and looking at
> > Node.getProperties) to do "event handling", and I've now discovered
> > that it could be done quite a bit nicer by using the provided event
> > mechanism.
> >
> > I've encountered a few things that annoy me about the event system:
> >
> > 1) I'm making use of Presentation classes (Models) that only associate
> > with things in javafx.base (Event, EventTarget, Properties). These
> > presentations need to register event handlers at some point, but this
> > can only be done on Nodes; having this logic close to the Presentation
> > code makes sense, but it would make them dependent on javafx.graphics;
> > if I could do this via EventTarget, the dependency would not be needed.
> >
> > 2) When you fire an event (via Node.fireEvent for example), there is
> > no feedback you can obtain whether the event was consumed or not as
> > the final event is not returned to check `isConsumed` on (internal
> > calls do have this information, but it is not exposed at the last step).
> >
> > 3) Related to 2), I think EventTarget could also have a method to
> > dispatch events (so you can dispatch an event easily to any target,
> > not just via the convenience method Node#fireEvent).  See this ticket:
> > https://bugs.openjdk.org/browse/JDK-8303209
> >
> > Perhaps we can work together on this, or if you're not currently
> > working on it I could take a stab at solving all of these issues.
> >
> > --John
> >
> > On 17/03/2022 21:01, Michael Strauß wrote:
> >> I'm working on an application that uses the JavaFX event system
> >> extensively, and I'm finding it quite hard to use common code for
> >> event handler registrations.
> >>
> >> The problem is that the `EventTarget` interface contains no
> >> addEventHandler/removeEventHandler methods, and as a consequence of
> >> that, code that uses `EventTarget` ends up requiring lots of brittle
> >> instanceof tests to call these methods on all the different
> >> implementations of `EventTarget`.
> >>
> >> There are three buckets of `EventTarget` implementations:
> >>
> >> 1) Implementations that declare the following methods:
> >>      <T extends Event> void addEventHandler(EventType<T>,
> >> EventHandler<? super T>);
> >>      <T extends Event> void removeEventHandler(EventType<T>,
> >> EventHandler<? super T>);
> >>      <T extends Event> void addEventFilter(EventType<T>, EventHandler<?
> >> super T>);
> >>      <T extends Event> void removeEventFilter(EventType<T>,
> >> EventHandler<? super T>);
> >> --> Node, Scene, Window, Transform, Task, Service
> >>
> >> 2) Implementations that declare the following methods:
> >>      <T extends Event> void addEventHandler(EventType<T>,
> >> EventHandler<T>);
> >>      <T extends Event> void removeEventHandler(EventType<T>,
> >> EventHandler<T>);
> >> --> MenuItem, TreeItem, TableColumnBase
> >>
> >> (Note that the EventHandler argument ist parameterized as
> >> EventHandler<T>, not EventHandler<? super T> as in the first set of
> >> methods.)
> >>
> >> 3) Implementations that don't declare any methods to add or remove
> >> event handlers:
> >> --> Dialog, Tab
> >>
> >>
> >> I think the situation can be improved by promoting the bucket 1
> >> methods to the `EventTarget` interface, so they can be used
> >> consistently across all implementations of `EventTarget`.
> >>
> >> This works seamlessly for bucket 1 and bucket 3 implementations.
> >>
> >> With bucket 2, there's the problem that, inconsistently, the
> >> EventHandler<T> argument is not a lower-bounded wildcard.
> >> Unfortunately, a method with an EventHandler<T> parameter cannot
> >> implement an interface method that expects EventHandler<? super T>.
> >>
> >> However, since the erasure of the method remains the same, changing
> >> the method signature would technically be a binary-compatible change.
> >>
> >> Do you think this is a useful improvement?
>