Refactor KeyboardListenerAPI #2087
Conversation
|
@jodeleeuw Sorry for requesting your reviews so often; low priority. |
| reset(root_element: Element) { | ||
| this.listeners.clear(); | ||
| this.heldKeys.clear(); | ||
| root_element.removeEventListener("keydown", this.rootKeydownListener); | ||
| root_element.removeEventListener("keyup", this.rootKeyupListener); | ||
| } | ||
|
|
||
| createKeyboardEventListeners(root_element: Element) { | ||
| root_element.addEventListener("keydown", this.rootKeydownListener); | ||
| root_element.addEventListener("keyup", this.rootKeyupListener); | ||
| } |
There was a problem hiding this comment.
@jodeleeuw These methods are not mentioned in the docs. Any reason for them to be publicly available? I can't think of a scenario in which users would need to call these. Also, calling reset() in a plugin without calling createKeyboardEventListeners() afterwards will break keyboard responses in subsequent trials. I think the root element should be passed to the constructor instead which sets everything up then.
There was a problem hiding this comment.
No, these were meant to be internal methods. I'm fine with moving to a constructor.
There was a problem hiding this comment.
A few notes on how I implemented this now:
- Only in the constructor wouldn't have worked since the JsPsych
DOM_containeris currently only known whenrun()is called - I didn't want to leave a public setup function, so a function is injected into the constructor now that returns the display element, if known. For unit tests (and if we change the
DOM_containersetup in jsPsych), that function is already invoked in the constructor. If it did not return an element then, the listeners are registered on the firstgetKeyboardResponse()call when the display element is already known. - I used this chance to remove the dependency on JsPsych and the html-keyboard-response plugin in
pluginapi.test.ts. - When we change the internal JsPsych setup behavior during some future refactoring, we can also get rid of the function injection approach without changing any public API :)
There was a problem hiding this comment.
Since the display_element parameter is passed to initJsPsych and not run() couldn't we know the value of DOM_container earlier? I suppose one reason for delaying it is if you wanted to dynamically setup a container between the time that you called initJsPsych and run but I can't think of a reason that you couldn't do the dynamic initialization prior to calling initJsPsych.
There's no legacy reason to not move it to initJsPsych as far as I know, since in v6.x run() and initJsPsych were essentially the same thing.
There was a problem hiding this comment.
I'd love to move it, but I just went over the code again and found the blocker: setupDOM() is asynchronous because it waits for the document to be ready before it creates the container elements (#948). So there's no way to reliably know the container elements in the constructor...
…jects directly in pluginapi tests
In an approach to https://github.com/jspsych/jsPsych/projects/6#card-64293394, I recently refactored the
KeyboardListenerAPIclass to type the parameters and clean up the logic quite a bit. I also replaced the two array and object-based solutions for listeners and held keys with ES6 sets, which fit the use case very well and could also improve the performance in scenarios with many listeners.I'll add some notes as review comments below.