<html><head></head><body><div style="font-family: Verdana;font-size: 12.0px;"><div>+1 for this change.<br/>
This also has the nice side effect that EventFilter/-Handler can be e.g. added to the Dialog now.</div>
<div>
<div>I wonder if it makes sense to make to also make those methods final then. At least where we can do it without breaking anything.</div>
<div> </div>
<div>-- Marius</div>
<div>
<div name="quote" style="margin:10px 5px 5px 10px; padding: 10px 0 10px 10px; border-left:2px solid #C3D9E5; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">
<div style="margin:0 0 10px 0;"><b>Gesendet:</b> Mittwoch, 12. April 2023 um 21:04 Uhr<br/>
<b>Von:</b> "Michael Strauß" <michaelstrau2@gmail.com><br/>
<b>An:</b> "Kevin Rushforth" <kevin.rushforth@oracle.com><br/>
<b>Cc:</b> openjfx-dev@openjdk.org<br/>
<b>Betreff:</b> Re: Promote addEventHandler/removeEventHandler to EventTarget interface</div>
<div name="quoted-content">I've created a draft PR with the proposed changes:<br/>
<a href="https://github.com/openjdk/jfx/pull/1090" target="_blank">https://github.com/openjdk/jfx/pull/1090</a><br/>
<br/>
Adding event handler management methods to `EventTarget` greatly<br/>
increases the usefulness of the event system for the reasons John and<br/>
I mentioned earlier.<br/>
I want to circle back to the compatibility issues that arise from the<br/>
proposed change:<br/>
<br/>
In general, adding a default-implemented method to an existing<br/>
interface is not in itself an incompatible change. However,<br/>
compatibility issues can arise when the new interface method doesn't<br/>
override, but clash with the erasure of a preexisting method on an<br/>
implementation of that interface.<br/>
This is exactly the issue we are facing, but the circumstances in<br/>
which this matters are extremely limited:<br/>
<br/>
The affected classes are Menu, MenuItem, TreeColumnBase and TreeItem.<br/>
<br/>
All of these classes provide a version of `addEventHandler` and<br/>
`removeEventHandler`, but in contast to all other implementations, the<br/>
signature of the method is slightly different.<br/>
While the usual signature is the following:<br/>
void addEventHandler(EventType<T>, EventHandler<? super T>),<br/>
the mentioned classes instead declare a method with this signature:<br/>
void addEventHandler(EventType<T>, EventHandler<T>).<br/>
<br/>
Note that the event handler is parameterized as <T>, not <? super T>.<br/>
This is most likely an oversight, since there doesn't seem to be a<br/>
justification to restrict event handlers to exactly T.<br/>
<br/>
Changing the method signature to conform to the correct version <?<br/>
super T> is a binary-compatible change, since the erasure of both<br/>
methods remains the same.<br/>
However, it is not a source-compatible change. But the ways in which<br/>
this source incompatibility manifests are very limited:<br/>
<br/>
Callers of the modified API are not affected, since T <= ? super T.<br/>
This probably accounts for the vast majority of cases.<br/>
Overriding implementations will fail to compile without changing the<br/>
method signature.<br/>
<br/>
To summarize, here are the two cases where the proposed changes cause<br/>
compatibility issuse:<br/>
<br/>
1. Existing `EventTarget` implementations that add a non-overriding,<br/>
but erasure-equal `addEventHandler` method.<br/>
2. Subclasses of Menu, MenuItem, TreeColumnBase, or TreeItem that<br/>
override the `addEventHandler` method.<br/>
<br/>
The first case is the kind of risk that always comes with new<br/>
default-implemented interface methods. In this case, I think this is<br/>
where most of real-world compatibility problems could arise.<br/>
The second case doesn't seem relevant to me, because I don't see a<br/>
good reason why a subclass of Menu, MenuItem, TreeColumnBase, or<br/>
TreeView would override the `addEventHandler`, ... methods.<br/>
<br/>
In the end, as with all additions to existing interfaces, we need to<br/>
carefully consider the benefits of the enhancement against potential<br/>
risks.<br/>
In this case, I think the risks are quite low, while the benefits are<br/>
very significant.<br/>
<br/>
<br/>
On Tue, Mar 7, 2023 at 3:35 PM Kevin Rushforth<br/>
<kevin.rushforth@oracle.com> wrote:<br/>
><br/>
> An incompatible change in a fundamental interface such as EventTarget is<br/>
> not something we would want to do, so any proposed changes will need to<br/>
> retain compatibility (both binary and source). At a minimum, this means<br/>
> that any newly added methods would need to have a default<br/>
> implementation, and we would need to look closely at other aspects of<br/>
> compatibility.<br/>
><br/>
> -- Kevin<br/>
><br/>
><br/>
> On 3/7/2023 1:24 AM, John Hendrikx wrote:<br/>
> > Hi Michael,<br/>
> ><br/>
> > Did you file a JIRA issue for this one?<br/>
> ><br/>
> > I've recently also been doing some rewriting to use the Event system<br/>
> > more. I'm removing custom Scene walking code (and looking at<br/>
> > Node.getProperties) to do "event handling", and I've now discovered<br/>
> > that it could be done quite a bit nicer by using the provided event<br/>
> > mechanism.<br/>
> ><br/>
> > I've encountered a few things that annoy me about the event system:<br/>
> ><br/>
> > 1) I'm making use of Presentation classes (Models) that only associate<br/>
> > with things in javafx.base (Event, EventTarget, Properties). These<br/>
> > presentations need to register event handlers at some point, but this<br/>
> > can only be done on Nodes; having this logic close to the Presentation<br/>
> > code makes sense, but it would make them dependent on javafx.graphics;<br/>
> > if I could do this via EventTarget, the dependency would not be needed.<br/>
> ><br/>
> > 2) When you fire an event (via Node.fireEvent for example), there is<br/>
> > no feedback you can obtain whether the event was consumed or not as<br/>
> > the final event is not returned to check `isConsumed` on (internal<br/>
> > calls do have this information, but it is not exposed at the last step).<br/>
> ><br/>
> > 3) Related to 2), I think EventTarget could also have a method to<br/>
> > dispatch events (so you can dispatch an event easily to any target,<br/>
> > not just via the convenience method Node#fireEvent). See this ticket:<br/>
> > <a href="https://bugs.openjdk.org/browse/JDK-8303209" target="_blank">https://bugs.openjdk.org/browse/JDK-8303209</a><br/>
> ><br/>
> > Perhaps we can work together on this, or if you're not currently<br/>
> > working on it I could take a stab at solving all of these issues.<br/>
> ><br/>
> > --John<br/>
> ><br/>
> > On 17/03/2022 21:01, Michael Strauß wrote:<br/>
> >> I'm working on an application that uses the JavaFX event system<br/>
> >> extensively, and I'm finding it quite hard to use common code for<br/>
> >> event handler registrations.<br/>
> >><br/>
> >> The problem is that the `EventTarget` interface contains no<br/>
> >> addEventHandler/removeEventHandler methods, and as a consequence of<br/>
> >> that, code that uses `EventTarget` ends up requiring lots of brittle<br/>
> >> instanceof tests to call these methods on all the different<br/>
> >> implementations of `EventTarget`.<br/>
> >><br/>
> >> There are three buckets of `EventTarget` implementations:<br/>
> >><br/>
> >> 1) Implementations that declare the following methods:<br/>
> >> <T extends Event> void addEventHandler(EventType<T>,<br/>
> >> EventHandler<? super T>);<br/>
> >> <T extends Event> void removeEventHandler(EventType<T>,<br/>
> >> EventHandler<? super T>);<br/>
> >> <T extends Event> void addEventFilter(EventType<T>, EventHandler<?<br/>
> >> super T>);<br/>
> >> <T extends Event> void removeEventFilter(EventType<T>,<br/>
> >> EventHandler<? super T>);<br/>
> >> --> Node, Scene, Window, Transform, Task, Service<br/>
> >><br/>
> >> 2) Implementations that declare the following methods:<br/>
> >> <T extends Event> void addEventHandler(EventType<T>,<br/>
> >> EventHandler<T>);<br/>
> >> <T extends Event> void removeEventHandler(EventType<T>,<br/>
> >> EventHandler<T>);<br/>
> >> --> MenuItem, TreeItem, TableColumnBase<br/>
> >><br/>
> >> (Note that the EventHandler argument ist parameterized as<br/>
> >> EventHandler<T>, not EventHandler<? super T> as in the first set of<br/>
> >> methods.)<br/>
> >><br/>
> >> 3) Implementations that don't declare any methods to add or remove<br/>
> >> event handlers:<br/>
> >> --> Dialog, Tab<br/>
> >><br/>
> >><br/>
> >> I think the situation can be improved by promoting the bucket 1<br/>
> >> methods to the `EventTarget` interface, so they can be used<br/>
> >> consistently across all implementations of `EventTarget`.<br/>
> >><br/>
> >> This works seamlessly for bucket 1 and bucket 3 implementations.<br/>
> >><br/>
> >> With bucket 2, there's the problem that, inconsistently, the<br/>
> >> EventHandler<T> argument is not a lower-bounded wildcard.<br/>
> >> Unfortunately, a method with an EventHandler<T> parameter cannot<br/>
> >> implement an interface method that expects EventHandler<? super T>.<br/>
> >><br/>
> >> However, since the erasure of the method remains the same, changing<br/>
> >> the method signature would technically be a binary-compatible change.<br/>
> >><br/>
> >> Do you think this is a useful improvement?<br/>
></div>
</div>
</div>
</div></div></body></html>