Add a way to remove a single event listener (#20983)

[ypujante]

I know it has been quite a while since I opened #20983 but I finally got the time to work on it. I followed the discussion we had at the time and did the following:

• added emscripten_remove_callback
  • iterate over all handlers and only remove the one matching the arguments provided
• changed all event setters to pass userData and eventTypeId which are required by this API
• added a test that shows that it works

This first pass is to make sure that this would be an ok change to Emscripten and that the changes work (no failures introduced by the changes). I can obviously change whatever you want or drop it entirely if this is not desired. And can work on documentation/examples if that gets approval.

I believe it is a good addition because it allows to remove a previously set listener, which is not possible at the moment, as the APIs provided will remove all of the ones set (see discussion in the #20983 ticket).

[ypujante]

@sbc100 The only failing test is the one about code size, which I can address if you are satisfied with the direction of this PR.

In the grand scheme of things it is a pretty low risk PR since the only changes to existing code (17 functions in total) is passing an additional 2 fields in the EventHandler map that are not used prior to this PR. Of course it makes the resulting code bigger.

I added a test which does a basic test of the functionality by counting the number of event handlers registered. That being said, during development of such test, I was running it manually in the browser and making sure that the result of removing a handler was actually having an action on the underlying JavaScript event handler being present or not.

Let me know your thoughts.

Thanks

[sbc100]

The approach seem pretty reasonable.

Do you really need to be able to register and un-register the same function with different user data?

[sbc100]

I don't really love all the repetition here, or the fact that we need to store two extra fields just for this new API, but I guess it makes sense, and the repetitive nature of this file is kind of a pre-existing condition :)

[ypujante]

I agree. I don't love it either, but you are right, this code is replicated over and over (17 times!). I feel like the API could have been simplified in the first place (having just 1 function call for all the calls using the same callback type and providing the eventTypeId... which would have reduced, for example, 9 mouse related calls into just 1...). It is what is now unfortunately. So that is the only solution I can think of to be able to implement this new API.

[ypujante]

Let me correct what I just said in regards to the count. There are indeed 17 places where I had to change the code, but there are way many more "emscripten_set_xxx_callback". There are 17 types of events supported, so we could not really simplyif this since they all have different callback (function pointer) types.

The example I gave for mouse counts as just 1 on these 17 places because there are 9 "emscripten_set_mousexxx_callback" functions which all funnel into 1 registerMouseEventCallback function call. I feel like this API is way too verbose and having just one emscripten_set_mouse_callback with the eventTypeId as a parameter (which is exactly what the internal registerMouseEventCallback is) would have been plenty in the first place...

[juj]

Indeed the original API shape is a bit more verbose than what I would like. Pardon, this was my very first API design into Emscripten way back in 2014.

I am not much worried about the repetition though, since it will closure minify to just a condensed ,a in code size for each parameter.

One could create a map of ID->string to avoid carrying both ID and string here, but storing that map would take up more bytes than the added ,a for each function. (and the map would not DCE individual fields out)

[sbc100]

@juj what do you think about adding this new API?

[juj]

Hmm, should we key the userData pointer into the removal? I.e. it would be better for this API to be

emscripten_remove_callback: (target, eventTypeId, callback)

The rationale is that the userData pointer is generally just data to the function call, and not identifying. I.e. it is more of the 'value' part in a key->value association.

It might be some function local value in the caller (e.g. a boolean for some behavior), and might result in user having to store the userData field in some global to be able to unregister. So requiring users to know to match the userData value could be a bit tedious.

(If users did specifically want to differentiate unregistrations based on what was seemingly a different userData value, they could do that by separating to a different function with a trampoline)

Also, this function would be good to follow the same _on_thread model, so that it can be called from pthreads symmetric to how registering on pthreads works.

The semantics of function emscripten_remove_callback are to remove all copies of callback registrations of the given function. I wonder if the function naming should somehow reflect that?

[juj]

@juj what do you think about adding this new API?

Looks good to me. I would consider removing the userData part from the unregistration, and also this would be good to support operation from pthreads to be symmetric/complete.

Apart from that, looks like a great addition.

[ypujante]

@juj Thank you for the feedback

Looks good to me. I would consider removing the userData part from the unregistration,

Personally I do not have a strong opinion either way. What matters is that if my library (ex: glfw) sets a listener, I am able to remove it without removing other listeners set by the client code on the same target. The eventTypeId/callback combination should be enough to achieve this result (although it might make the code a bit more tedious when the caller wants to include userData as a differentiator). It will also simplify a bit the code and make it smaller since we don't need to store userData anymore. So if @sbc100 is fine with it I will remove it.

and also this would be good to support operation from pthreads to be symmetric/complete.

At first I tried to implement support for pthread but it doesn't seem to make sense from my understanding of the code. pthread is only used to specify how to invoke the callback once it has been registered. Registering the callback itself (which is done by calling JSEvents.registerOrRemoveHandler(eventHandler);) has nothing to do with pthread. So I don't see why we need pthread to remove/unregister the callback. Am I missing something?

The semantics of function emscripten_remove_callback are to remove all copies of callback registrations of the given function. I wonder if the function naming should somehow reflect that?

I suppose we could call it emscripten_remove_callbacks instead? Either way, I will also add this function to the documentation once approved to move forward to describe what it does.

From a schedule point of view, I am traveling so I won't be able to work on it for a couple weeks. I don't think there is urgency anyway ;)