keyboardio / KeyboardioHID

A HID library for Arduino
MIT License
34 stars 17 forks source link

Simplify and correct Mouse features #82

Closed gedankenexperimenter closed 2 years ago

gedankenexperimenter commented 2 years ago

Two changes: a simplification and correction of the suppression of no-op mouse reports, and a correction of the click() method.

The logic for no-op report suppression had been incorrect, so we would sometimes send a report with the button state unchanged and all zeros for cursor and scroll movement. This fixes that, and also makes the code clearer and streamlines things (there's no point in storing a full copy of the previous mouse report when the only thing that it's valid to compare against is the button state).

The click() method was built assuming that a report would be sent every cycle, so it didn't bother sending the second ("release") report explicitly. This is not a safe assumption (current Kaleidoscope doesn't do this). Also, if the button(s) in question were already held, they would need to be released first in order to make a "click".

Fixes #52

gedankenexperimenter commented 2 years ago

I'll fix the typo and add comments later today.

gedankenexperimenter commented 2 years ago

On further reflection, though I still think it would be best to remove click() from the Mouse HID API, I see that my earlier interpretation was perhaps misguided. If we look at the following two lines as a single unit:

Mouse.click(buttons);
Mouse.sendReport();

…it's pretty much equivalent to this:

Mouse.move(x, y, 0, 0);
Mouse.sendReport();

…in that the former clicks the mouse button(s), and the latter moves the cursor, so in that way it's defensible to say that click() should not call sendReport() after release().

On the other hand, calling click() without immediately calling sendReport() is quite different from calling move(), or even press(), in that it does send the press report, and with the additional report sent after release() (within click()), the additional call to sendReport() (after click() returns) is harmless in the code above.

On the gripping hand, no (core) Kaleidoscope plugin ever calls Mouse.click(), and there probably aren't any other users of the library that do, either, so it likely doesn't matter at all whether or not it's consistent, buggy, or both…

obra commented 2 years ago

On the gripping hand, no (core) Kaleidoscope plugin ever calls Mouse.click(), and there probably aren't any other users of the library that do, either, so it likely doesn't matter at all whether or not it's consistent, buggy, or both…

That we never call mouse.click is quite reassuring.

If we were going to actually support click, I think I'd want to have sendReport() do the magic of sending the newly-added earlier report, the click report, and the release, sort of how we have the keyboard library break apart reports containing both 'shift' and letter key for ChromeOS.

I'd be fine, I think, with just deprecating click(). but given that we don't use it, I'm ok merging the changes.