Change streak time to be based on tap-hold key

[gazpachoking]

Based on my discussion in https://github.com/getreuer/qmk-keymap/discussions/59

This PR fixes a few issues with typing streak logic:

• The achordion_streak_timeout function was meant to be passed the tap_hold_keycode. Instead it was being passed the key pressed before the tap-hold key. This made it impossible to use it to exempt a shift mod-tap key from the streak logic. This PR fixes that issue so that shift (or any other tap-hold key) can have different (or no) streak behavior now.
• It changes to use the time of the key event rather than timer_read to do the timings. I think this makes things more accurate, as we don't process the key until after the initial tap-hold time has elapsed, so you needed much longer streak timeouts to be effective before. I realized that the longer needed timeout times were because the original code was timing between the key before the tap-hold, and the key after the tap-hold (so a 2 key span.) I only realized this after I decided to (re)implement that behavior myself in this branch.
• If there were rolls on mod-tap keys during a streak (that evaluated to holds), previously only the first one would be converted to a tap by the streak logic. Now all of them will be converted to taps (if the subsequent ones are still within the streak timeout from the last.)
• Replace the achordion_streak_timeout function with achordion_streak_chord_timeout which is passed both the mod-tap key, and the key after (similar to achordion_chord.) This will allow exemption of combo keys outside of the normal letter area, e.g. alt-enter from the streak behavior.
• Adds a separate function achordion_streak_continue to define which keys start (or continue) the typing streak logic. By default locks this to the main letter keys, punctuation and space with no modifiers held. This way streak logic doesn't intervene when you are moving around a document with navigation keys in combination with something like copying, pasting, or using emacs.
• Makes sure quick hold sequences don't count towards streaks. e.g. the first keyup of the 'v' key in this combo would previously count toward a streak, and cause the second paste to not work: ctrl-v,ctrl-v Rather than hard code this, I added logic to the default implementation of achordion_streak_continue to not count any keys pressed while a non-shift modifier is held toward a streak.

Notes/issues with this implementation:

• Since the streak timeout callback is only called after the delay, there is a hard coded 800ms cap on streak timeout right now. That could be lengthened or changed to be user configurable if needed.
• If multiple mod-tap keys are evaluated as holds in a row (within the streak timeout limits) all of the keydown events will be converted to tap events. However (I think) only the last keyup event will be converted to a tap event. I haven't experienced any issues with this in practice so far, but I don't know if there might be any hidden issues. Any ideas for a proper solution would be welcomed.
• I have some repeated code where it accepts the next tap-hold key as unsettled during a streak. I'm sure there is a way to clean that up, but I didn't find it yet.

I'm also wondering if it would be useful to have the key before the tap-hold key when making the decision as well. Perhaps exempting something like arrow keys or backspace would make sense, to allow e.g. right, right, ctrl-v or backspace, ctrl-v.

This alters the behavior of how streaks work. Instead of each key being able to define a timeout needed after that key was pressed, each tap hold key can define the timeout needed before it was pressed. The main goal is to be able to shorten the streak timeout for shift, while keeping it long for other mod tap keys.

I think there are some small bugs with this though, perhaps to do with my misunderstanding of how timers work. Occasionally keys get resolved as taps due to being in a streak, even when no keys have been pressed recently. Maybe because the timer resets itself?

Mostly just looking for feedback on this alternative method, maybe some help fixing the small bugs left in this implementation.

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[gazpachoking]

I think there was timer rollover messing things up, so I added a maximum streak timeout that resets even if no keys were pressed.

I also made is so that if you press and hold a mod-tap during the streak time, and then press another mod tap while that's still held down (within the streak time) the second (and more) keys are still considered taps. I believe it's slightly wrong, because the keyups for all but the last mod-tap key in the simultaneously held keys will all be fore a hold rather than a tap, but it doesn't seem to have any bad effects in practice.

[gazpachoking]

It turns out a variable timeout may be overkill. I tried with a very short timeout for shift, but even that was too long. Seems like my only requirements are a streak timeout on all the mod tap keys except shift. I was thinking about changing it so that the timeout callback got both the previous key, and mod tap key for even more configurability, but I'm not sure that's necessary either.

[gazpachoking]

Crap. Now I'm wanting to make exemptions based on the key pushed after the modifier key. Like allow alt-enter right after typing a web address.

[gazpachoking]

Well, I figured out my issues with using timer_expired at least. Somehow timer_expired(timer_read(), someval + 100) is different than timer_expired(timer_read(), (someval + 100)) No idea why the extra parens are required.

[gazpachoking]

Okay, I feel like I actually understand what I'm doing now. Reverted logic to closer to the original state, but with fixed bugs. Updating pr description with the new state.

[philong]

I have no understanding how typing streak works, but just wanted to comment on this:

Well, I figured out my issues with using timer_expired at least. Somehow timer_expired(timer_read(), someval + 100) is different than timer_expired(timer_read(), (someval + 100)) No idea why the extra parens are required.

Looks like timer_expired is actually a macro. Because of how macro expansion works, I think it should be fixed upstream by adding parentheses.

https://github.com/qmk/qmk_firmware/blob/0.24.4/platforms/timer.h#L47

[gazpachoking]

Looks like timer_expired is actually a macro. Because of how macro expansion works, I think it should be fixed upstream by adding parentheses.

Ahh, I understand now. Thanks for the explanation. This was the source of much frustration before I figured out I needed those parens.

[gazpachoking]

I've been running a new version which passes both tap_hold_keycode and the next pressed key which is working well. I think I have a good idea to store the previous streak key (and not just the event time of the previous streak key) which would allow exempting certain keys from having a timeout after, and should also allow for having matching keyups in more situations where multiple tap-hold keys got held down at once.

[gazpachoking]

I went ahead and added the bit where it passes the key after the tap-hold key to the timeout function. I'm still unclear if I want/need the key before when making the decision. I'll keep using it and see if there's cases that come up.

[gazpachoking]

Here's how I use this right now, for reference https://github.com/gazpachoking/qmk-keymap/blob/70d0ec8a9d994471d5c0583defce7ca4c5f113c1/keymap.c#L210-L225

[getreuer]

@gazpachoking thank you so much for these contributions! I agree on the whole with all the changes you describe. Really appreciated.

The achordion_streak_timeout function was meant to be passed the tap_hold_keycode. Instead it was being passed the key pressed after the tap-hold key. This made it impossible to use it to exempt a shift mod-tap key from the streak logic. This PR fixes that issue so that shift (or any other tap-hold key) can have different (or no) streak behavior now.

Aha, you are right that the achordion_streak_timeout() callback is called with keycodes other than tap-hold keys, despite the arg name. In a fast typing sequence of "non-tap-hold release, tap-hold key press, non-tap-hold-key press," it looks like the actual behavior is:

1. A non-tap-hold key is released. This resets the streak timer, passing that non-tap-hold key to the callback: https://github.com/getreuer/qmk-keymap/blob/62eff1ced0177630faef0c93a2b23404d23f9a19/features/achordion.c#L138
2. A tap-hold key is pressed, and Achordion goes to unsettled state. The streak timer is not reset.
3. A non-tap-hold key is pressed. Supposing these events happened quickly enough, the streak timer hasn't expired yet, so is_streak is true: https://github.com/getreuer/qmk-keymap/blob/62eff1ced0177630faef0c93a2b23404d23f9a19/features/achordion.c#L176
4. The tap-hold key is settled as a tap.

Do you agree with that description of the current implementation?

If I understand correctly, the revised behavior that this PR is intending is

• The time difference being measured is between (2) and (3) [rather than (1) and (3)].
• achordion_streak_timeout() is called on the tap-hold keycode.

Did I get that right?

It changes to use the time of the key event rather than timer_read to do the timings.

That's a sensible change. For a tap-hold key these times can differ by up to the tapping term: record->event.time is when the key was first pressed vs. timer_read() is the time when Achordion receives it, after QMK core has already settled it as held. The former seems more meaningful and easier to understand.

If there were rolls on mod-tap keys during a streak (that evaluated to holds), previously only the first one would be converted to a tap by the streak logic. Now all of them will be converted to taps (if the subsequent ones are still within the streak timeout from the last.)

Wonderful!

Change the achordion_streak_timeout function so that it's passed both the mod-tap key, and the key after (similar to achordion_chord.) This will allow exemption of combo keys outside of the normal letter area, e.g. alt-enter from the streak behavior.

I agree with this in principle. Customizability like this puts the "chord" in "Achordion"! One consideration is whether the API can be revised in a backwards compatible way to keep existing users building. I'll comment more specifically on the code.

Makes sure quick hold sequences don't count towards streaks. e.g. the first keyup of the 'v' key in this combo would previously count toward a streak, and cause the second paste to not work: ctrl-v,ctrl-v

Is this so that fast back-to-back hotkey chords are considered holds? That seems sensible, like a "permissive hold" like exception to the streak logic.

Notes/issues with this implementation:

Since the streak timeout callback is only called after the delay, there is a hard coded 500ms cap on streak timeout right now. That could be lengthened or changed to be user configurable if needed.

That's fair. For comparison, QMK core has a hardcoded cap of 1000 ms on the tapping term. Some kind of cap is needed because 16-bit timers wrap pretty quickly (once every ~65 seconds). Plus, imposing a reasonable cap is a mitigation to ensure the keyboard usable even when the user messed up their configuration.

If multiple mod-tap keys are evaluated as holds in a row (within the streak timeout limits) all of the keydown events will be converted to tap events. However (I think) only the last keyup event will be converted to a tap event. I haven't experienced any issues with this in practice so far, but I don't know if there might be any hidden issues. Any ideas for a proper solution would be welcomed.

Thanks for the heads up. Are you observing stuck keys at all?

I'm also wondering if it would be useful to have the key before the tap-hold key when making the decision as well. Perhaps exempting something like arrow keys or backspace would make sense, to allow e.g. right, right, ctrl-v or backspace, ctrl-v.

Maybe. But streaks are complicate as is! I'd rather get the streak implementation fixed first.

[gazpachoking]

If I understand correctly, the revised behavior that this PR is intending is

* The time difference being measured is between (2) and (3) [rather than (1) and (3)].

* `achordion_streak_timeout()` is called on the tap-hold keycode.

Did I get that right?

Your understanding of the original is correct. I first changed it to as you described, timing between (2) and (3), but then decided it would be better to go between (1) and (3). (I only realized that's what the original code was doing after I decided to switch it to that may myself.) I think this behavior makes sense. The idea is that we shouldn't settle a tap-hold key as a hold when it was quickly sandwiched between two regular tap keys.

Is this so that fast back-to-back hotkey chords are considered holds? That seems sensible, like a "permissive hold" like exception to the streak logic.

Yep. I don't consider either the ctrl or the v in a ctrl-v combo to be a tap for the purposes of streak, so I don't think either one of them should start the streak timer.

Thanks for the heads up. Are you observing stuck keys at all?

I haven't observed any stuck keys or other ill effects, but I can't guarantee some programs wouldn't have problems. I'm also not 100% sure it even has mismatched keyups, but my understanding of the code is that it's theoretically possible.

Maybe. But streaks are complicate as is! I'd rather get the streak implementation fixed first.

Yep, agreed. I haven't had too many instance where I've wanted it, so haven't even tried to implement yet. The only case I've sorta noticed is when I try to quickly backspace then paste something.

[gazpachoking]

I'm also wondering if it would be useful to have the key before the tap-hold key when making the decision as well. Perhaps exempting something like arrow keys or backspace would make sense, to allow e.g. right, right, ctrl-v or backspace, ctrl-v.

Maybe. But streaks are complicate as is! I'd rather get the streak implementation fixed first.

I ended up doing this anyway because I was hard coding some behavior in there that I'm not sure should be (like any keys typed while a tap-hold or layer-hold was held down don't count towards a streak.) It's called achordion_streak_continue (not sure if that's a great name) and it just returns a bool whether the pressed key should allow the streak timer to start. The default implementation only starts the streak timer for letters, common punctuation and space, and disallows those if they are pressed while a non-shift mod key is held. I think this one is actually mostly the same as the old achordion_streak_timeout function, except that you can't define the actual timeout time anymore, just whether a key continues the streak.

[gazpachoking]

I'm also thinking the default streak time should be increased, 100 ms is very short since it has to encompass both the mod-tap key press as well as the next key press. Maybe 200 ms or so. I'm using 230 right now in my personal config.

[getreuer]

Thank you for the continued refinement on this PR! This looks about good to go.

Your understanding of the original is correct. I first changed it to as you described, timing between (2) and (3), but then decided it would be better to go between (1) and (3). (I only realized that's what the original code was doing after I decided to switch it to that may myself.) I think this behavior makes sense. The idea is that we shouldn't settle a tap-hold key as a hold when it was quickly sandwiched between two regular tap keys.

Fantastic: "we shouldn't settle a tap-hold key as a hold when it was quickly sandwiched between two regular tap keys" is a great way to put it.

I'm also thinking the default streak time should be increased, 100 ms is very short since it has to encompass both the mod-tap key press as well as the next key press. Maybe 200 ms or so. I'm using 230 right now in my personal config.

That is very reasonable. Yes, please update the default timeout to 200.

[gazpachoking]

Thanks for the continued feedback! I'll still get to this, but have a newborn in the house now so waiting to get a bit of time.

[getreuer]

Congratulations, that's fantastic! Thanks for the message. Please, there's no rush to act on this PR, understood that you are fully occupied in these first few weeks especially. I wish you and your family much happiness!

[gazpachoking]

Okay, got a little time for this, I think I addressed all your comments. It's still working good on my setup too.

[getreuer]

This is good to go, thank you for your contribution! Wishing you best of luck and happy times with the baby.