From c7f823c25b7a7f739715306617458321288c922c Mon Sep 17 00:00:00 2001 From: Pekka Maanpaa Date: Thu, 19 Jul 2018 11:20:19 +0300 Subject: [PATCH 1/3] Add component-level API for customizing DOM event listeners 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. --- .../flow/component/ComponentEventBus.java | 201 +++++++++++------- .../vaadin/flow/component/ComponentUtil.java | 37 ++++ .../flow/component/ComponentEventBusTest.java | 16 ++ .../flow/uitest/ui/DomEventFilterView.java | 41 +++- .../flow/uitest/ui/DomEventFilterIT.java | 28 +++ 5 files changed, 241 insertions(+), 82 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java b/flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java index ae1701849dd..a72b4761a8e 100644 --- a/flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java +++ b/flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java @@ -22,6 +22,9 @@ import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; +import java.util.Objects; +import java.util.Set; +import java.util.function.Consumer; import com.vaadin.flow.dom.DebouncePhase; import com.vaadin.flow.dom.DisabledUpdateMode; @@ -51,10 +54,14 @@ */ public class ComponentEventBus implements Serializable { - private static class ComponentEventData implements Serializable { - private Registration domEventRemover = null; - private List>> listeners = new ArrayList<>( - 1); + /** + * Map that contains all the active listeners of one event-type as the keys, + * and their corresponding DOM listener registrations as the values (or null + * if the event-type is not annotated with {@link DomEvent}). + */ + @SuppressWarnings("serial") + private static class ComponentEventData extends + LinkedHashMap>, DomListenerRegistration> { } // Package private to enable testing only @@ -86,15 +93,63 @@ public ComponentEventBus(Component component) { */ public > Registration addListener( Class eventType, ComponentEventListener listener) { - addDomTriggerIfNeeded(eventType); + DomListenerRegistration domEventRemover = addDomTriggerIfNeeded( + eventType, listener); + + componentEventData + .computeIfAbsent(eventType, t -> new ComponentEventData()) + .put(listener, domEventRemover); - List>> listeners = componentEventData - .computeIfAbsent(eventType, - t -> new ComponentEventData()).listeners; - listeners.add(listener); return () -> removeListener(eventType, listener); } + /** + * Adds a listener for the given event type, and customizes the + * corresponding DOM event listener with the given consumer. This allows + * overriding eg. the debounce settings defined in the {@link DomEvent} + * annotation. + *

+ * Note that customizing the DOM event listener works only for event types + * which are annotated with {@link DomEvent}. Use + * {@link #addListener(Class, ComponentEventListener)} for other listeners, + * or if you don't need to customize the DOM listener. + * + * @param + * the event type + * @param eventType + * the event type for which to call the listener, must be + * annotated with {@link DomEvent} + * @param listener + * the listener to call when the event occurs + * @param domListenerConsumer + * a consumer to customize the behavior of the DOM event + * listener, not {@code null} + * @return an object which can be used to remove the event listener + * @throws IllegalArgumentException + * if the event type is not annotated with {@link DomEvent} + */ + public > Registration addListener( + Class eventType, ComponentEventListener listener, + Consumer domListenerConsumer) { + Objects.requireNonNull(domListenerConsumer, + "DOM listener consumer cannot be null"); + + Registration registration = addListener(eventType, listener); + + DomListenerRegistration domListenerRegistration = componentEventData + .get(eventType).get(listener); + + if (domListenerRegistration == null) { + throw new IllegalArgumentException(String.format( + "This method can be used only for DOM events. The given event type %s is not annotated with %s.", + eventType.getSimpleName(), DomEvent.class.getSimpleName())); + } + + domListenerConsumer.accept(domListenerRegistration); + + return registration; + } + /** * Checks if there is at least one listener registered for the given event * type. @@ -124,35 +179,44 @@ public void fireEvent(ComponentEvent event) { if (!hasListener(eventType)) { return; } - List listeners = (List) componentEventData - .get(event.getClass()).listeners; - for (ComponentEventListener l : new ArrayList<>(listeners)) { - event.setUnregisterListenerCommand(() -> { - removeListener(eventType, l); - }); - l.onComponentEvent(event); - event.setUnregisterListenerCommand(null); + Set>> listeners = componentEventData + .get(event.getClass()).keySet(); + for (ComponentEventListener listener : new ArrayList<>(listeners)) { + fireEventForListener(event, listener); } } + @SuppressWarnings("unchecked") + private > void fireEventForListener(T event, + ComponentEventListener listener) { + Class eventType = (Class) event.getClass(); + event.setUnregisterListenerCommand(() -> { + removeListener(eventType, listener); + }); + listener.onComponentEvent(event); + event.setUnregisterListenerCommand(null); + } + /** * Adds a DOM listener for the given component event if it is mapped to a - * DOM event and the event is not yet registered. + * DOM event. * * @param eventType * the type of event + * @param listener + * the listener that is being registered + * @return the registration for the added DOM listener, or {@code null} if + * the event is not a DOM event. */ - private void addDomTriggerIfNeeded( - Class> eventType) { - boolean alreadyRegistered = hasListener(eventType); - if (alreadyRegistered) { - return; - } - - AnnotationReader + private > DomListenerRegistration addDomTriggerIfNeeded( + Class eventType, ComponentEventListener listener) { + DomListenerRegistration domEventRemover = AnnotationReader .getAnnotationFor(eventType, com.vaadin.flow.component.DomEvent.class) - .ifPresent(annotation -> addDomTrigger(eventType, annotation)); + .map(annotation -> addDomTrigger(eventType, annotation, + listener)) + .orElse(null); + return domEventRemover; } /** @@ -163,14 +227,16 @@ private void addDomTriggerIfNeeded( * * @param eventType * the component event type - * @param the + * @param annotation * annotation with event configuration + * @param listener + * the listener that is being registered + * @return the registration for the added DOM listener */ - private void addDomTrigger(Class> eventType, - com.vaadin.flow.component.DomEvent annotation) { + private > DomListenerRegistration addDomTrigger( + Class eventType, com.vaadin.flow.component.DomEvent annotation, + ComponentEventListener listener) { assert eventType != null; - assert !componentEventData.containsKey(eventType) - || componentEventData.get(eventType).domEventRemover == null; assert annotation != null; String domEventType = annotation.value(); @@ -188,11 +254,12 @@ private void addDomTrigger(Class> eventType, // Register DOM event handler DomListenerRegistration registration = element.addEventListener( - domEventType, event -> handleDomEvent(eventType, event)); + domEventType, + event -> handleDomEvent(eventType, event, listener)); registration.setDisabledUpdateMode(mode); - LinkedHashMap> eventDataExpressions = - ComponentEventBusUtil.getEventDataExpressions(eventType); + LinkedHashMap> eventDataExpressions = ComponentEventBusUtil + .getEventDataExpressions(eventType); eventDataExpressions.keySet().forEach(registration::addEventData); if (!"".equals(filter)) { @@ -212,8 +279,7 @@ private void addDomTrigger(Class> eventType, registration.debounce(debounceTimeout, phases[0], rest); } - componentEventData.computeIfAbsent(eventType, - t -> new ComponentEventData()).domEventRemover = registration; + return registration; } /** @@ -231,14 +297,14 @@ private List createEventDataObjects(DomEvent domEvent, Class> eventType) { List eventDataObjects = new ArrayList<>(); - LinkedHashMap> expressions = - ComponentEventBusUtil.getEventDataExpressions(eventType); + LinkedHashMap> expressions = ComponentEventBusUtil + .getEventDataExpressions(eventType); expressions.forEach((expression, type) -> { JsonValue jsonValue = domEvent.getEventData().get(expression); if (jsonValue == null) { jsonValue = Json.createNull(); } - Object value = JsonCodec.decodeAs(jsonValue,type); + Object value = JsonCodec.decodeAs(jsonValue, type); eventDataObjects.add(value); }); return eventDataObjects; @@ -265,49 +331,19 @@ private > void removeListener( throw new IllegalArgumentException( "No listener of the given type is registered"); } - List>> listeners = eventData.listeners; - assert listeners != null; - if (!listeners.remove(listener)) { + if (!eventData.containsKey(listener)) { throw new IllegalArgumentException( "The given listener is not registered"); } - if (listeners.isEmpty()) { - // No more listeners for this event type - AnnotationReader - .getAnnotationFor(eventType, - com.vaadin.flow.component.DomEvent.class) - .ifPresent(annotation -> unregisterDomEvent(eventType, - annotation.value())); - - componentEventData.remove(eventType); - } - } - - /** - * Removes the DOM listener for the given event type. - * - * @param eventType - * the component event type - * @param domEventType - * the DOM event type for the component event type - */ - private void unregisterDomEvent( - Class> eventType, String domEventType) { - assert eventType != null; - assert domEventType != null && !domEventType.isEmpty(); - - Registration domEventRemover = componentEventData - .get(eventType).domEventRemover; + Registration domEventRemover = eventData.remove(listener); if (domEventRemover != null) { domEventRemover.remove(); - componentEventData.get(eventType).domEventRemover = null; - } else { - throw new IllegalArgumentException( - "No remover found when unregistering event type " - + eventType.getName() + " from DOM event " - + domEventType); + } + + if (eventData.isEmpty()) { + componentEventData.remove(eventType); } } @@ -319,12 +355,15 @@ private void unregisterDomEvent( * the component event type which should be fired * @param domEvent * the DOM event + * @param listener + * the component event listener to call when the DOM event is + * fired */ - private void handleDomEvent(Class> eventType, - DomEvent domEvent) { - ComponentEvent e = createEventForDomEvent(eventType, domEvent, - component); - fireEvent(e); + private > void handleDomEvent( + Class eventType, DomEvent domEvent, + ComponentEventListener listener) { + T event = createEventForDomEvent(eventType, domEvent, component); + fireEventForListener(event, listener); } /** diff --git a/flow-server/src/main/java/com/vaadin/flow/component/ComponentUtil.java b/flow-server/src/main/java/com/vaadin/flow/component/ComponentUtil.java index 0dcf5ae213b..4ec3b9988fd 100644 --- a/flow-server/src/main/java/com/vaadin/flow/component/ComponentUtil.java +++ b/flow-server/src/main/java/com/vaadin/flow/component/ComponentUtil.java @@ -31,6 +31,8 @@ import com.vaadin.flow.component.internal.ComponentMetaData.DependencyInfo; import com.vaadin.flow.component.internal.ComponentMetaData.SynchronizedPropertyInfo; import com.vaadin.flow.di.Instantiator; +import com.vaadin.flow.dom.DomEvent; +import com.vaadin.flow.dom.DomListenerRegistration; import com.vaadin.flow.dom.Element; import com.vaadin.flow.function.SerializableTriConsumer; import com.vaadin.flow.i18n.LocaleChangeEvent; @@ -333,6 +335,41 @@ public static > Registration addListener( return component.addListener(eventType, listener); } + /** + * Adds a listener for an event of the given type to the {@code component}, + * and customizes the corresponding DOM event listener with the given + * consumer. This allows overriding eg. the debounce settings defined in the + * {@link DomEvent} annotation. + *

+ * Note that customizing the DOM event listener works only for event types + * which are annotated with {@link DomEvent}. Use + * {@link #addListener(Component, Class, ComponentEventListener)} for other + * listeners, or if you don't need to customize the DOM listener. + * + * @param + * the event type + * @param component + * the component to add the {@code listener} + * @param eventType + * the event type for which to call the listener, must be + * annotated with {@link DomEvent} + * @param listener + * the listener to call when the event occurs, not {@code null} + * @param domListenerConsumer + * a consumer to customize the behavior of the DOM event + * listener, not {@code null} + * @return a handle that can be used for removing the listener + * @throws IllegalArgumentException + * if the event type is not annotated with {@link DomEvent} + */ + public > Registration addListener( + Component component, Class eventType, + ComponentEventListener listener, + Consumer domListenerConsumer) { + return component.getEventBus().addListener(eventType, listener, + domListenerConsumer); + } + /** * Dispatches the event to all listeners registered for the event type. * diff --git a/flow-server/src/test/java/com/vaadin/flow/component/ComponentEventBusTest.java b/flow-server/src/test/java/com/vaadin/flow/component/ComponentEventBusTest.java index 8f6e3e07796..c6aa27eaec4 100644 --- a/flow-server/src/test/java/com/vaadin/flow/component/ComponentEventBusTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/component/ComponentEventBusTest.java @@ -263,6 +263,22 @@ public void nonDomEvent_fireEvent() { eventTracker.assertEventCalled(component, false); } + @Test + public void domEvent_addListenerWithDomListenerConsumer() { + TestComponent component = new TestComponent(); + EventTracker eventTracker = new EventTracker<>(); + component.getEventBus().addListener(MappedToDomEvent.class, + eventTracker, domRegistration -> domRegistration.debounce(200)); + } + + @Test(expected = IllegalArgumentException.class) + public void nonDomEvent_addListenerWithDomListenerConsumer_throws() { + TestComponent component = new TestComponent(); + EventTracker eventTracker = new EventTracker<>(); + component.getEventBus().addListener(ServerEvent.class, eventTracker, + domRegistration -> domRegistration.debounce(200)); + } + @Test public void multipleEventsForSameDomEvent_removeListener() { TestComponent component = new TestComponent(); diff --git a/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/DomEventFilterView.java b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/DomEventFilterView.java index 1643a2f1e69..5ccf59cb1ca 100644 --- a/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/DomEventFilterView.java +++ b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/DomEventFilterView.java @@ -15,13 +15,46 @@ */ package com.vaadin.flow.uitest.ui; +import com.vaadin.flow.component.Component; +import com.vaadin.flow.component.ComponentEvent; +import com.vaadin.flow.component.ComponentEventListener; +import com.vaadin.flow.component.DomEvent; +import com.vaadin.flow.component.EventData; +import com.vaadin.flow.component.Tag; import com.vaadin.flow.dom.DebouncePhase; import com.vaadin.flow.dom.Element; import com.vaadin.flow.router.Route; +import com.vaadin.flow.shared.Registration; import com.vaadin.flow.uitest.servlet.ViewTestLayout; @Route(value = "com.vaadin.flow.uitest.ui.DomEventFilterView", layout = ViewTestLayout.class) public class DomEventFilterView extends AbstractDivView { + + @Tag("input") + public static class DebounceComponent extends Component { + public Registration addInputListener( + ComponentEventListener listener, + int debounceTimeout) { + return getEventBus().addListener(InputEvent.class, listener, + domReg -> domReg.debounce(debounceTimeout)); + } + } + + @DomEvent("input") + public static class InputEvent extends ComponentEvent { + private String value; + + public InputEvent(DebounceComponent source, boolean fromClient, + @EventData("element.value") String value) { + super(source, fromClient); + this.value = value; + } + + public String getValue() { + return value; + } + } + private final Element messages = new Element("div"); public DomEventFilterView() { @@ -49,8 +82,14 @@ public DomEventFilterView() { + e.getEventData().getString("element.value"))) .throttle(1000); + DebounceComponent component = new DebounceComponent(); + component.setId("debounce-component"); + component.addInputListener( + e -> addMessage("Component: " + e.getValue()), 1000); + messages.setAttribute("id", "messages"); - getElement().appendChild(space, debounce, messages); + getElement().appendChild(space, debounce, component.getElement(), + messages); } private void addMessage(String message) { diff --git a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/DomEventFilterIT.java b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/DomEventFilterIT.java index 51f342f096a..542030dbd41 100644 --- a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/DomEventFilterIT.java +++ b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/DomEventFilterIT.java @@ -78,6 +78,34 @@ public void debounce() throws InterruptedException { assertMessages(4); } + @Test + public void componentWithDebounce() throws InterruptedException { + open(); + + WebElement input = findElement(By.id("debounce-component")); + + input.sendKeys("a"); + assertMessages(0); + + Thread.sleep(750); + input.sendKeys("b"); + assertMessages(0); + + Thread.sleep(1100); + assertMessages(0, "Component: ab"); + + input.sendKeys("c"); + Thread.sleep(200); + assertMessages(1); + + input.sendKeys("d"); + Thread.sleep(800); + assertMessages(1); + Thread.sleep(300); + assertMessages(1, "Component: abcd"); + + } + private void assertMessages(int skip, String... expectedTail) { List messages = getMessages(); if (messages.size() < skip) { From 21a2a0b3a865f7eba5176b5ee46c0a8c955992e0 Mon Sep 17 00:00:00 2001 From: Pekka Maanpaa Date: Wed, 1 Aug 2018 14:04:40 +0300 Subject: [PATCH 2/3] Change data-structure to allow same listener to be added multiple times --- .../flow/component/ComponentEventBus.java | 136 +++++++++--------- .../flow/component/ComponentEventBusTest.java | 39 +++++ 2 files changed, 111 insertions(+), 64 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java b/flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java index a72b4761a8e..571737b311a 100644 --- a/flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java +++ b/flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java @@ -23,7 +23,6 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Objects; -import java.util.Set; import java.util.function.Consumer; import com.vaadin.flow.dom.DebouncePhase; @@ -55,17 +54,22 @@ public class ComponentEventBus implements Serializable { /** - * Map that contains all the active listeners of one event-type as the keys, - * and their corresponding DOM listener registrations as the values (or null - * if the event-type is not annotated with {@link DomEvent}). + * Pairs a component-level listener for its DOM listener registration, if + * the event-type is annotated with {@link DomEvent}. */ - @SuppressWarnings("serial") - private static class ComponentEventData extends - LinkedHashMap>, DomListenerRegistration> { + private static class ListenerWrapper> + implements Serializable { + private ComponentEventListener listener; + private DomListenerRegistration domRegistration; + + public ListenerWrapper(ComponentEventListener listener) { + this.listener = listener; + } + } // Package private to enable testing only - HashMap>, ComponentEventData> componentEventData = new HashMap<>(); + HashMap>, ArrayList>> componentEventData = new HashMap<>(); private Component component; @@ -93,14 +97,7 @@ public ComponentEventBus(Component component) { */ public > Registration addListener( Class eventType, ComponentEventListener listener) { - DomListenerRegistration domEventRemover = addDomTriggerIfNeeded( - eventType, listener); - - componentEventData - .computeIfAbsent(eventType, t -> new ComponentEventData()) - .put(listener, domEventRemover); - - return () -> removeListener(eventType, listener); + return addListenerInternal(eventType, listener, null); } /** @@ -133,21 +130,31 @@ public > Registration addListener( Consumer domListenerConsumer) { Objects.requireNonNull(domListenerConsumer, "DOM listener consumer cannot be null"); + return addListenerInternal(eventType, listener, domListenerConsumer); + } - Registration registration = addListener(eventType, listener); + private > Registration addListenerInternal( + Class eventType, ComponentEventListener listener, + Consumer domListenerConsumer) { + + ListenerWrapper wrapper = new ListenerWrapper<>(listener); - DomListenerRegistration domListenerRegistration = componentEventData - .get(eventType).get(listener); + boolean isDomEvent = addDomTriggerIfNeeded(eventType, wrapper); - if (domListenerRegistration == null) { - throw new IllegalArgumentException(String.format( - "This method can be used only for DOM events. The given event type %s is not annotated with %s.", - eventType.getSimpleName(), DomEvent.class.getSimpleName())); + if (domListenerConsumer != null) { + if (!isDomEvent) { + throw new IllegalArgumentException(String.format( + "DomListenerConsumer can be used only for DOM events. The given event type %s is not annotated with %s.", + eventType.getSimpleName(), + DomEvent.class.getSimpleName())); + } + domListenerConsumer.accept(wrapper.domRegistration); } - domListenerConsumer.accept(domListenerRegistration); + componentEventData.computeIfAbsent(eventType, t -> new ArrayList<>()) + .add(wrapper); - return registration; + return () -> removeListener(eventType, wrapper); } /** @@ -179,21 +186,22 @@ public void fireEvent(ComponentEvent event) { if (!hasListener(eventType)) { return; } - Set>> listeners = componentEventData - .get(event.getClass()).keySet(); - for (ComponentEventListener listener : new ArrayList<>(listeners)) { - fireEventForListener(event, listener); + + // Copy the list to avoid ConcurrentModificationException + for (ListenerWrapper wrapper : new ArrayList<>( + componentEventData.get(event.getClass()))) { + fireEventForListener(event, wrapper); } } @SuppressWarnings("unchecked") private > void fireEventForListener(T event, - ComponentEventListener listener) { + ListenerWrapper wrapper) { Class eventType = (Class) event.getClass(); event.setUnregisterListenerCommand(() -> { - removeListener(eventType, listener); + removeListener(eventType, wrapper); }); - listener.onComponentEvent(event); + wrapper.listener.onComponentEvent(event); event.setUnregisterListenerCommand(null); } @@ -203,39 +211,36 @@ private > void fireEventForListener(T event, * * @param eventType * the type of event - * @param listener + * @param wrapper * the listener that is being registered - * @return the registration for the added DOM listener, or {@code null} if - * the event is not a DOM event. + * @return {@code true} if a DOM-trigger was added (the event is annotated + * with {@link DomEvent}), {@code false} otherwise. */ - private > DomListenerRegistration addDomTriggerIfNeeded( - Class eventType, ComponentEventListener listener) { - DomListenerRegistration domEventRemover = AnnotationReader + private > boolean addDomTriggerIfNeeded( + Class eventType, ListenerWrapper wrapper) { + return AnnotationReader .getAnnotationFor(eventType, com.vaadin.flow.component.DomEvent.class) - .map(annotation -> addDomTrigger(eventType, annotation, - listener)) - .orElse(null); - return domEventRemover; + .map(annotation -> { + addDomTrigger(eventType, annotation, wrapper); + return true; + }).orElse(false); } /** * Adds a DOM listener of the given type for the given component event and * annotation. - *

- * Assumes that no listener exists. * * @param eventType * the component event type * @param annotation * annotation with event configuration - * @param listener + * @param wrapper * the listener that is being registered - * @return the registration for the added DOM listener */ - private > DomListenerRegistration addDomTrigger( - Class eventType, com.vaadin.flow.component.DomEvent annotation, - ComponentEventListener listener) { + private > void addDomTrigger(Class eventType, + com.vaadin.flow.component.DomEvent annotation, + ListenerWrapper wrapper) { assert eventType != null; assert annotation != null; @@ -255,7 +260,10 @@ private > DomListenerRegistration addDomTrigger( // Register DOM event handler DomListenerRegistration registration = element.addEventListener( domEventType, - event -> handleDomEvent(eventType, event, listener)); + event -> handleDomEvent(eventType, event, wrapper)); + + wrapper.domRegistration = registration; + registration.setDisabledUpdateMode(mode); LinkedHashMap> eventDataExpressions = ComponentEventBusUtil @@ -278,8 +286,6 @@ private > DomListenerRegistration addDomTrigger( registration.debounce(debounceTimeout, phases[0], rest); } - - return registration; } /** @@ -318,28 +324,31 @@ private List createEventDataObjects(DomEvent domEvent, * * @param eventType * the component event type - * @param listener + * @param wrapper * the listener to remove */ private > void removeListener( - Class eventType, ComponentEventListener listener) { + Class eventType, ListenerWrapper wrapper) { assert eventType != null; - assert listener != null; + assert wrapper != null; + assert wrapper.listener != null; - ComponentEventData eventData = componentEventData.get(eventType); + ArrayList> eventData = componentEventData + .get(eventType); if (eventData == null) { throw new IllegalArgumentException( "No listener of the given type is registered"); } - if (!eventData.containsKey(listener)) { + if (!eventData.contains(wrapper)) { throw new IllegalArgumentException( "The given listener is not registered"); } - Registration domEventRemover = eventData.remove(listener); - if (domEventRemover != null) { - domEventRemover.remove(); + eventData.remove(wrapper); + + if (wrapper.domRegistration != null) { + wrapper.domRegistration.remove(); } if (eventData.isEmpty()) { @@ -355,15 +364,14 @@ private > void removeListener( * the component event type which should be fired * @param domEvent * the DOM event - * @param listener + * @param wrapper * the component event listener to call when the DOM event is * fired */ private > void handleDomEvent( - Class eventType, DomEvent domEvent, - ComponentEventListener listener) { + Class eventType, DomEvent domEvent, ListenerWrapper wrapper) { T event = createEventForDomEvent(eventType, domEvent, component); - fireEventForListener(event, listener); + fireEventForListener(event, wrapper); } /** diff --git a/flow-server/src/test/java/com/vaadin/flow/component/ComponentEventBusTest.java b/flow-server/src/test/java/com/vaadin/flow/component/ComponentEventBusTest.java index c6aa27eaec4..7f55575031e 100644 --- a/flow-server/src/test/java/com/vaadin/flow/component/ComponentEventBusTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/component/ComponentEventBusTest.java @@ -279,6 +279,45 @@ public void nonDomEvent_addListenerWithDomListenerConsumer_throws() { domRegistration -> domRegistration.debounce(200)); } + private int calls = 0; + + @Test + public void domEvent_addSameListenerTwice() { + TestComponent component = new TestComponent(); + + ComponentEventListener listener = e -> calls++; + + Registration reg1 = component.addListener(MappedToDomEvent.class, + listener); + Registration reg2 = component.addListener(MappedToDomEvent.class, + listener); + + Assert.assertEquals(1, + component.getEventBus().componentEventData.size()); + Assert.assertEquals(2, component.getEventBus().componentEventData + .get(MappedToDomEvent.class).size()); + + fireDomEvent(component, "dom-event", Json.createObject()); + Assert.assertEquals(2, calls); + + reg1.remove(); + Assert.assertEquals(1, + component.getEventBus().componentEventData.size()); + Assert.assertEquals(1, component.getEventBus().componentEventData + .get(MappedToDomEvent.class).size()); + + fireDomEvent(component, "dom-event", Json.createObject()); + + Assert.assertEquals(3, calls); + + reg2.remove(); + Assert.assertEquals(0, + component.getEventBus().componentEventData.size()); + + fireDomEvent(component, "dom-event", Json.createObject()); + Assert.assertEquals(3, calls); + } + @Test public void multipleEventsForSameDomEvent_removeListener() { TestComponent component = new TestComponent(); From 43a2ee4dd94f0af8b8135572cb8c8c674683ab14 Mon Sep 17 00:00:00 2001 From: Pekka Maanpaa Date: Thu, 2 Aug 2018 12:28:12 +0300 Subject: [PATCH 3/3] Optimize removeListener() a bit --- .../java/com/vaadin/flow/component/ComponentEventBus.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java b/flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java index 571737b311a..4b558523856 100644 --- a/flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java +++ b/flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java @@ -340,13 +340,11 @@ private > void removeListener( "No listener of the given type is registered"); } - if (!eventData.contains(wrapper)) { + if (!eventData.remove(wrapper)) { throw new IllegalArgumentException( "The given listener is not registered"); } - eventData.remove(wrapper); - if (wrapper.domRegistration != null) { wrapper.domRegistration.remove(); }