Add component-level API for customizing DOM event listeners #4445
Conversation
Each component-level event-listener is now mapped to its corresponding DOM event listener registration. An overload of ComponentEventBus::addListener is added which takes a consumer for customizing the DOM event listener. The method still returns a component-level registration. This allows creating Component-level API for eg. overriding the debounce or filter settings defined in the event's @DomEvent annotation.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1.
Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java, line 148 at r1 (raw file):
Class<? extends ComponentEvent<?>> eventType) { boolean alreadyRegistered = hasListener(eventType); if (alreadyRegistered) {
What happens now if the same eventType with the same listener is registered more than once?
(Answer that with a comment in the javadocs)
flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java, line 335 at r1 (raw file):
} if (!eventData.containsKey(listener)) {
Is it possible to have the listener registered in the eventData (so the eventData.containsKey(listener) would return true) but the domEventRemover (from eventData.get(listener)) is null ?
If not, then there's one optimization that could be done here:
Registration domEventRemover = eventData.remove(listener);
if (domEventRemover == null) {
throw new IllegalArgumentException("The given listener is not registered");
}
domEventRemover.remove();
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java, line 148 at r1 (raw file):
Previously, gilberto-torrezan (Gilberto Torrezan) wrote…
What happens now if the same
eventTypewith the same listener is registered more than once?
(Answer that with a comment in the javadocs)
Currently adding the same listener twice causes overriding the DomListenerRegistration in the ComponentEventData map, but the first dom-listener never gets removed. So even after component-level Registration::remove, it only removes the latest dom-registration that is saved in the map. The old one is not in the map so it remains registered for ever.
To correctly support adding same listener multiple times with separate Registrations, I need to change the data-structure from a map to some kind of a list of pairs, because a map can't have duplicate keys.
This was a good point. I think this PR kinda broke this use-case, as there was no test for it.
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java, line 148 at r1 (raw file):
Previously, pekam (Pekka Maanpää) wrote…
Currently adding the same listener twice causes overriding the
DomListenerRegistrationin theComponentEventDatamap, but the first dom-listener never gets removed. So even after component-levelRegistration::remove, it only removes the latest dom-registration that is saved in the map. The old one is not in the map so it remains registered for ever.To correctly support adding same listener multiple times with separate
Registrations, I need to change the data-structure from a map to some kind of a list of pairs, because a map can't have duplicate keys.This was a good point. I think this PR kinda broke this use-case, as there was no test for it.
Done.
flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java, line 335 at r1 (raw file):
Previously, gilberto-torrezan (Gilberto Torrezan) wrote…
Is it possible to have the listener registered in the eventData (so the
eventData.containsKey(listener)would returntrue) but thedomEventRemover(fromeventData.get(listener)) isnull?If not, then there's one optimization that could be done here:
Registration domEventRemover = eventData.remove(listener); if (domEventRemover == null) { throw new IllegalArgumentException("The given listener is not registered"); } domEventRemover.remove();
The data structure is a bit different but there was a similar optimization to be made.
| private <T extends ComponentEvent<?>> void fireEventForListener(T event, | ||
| ListenerWrapper<T> wrapper) { | ||
| Class<T> eventType = (Class<T>) event.getClass(); | ||
| event.setUnregisterListenerCommand(() -> { |
There was a problem hiding this comment.
|
SonarQube analysis reported 1 issue
Watch the comments in this conversation to review them. |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained, and 1 stale
There was a problem hiding this comment.
Dismissed @vaadin-bot from a discussion.
Reviewable status:complete! all discussions resolved, 1 of 1 LGTMs obtained
Each component-level event-listener is now mapped to its corresponding
DOM event listener registration.
An overload of ComponentEventBus::addListener is added which takes a
consumer for customizing the DOM event listener. The method still
returns a component-level registration.
This allows creating Component-level API for eg. overriding the debounce
or filter settings defined in the event's @DomEvent annotation.
This change is