RFR: 8337246: SpinnerSkin does not consume ENTER KeyEvent when editor ActionEvent is consumed

[Andy Goryachev]

On Thu, 8 Aug 2024 17:01:47 GMT, Martin Fox <mfox at openjdk.org> wrote:

>>> This PR assumes that during dispatch events are copied using copyFor. But the documentation states that an EventDispatcher can do whatever it wants in `dispatchEvent`. A developer could provide a dispatcher that creates entirely new events rather than just copy existing ones and that would break this code. The only thing that’s guaranteed is that the dispatcher will return null to stop event dispatch and that's all that the solution in [JDK-8303209](https://bugs.openjdk.org/browse/JDK-8303209) relies on.
>> 
>> a) If the EventDispatcher does something different like creating the new events, it is up to that ED to deal with consequences.  It means the developer wants to create new events and not to have the link between the events.
>> 
>> b) I am not convinced that returning null from a dispatcher is a good solution.  We already have an API for that: `Event.isConsumed()`.  Adding a new, parallel paradigm, in my opinion is unnecessary.
>> 
>> The bigger issue I see, as I mentioned earlier, is that 
>> 1. the `Event` has the target field which is absolutely, totally unnecessary (the target already knows who the target is, it is the target) and is an anti-pattern because
>> 2. it forces the dispatcher to create new event instances for new targets, breaking the `isConsumed` logic
>> 
>> I don't think we'll ever change the event dispatching mechanism and remove the target field from Event.  So the only remaining solutions are little hacks and workarounds here and there.
>> 
>> I don't think adding return value to the dispatcher is the right solution.
>
>> b) I am not convinced that returning null from a dispatcher is a good solution. We already have an API for that: `Event.isConsumed()`. Adding a new, parallel paradigm, in my opinion is unnecessary.
> 
> We would not be adding anything. Consuming an event is how a developer communicates to one EventDispatcher that the event was dealt with. But this information is propagated through the dispatch chain by returning null from dispatchEvent. That's all in place already.
> 
> There's a variant of `EventUtil.fireEvent` that returns the result of dispatchEvent so a client can check for null to see if the event was consumed. And there are already places in the code where this happens. [JDK-8303209](https://bugs.openjdk.org/browse/JDK-8303209) calls for this to be cleaned up and made public.

@beldenfox :
EventUtil is not a public API.  Checking the return value of the dispatchEvent is, in my opinion, an anti-pattern.  The right pattern is this (TabPaneBehavior:85):


    public boolean canCloseTab(Tab tab) {
        Event event = new Event(tab,tab,Tab.TAB_CLOSE_REQUEST_EVENT);
        Event.fireEvent(tab, event);
        return ! event.isConsumed();
    }


Using your own argument, a dev can write a dispatcher which returns garbage from its `dispatchEvent` method, breaking the thing.  So I am still not convinced.

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1523#issuecomment-2276304526