Remove IGNORE_MOD_TAP_INTERRUPT_PER_KEY in favour of HOLD_ON_OTHER_KEY_PRESS_PER_KEY

[precondition]

Description

Deprecate IGNORE_MOD_TAP_INTERRUPT_PER_KEY as a stepping stone towards making IGNORE_MOD_TAP_INTERRUPT the new default behaviour for modtaps.

Old description from the time the PR was about setting IGNORE_MOD_TAP_INTERRUPT as the new default behaviour

TL;DR This PR removes `IGNORE_MOD_TAP_INTERRUPT` and makes it the default behaviour for mod-taps.

If we compare mod-tap behaviour with layer-tap behaviour, we get this:

• Old 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 and "old" refers to any QMK version without this PR)

To make this easier, let's call the current/old default MT behaviour as HOLD_ON_INTERRUPT since it chooses the hold action as soon as an interrupting key is pressed. @sigprof 's HOLD_ON_OTHER_KEY_PRESS works exactly the same way as HOLD_ON_INTERRUPT except that HOLD_ON_OTHER_KEY_PRESS has shorter key delays, i.e. it takes the tap-or-hold decision slightly faster and sends the keyboard report to the host earlier.

Ideally, the default behaviour for layer-taps should be consistent with that of mod-taps. The best candidate for "default" behaviour is an option that takes the longest to make the tap-or-hold decision.

It is a bad idea to keep a HOLD_ON_INTERRUPT-like behaviour as default for modtaps (like it currently is) because this is the option that takes the shortest to make the tap-or-hold decision. This means that enabling additional options like PERMISSIVE_HOLD is unnoticeable because HOLD_ON_INTERRUPT short-circuits the decision. Users have to explicitly disable this hold-on-interrupt by defining IGNORE_MOD_TAP_INTERRUPT in order for extra tap-hold settings to actually take effect.

Consider the following chain of event, highlighting how HOLD_ON_INTERRUPT/HOLD_ON_OTHER_KEY_PRESS short-circuits the tap-or-hold decision (mt is released before the tapping term expired):

• mt down
• other down ; HOLD_ON_OTHER_KEY_PRESS triggers
• other up ; PERMISSIVE_HOLD triggers
• mt up ; IGNORE_INTERRUPT triggers

Furthermore, the doc page on tap hold settings clearly states that "IGNORE_INTERRUPT" is the default behaviour for dual-role keys:

Tap-Or-Hold Decision Modes

The code which decides between the tap and hold actions of dual-role keys supports three different modes, in increasing order of preference for the hold action:

1. The default mode selects the hold action only if the dual-role key is held down longer than the tapping term. In this mode pressing other keys while the dual-role key is held down does not influence the tap-or-hold decision.

[...]
The default mode gives the most delay (if the dual-role key is held down, this mode always waits for the whole tapping term), and the other modes may give less delay when other keys are pressed, because the hold action may be selected earlier.

This PR brings the code in alignment to what the documentation says.

These changes also simplified a great deal the tap hold documentation page that most people find very hard to understand.

Finally, this does not break any existing build (#define IGNORE_MOD_TAP_INTERRUPT[_PER_KEY] and get_ignore_mod_tap_interrupt() merely become a no-op, with no compiler error) and despite the diff looking mostly red, no loss of functionality occurs.

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

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).

[IsaacElenbaas]

With this PR and HOLD_ON_OTHER_KEYPRESS, rolling from mod taps under TAPPING_TERM produces the two tap actions unlike the old behavior when not using IGNORE_MOD_TAP_INTERRUPT. I assume this is an unintentional change, but if not it is an issue for Retro Shift. As referencing hold times makes little sense when they also affect whether things are shifted, it would need a workaround.

[IsaacElenbaas]

Nice job on the edge cases - you caught a lot of things I wouldn't have thought of.

[precondition]

With this PR and HOLD_ON_OTHER_KEYPRESS, rolling from mod taps under TAPPING_TERM produces the two tap actions unlike the old behavior when not using IGNORE_MOD_TAP_INTERRUPT. I assume this is an unintentional change, but if not it is an issue for Retro Shift. As referencing hold times makes little sense when they also affect whether things are shifted, it would need a workaround.

@IsaacElenbaas HOLD_ON_OTHER_KEYPRESS doesn't exist, the correct name is HOLD_ON_OTHER_KEY_PRESS (notice the underscore between KEY and PRESS). If your config.h contains #define HOLD_ON_OTHER_KEYPRESS and you still observe the default "ignore interruptions" behaviour, that's normal.

The expected behaviour is described in the new tables I've added to the tap-hold docs.

Here's how rolling key presses should behave in theory (This is taken from the new docs so "Default" means the new default):

Time	Physical key event	Default	PERMISSIVE_HOLD	HOLD_ON_OTHER_KEY_PRESS
0	LSFT_T(KC_A) down
110	KC_B down			B
130	LSFT_T(KC_A) up	ab	ab	B
140	KC_B up	ab	ab	B

Time	Physical key event	Default	PERMISSIVE_HOLD	HOLD_ON_OTHER_KEY_PRESS
0	LSFT_T(KC_A) down
110	KC_B down			B
200	LSFT_T(KC_A) held	B	B	B
205	LSFT_T(KC_A) up	B	B	B
210	KC_B up	B	B	B

I couldn't reproduce the bug with #define HOLD_ON_OTHER_KEY_PRESS so I have to assume that you simply misspelled the option when testing.

If you did in fact spell it correctly in config.h then I'd interested in hearing more about your tap hold setup.

[IsaacElenbaas]

I've done it before and did it again 🙁

Looks good with Retro Shift on my end.

[drashna]

I've done it before and did it again 🙁

Looks good with Retro Shift on my end.

If it makes you feel any better, I've done similar ... more than a few times.

[precondition]

I squashed and restructured my commits so that each commit only touches a specific folder. This should be easier to follow. The underlying code changes remain the same though.

[KarlK90]

Thanks @precondition for wadding through the action_tapping.c pasta and implementing this feature. The changes look good to me although I have yet to try them out on a board, but you have my approval on this PR. During review I found these issues, let me know if you disagree:

Unit-tests:

• Please move the ignore_mod_tap_interrupt tests into default_mod_tap (maybe rename to default tapping?) behaviour tests, as they are the new default behaviour.
• Altough you didn't implement the HOLD_ON_OTHER_KEY_PRESS feature I still would like unit-tests for this behaviour to be added as part of this PR, to increase the coverage and I think it is a good oppurtunity as you are familiar with the behavour. Would that be acceptable for you?

Backwards compatibility:

• The now de-funct IGNORE_MOD_TAP_INTERRUPT_PER_KEY and IGNORE_MOD_TAP_INTERRUPT defines should be removed from all keebs as they are dead code prefeerably in a seperate commit to make the review easier.

• To mimic the old behavior especially for people that prefer the old default, we need to rename get_ignore_mod_tap_interrupt to get_hold_on_other_key_press on keymap level plus inversion of logic by enabling HOLD_ON_OTHER_KEY_PRESS for these keymaps.

I found these keymaps to still use the now defunct get_ignore_mod_tap_interrupt override:

rg get_ignore_mod_tap_interrupt

keyboards/converter/usb_usb/keymaps/chriskopher/keymap.c
178:bool get_ignore_mod_tap_interrupt(uint16_t keycode, keyrecord_t *record) {

keyboards/handwired/dactyl_manuform/5x6_5/keymaps/cykedev/keymap.c
198:bool get_ignore_mod_tap_interrupt(uint16_t keycode, keyrecord_t *record) {

keyboards/bastardkb/scylla/keymaps/cykedev/keymap.c
172:bool get_ignore_mod_tap_interrupt(uint16_t keycode, keyrecord_t *record) {

keyboards/torn/keymaps/kinesish/keymap.c
140:bool get_ignore_mod_tap_interrupt(uint16_t keycode, keyrecord_t *record) {

keyboards/crkbd/keymaps/snowe/keymap.c
182:// bool get_ignore_mod_tap_interrupt(uint16_t keycode, keyrecord_t *record) {

keyboards/ergodox_ez/keymaps/stamm/keymap.c
209:bool get_ignore_mod_tap_interrupt(uint16_t keycode, keyrecord_t *record) {

keyboards/planck/keymaps/rootiest/keymap.c
1373:bool get_ignore_mod_tap_interrupt(uint16_t keycode, keyrecord_t* record) {

keyboards/planck/keymaps/adamtabrams/keymap.c
258:bool get_ignore_mod_tap_interrupt(uint16_t keycode, keyrecord_t *record) {

keyboards/lily58/keymaps/cykedev/keymap.c
99:bool get_ignore_mod_tap_interrupt(uint16_t keycode, keyrecord_t *record) {

users/drashna/keyrecords/tapping.c
39:__attribute__((weak)) bool get_ignore_mod_tap_interrupt(uint16_t keycode, keyrecord_t *record) {

[precondition]

Backwards compatibility:

[ ] The now de-funct IGNORE_MOD_TAP_INTERRUPT_PER_KEY and IGNORE_MOD_TAP_INTERRUPT defines should be removed from all keebs as they are dead code prefeerably in a seperate commit to make the review easier.

[ ] To mimic the old behavior especially for people that prefer the old default, we need to rename get_ignore_mod_tap_interrupt to get_hold_on_other_key_press on keymap level plus inversion of logic by enabling HOLD_ON_OTHER_KEY_PRESS for these keymaps.

I found these keymaps to still use the now defunct get_ignore_mod_tap_interrupt override:

@KarlK90 Doesn't that count as “changing user code en masse”, which @fauxpark advised against, in this comment on another PR?

[KarlK90]

@KarlK90 Doesn't that count as “changing user code en masse”, which @fauxpark advised against, in this comment on another PR?

Probably, but I would argue that 9 keymaps (1 is already commented out) to change don't count as a en masse change. But the people would have to be notified and sign off on this change though.

[precondition]

@cykedev @chriskopher @snowe2010 @stamm @adamtabrams @rootiest @Cy6erBr4in @drashna
You are tagged here because this is a pull request that makes IGNORE_MOD_TAP_INTERRUPT (trigger hold action only if the key is held for longer than TAPPING_TERM) the new defaut behaviour for dual-role keys. This means that defines like IGNORE_MOD_TAP_INTERRUPT and functions like get_ignore_mod_tap_interrupt are replaced with HOLD_ON_OTHER_KEY_PRESS and get_hold_on_other_key_press (with inverted logic).

Your personal keymap code was adapted in a backwards-compatible way (meaning, it will still act the same as before) in commit e94cdb5. Since this commit/update affects your personal keymap files, I need a sign-off from you, agreeing to the proposed changes in order to proceed.

PS: When I say "proposed changes", I'm only talking about commit e94cdb5. I'm not asking you to review this whole PR but feel free to do it if you want ;)

[precondition]

The now de-funct IGNORE_MOD_TAP_INTERRUPT_PER_KEY and IGNORE_MOD_TAP_INTERRUPT defines should be removed from all keebs as they are dead code prefeerably in a seperate commit to make the review easier.

@KarlK90 Do you mean only removing it from keyboard-level config.h files as I've done in commit ab78408 or should I also remove them from users' keymap-level config.h files?

[cykedev]

@precondition this is fine for me

[KarlK90]

@KarlK90 Do you mean only removing it from keyboard-level config.h files as I've done in commit ab78408 or should I also remove them from users' keymap-level config.h files?

As they have no usage anymore or rather are the new default, please remove them completely. A ping is not necessary here because the functionality doesn't change.

[KarlK90]

Looking forward to have IGNORE_MOD_TAP_INTERRUPT the default behavior in the future! Thanks for pushing this and your work.

[KarlK90]

@precondition sorry one more rebase after #17007 was merged

[tzarc]

Ticking off the director review. Will merge once conflicts are sorted.

[tzarc]

Let's try that again with the correct permissions.

[precondition]

When rebasing commit e1c457456f86b1c5024ca38b58df3f6a76d161e7, I realized that I can't just rename get_ignore_mod_tap_interrupt by get_hold_on_other_key_press and swap every true by false and vice versa because the default case of get_ignore_mod_tap_interrupt only encompassed mod-taps while the default case of get_hold_on_other_key_press encompasses all tapping keys (layer taps, mod taps, one shot mods, one shot layers, toggle tap, ...). Unfortunately, I can't just add a QK_MOD_TAP ... QK_MOD_TAP_MAX case because that would create duplicated/overlapping case values in the common situation where the user specified a special rule for a certain modtap. Give me a little more time to work through this.