Add a way to remove a single event listener

[ypujante]

As far as I understand the current API in regards to events, you can add as many listeners as you want, but there is no way to remove just one.

Here is an example:

If I call:

emscripten_set_resize_callback(EMSCRIPTEN_EVENT_TARGET_WINDOW, window, false, callback1);
emscripten_set_resize_callback(EMSCRIPTEN_EVENT_TARGET_WINDOW, window, false, callback2);

in order to remove the callback, you need to call

emscripten_set_resize_callback(EMSCRIPTEN_EVENT_TARGET_WINDOW, window, false, nullptr);

but then it will remove both callbacks as the code shows:

        if (eventHandler.callbackfunc) {
          eventHandler.eventListenerFunc = jsEventHandler;
          eventHandler.target.addEventListener(eventHandler.eventTypeString, jsEventHandler, eventHandler.useCapture);
          JSEvents.eventHandlers.push(eventHandler);
          JSEvents.registerRemoveEventListeners();
        } else {
          for (var i = 0; i < JSEvents.eventHandlers.length; ++i) {
            if (JSEvents.eventHandlers[i].target == eventHandler.target
             && JSEvents.eventHandlers[i].eventTypeString == eventHandler.eventTypeString) {
               JSEvents._removeHandler(i--);
             }
          }
        }

The else block is executed which indiscriminately remove all listeners that have the same target and event type.

The issue I am bumping into with this is that the glfw/emscripten/cpp port I am currently implementing, is setting a bunch of listeners for its own purpose. But the client code can also set its own listeners, the typical use case is the one I showed above to react on window resize to call glfwSetSize when the canvas size changes. But if the client then decides to remove its own listener, then it will remove the one from the library as well, thus crippling the proper functioning of the library.

I do realize that the API is named emscripten_set_resize_callback which implies that you are setting (vs adding) a callback, but the underlying code is adding a listener not replacing a current one.

I realize that changing the current APIs may break existing code, but maybe we could add a new set of APIs (and maybe deprecate the current one if that makes sense)... something like:

int emscripten_add_XXXXX_callback(....);
void emscripten_remove_callback(id);

• instead of set it is add
• it returns an id (and 0 or negative id means failure)
• the id is used to remove this specific callback
• this is similar to emscripten_set_timeout / emscripten_clear_timeout apis

What do you think? I can take a look at some implementation if you think that is an approach you would be interested in.

[sbc100]

It does looks like we are currently lacking this feature (the ability to remove just one listener).

However, before we look into adding this perhaps you could help me understand the use case a little more. In you case is the user code expected to be using GLFW APIs? Does GLFW itself not support registering resize callbacks? Assuming it does it seems reasonable to expect the user code to using the GLFW APIs and not go under the hood to call the emscripten APIs directly?

[ypujante]

It is true that GLFW does offer callbacks, but due to the nature of the GLFW API and the fact that a GLFW window is an HTML canvas makes it not possible to have a 1 to 1 match. The best example is if you want to have a canvas (= GLFW window) which occupies the entire web browser which is for example implemented in ImGui

The way to achieve this behavior is the following:

1. the client code sets a resize callback on the browser window (emscripten_set_resize_callback)
2. the canvas (or more likely a canvas container, like a div) is set to width/height 100% (in css)
3. in the callback code, the size of the canvas container is read (emscripten_get_element_css_size)
4. then glfw is instructed to set the window size to this new size (glfwSetWindowSize)

Setting a resize callback at the glfw level (glfwSetWindowSizeCallback) is simply calling the callback when the size changes (when glfwSetWindowSize is called), and is obviously not doing any of the steps described in 1-4 as it is impossible to know the structure of the html document, css, etc...

You could also imagine adding a little "handle" at the bottom of the canvas with a listener that would similarly call glfwSetWindowSize whenever that handle is dragged around. There is just no way to implement all use cases at the glfw level (stemming from the fact that a "window" is not a traditional window).

Setting this example aside, here are a few data points:

• the current emscripten implementation of glfw (library_glfw.js) is setting its own listeners and is not using any of the emsripten_add.../html5 calls so is not affected by this issue
• the current "official" implementation of SDL is using the html5/emscripten API
• my current wip port of glfw is using html5/emscripten API

I think it is fair to say that the html5/emscripten API can be used by libraries, as well as client code. The issue comes whenever one side (the library or the client code) removes a listener. For example, in my cleanup code (when calling glfwTerminate()) or in the SDL cleanup code, the listeners are removed => it also removes the listeners set by the clients. Vice versa, if the client, for some reason, removes a listener, it will also potentially removes listeners set by the library, thus effectively breaking the proper functioning of the library. In the end using these APIs end up having side effects on other parts of the code which can have unintended consequences.

When you look at the low level javascript API for event handling, you also don't get this add single/remove all behavior.

With all that being said, this is the way it is right now. Clearly SDL is using it and not having issues about it. I can also handle it by just making it clear in the documentation of my library that I am setting/removing listeners with the potential side effects that it brings. I am more worried with a client accidentally removing a listener, which ends up breaking the library, but again I don't think it is the end of the world.

In the end, it is my opinion that the current design is flawed and that the "real" fix should be to deprecate all the current APIs and have new ones become standard, but I do realize that the implications and changes are quite daunting. The workaround of a big disclaimer in my library (and I suppose other libraries) is certainly a far easier approach...

[sbc100]

I agree we should find a way to make it possible to only remove the handlers you installed. Perhaps we can find a way to do this in a backwards compatible way?

Perhaps we can keep the existing emscripten_set_resize_callback and just add something like emscripten_remove_resize_callback which takes a specific function pointer to remove. Given that we can already identify function callbacks by their function address perhaps we can avoid inventing another concept of "handle".

[ypujante]

I think you are correct that it would be better to handle it in a backward compatible way. It also would match the lower level javascript API, since to remove a listener, you just call element.removeEventListener('xxx', listener) (the addEventListener API does not return a handle for example).

I am wondering if we could provide a single API right now, something like:

int emscripten_remove_callback(char const *target, void *userData, void *callback);

that would do a match on target/userData/callback (callback being the eventHandler.callbackfunc and we would need to store userData in eventHandler as well), something like:

        target = findEventTarget(target);
        for (var i = 0; i < JSEvents.eventHandlers.length; ++i) {
          if (JSEvents.eventHandlers[i].target === target
           && JSEvents.eventHandlers[i].userData === userData
            && JSEvents.eventHandlers[i].callbackfunc === callback) {
             JSEvents._removeHandler(i--);
           }
        } 

What do you think?

[sbc100]

Sounds good to me

[ypujante]

Ok great. I will take a look at this when I have a moment and submit a PR.

[sbc100]

I guess the downside of a single emscripten_remove_callback is that it won't support a single callback for multiple event types..

[sbc100]

I guess the function could take int eventType too