-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add a way to remove a single event listener (#20983) #25535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
adeb50b
4260b0b
6925d65
d489b58
9fc8bd1
55d37cf
b7b7240
6583129
cdf9b94
8578fb2
581444b
42410ab
5aea670
d1b426f
f7bd5d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -205,6 +205,25 @@ var LibraryHTML5 = { | |
| return {{{ cDefs.EMSCRIPTEN_RESULT_SUCCESS }}}; | ||
| }, | ||
|
|
||
| removeSingleHandler(eventHandler) { | ||
| if (!eventHandler.target) { | ||
| #if ASSERTIONS | ||
| err('removeSingleHandler: the target element for event handler registration does not exist, when processing the following event handler registration:'); | ||
| console.dir(eventHandler); | ||
| #endif | ||
| return {{{ cDefs.EMSCRIPTEN_RESULT_UNKNOWN_TARGET }}}; | ||
| } | ||
| for (var i = 0; i < JSEvents.eventHandlers.length; ++i) { | ||
| if (JSEvents.eventHandlers[i].target === eventHandler.target | ||
| && JSEvents.eventHandlers[i].eventTypeId === eventHandler.eventTypeId | ||
| && JSEvents.eventHandlers[i].callbackfunc === eventHandler.callbackfunc | ||
| && JSEvents.eventHandlers[i].userData === eventHandler.userData) { | ||
| JSEvents._removeHandler(i--); | ||
| } | ||
| } | ||
| return {{{ cDefs.EMSCRIPTEN_RESULT_SUCCESS }}}; | ||
| }, | ||
|
|
||
| #if PTHREADS | ||
| getTargetThreadForEventCallback(targetThread) { | ||
| switch (targetThread) { | ||
|
|
@@ -295,6 +314,8 @@ var LibraryHTML5 = { | |
| var eventHandler = { | ||
| target: findEventTarget(target), | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: keyEventHandlerFunc, | ||
| useCapture | ||
|
|
@@ -409,6 +430,18 @@ var LibraryHTML5 = { | |
| }, | ||
| #endif | ||
|
|
||
| emscripten_html5_remove_event_listener__proxy: 'sync', | ||
| emscripten_html5_remove_event_listener__deps: ['$JSEvents', '$findEventTarget'], | ||
| emscripten_html5_remove_event_listener: (target, userData, eventTypeId, callback) => { | ||
| var eventHandler = { | ||
| target: findEventTarget(target), | ||
| userData, | ||
| eventTypeId, | ||
| callbackfunc: callback, | ||
| }; | ||
| return JSEvents.removeSingleHandler(eventHandler); | ||
| }, | ||
|
|
||
| emscripten_set_keypress_callback_on_thread__proxy: 'sync', | ||
| emscripten_set_keypress_callback_on_thread__deps: ['$registerKeyEventCallback'], | ||
| emscripten_set_keypress_callback_on_thread: (target, userData, useCapture, callbackfunc, targetThread) => | ||
|
|
@@ -499,6 +532,8 @@ var LibraryHTML5 = { | |
| allowsDeferredCalls: eventTypeString != 'mousemove' && eventTypeString != 'mouseenter' && eventTypeString != 'mouseleave', // Mouse move events do not allow fullscreen/pointer lock requests to be handled in them! | ||
| #endif | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: mouseEventHandlerFunc, | ||
| useCapture | ||
|
|
@@ -592,6 +627,8 @@ var LibraryHTML5 = { | |
| allowsDeferredCalls: true, | ||
| #endif | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: wheelHandlerFunc, | ||
| useCapture | ||
|
|
@@ -664,6 +701,8 @@ var LibraryHTML5 = { | |
| var eventHandler = { | ||
| target, | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: uiEventHandlerFunc, | ||
| useCapture | ||
|
|
@@ -708,6 +747,8 @@ var LibraryHTML5 = { | |
| var eventHandler = { | ||
| target: findEventTarget(target), | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 |
||
| callbackfunc, | ||
| handlerFunc: focusEventHandlerFunc, | ||
| useCapture | ||
|
|
@@ -765,6 +806,8 @@ var LibraryHTML5 = { | |
| var eventHandler = { | ||
| target: findEventTarget(target), | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: deviceOrientationEventHandlerFunc, | ||
| useCapture | ||
|
|
@@ -835,6 +878,8 @@ var LibraryHTML5 = { | |
| var eventHandler = { | ||
| target: findEventTarget(target), | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: deviceMotionEventHandlerFunc, | ||
| useCapture | ||
|
|
@@ -917,6 +962,8 @@ var LibraryHTML5 = { | |
| var eventHandler = { | ||
| target, | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: orientationChangeEventHandlerFunc, | ||
| useCapture | ||
|
|
@@ -1024,6 +1071,8 @@ var LibraryHTML5 = { | |
| var eventHandler = { | ||
| target, | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: fullscreenChangeEventhandlerFunc, | ||
| useCapture | ||
|
|
@@ -1522,6 +1571,8 @@ var LibraryHTML5 = { | |
| var eventHandler = { | ||
| target, | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: pointerlockChangeEventHandlerFunc, | ||
| useCapture | ||
|
|
@@ -1566,6 +1617,8 @@ var LibraryHTML5 = { | |
| var eventHandler = { | ||
| target, | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: pointerlockErrorEventHandlerFunc, | ||
| useCapture | ||
|
|
@@ -1716,6 +1769,8 @@ var LibraryHTML5 = { | |
| var eventHandler = { | ||
| target, | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: visibilityChangeEventHandlerFunc, | ||
| useCapture | ||
|
|
@@ -1831,6 +1886,8 @@ var LibraryHTML5 = { | |
| allowsDeferredCalls: eventTypeString == 'touchstart' || eventTypeString == 'touchend', | ||
| #endif | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: touchEventHandlerFunc, | ||
| useCapture | ||
|
|
@@ -1914,6 +1971,8 @@ var LibraryHTML5 = { | |
| allowsDeferredCalls: true, | ||
| #endif | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: gamepadEventHandlerFunc, | ||
| useCapture | ||
|
|
@@ -2000,6 +2059,8 @@ var LibraryHTML5 = { | |
| var eventHandler = { | ||
| target: findEventTarget(target), | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: beforeUnloadEventHandlerFunc, | ||
| useCapture | ||
|
|
@@ -2050,6 +2111,8 @@ var LibraryHTML5 = { | |
| var eventHandler = { | ||
| target: battery, | ||
| eventTypeString, | ||
| eventTypeId, | ||
| userData, | ||
| callbackfunc, | ||
| handlerFunc: batteryEventHandlerFunc, | ||
| useCapture | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe after this paragraph, add something like
to try to reinforce that people will track the userData pointer as being relevant.