SuperFlyTV / xkeys

A Node.js module to interact with the X-keys panels.
MIT License
33 stars 12 forks source link

Add implementation of XKeysWatcher for browser environments under web package #105

Closed loucadufault closed 2 months ago

loucadufault commented 3 months ago

104

Largely based on the existing implementation in https://github.com/SuperFlyTV/xkeys/blob/838adffd0b24b9a749e131855ea5d934f1c6c086/packages/node/src/watcher.ts

Key differences:

Missing features:

I tried to keep the code stylistically similar: comments, names, etc. Also included the "dead" debug logging code for convenience.

loucadufault commented 3 months ago

Full disclosure: this is completely untested. I'm hoping to be able to do some testing in the near future.

Julusian commented 3 months ago

would the https://developer.mozilla.org/en-US/docs/Web/API/HID/connect_event and https://developer.mozilla.org/en-US/docs/Web/API/HID/disconnect_event be usable for this instead of polling?

loucadufault commented 3 months ago

would the https://developer.mozilla.org/en-US/docs/Web/API/HID/connect_event and https://developer.mozilla.org/en-US/docs/Web/API/HID/disconnect_event be usable for this instead of polling?

Thanks for the suggestion.

Did some testing on my end with an X-Keys 128. Findings:

The navigator.hid.connect event not being fired can be explained by the browser's inability to remember the keyboard, and an important fact missing from the MDN docs, that this chrome dev article alludes to:

When the website has been granted permission to access an HID device, it can actively receive connection and disconnection events by listening to "connect" and "disconnect" events.

Which suggests the events are only fired for devices which have been granted permission through a call to requestDevices. This makes sense, as it would otherwise leak devices to non-permissioned pages.

At least with my X-Keys device this suggests the connect event is not suitable. The disconnect event might be useful, remains to be seen how it compares to the library XKeys instance disconnected event. Perhaps both can be used to cover different cases.

Also, through testing the polling approach works quite well (have yet to validate the entire implementation).

nytamin commented 2 months ago

Thanks for this contribution! Since this is a work-in-progress, I'll mark this PR as a draft for now.

I gave this an initial test in Chrome, these are my findings:

Do you know if it's possible to get a browser to remember past connected panels? If not, I don't really see the point of adding the XKeysWatcher for browsers, since it won't add any functionality anyway? Or do you have a use case I'm missing?

loucadufault commented 2 months ago

Thanks for the feedback and review @nytamin .

When I pull the plug of an xkeys panel and then reconnect it, the browser doesn't seem to remember the panel, so I need to request permissions for it again for it to show up in the list of devices.

This matches the testing I had done.

Do you know if it's possible to get a browser to remember past connected panels?

I believe it has to do with the XKeys devices (at least the ones you and I have tested) not having a serial number https://issues.chromium.org/issues/40625708:

The change in #6 adds persistent permissions for USB devices that expose a serial number

And here is the log from chrome://device-log/:

[09:24:36] USB device added: vendor=1523 "P. I. Engineering", product=1227 "XK128Pi4", serial="", guid=3b3b6407-6770-4806-b0d7-17d1335120a4

It would seem this info would need to be set on the XKeys panels devices themselves... I am not sure whether this is achievable.


Regarding use cases for this implementation, the two use cases I have for this are:

nytamin commented 2 months ago

if the XKeys 'disconnected' event even fires at all

Oh, that's a good spot that I haven't noticed before. The disconnect event doesn't fire in the WebHID version, which makes it inconsistent to the node-version. I addressed this in a separate PR: https://github.com/SuperFlyTV/xkeys/pull/107

And here is the log from chrome://device-log/ [...] serial="" [...]

Yeah, unfortunately the xkeys panels do not have any serial numbers in their hardware.

use case: to poll for new devices in a different part of the application as the requestXKeysPanels call was made

Ah, yes that's a reasonable use case.


I think, in order to make documentation easier and the code maintainable, that I'd like to refactor this so that most of the code is shared between the node and the webhid versions.

I've started to look into the refactoring, so I'll open another PR in a little while with those changes.

Note to self: Before merging, make sure to update the documentation to make it clear that the watcher won't work "out of the box" in a browser, and that it must be paired with a requestXkeysPanels() call to work.

loucadufault commented 2 months ago

Sounds good to the above, and thanks for the patch in #107.

Let me know if you would like me to make changes to this PR once the refactors are opened.