Promote addEventHandler/removeEventHandler to EventTarget interface

[John Hendrikx]

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.
I think it would make good sense that they also have the 
EventHandler/Filters methods for consistency.  If they're targets for 
Events, then you should be able to add handlers or filters for them.
> 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.
They also forward directly to the internal EventHandlerManager which 
does accept EventHandler<? super T>.  This seems like an oversight when 
those implementations were added to MenuItem, TreeItem and TableColumnBase.
> Do you think this is a useful improvement?

I think that all the implementations should definitely be consistent; 
Dialog and Tab probably should have the means to add EventHandlers and 
EventFilters; and MenuItem, TreeItem, TableColumnBase could do with the 
EventFilter variants.

If they're going to be consistent, might as well add these methods to 
the EventTarget interface to enforce this for any future EventTarget 
implementations.

EventTarget is already public API, and so there might be 3rd party 
implementations.  This means that the methods added to the EventTarget 
interface must be default methods. It would be super if these default 
implementations would just work out of the box, but that would require 
exposing the internal class EventHandlerManager (and adding a 
`getEventHandlerManager` to the `EventTarget` interface).  If that's not 
realistic, then initially the default implementations would have to 
throw UnsupportedOperationException.

--John