On Thu, 8 Aug 2024 17:01:47 GMT, Martin Fox <m...@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
