<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:"Times New Roman \(Body CS\)";
panose-1:2 11 6 4 2 2 2 2 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
font-size:10.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
span.EmailStyle19
{mso-style-type:personal-reply;
font-family:"Courier New";
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;
mso-ligatures:none;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style>
</head>
<body lang="EN-US" link="blue" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New"">I think this is a good idea, look what I had to do:<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""><a href="https://github.com/openjdk/jfx/blob/f28896aa63592a37e7f78263548f3b2d4f2bc381/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ListenerHelper.java#L396">https://github.com/openjdk/jfx/blob/f28896aa63592a37e7f78263548f3b2d4f2bc381/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ListenerHelper.java#L396</a><o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New"">I do agree with Kevin that we should look closely at any possible compatibility issues. I suspect the impact might be minimal due to the fact that it's highly unlikely for anyone
to extend the EventTarget interface, but I could be wrong.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New"">-andy<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""><o:p> </o:p></span></p>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal" style="margin-bottom:12.0pt"><b><span style="font-size:12.0pt;color:black">From:
</span></b><span style="font-size:12.0pt;color:black">openjfx-dev <openjfx-dev-retn@openjdk.org> on behalf of Michael Strauß <michaelstrau2@gmail.com><br>
<b>Date: </b>Wednesday, April 12, 2023 at 12:05<br>
<b>To: </b>Kevin Rushforth <kevin.rushforth@oracle.com><br>
<b>Cc: </b>openjfx-dev@openjdk.org <openjfx-dev@openjdk.org><br>
<b>Subject: </b>Re: Promote addEventHandler/removeEventHandler to EventTarget interface<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt">I've created a draft PR with the proposed changes:<br>
<a href="https://github.com/openjdk/jfx/pull/1090">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">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>
><o:p></o:p></span></p>
</div>
</div>
</body>
</html>