-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(behaviors): Adding global quick tap #1187
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can merge the 'global' and 'local' quick-tap behaviors a bit;
- keep the
last_tapped
andstore_last_tapped
names, renamelast_tapped.tap_deadline
totimestamp
as you did. - change the signature of
store_last_tapped
to take a position + timestamp - store the last tapped key timestamp (either a normal keypress or the hold-tap) in
last_tapped.timestamp
This would make the global & local behaviors use the same data, and fix a bug in the current quick-tap implementation too. In the current implementation, only hold-tap keys are considered for quick-taps, which I think is a bug;
- tap hold-tap A
- tap &kp B
- tap hold-tap A again
this sequence should never trigger local 'quick-tap', but if the hold-taps are pressed within quick_tap_ms
it will quick-tap in the existing implementation.
Rather than holding two timestamps they are now rolled into one.
@@ -619,6 +631,10 @@ static int keycode_state_changed_listener(const zmk_event_t *eh) { | |||
// we want to catch layer-up events too... how? | |||
struct zmk_keycode_state_changed *ev = as_zmk_keycode_state_changed(eh); | |||
|
|||
if (ev->state && !is_mod(ev->usage_page, ev->keycode)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of checking for mods, we should check if the timestamps are identical. That would be a better filter to find a hold-taps' own keypresses (although it's not perfect).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
I don't love the solution on line 634 but I'm not sure if my suggestion is much better. I'd love to hear some arguments for or against. I'm not opposed to merging as-is.
I agree @okke-formsma, the solution to avoid hold events updating the last tapped timestamp is very hack. We briefly discussed what I would consider to be the proper solution on Discord the other day (https://discord.com/channels/719497620560543766/719544230497878116/961355224566624286), but that relies on a larger, future, feature. If we want to use timestamps to identify the holds I'm fine with that as well, it's definitely a bit less hack, though it will require an extra variable / logic to keep track of the last hold timestamp. I'll leave it up to whoever else wants to review ;). At the very least I think some comments should likely be added to note the preferred implementation down the line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes make sense, one comment on the documentation.
docs/docs/behaviors/hold-tap.md
Outdated
If global quick tap is enabled, then the quick tap will apply not only when the given hold tap is tapped, but for any key tap before it. This effectively disables the hold tap when typing quickly, which is can be quite useful for homerow mods. It can also have the effect of removing the input delay when typing quickly. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think regular users will understand what this means. Can we be a bit more verbose, or give some example sequences so this is clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some docs questions and nits.
docs/docs/behaviors/hold-tap.md
Outdated
@@ -48,6 +48,28 @@ If you press a tapped hold-tap again within `quick-tap-ms` milliseconds, it will | |||
|
|||
In QMK, unlike ZMK, this functionality is enabled by default, and you turn it off using `TAPPING_FORCE_HOLD`. | |||
|
|||
#### `global-quick-tap` | |||
|
|||
If global quick tap is enabled, then the quick tap will apply not only when the given hold tap is tapped, but for any key tap before it. This effectively disables the hold tap when typing quickly, which can be quite useful for home row mods. It can also have the effect of removing the input delay when typing quickly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If global quick tap is enabled, then the quick tap will apply not only when the given hold tap is tapped, but for any key tap before it. This effectively disables the hold tap when typing quickly, which can be quite useful for home row mods. It can also have the effect of removing the input delay when typing quickly. | |
If global quick tap is enabled, then `quick-tap-ms` will apply not only when the given hold-tap is tapped but for any key tap before it. This effectively disables the hold behavior when typing quickly, which can be quite useful for home row mods. It can also have the effect of removing the input delay when typing quickly. |
docs/docs/behaviors/hold-tap.md
Outdated
|
||
If global quick tap is enabled, then the quick tap will apply not only when the given hold tap is tapped, but for any key tap before it. This effectively disables the hold tap when typing quickly, which can be quite useful for home row mods. It can also have the effect of removing the input delay when typing quickly. | ||
|
||
For example, the following hold tap configuration enables global quick tap with a 125 millisecond term. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, the following hold tap configuration enables global quick tap with a 125 millisecond term. | |
For example, the following hold-tap configuration enables global quick tap with a 125 millisecond term. |
docs/docs/behaviors/hold-tap.md
Outdated
}; | ||
``` | ||
|
||
If you press `&kp A`, and then `&gqt LEFT_SHIFT B` **within** 125 ms after, then `ab` will be output. Importantly, the `b` will be output immediately since it was within the `quick-tap-ms`. This behavior will hold for any key that is output to the computer. The `&gqt LEFT_SHIFT B` binding will only have its underlying hold-tap behavior if it is pressed 125 ms **after** any keycode is sent to the computer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you press `&kp A`, and then `&gqt LEFT_SHIFT B` **within** 125 ms after, then `ab` will be output. Importantly, the `b` will be output immediately since it was within the `quick-tap-ms`. This behavior will hold for any key that is output to the computer. The `&gqt LEFT_SHIFT B` binding will only have its underlying hold-tap behavior if it is pressed 125 ms **after** any keycode is sent to the computer. | |
If you press `&kp A` and then `&gqt LEFT_SHIFT B` **within** 125 ms, then `ab` will be output. Importantly, `b` will be output immediately since it was within the `quick-tap-ms`. This behavior will hold for any key that is output to the computer. The `&gqt LEFT_SHIFT B` binding will only have its underlying hold-tap behavior if it is pressed 125 ms **after** any keycode is sent to the computer. |
This behavior will hold for any key that is output to the computer
It's not very clear to me what this means. Does it mean it only applies to kp
behaviors?
The
&gqt LEFT_SHIFT B
binding will only have its underlying hold-tap behavior if it is pressed 125 ms after any keycode is sent to the computer
I think involving "sent to the computer" might be confusing here, since AFAICT the timing logic is not related to sending HID events. Maybe we can just say "after any key is pressed" (or released? I am not sure which one when we say "tapped.")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the implementation hooks into the keycode state changed event, which is reflective of what actual HID codes are being sent. But I agree that it's likely not the best way to describe the behavior. I've switched to what I hope is a more clear description.
docs/docs/behaviors/hold-tap.md
Outdated
|
||
If you press `&kp A`, and then `&gqt LEFT_SHIFT B` **within** 125 ms after, then `ab` will be output. Importantly, the `b` will be output immediately since it was within the `quick-tap-ms`. This behavior will hold for any key that is output to the computer. The `&gqt LEFT_SHIFT B` binding will only have its underlying hold-tap behavior if it is pressed 125 ms **after** any keycode is sent to the computer. | ||
|
||
Note that the higher the `quick-tap-ms`, the harder it will be to use the hold behavior, making this unideal for capitalizing letter while typing normally. However, paired with [mod-tap](mod-tap.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be missing a part at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, I forgot to save the file, I actually had a different blurb that was supposed to be there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs look great to me, thanks!
I believe @okke-formsma and @petejohanson approve of these changes based on their comments above and in Discord, but it would be good to have at least one explicit approval of the code on this PR before merging – just to be sure!
Adding an additional option to hold taps to make the quick tap functionality apply to all taps, rather than just the given hold tap. The current implementation uses the key state listener and is perfectly functional. However, I would've liked to have been able to identify any key being tapped (even if it's internal) as well as ignore any time a key was tapped in conjunction with a hold. I looked into this but it seemed more complicated then it was worth since this current implementation covers almost all cases while being much simpler.
So far I've been daily driving this implementation for almost a week and loving it! The fact that it gets rid of almost all input delay when typing is an added bonus I hadn't even considered.