Fix DomEventListener filter (and other stuff) removal

[ujoni]

Fixes #5090

Also fixes #5480 (but adding a test for this would be nice)

---

This change is 

[ujoni]

Cherry-pickable for v14.0 and V13

[denis-anisimov]

Reviewed 3 of 3 files at r1.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/ElementListenerMap.java, line 375 at r1 (raw file):

 if (listeners != null && listeners.containsKey(eventType)) {

This check confuses me:

• it doesn't seem relate to the logic below. So it will work regardless of this check. It means that this is a business logic .
• as a business logic it's executed only if listeners contains key. But this method is called after the listener with the eventType is removed. So if there is no anymore listeners with the same event type the code won't be executed. Is it correct ?

I would say that the check is excessive : what will happen if a new listener is added later on with the same event type ?

[ujoni]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/ElementListenerMap.java, line 375 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

 if (listeners != null && listeners.containsKey(eventType)) {

This check confuses me:

• it doesn't seem relate to the logic below. So it will work regardless of this check. It means that this is a business logic .
• as a business logic it's executed only if listeners contains key. But this method is called after the listener with the eventType is removed. So if there is no anymore listeners with the same event type the code won't be executed. Is it correct ?

I would say that the check is excessive : what will happen if a new listener is added later on with the same event type ?

Hmm. It seems that I added random defensive code without thinking. I removed it now. Thanks

[denis-anisimov]

Reviewed 1 of 1 files at r2.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/ElementListenerMap.java, line 375 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Hmm. It seems that I added random defensive code without thinking. I removed it now. Thanks

Is it possible to add a unit test for :

• overall remove functionality (so that updateEventSetting is invoked)
• and this specific case: even if there are no listeners for the type updateEventSetting is invoked anyway ?

[ujoni]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/ElementListenerMap.java, line 375 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

Is it possible to add a unit test for :

• overall remove functionality (so that updateEventSetting is invoked)
• and this specific case: even if there are no listeners for the type updateEventSetting is invoked anyway ?

I can add those. It seems like the tests failed. To me it seems like the remove sync to frontend only happens, when all of the listeners for the type have been removed, and setting settings for for removed types might be an error.

I tried having the setting code before removal, but that was not clean enough (i could probably make it clean, but removing before updating settings seemed cleaner).

I guess I'll have to add the defensive code back, but maybe put it inside remove()

[ujoni]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/ElementListenerMap.java, line 375 at r1 (raw file):

Previously, ujoni (Joni) wrote…

I can add those. It seems like the tests failed. To me it seems like the remove sync to frontend only happens, when all of the listeners for the type have been removed, and setting settings for for removed types might be an error.

I tried having the setting code before removal, but that was not clean enough (i could probably make it clean, but removing before updating settings seemed cleaner).

I guess I'll have to add the defensive code back, but maybe put it inside remove()

Done.

[denis-anisimov]

Reviewed 2 of 2 files at r3.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/ElementListenerMap.java, line 141 at r3 (raw file):

            // fixes #5090
            if (listenerMap.listeners != null
                    && listenerMap.listeners.containsKey(type)) {

And I still have the same question as before:
what happens if I remove the last listener for the given type (listenerMap.updateEventSettings(type) won't be called in this case).
But then I add a new listener for this type again.

Doesn't it cause any issue when a new listener is added ?
I understood that some exception is thrown if listenerMap.updateEventSettings(type) is called if listeners doesn't contain type.

But:

• should the place where the exception is thrown be fixed so that it doesn't throw (I don't know, I'm just asking)
• would be nice to have a test which checks that exception is not thrown (this is another functionality, I know, but still...)
• and the last thing mentioned above: what about a new listener added after removal for the same type ? If it's fine then let's have a test for this.

[ujoni]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/ElementListenerMap.java, line 141 at r3 (raw file):
Adding a listener for a type, removing it, and then adding another again is fine. For example, it might be a KeyDownEvent listener. Adding and removing those should and does work.

I understood that some exception is thrown if listenerMap.updateEventSettings(type) is called if listeners doesn't contain type.

A null pointer exception will be thrown when adding a new listener for the same type, as the add method uses EventSettings map verify whether a list of listeners for that type exist. If I update the settings without looking whether the listeners for the same type still exist, I end up creating a dummy settings object for a non-existing listener type. This then breaks the code in add(Type, Listener) and it will throw (trying to add a listener to a null list).

So, checking that we do not add settings for removed types is, in my opinion, correct.

Am I still missing something? :)

[denis-anisimov]

Reviewed 1 of 1 files at r4.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained

[denis-anisimov]

Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained