Skip to content

Conversation

@Glodigit
Copy link
Contributor

@Glodigit Glodigit commented Mar 8, 2025

This is a chunk of #1087 for a module that

  • Adds the 2 buttons found on a SpaceMouse Compact.
  • Adds buttons for increasing/decreasing movement in the 6 axes.

PR dependencies: #1092

Copy link
Collaborator

@xs5871 xs5871 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is basically a copy-paste of the mouse keys module, but having a dozen methods that all don't actually do anything except binding over a parameter is a terrible design pattern.
The movement direction should be an attribute of SpacemouseKey and we only need a single on_press and on_release.

@Glodigit
Copy link
Contributor Author

Glodigit commented Apr 1, 2025

So something like the below?

for n, names in enumerate(keys):
    make_key(
        names=names,
        on_press=self.on_press,
        on_release=self.on_release,
        constructor=SpacemouseKey,
        direction=1<<n
        )

or to use the existing .code?

for n, names in enumerate(keys):
    make_key(
        names=names,
        on_press=self.on_press,
        on_release=self.on_release,
        constructor=SpacemouseKey,
        code=0x80+n
        )

or would it be a new SpacemouseDirectionKey constructor to avoid unintended HID reports?

@xs5871
Copy link
Collaborator

xs5871 commented Apr 1, 2025

The way it's implemented right now SpacemouseKey should only be used for buttons. I think axes need a now derived key class, i.e. a SpacemouseDirectionKey. You can still define that the same way as SpacemouseKey and assign the direction to .code.

@Glodigit Glodigit mentioned this pull request Apr 2, 2025
Comment on lines 122 to 137
def _maybe_start_move(self, mask):
self._movement |= mask
if self._movement == mask:
self._task.restart()

def _maybe_stop_move(self, mask):
self._movement &= ~mask
if not self._movement:
cancel_task(self._task)
self._move_step = 0

def _on_press(self, key, keyboard, *args, **kwargs):
self._maybe_start_move(key.code)

def _on_release(self, key, keyboard, *args, **kwargs):
self._maybe_stop_move(key.code)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny nitpick:

Suggested change
def _maybe_start_move(self, mask):
self._movement |= mask
if self._movement == mask:
self._task.restart()
def _maybe_stop_move(self, mask):
self._movement &= ~mask
if not self._movement:
cancel_task(self._task)
self._move_step = 0
def _on_press(self, key, keyboard, *args, **kwargs):
self._maybe_start_move(key.code)
def _on_release(self, key, keyboard, *args, **kwargs):
self._maybe_stop_move(key.code)
def _on_press(self, key, keyboard, *args, **kwargs):
self._movement |= key.code
if self._movement == key.code:
self._task.restart()
def _on_release(self, key, keyboard, *args, **kwargs):
self._movement &= ~key.code
if not self._movement:
cancel_task(self._task)
self._move_step = 0

@xs5871 xs5871 merged commit 38dfb09 into KMKfw:main Apr 15, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants