Registration to remove state.registeredEventListeners (fixes #9634) (#…

…10130)

This changes SharedState.registeredEventListeners to be a Map that keeps track of how many listeners of each type have been added, and handles unregistering of such listeners correctly.

all/src/main/templates/release-notes.html

@@ -120,6 +120,7 @@ <h2 id="incompatible">Incompatible or Behavior-altering Changes in @version-mino
         <li>Error indicators are now <tt>&lt;span class="v-errorindicator"&gt;&lt;/span&gt;</tt> elements.</li>
         <li><tt>Embedded</tt> is not a <tt>LegacyComponent</tt> anymore.</li>
         <li><tt>Notification</tt> method <tt>show</tt> returns <tt>Notification</tt>, instead of <tt>void</tt>.</li>
+        <li><tt>SharedState</tt> field <tt>registeredEventListeners</tt> is a <tt>Map</tt> instead of <tt>Set</tt>.</li>
         <li>The client side <tt>SelectionModel</tt> interface has a new method <tt>isMultiSelectionAllowed</tt>.</li>
 
         <h2>For incompatible or behavior-altering changes in 8.1, please see <a href="https://vaadin.com/download/release/8.1/8.1.0/release-notes.html#incompatible">8.1 release notes</a></h2>

client/src/main/java/com/vaadin/client/ui/AbstractConnector.java

@@ -20,7 +20,7 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Set;
+import java.util.Map;
 
 import com.google.gwt.core.client.JsArrayString;
 import com.google.gwt.dom.client.Element;
@@ -498,8 +498,8 @@ public String getResourceUrl(String key) {
      */
     @Override
     public boolean hasEventListener(String eventIdentifier) {
-        Set<String> reg = getState().registeredEventListeners;
-        return (reg != null && reg.contains(eventIdentifier));
+        Map<String, Integer> reg = getState().registeredEventListeners;
+        return reg != null && reg.containsKey(eventIdentifier);
     }
 
     /**

server/src/main/java/com/vaadin/event/EventRouter.java

@@ -28,6 +28,8 @@
 import com.vaadin.server.ErrorEvent;
 import com.vaadin.server.ErrorHandler;
 import com.vaadin.shared.Registration;
+import com.vaadin.shared.communication.SharedState;
+import com.vaadin.shared.ui.ComponentStateUtil;
 
 /**
  * <code>EventRouter</code> class implementing the inheritable event listening
@@ -63,6 +65,67 @@ public Registration addListener(Class<?> eventType, Object object,
         return () -> listenerList.remove(listenerMethod);
     }
 
+    /**
+     * Registers a new event listener with the specified activation method to
+     * listen events generated by this component. If the activation method does
+     * not have any arguments the event object will not be passed to it when
+     * it's called.
+     *
+     * <p>
+     * This method additionally informs the event-api to stop routing events
+     * with the given {@code eventIdentifier} to the components handleEvent
+     * function call.
+     * </p>
+     *
+     * <p>
+     * The only way to remove the listener is to use the returned
+     * {@link Registration}. The other methods, e.g.
+     * {@link #removeAllListeners()} do not do that.
+     * </p>
+     *
+     * <p>
+     * For more information on the inheritable event mechanism see the
+     * {@link com.vaadin.event com.vaadin.event package documentation}.
+     * </p>
+     *
+     * @param eventType
+     *            the type of the listened event. Events of this type or its
+     *            subclasses activate the listener.
+     * @param target
+     *            the object instance who owns the activation method.
+     * @param method
+     *            the activation method.
+     * @param eventIdentifier
+     *            the identifier of the event to listen for
+     * @param state
+     *            The component State
+     * @return a registration object for removing the listener
+     * @throws IllegalArgumentException
+     *             unless {@code method} has exactly one match in {@code target}
+     * @throws NullPointerException
+     *             if {@code target} is {@code null}
+     * @since
+     */
+    public Registration addListener(Class<?> eventType, Object target,
+            Method method, String eventIdentifier, SharedState state) {
+        Objects.requireNonNull(target, "Listener must not be null.");
+        if (listenerList == null) {
+            listenerList = new LinkedHashSet<>();
+        }
+        ListenerMethod listenerMethod = new ListenerMethod(eventType, target,
+                method);
+        listenerList.add(listenerMethod);
+
+        Registration registration = ComponentStateUtil
+                .addRegisteredEventListener(state, eventIdentifier);
+
+        return () -> {
+            registration.remove();
+
+            listenerList.remove(listenerMethod);
+        };
+    }
+
     /*
      * Registers a new listener with the specified named activation method to
      * listen events generated by this component. Don't add a JavaDoc comment

server/src/main/java/com/vaadin/event/MethodEventSource.java

@@ -37,12 +37,10 @@
 public interface MethodEventSource extends Serializable {
 
     /**
-     * <p>
      * Registers a new event listener with the specified activation method to
      * listen events generated by this component. If the activation method does
      * not have any arguments the event object will not be passed to it when
      * it's called.
-     * </p>
      *
      * <p>
      * For more information on the inheritable event mechanism see the
@@ -68,12 +66,10 @@ public Registration addListener(Class<?> eventType, Object object,
             Method method);
 
     /**
-     * <p>
      * Registers a new listener with the specified activation method to listen
      * events generated by this component. If the activation method does not
      * have any arguments the event object will not be passed to it when it's
      * called.
-     * </p>
      *
      * <p>
      * This version of <code>addListener</code> gets the name of the activation
@@ -151,11 +147,9 @@ public void removeListener(Class<?> eventType, Object target,
             Method method);
 
     /**
-     * <p>
      * Removes one registered listener method. The given method owned by the
      * given object will no longer be called when the specified events are
      * generated by this component.
-     * </p>
      *
      * <p>
      * This version of <code>removeListener</code> gets the name of the

server/src/main/java/com/vaadin/server/AbstractClientConnector.java

@@ -723,12 +723,10 @@ protected void setResource(String key, Resource resource) {
     /* Listener code starts. Should be refactored. */
 
     /**
-     * <p>
      * Registers a new listener with the specified activation method to listen
      * events generated by this component. If the activation method does not
      * have any arguments the event object will not be passed to it when it's
      * called.
-     * </p>
      *
      * <p>
      * This method additionally informs the event-api to route events with the
@@ -757,16 +755,8 @@ protected Registration addListener(String eventIdentifier,
         if (eventRouter == null) {
             eventRouter = new EventRouter();
         }
-        boolean needRepaint = !eventRouter.hasListeners(eventType);
-        Registration registration = eventRouter.addListener(eventType, target,
-                method);
-
-        if (needRepaint) {
-            ComponentStateUtil.addRegisteredEventListener(getState(),
-                    eventIdentifier);
-        }
-
-        return registration;
+        return eventRouter.addListener(eventType, target, method,
+                eventIdentifier, getState());
     }
 
     /**
@@ -789,8 +779,8 @@ protected boolean hasListeners(Class<?> eventType) {
      *
      * <p>
      * This method additionally informs the event-api to stop routing events
-     * with the given eventIdentifier to the components handleEvent function
-     * call.
+     * with the given {@code eventIdentifier} to the components handleEvent
+     * function call.
      * </p>
      *
      * <p>
@@ -824,12 +814,10 @@ protected void removeListener(String eventIdentifier, Class<?> eventType,
     }
 
     /**
-     * <p>
      * Registers a new listener with the specified activation method to listen
      * events generated by this component. If the activation method does not
      * have any arguments the event object will not be passed to it when it's
      * called.
-     * </p>
      *
      * <p>
      * For more information on the inheritable event mechanism see the
@@ -855,12 +843,10 @@ public Registration addListener(Class<?> eventType, Object target,
     }
 
     /**
-     * <p>
      * Convenience method for registering a new listener with the specified
      * activation method to listen events generated by this component. If the
      * activation method does not have any arguments the event object will not
      * be passed to it when it's called.
-     * </p>
      *
      * <p>
      * This version of <code>addListener</code> gets the name of the activation

server/src/test/java/com/vaadin/tests/event/EventRouterTest.java

@@ -16,6 +16,8 @@
 package com.vaadin.tests.event;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import java.lang.reflect.Method;
@@ -25,7 +27,10 @@
 import org.junit.Test;
 
 import com.vaadin.event.EventRouter;
+import com.vaadin.event.MouseEvents.ClickEvent;
 import com.vaadin.server.ErrorHandler;
+import com.vaadin.shared.Registration;
+import com.vaadin.shared.communication.SharedState;
 import com.vaadin.ui.Component;
 import com.vaadin.ui.Component.Listener;
 import com.vaadin.util.ReflectTools;
@@ -109,4 +114,15 @@ public void fireEvent_multipleListenersAndException_errorHandlerCalled() {
         router.fireEvent(new Component.Event(component), errorHandler);
         EasyMock.verify(listener, listener2, errorHandler);
     }
+
+    @Test
+    public void registrationToRemoveRegisteredEventListener() {
+        SharedState state = new SharedState();
+        Listener listener2 = EasyMock.createMock(Component.Listener.class);
+        Registration registration = router.addListener(ClickEvent.class,
+                listener2, COMPONENT_EVENT_METHOD, "click", state);
+        assertTrue(!state.registeredEventListeners.isEmpty());
+        registration.remove();
+        assertNull(state.registeredEventListeners);
+    }
 }

shared/src/main/java/com/vaadin/shared/communication/SharedState.java

@@ -19,7 +19,6 @@
 import java.io.Serializable;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Set;
 
 import com.vaadin.shared.Connector;
 import com.vaadin.shared.annotations.NoLayout;
@@ -62,10 +61,14 @@ public class SharedState implements Serializable {
     public Map<String, URLReference> resources = new HashMap<>();
 
     public boolean enabled = true;
+
     /**
-     * A set of event identifiers with registered listeners.
+     * A Map of event identifiers with registered listeners, {@code key} is
+     * event identifier, {@code value} is the listeners count.
+     * 
+     * @since
      */
     @NoLayout
-    public Set<String> registeredEventListeners = null;
+    public Map<String, Integer> registeredEventListeners;
 
 }

shared/src/main/java/com/vaadin/shared/ui/ComponentStateUtil.java

@@ -16,7 +16,7 @@
 package com.vaadin.shared.ui;
 
 import java.io.Serializable;
-import java.util.HashSet;
+import java.util.HashMap;
 
 import com.vaadin.shared.AbstractComponentState;
 import com.vaadin.shared.Registration;
@@ -67,12 +67,19 @@ public static final boolean isRelativeHeight(AbstractComponentState state) {
     @Deprecated
     public static final void removeRegisteredEventListener(SharedState state,
             String eventIdentifier) {
-        if (state.registeredEventListeners == null) {
-            return;
-        }
-        state.registeredEventListeners.remove(eventIdentifier);
-        if (state.registeredEventListeners.size() == 0) {
-            state.registeredEventListeners = null;
+        if (state.registeredEventListeners != null) {
+            Integer count = state.registeredEventListeners.get(eventIdentifier);
+            if (count != null) {
+                if (count > 1) {
+                    state.registeredEventListeners.put(eventIdentifier,
+                            count - 1);
+                } else {
+                    state.registeredEventListeners.remove(eventIdentifier);
+                    if (state.registeredEventListeners.isEmpty()) {
+                        state.registeredEventListeners = null;
+                    }
+                }
+            }
         }
     }
 
@@ -87,9 +94,13 @@ public static final void removeRegisteredEventListener(SharedState state,
     public static final Registration addRegisteredEventListener(
             SharedState state, String eventListenerId) {
         if (state.registeredEventListeners == null) {
-            state.registeredEventListeners = new HashSet<>();
+            state.registeredEventListeners = new HashMap<>();
+        }
+        Integer count = state.registeredEventListeners.get(eventListenerId);
+        if (count == null) {
+            count = 0;
         }
-        state.registeredEventListeners.add(eventListenerId);
+        state.registeredEventListeners.put(eventListenerId, count + 1);
         return () -> removeRegisteredEventListener(state, eventListenerId);
     }
 }