Implement HOLD_TAP.

[zelch]

Description

This changes the behavior of tap and hold so that the hold action happens immediately, and if we later decide that we actually have a tap, we release the hold action just before sending the tap.

This is very helpful when the hold action changes the behavior of some external device, such as altering the behavior of mouse button clicks.

As an aside, this is my first time poking at the guts of QMK, so I would not be entirely shocked if changes needed to be made before this can be merged.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[sigprof]

This option really needs a per-key variant, because the modified behavior may not apply to all dual-role keys (note that OSM() and OSL() also use the same code).

Also the current implementation violates the assumption that every press event is followed by a corresponding release event (on a tap the modified code will emit a press with record->tap.count = 0 first, then another press with record->tap.count = 1, then a release with record->tap.count = 1). Maybe a more proper way would be to send an additional release event with record->tap.count = 0 when a tap is detected (this would probably make the existing MT() and LT() implementation work as is); however, some code may want to distinguish between a real hold release and a hold which reverts to a tap. There are some spare bits in record->tap — maybe it's time to use one of them.

BTW, the existing code also has process_record_tap_hint(), but that function does not have *_user() and *_kb() hooks; so another way to implement this feature is to add those hooks and make MT() optionally use that function to set the modifiers early. This could break some existing user code that was overriding the actions of those keycodes though.

Also I suppose that if you press more than one MT() key at the same time, the press handling for the second modifier will be delayed anyway until the first tap/hold decision is actually completed. Not sure what to do about that though — the existing code is able to handle only a single tap/hold decision at a time, and with the presence of LT() it's even impossible to know the real keycodes of queued events before the first tap/hold decision actually resolves (also some people abuse LT() to get custom functionality, so you can't assume anything from just a keycode, although the common convention is to use LT(0, x) for custom usage).

Finally, any kind of additional behavior in this area should come with tests.

[zelch]

This option really needs a per-key variant, because the modified behavior may not apply to all dual-role keys (note that OSM() and OSL() also use the same code).

Fair enough.

Also the current implementation violates the assumption that every press event is followed by a corresponding release event (on a tap the modified code will emit a press with record->tap.count = 0 first, then another press with record->tap.count = 1, then a release with record->tap.count = 1). Maybe a more proper way would be to send an additional release event with record->tap.count = 0 when a tap is detected (this would probably make the existing MT() and LT() implementation work as is); however, some code may want to distinguish between a real hold release and a hold which reverts to a tap. There are some spare bits in record->tap — maybe it's time to use one of them.

I am a little confused here, but I suspect that the confusion that I'm having is the distinction between an internal QMK event and an external USB HID event.

At the HID level, a tap shows up as a key down event on the hold key, a key up event on the hold key, then the key down event on the tap key, and then the key up event on the tap key.

There was a bug around LT in my first iteration of the code, where on a tap we would end up being stuck with the altered layer being on, however that was resolved with the addition of the layer_off call in process_action, just below line 699.

If you're suggesting generating a new keyrecord_t, and sending that through the whole system...

I'm assuming that we would use process_record as process_tapping does, but I'm at a mild loss on where to put that, especially for ensuring that we get the ordering correct and don't confuse the internal state machine.

It is fairly important that the tap key events be sent after the key up for the hold key, so that you don't end up getting the tap key processed with the modifier applied. That would be a pretty significant behavior change from the current tap-hold behavior, where as the current hold-tap code should be nearly invisible, at least assuming that HOLD_ON_OTHER_KEY_PRESS is in play.

BTW, the existing code also has process_record_tap_hint(), but that function does not have *_user() and *_kb() hooks; so another way to implement this feature is to add those hooks and make MT() optionally use that function to set the modifiers early. This could break some existing user code that was overriding the actions of those keycodes though.

I'm definitely aiming to avoid breaking any existing logic, and we need to be able to suppress the generation of the second press event done as part of the existing tap-hold code, while still doing everything else that gets done at the same time. (After the tap period expires, or someone presses another key with the correct tap-hold mode in place.)

Also I suppose that if you press more than one MT() key at the same time, the press handling for the second modifier will be delayed anyway until the first tap/hold decision is actually completed. Not sure what to do about that though — the existing code is able to handle only a single tap/hold decision at a time, and with the presence of LT() it's even impossible to know the real keycodes of queued events before the first tap/hold decision actually resolves (also some people abuse LT() to get custom functionality, so you can't assume anything from just a keycode, although the common convention is to use LT(0, x) for custom usage).

Finally, any kind of additional behavior in this area should come with tests.

I need to spend some time learning enough about the test code to write new tests.

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.