Add HOLD_ON_OTHER_KEY_PRESS option for dual-role keys

[sigprof]

Description

Implement an additional HOLD_ON_OTHER_KEY_PRESS option for dual-role keys which converts the dual-role key press into a hold action immediately when another key is pressed (this is different from the existing PERMISSIVE_HOLD option, which selects the hold action when another key is tapped (pressed and then released) while the dual-role key is pressed). The Mod-Tap keys already behave in a similar way, unless the IGNORE_MOD_TAP_INTERRUPT option is enabled (but with some additional delays); the added option makes this behavior available for all other kinds of dual-role keys.

Types of Changes

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

Issues Fixed or Closed by This PR

• [Feature Request] Permissive Hold and Ignore Mod Tap Interrupt for LT #8971

Checklist

• My code follows the code style of this project.
• 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]

In this configuration a rolling press (LT down, / down, LT up, / up) for some reason gives the slash character, as if the layer switch was not performed.

After some more testing I found that the same thing happens even without LT and without any of changes from this PR — OSM(MOD_RCTL) on a layer just does not work properly even when using a simple MO(_FN) if the layer switch key is released before the OSM key. Looks like the underlying problem is that OSM is also a dual-role key, and the process_tapping() code does not handle the case when the dual-role keycode is on a non-default layer, and the layer state changes between the press and release of the dual-role key (it remembers only the matrix position during the press, and actually tries to map it to the keycode during release, and with a rolling press the proper layer is no longer active at that time).

So the code from this PR does not actually cause that bug, and I could mark it as ready for review.

[drashna]

So, basically .... this would be Interrupt Layer Tap, as opposed to Interrupt Mod Tap?

In this configuration a rolling press (LT down, / down, LT up, / up) for some reason gives the slash character, as if the layer switch was not performed.

Yeah, this is normal, and is what Tapping Force Hold exists for.

[sigprof]

So, basically .... this would be Interrupt Layer Tap, as opposed to Interrupt Mod Tap?

Yes, but not just for Layer Tap (although the only other thing which seems to be similar to Mod Tap and Layer Tap is SH_T()). This can also provide a slight benefit to Mod Tap (even though the final decision would be the same as with the default “Interrupt Mod Tap” code without the option, with HOLD_ON_OTHER_KEY_PRESS the second keypress event will be sent to the host without any additional delay).

Yeah, this is normal, and is what Tapping Force Hold exists for.

I'm not sure how Tapping Force Hold would help here — it affects the case when the same key is pressed multiple times, and in that case (OSM() on a layer behind MO() or LT()) both keys are pressed for a single time; the non-obvious thing here is that OSM() also uses the same action_tapping.c code, and that code does not capture the layer state at the moment of keypress. Anyway, that problem does not have anything with this PR, because it was also happening without these changes, and the configuration that I attempted to use is probably too weird anyway.

[drashna]

Also, this should probably target the develop branch.

[stale]

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 awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[sigprof]

Also, this should probably target the develop branch.

Probably it should, but I don't see where in the GitHub interface I can change the requested target branch; maybe only someone with more privileges can do it?

[fauxpark]

[takagi]

I've tried this on my fork. It works completely as I expect and makes me so happy. Thanks!

[precondition]

This is a great addition that brings layer-taps customization on par with mod-taps but I can see how it can confuse a lot of users.
If we compare mod-tap behaviour with layer-tap behaviour, we get this:

• Default mod-tap behaviour = HOLD_ON_OTHER_KEY_PRESS for layer-taps
• IGNORE_MOD_TAP_INTERRUPT for mod-taps = default layer-tap behaviour

(Where "default" refers to no particular tap hold settings enabled for this type of dual-role key)

Ideally, we would make HOLD_ON_OTHER_KEY_PRESS the default behaviour for layer-taps to be consistent with that of mod-taps and instead add a new option titled IGNORE_LAYER_TAP_INTERRUPT which would act in a similar manner to IGNORE_MOD_TAP_INTERRUPT.

EDIT: After mulling it over, I've come to the conclusion that doing it the other way around is better i.e. make IGNORE_LAYER_TAP_INTERRUPT and IGNORE_MOD_TAP_INTERRUPT the new default. See my next comment to this thread just below.

It would be much more consistent and easier to reason about. Tap hold configuration settings are hard enough to understand as is.

The downside to my suggestion is that people who are used to a IGNORE_LAYER_TAP_INTERRUPT style for their layer-taps (the current default) would find their layer-tap to behave a little different from usual when updating their fork/firmware. In which case, we can add a note about it in the Breaking Changes log.

(Judging from the QMK-related discussions I've had about layer-taps, I feel like HOLD_ON_OTHER_KEY_PRESS is what most people expect the default behaviour to be. It is indeed much closer to how a normal, pure modifier key works)
(PS: My impression is probably biased by availability bias though. The majority(?) of people using layer-tap without any problems on keys as critical as space obviously don't bring up the topic of layer-tap behaviour, only those who are not satisfied with it bring up the topic)

[drashna]

I think that @precondition makes a very good point here.

Also, it looks like you have some merge conflicts here.

[serebrov]

I am trying to figure out how tap-hold configuration works and I ended up at #8971 and here.
Not really sure if the below makes sense (maybe I am missing something as I have no experience working with QMK code), but I hope this "external" point of view might be useful.

Short summary of my post is: current options are already quite confusing and adding more would make the situation even worse. I think it is better to introduce new, clearer and more consistent, options to control LT and MT behavior instead.

First, some observations, looking at the summary table with MT/LT behavior (made by @sigprof in #8971):

• "Single tap less than tapping term" does not depend on settings, always Tap
• "Nested press of another key (Caps Lock held longer than the tapping term)" does not depend on settings, always Hold
• The "Rolling press of another key (everything during the tapping term)" and "Rolling press of another key (both keys held longer than the tapping term)" are same in all cases
• The table does not study the effect of IGNORE_MOD_TAP_INTERRUPT on LT (I assume it has no effect on LT and only works for MT)

Taking these into account, we can reduce the states to these (a copy of the original table, see the "Summary (short)" sheet):

• MT
  • Always Hold -> Default = PERMISSIVE_HOLD = HOLD_ON_OTHER_KEY_PRESS = HOLD_ON_OTHER_KEY_PRESS + IGNORE_MOD_TAP_INTERRUPT
  • Always Tap -> IGNORE_MOD_TAP_INTERRUPT
  • Nested=Hold, Rolling=Tap -> PERMISSIVE_HOLD + IGNORE_MOD_TAP_INTERRUPT
• LT
  • Always Hold -> HOLD_ON_OTHER_KEY_PRESS (new option, not supported currently)
  • Always Tap -> Default
  • Nested=Hold, Rolling=Tap -> PERMISSIVE_HOLD

Side note: according to the data in the table, PERMISSIVE_HOLD alone has no effect on MT, so either the documentation is incorrect or current QMK behavior is wrong.
The documentation says that PERMISSIVE_HOLD "makes tap and hold keys (like Mod Tap) work better for fast typists, or for high TAPPING_TERM settings.". While, according to the table, PERMISSIVE_HOLD corresponds to the default MT behavior.

Summarizing the information:

• 3 behaviors are possible:
  • Always Hold - Mod key is always used when we press another key at the same time
  • Always Tap - Tap key is used when we press another key at the same time (unless we hold them longer than tapping term)
  • Nested=Hold, Rolling=Tap - Mod key is used if another key is nested into the Mod keypress; Tap key is used for rolling keypress (unless we hold them longer than tapping term)
• Currently all behaviors are supported by MT, but not by LT (LT has no Always Hold state)
• The default behavior for MT and LT is different
• Options work differently for MT and LT (PERMISSIVE_HOLD is Always Hold for MT, but it is Nested=Hold, Rolling=Tap for LT)

Now, the suggestion by @precondition above, if I understand it right, would look like this:

• MT
  • Always Hold -> Default = PERMISSIVE_HOLD (same as now)
  • Always Tap -> IGNORE_MOD_TAP_INTERRUPT (same as now)
  • Nested=Hold, Rolling=Tap -> PERMISSIVE_HOLD + IGNORE_MOD_TAP_INTERRUPT (same as now)
• LT
  • Always Hold -> Default = HOLD_ON_OTHER_KEY_PRESS (breaking change, new default, not supported currently)
  • Always Tap -> IGNORE_LAYER_TAP_INTERRUPT (new option, current LT default)
  • Nested=Hold, Rolling=Tap -> HOLD_ON_OTHER_KEY_PRESS + IGNORE_LAYER_TAP_INTERRUPT (another breaking change, for consistency with MT, current option for this is PERMISSIVE_HOLD)

Putting breaking changes aside, I think it will still be quite confusing due to the amount of options and inconsistent naming: here we still have to play with combinations of options, plus we now have PERMISSIVE_HOLD for MT and HOLD_ON_OTHER_KEY_PRESS for LT.

I think, ideally, the behavior would be set by choosing one of the possible options instead of setting flags.
For example #define MT_BEHAVIOR_HOLD instead of #define PERMISSIVE_HOLD, other options could be MT_BEHAVIOR_TAP and MT_BEHAVIOR_NESTED_HOLD_ROLLING_TAP (it is good to make sure that only one option is set at the same time, maybe use some other language construct here instead of define, not sure).

Similarly, there could be LT_BEHAVIOR_HOLD, LT_BEHAVIOR_TAP, LT_BEHAVIOR_NESTED_HOLD_ROLLING_TAP.

I do not know if having different settings for MT and LT is good or not. Personally, I'd go with the same setting for both MT and LT as logically, I see no difference, both enable another logical layer. So it might be fine to just have one option to choose the behavior for both LT and MT.

Not sure if this makes sense, but I would suggest introducing new-style options here and deprecating both PERMISSIVE_HOLD and IGNORE_MOD_TAP_INTERRUPT.
If new options are not set, old one can be used as a backward-compatibility fallback.

[precondition]

@serebrov

Taking these into account, we can reduce the states to these (a copy of the original table, see the "Summary (short)" sheet):

* MT
  
  * `Always Hold` -> Default = PERMISSIVE_HOLD = HOLD_ON_OTHER_KEY_PRESS = HOLD_ON_OTHER_KEY_PRESS + IGNORE_MOD_TAP_INTERRUPT
  * `Always Tap` -> IGNORE_MOD_TAP_INTERRUPT
  * `Nested=Hold, Rolling=Tap` -> PERMISSIVE_HOLD + IGNORE_MOD_TAP_INTERRUPT

* LT
  
  * `Always Hold` -> HOLD_ON_OTHER_KEY_PRESS (new option, not supported currently)
  * `Always Tap` -> Default
  * `Nested=Hold, Rolling=Tap` -> PERMISSIVE_HOLD

It's not a good idea to use the word "always" here. "Always Hold" would no longer be a dual-role key, it would just be the equivalent of MO(layer) or KC_mod. Same goes for "Always Tap", an always tap key would be the most basic of keys like KC_A. If we wanted always hold or tap, we wouldn't use dual-role keys.

Side note: according to the data in the table, PERMISSIVE_HOLD alone has no effect on MT, so either the documentation is incorrect or current QMK behavior is wrong.
The documentation says that PERMISSIVE_HOLD "makes tap and hold keys (like Mod Tap) work better for fast typists, or for high TAPPING_TERM settings.". While, according to the table, PERMISSIVE_HOLD corresponds to the default MT behavior.

It's normal that DefaultMT+PERMISSIVE_HOLD has no noticeable effect over just DefaultMT

Permissive hold is an option that adds another way to trigger the hold function of dual-role keys.

Whereas the modifier of the mod-tap is activated when another key gets pressed by default or gets activated when held down for longer than the tapping term if IGNORE_MOD_TAP_INTERRUPT is defined, permissive hold activates the modifier when another key is pressed and released while the mod-tap is held, regardless of the tapping term. This means that even when IGNORE_MOD_TAP_INTERRUPT is defined, this option allows the user to trigger a keyboard shortcut with a mod-tap key before the end of the tapping term.
(Copied extract from https://precondition.github.io/home-row-mods#permissive-hold)

The default HOLD_ON_OTHER_KEY_PRESS behaviour of mod-taps makes the Tap-Or-Hold decision immediately on the press of the other key whereas PERMISSIVE_HOLD has to wait until the release of the other key to decide. The decision is "short-circuited" in sense.

To be honest, phrases like "work better for fast typists" should be removed from the Tap Hold docs in my opinion. It's way too subjective and potentially misleading. It makes it seem like people who type under 100WPM have nothing to gain from enabling this feature.

Summarizing the information:

* 3 behaviors are possible:
  
  * `Always Hold` - Mod key is always used when we press another key at the same time
  * `Always Tap` - Tap key is used when we press another key at the same time (unless we hold them longer than tapping term)
  * `Nested=Hold, Rolling=Tap` - Mod key is used if another key is nested into the Mod keypress; Tap key is used for rolling keypress (unless we hold them longer than tapping term)

You're paraphrasing this section of the Tap Hold docs. Although as I've already explained before, the use of the word "always" is rather misleading in this context. I personally prefer the default–permissive hold–hold on other key press vocabulary that is used in the docs of this PR. It puts the Tap-or-Hold decision modes on a spectrum from least likely to trigger the hold function to most likely to trigger the hold function.

* Currently all behaviors are supported by MT, but not by LT (LT has no `Always Hold` state)

I confirm.

* The default behavior for MT and LT is different

Yes, and that's very problematic.

* Options work differently for MT and LT (PERMISSIVE_HOLD is `Always Hold` for MT, but it is `Nested=Hold, Rolling=Tap` for LT)

No, they work the same. The reason you think PERMISSIVE_HOLD is Always Hold for MT is simply an artefact of the fact that you never get to see PERMISSIVE_HOLD in action because of the "short-circuit" in the Tap-or-Hold decision by MT's default HOLD_ON_OTHER_KEY_PRESS behaviour.

Now, the suggestion by @precondition above, if I understand it right, would look like this:

* MT
  
  * `Always Hold` -> Default = PERMISSIVE_HOLD (same as now)
  * `Always Tap` -> IGNORE_MOD_TAP_INTERRUPT (same as now)
  * `Nested=Hold, Rolling=Tap` -> PERMISSIVE_HOLD + IGNORE_MOD_TAP_INTERRUPT (same as now)

* LT
  
  * `Always Hold` -> Default = HOLD_ON_OTHER_KEY_PRESS (breaking change, new default, not supported currently)
  * `Always Tap` -> IGNORE_LAYER_TAP_INTERRUPT (new option, current LT default)
  * `Nested=Hold, Rolling=Tap` -> HOLD_ON_OTHER_KEY_PRESS + IGNORE_LAYER_TAP_INTERRUPT (another breaking change, for consistency with MT, current option for this is PERMISSIVE_HOLD)

The way to do what you call Nested=Hold, Rolling=Tap for LT wouldn't be by doing HOLD_ON_OTHER_KEY_PRESS + IGNORE_LAYER_TAP_INTERRUPT because those two options are at odds with each other and the hold action will get selected as soon as an other key is pressed while holding down the dual-role key, thus making IGNORE_LAYER_TAP_INTERRUPT totally useless. The way to do it would be by the combination of IGNORE_LAYER_TAP_INTERRUPT + PERMISSIVE_HOLD.

But other than that, yes, that's what I was suggesting.

However, after re-reading "Tap-Or-Hold Decision Modes", I'm starting to think that @sigprof's decision of what should be the default behaviour is more sound. The default behaviour should be the one that chooses the tap function in most scenarios. That means making IGNORE_MOD_TAP_INTERRUPT the (new) default for MTs.

Making HOLD_ON_OTHER_KEY_PRESS the new default is very bothersome because as I've explained again and again in my comment here, this option short-circuits all other Tap-or-Hold decision modes so it's hard to build on it. You have to explicitly disable it to see the effects of additional options, thus making it a bad candidate for the new default behaviour.

Putting breaking changes aside, I think it will still be quite confusing due to the amount of options and inconsistent naming: here we still have to play with combinations of options, plus we now have PERMISSIVE_HOLD for MT and HOLD_ON_OTHER_KEY_PRESS for LT.

Yes, the tap hold options should be standardized and use the same naming schemes for LT and MT. Although I'm not sure what you mean by "plus we now have PERMISSIVE_HOLD for MT and HOLD_ON_OTHER_KEY_PRESS for LT". MT with permissive hold isn't the same thing as LT with hold_on_other_key_press.

I think, ideally, the behavior would be set by choosing one of the possible options instead of setting flags.
For example #define MT_BEHAVIOR_HOLD instead of #define PERMISSIVE_HOLD, other options could be MT_BEHAVIOR_TAP and MT_BEHAVIOR_NESTED_HOLD_ROLLING_TAP (it is good to make sure that only one option is set at the same time, maybe use some other language construct here instead of define, not sure).

Similarly, there could be LT_BEHAVIOR_HOLD, LT_BEHAVIOR_TAP, LT_BEHAVIOR_NESTED_HOLD_ROLLING_TAP.

I do not know if having different settings for MT and LT is good or not. Personally, I'd go with the same setting for both MT and LT as logically, I see no difference, both enable another logical layer. So it might be fine to just have one option to choose the behavior for both LT and MT.

I believe it is important to keep the ability to customize both. To give an example, I know some people who can't get used to PERMISSIVE_HOLD on their home row mods but came to rely on that permissive hold behaviour for their layer-tap (thumb) keys.

If we don't want duplicate Tap-or-Hold options for MT and for LT, we can just propose a per-key variant and let people define the dual role keys on which they want a particular option to apply on. That's exactly what the person whose message I've shared here does with #define PERMISSIVE_HOLD_PER_KEY in config.h and bool get_permissive_hold(uint16_t keycode, keyrecord_t *record) in keymap.c

Not sure if this makes sense, but I would suggest introducing new-style options here and deprecating both PERMISSIVE_HOLD and IGNORE_MOD_TAP_INTERRUPT.
If new options are not set, old one can be used as a backward-compatibility fallback.

We should leave PERMISSIVE_HOLD as is but make IGNORE_MOD_TAP_INTERRUPT the new default.

[serebrov]

@precondition thank you for the details, as well as for the home row mods guide (which, by the way, was a starting point for my research of these settings).

I agree on the terminology point, by "Always Hold" and "Always Tap" I only meant interactions that are happening during the tapping term, so there should be better naming for this.

Although my main point was that current settings are quite confusing and it would be good to step back, review the current state and see how to improve it.
It could be a breaking change with new settings replacing existing or a backward-compatible, where new settings work on top of old one. For example, LT_BEHAVIOR_NESTED_HOLD_ROLLING_TAP (the name could be better here too) can set HOLD_ON_OTHER_KEY_PRESS and IGNORE_LAYER_TAP_INTERRUPT under the hood.

I also just got an idea to look around and see how this is done in other firmwares and it turns out ZMK has it like this:

• The 'hold-preferred' flavor triggers the hold behavior when the tapping_term_ms has expired or another key is pressed.
• The 'balanced' flavor will trigger the hold behavior when the tapping_term_ms has expired or another key is pressed and released.
• The 'tap-preferred' flavor triggers the hold behavior when the tapping_term_ms has expired. It triggers the tap behavior when another key is pressed.

These flavors look very close to the three behaviors I we have discussed above and it is easier to have just one setting to choose the desired behavior (like flavor = "tap-preferred" in zmk).

Anyway, I think at this point it would be beneficial to merge this PR as it is: the situation is already complex, but at least there will be a way to configure the LT behavior. Improvements to make it clearer could be done later.

And it looks like HOLD_ON_OTHER_KEY_PRESS has already got a semi-official status, even though it is not merged yet: this setting is mentioned in your home row mods article as well as in ZMK docs ("The hold-preferred flavor works similar to the HOLD_ON_OTHER_KEY_PRESS setting in QMK").

[sigprof]

I rebased the branch and fixed conflicts in the documentation (the actual code is still unchanged and works).

The suggestion by @serebrov to change the hold-tap configuration to an enum instead of some boolean options which are not really independent looks nice, but removing unused code would not be as easy as it is with the existing boolean options (if you don't use PERMISSIVE_HOLD or HOLD_ON_OTHER_KEY_PRESS, the corresponding code is not compiled). Names used by ZMK also look nice (I don't really like the HOLD_ON_OTHER_KEY_PRESS name, it's too long).

BTW, another piece of software that supports dual-role keys is KMonad, and it has these options to configure the behavior of dual-role keys:

• tap-next — looks like HOLD_ON_OTHER_KEY_PRESS with infinite tapping term (which is not really supported by QMK — tapping term above 500 ms forces PERMISSIVE_HOLD, and RETRO_TAPPING executes the tap action as if the tapping term was infinite, but still allows the hold action to go through after the tapping term expires);
• tap-hold — looks like the default QMK behavior (with IGNORE_MOD_TAP_INTERRUPT for MT());
• tap-hold-next — looks like HOLD_ON_OTHER_KEY_PRESS;
• tap-next-release — looks like PERMISSIVE_HOLD with infinite tapping term (with IGNORE_MOD_TAP_INTERRUPT for MT());
• tap-hold-next-release — looks like PERMISSIVE_HOLD (with IGNORE_MOD_TAP_INTERRUPT for MT()).

I also really don't like the current non-IGNORE_MOD_TAP_INTERRUPT implementation (it should be working at the HOLD_ON_OTHER_KEY_PRESS level to avoid extra key delays). However, doing it that way would probably mean having HOLD_ON_OTHER_KEY_PRESS_PER_KEY effectively always on, so that it could enable that behavior for Mod Tap keys.

[precondition]

I also really don't like the current non-IGNORE_MOD_TAP_INTERRUPT implementation (it should be working at the HOLD_ON_OTHER_KEY_PRESS level to avoid extra key delays). However, doing it that way would probably mean having HOLD_ON_OTHER_KEY_PRESS_PER_KEY effectively always on, so that it could enable that behavior for Mod Tap keys.

@sigprof If we don't want misleading docs, the default behaviour of modtaps will need to also be the "mode [that] selects the hold action only if the dual-role key is held down longer than the tapping term". Given that, we can make IGNORE_MOD_TAP_INTERRUPT the new default and let HOLD_ON_OTHER_KEY_PRESS replace the existing hold-preferred behaviour of modtaps. That way, we can also avoid having HOLD_ON_OTHER_KEY_PRESS_PER_KEY effectively always on.

[Grazfather]

Just a note for people coming from Karabiner/google: This would support to_if_alone-like behaviour.

[sigprof]

Rebased onto the current develop branch, because the code needed a minor change for compatibility with #8591 (get_event_keycode() needed to be replaced by get_record_keycode() to handle the case when the dual-role keycode was actually emitted by a combo). Verified the functionality again, including the combo case.

[rflcrz]

@sigprof , thanks for your work!

What about SH_T()? Can we use this new feature to modify Swap Hands behaviour?

As far as I could test, the answer is "no". Am I missing something?