<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
I also think this would be a good change, after a little more
looking at the compatibility impacts. Given that it is binary
compatible (we need to verify that with some testing), and that the
source incompatibility is limited to overriding classes of the four
identified classes, which looks to be easy to resolve if an
application runs into it, we should consider doing this.<br>
<br>
There are two cases to consider: 1) the case of an application or
custom control that overrides Menu, MenuItem, TreeColumnBase and
TreeItem; 2) the case of an application that implements EventTarget
in an application class, and adds their own addEventTarget method
with a different signature (basically an app that copies the pattern
of the 4 above mentioned classes).<br>
<br>
Intuitively, I agree with Andy and Michael that the impact seems
limited, but it would be helpful to survey applications, at least
for those who are on this list, to get more feedback.<br>
<br>
-- Kevin<br>
<br>
<br>
<br>
<div class="moz-cite-prefix">On 4/13/2023 12:44 PM, Andy Goryachev
wrote:<br>
</div>
<blockquote type="cite" cite="mid:DM5PR1001MB2172D15095C01C4D293EFCE5E5989@DM5PR1001MB2172.namprd10.prod.outlook.com">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style>@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;}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;}div.WordSection1
{page:WordSection1;}</style>
<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" moz-do-not-send="true" class="moz-txt-link-freetext">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
<a class="moz-txt-link-rfc2396E" href="mailto:openjfx-dev-retn@openjdk.org"><openjfx-dev-retn@openjdk.org></a> on behalf of Michael
Strauß <a class="moz-txt-link-rfc2396E" href="mailto:michaelstrau2@gmail.com"><michaelstrau2@gmail.com></a><br>
<b>Date: </b>Wednesday, April 12, 2023 at 12:05<br>
<b>To: </b>Kevin Rushforth
<a class="moz-txt-link-rfc2396E" href="mailto:kevin.rushforth@oracle.com"><kevin.rushforth@oracle.com></a><br>
<b>Cc: </b><a class="moz-txt-link-abbreviated" href="mailto:openjfx-dev@openjdk.org">openjfx-dev@openjdk.org</a>
<a class="moz-txt-link-rfc2396E" href="mailto:openjfx-dev@openjdk.org"><openjfx-dev@openjdk.org></a><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" moz-do-not-send="true" class="moz-txt-link-freetext">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>
<a class="moz-txt-link-rfc2396E" href="mailto:kevin.rushforth@oracle.com"><kevin.rushforth@oracle.com></a> 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" moz-do-not-send="true" class="moz-txt-link-freetext">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>
</blockquote>
<br>
</body>
</html>