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?

[sbc100]

I think this makes sense to me, but in that case I think it would also be good it if we make it impossible to register the same callback function for the same event with different userData.

emscripten_set_click_callback(window, data1, 0, mycallback);   // success
emscripten_set_click_callback(window, data2, 0, mycallback);   // error `mycallback` is already registered as a click callback

If we didn't error in the second case, and allowed the same callback to be registred twice, there would be no way to unregister just one of them.

[ypujante]

• if we want to error when registering the same callback with a different user data, then it means we need to store the user data (which we don't prior to this changes)
• it also means that code that is currently working could fail after this change

I think what @juj suggested was to indeed un-register all callbacks that were registered with different data but with the same target/event_id/callback combination. That would have eliminated having to store the user data.

It sounds like you don't like this because then "there would be no way to unregister just one of them".

I am definitely not in favor of erroring when registering 2 identical callbacks with different user data, because from a developer point of view, this is absolutely not an error and I can imagine scenarios where that could be useful.

So I guess the solution is to leave the API and changes as implemented in this PR. That way we can un-register a single callback (which IS the point of this PR). So I am personally in favor of this.

I will now work on updating the documentation and fixing the "Code Size" failure.

[sbc100]

I am definitely not in favor of erroring when registering 2 identical callbacks with different user data, because from a developer point of view, this is absolutely not an error and I can imagine scenarios where that could be useful.

Can you give an example of when this might be useful?

[ypujante]

This a theoretical example, because I am not implementing it this way at the moment, but imagine for GLFW, you have 2 windows (represented in C by GLFWwindow pointers and canvases in HTML) and you need to set mouse move callbacks to track the mouse movements.

In order to detect moving the mouse outside the canvas while the button is clicked (something my library supports where others don't) you cannot set the callback on each canvas, it has to be set on the window... so the callbacks would be:

emscripten_set_mousemove_callback(WINDOW, glfwWindow1, true, handle_move_callback);
emscripten_set_mousemove_callback(WINDOW, glfwWindow2, true, handle_move_callback);

Again I am not doing it this way, but that could be a way to implement it...

[sbc100]

Fair enough. If we do want to officially support that then we probably also want to officially support unregistering them individually (sadly)

[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 ;)

[ypujante]

I am done with traveling so I can work on this again. And I would like to wrap this up if at all possible.

@sbc100 do you have any opinion on @juj feedback? Do you have an opinion on removing the userData parameter?

@juj Can you provide feedback on my comment about pthread?

Thanks

[ypujante]

I think I am done on my side:

Based on the discussion, I left the code as-is with the userData parameter included. If we exclude it, then we need to disallow registering multiple callbacks with the same userData. It is still doable if you prefer to go down this route, but it means that it will break code relying on the fact that it is allowed now.

• I have changed the code
• Updated the documentation
• Updated the Changelog
• Fixed the size (after many attempts...)
• All tests are green

Let me know if there is anything else or if this can be merged as-is.

[sbc100]

Given hat we decided to have N different ways to register each callback it seems like we should probably just a N different un-registering functions too.

e.g. emscripten_set_mousedown_callback should have a corresponding emscripten_remove_mousedown_callback or emscripten_unset_mousdown_callback.

Alternatively we are doing to add a generic form such as emscripten_remove_callback the obvious question is why can't we have a generic emscripten_add_callback?

I guess one issue is that each callback has its own signature, which means the generic version requires the use of void* for the callback argument? So one advantage of using N different function is that we get better type safety.

I guess my preference would probably be to stick with current convention and have N different functions?

If we do go with emscripten_remove_callback maybe we can some up with less generic name .. there nothing in the name that suggests html5 or events.. so in isolation I think maybe it could be better named.

My apologies for opening up an log of last minute questions..

[ypujante]

@sbc100 No worries about the back and forth. I suppose that is the point of a PR.

So the "N" you are talking about is 35 in html5.h (and some additional in WebGL).

Honestly, I don't think it is very reasonable to add this kind of bloat to Emscripten to add this lacking feature (or fix a bug depending on how you look at it, since after all there is a way to remove callbacks right now, just that removing them, will remove more than you want, see #20983 for details).

In addition, I feel like the design was way too granular. I understand the need/want to have a type safe API with the proper function pointers, but there could have been 1 "set_callback" API per type (ex: 1 for all mouse events, instead of 9 like there is...). And yes you could totally have a single generic set_callback api, you would just loose the type safety...

For these reasons, I feel like adding an additional 35+ calls just for this is somewhat not the right approach.

Having a single generic call to remove any callback is by far the least intrusive. Yes you loose on type safety, but in this instance it is really unnecessary to have it since the function pointer is never used as a type pointer to invoke, but as a comparison argument.

I agree with you that taken out of context the name emscripten_remove_callback is not a good name. I wished I had realized this sooner. I noticed that there is already a function called emscripten_html5_remove_all_event_listeners so maybe we could name this one emscripten_html5_remove_event_listener instead?

Finally I am wondering if the API should return an error in case the event listener being asked to be removed doesn't exist (right now it doesn't in this instance). That way it could somehow cover cases where the wrong callback/function pointer is being used accidentally (a poor type safety check in some way).

In any case, these are my opinion and observations. You have the final call, and I will be fine with any way you want to go. Even if you want to go down the 35+ way, it will be far more work, but I am fine doing it. There is also the other alternative to simply close this PR and forget about it. There is currently no way to remove a specific callback (which is what is being addressed by this PR), but beside me, it doesn't seem that it has bothered anybody else. So I suppose we could just not do it.

[sbc100]

Agree that adding 35 new API calls also seems like a bad idea :-/

Maybe we should at at least add a new generic API for this? i.e. if we also add a single API for registering callbacks then we could at least say that the old API is deprecated? On the other hand we do have a general policy of avoiding adding API that do too much multi-plexing since they prevent DCE. Perhaps that is good argument for allowing an asymmetry in this case.

[ypujante]

I have no opinion on whether to add a generic API or not but I have no issue implementing it. From your last message it looks like the 35+ option is out of the debate.

So what do you want me to do:

1. Rename emscripten_remove_callback into emscripten_html5_remove_event_listener (or some other name / please specify)
2. Implement 1) and add a generic API (like emscripten_html5_set_event_listener)
3. Close this PR
4. Something else? Please specify

Please let me know which option you want me to do.

[sbc100]

I'm afraid I don't have an answer for you now. I think some more thought/discussion needs to go into this. Lets involve @juj and @kripken and see what they think.

Am I right in thinking that you can use this downstream today (as an out-of-tree JS library function) until we resolve it here?

[ypujante]

Am I right in thinking that you can use this downstream today (as an out-of-tree JS library function) until we resolve it here?

Not really no, because it requires changing the code of 17+ functions to pass the information that is currently not stored and needed by emscripten_html5_remove_event_listener. So I cannot just add emscripten_html5_remove_event_listener as an out-of-tree JS library function.

But waiting and not having an answer right now until we get it right is fine. I am not blocked. This has been like this for my library since I released it: if a user register/unregister event(s) using any of these functions, it will break the functioning of the library.

Also note that adding this function will still not fix the issue that my library can call the new emscripten_html5_remove_event_listener (and do the right thing) while the user code calls emscripten_set_resize_callback(EMSCRIPTEN_EVENT_TARGET_WINDOW, window, false, nullptr); to remove their listeners that would impact the ones set by my library because they would be removed as well (unless we deprecate and later forbid passing nullptr in these functions...)

In any case, I am good in waiting.