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

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

Issues Fixed or Closed by This PR

Checklist

• [x] My code follows the code style of this project: C, Python
• [x] I have read the PR Checklist document and have made the appropriate changes.
• [x] My change requires a change to the documentation.
• [x] I have updated the documentation accordingly.
• [x] I have read the CONTRIBUTING document.
• [x] I have added tests to cover my changes.
• [x] 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 :slightly_frowning_face:

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.

[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 https://github.com/qmk/qmk_firmware/pull/15741/commits/e94cdb549e2be3d23f64f5e9f732619109eaa53c. 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 https://github.com/qmk/qmk_firmware/pull/15741/commits/e94cdb549e2be3d23f64f5e9f732619109eaa53c. 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 https://github.com/qmk/qmk_firmware/pull/15741/commits/ab78408494d878d62c0c1640ad2ada55d36f1ca1 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.

[snowe2010]

does this mean I'll need to change my keymap to include HOLD_ON_OTHER_KEY_PRESS?

[precondition]

does this mean I'll need to change my keymap to include HOLD_ON_OTHER_KEY_PRESS?

@snowe2010 You need to add the per-key version (HOLD_ON_OTHER_KEY_PRESS_PER_KEY) only if you ever decide to uncomment those lines. I did not add HOLD_ON_OTHER_KEY_PRESS_PER_KEY to your config.h because you commented out the per-key settings and only kept the global IGNORE_MOD_TAP_INTERRUPT setting; which is no longer necessary as that's already the global default in this PR.

Strictly speaking, you could do nothing and it would still work as before. It is just that #define IGNORE_MOD_TAP_INTERRUPT is dead, useless and redundant code with this PR so we remove it.

[chriskopher]

@precondition I'm fine with your changes! Thanks!

[snowe2010]

@precondition sounds good to me then! go for it!

[KarlK90]

I tried out the changes on my keyboard and everything works as expected, very well done you have my approval. Only thing left is that some IGNORE_MOD_TAP_INTERRUPT defines silently sneaked into develop in the meantime.

❯ rg IGNORE_MOD_TAP_INTERRUPT
keyboards/massdrop/ctrl/keymaps/xanimos/config.h
48:// #define IGNORE_MOD_TAP_INTERRUPT    // Makes it possible to do rolling combos (zx) with keys that convert to other keys on hold, by enforcing the TAPPING_TERM for both keys. See Mod tap interrupt for details

keyboards/handwired/dactyl_manuform/3x5_3/keymaps/dlford/config.h
28:#define IGNORE_MOD_TAP_INTERRUPT // ignore hold mod if another tap occurs within tapping term

docs/zh-cn/mod_tap.md
120:在数字及字母键上使用Mod-Tap时推荐启用 `IGNORE_MOD_TAP_INTERRUPT`，以避免在快速按下下一个键时保持功能优先级。参见[忽略Mod Tap中断](zh-cn/tap_hold.md#ignore-mod-tap-interrupt)。

[precondition]

I should almost somehow subscribe to keymap PRs like #16696 getting merged so that I can prevent more #define IGNORE_MOD_TAP_INTERRUPT from sneaking in under my nose.

[KarlK90]

I should almost somehow subscribe to keymap PRs like #16696 getting merged so that I can prevent more #define IGNORE_MOD_TAP_INTERRUPT from sneaking in under my nose.

As part of an internal discussion on how to handle breaking changes like this PR gracefully and how to raise the visibility to end users https://github.com/qmk/qmk_firmware/pull/17063 was created. This should handle the #16696 situation better in the future.

[drashna]

Sorry didn't mean to close!

But came to comment this:

A point in this favor is that ZSA, we have this enabled by default, and undef it when it's not wanted, because we've found that this is a much more natural behavior for the keys, and closer to what customers expect when using the keyboard.

[precondition]

Fixed merge conflict in keyboards/ktec/ergodone/config.h

[precondition]

The gergoplex lint check fail is unrelated to the PR, see https://github.com/qmk/qmk_firmware/pull/16667#issuecomment-1068710785

[KarlK90]

Has merge conflicts after #17284 would you mind converting the tests to EXPECT_REPORT, EXPECT_EMPTY_REPORT and EXPECT_EMPTY_REPORT?

[precondition]

Has merge conflicts after #17284 would you mind converting the tests to EXPECT_REPORT, EXPECT_EMPTY_REPORT and EXPECT_EMPTY_REPORT?

@KarlK90 Done in https://github.com/qmk/qmk_firmware/pull/15741/commits/f8bc774ef8e3af1bb0cf8c49b4bfe3153eab7147 and https://github.com/qmk/qmk_firmware/pull/15741/commits/1a0154d163ddf28deb51b6c3c8f88df1dc98cdf8

[precondition]

Another rebase to remove get_ignore_mod_tap_interrupt from /keyboards/adm42/rev4/keymaps/default, as part of commit https://github.com/qmk/qmk_firmware/pull/15741/commits/1e45db5fb09a14c4cb7564c72fcf8c51aad1fa5a

[precondition]

Would it be better if I extracted commits purely related to HOLD_ON_OTHER_KEY_PRESS like https://github.com/qmk/qmk_firmware/pull/15741/commits/f64861ac2008b5c284826adcb8cfd07fb9a33fd4 and https://github.com/qmk/qmk_firmware/pull/15741/commits/9b9d411997a28bc70fe28748c98ffaceb841afa5 into another PR?

[KarlK90]

Would it be better if I extracted commits purely related to HOLD_ON_OTHER_KEY_PRESS like f64861a and 9b9d411 into another PR?

From my side it is fine to keep these in this PR, as it is already reviewed and approved. Sorry for the delay again.

[KarlK90]

After re-iterating your the mod-tap interrupt PR again internally we came to a decision. The main concern was how to deal with this breaking change for users that want to keep to old behavior as much as possible, so completely rolling out the new behavior by default for this breaking change would give no grace period / or indication for users who do want to keep the existing behavior.

Therefore I would ask you once more for changes to the PR:

1. For a grace period (TBA. but I would say one cycle) after which IGNORE_MOD_TAP_INTERRUPT is phased out and becomes the default, we still want want to keep it as a marker define. This would mean that the commit which removed it would have to be reverted. IGNORE_MOD_TAP_INTERRUPT_PER_KEY on the other hand is deprecated for good and will produce an error.

2. A compile-time switch enables HOLD_ON_OTHER_KEY_PRESS if no other tapping behavior is chosen, therefore we still need IGNORE_MOD_TAP_INTERRUPT. A message which announces the deprecation (with information) should be added to the compilation to hopefully catch as many users as possible:

#if defined(IGNORE_MOD_TAP_INTERRUPT_PER_KEY)
#    error "IGNORE_MOD_TAP_INTERRUPT_PER_KEY has been removed; the code needs to be ported to use HOLD_ON_OTHER_KEY_PRESS_PER_KEY instead."
#elif !defined(IGNORE_MOD_TAP_INTERRUPT)
#    if !defined(PERMISSIVE_HOLD) && !defined(PERMISSIVE_HOLD_PER_KEY) && !defined(HOLD_ON_OTHER_KEY_PRESS) && !defined(HOLD_ON_OTHER_KEY_PRESS_PER_KEY)
#        define HOLD_ON_OTHER_KEY_PRESS
#        pragma message "IGNORE_MOD_TAP_INTERRUPT has been deprecated and will become the new standard behavior of mod and layer taps in the future. Please use HOLD_ON_OTHER_KEY_PRESS if you want to keep the behavior for both layer and mod-taps."
#    endif
#endif

3. Zvecr has a PR in the pipeline to expand linting to keymap level so we would have to add IGNORE_MOD_TAP_INTERRUPT_PER_KEY to the invalid linting rules (info_rules.json) and IGNORE_MOD_TAP_INTERRUPT to deprecated.

[precondition]

For a grace period (TBA. but I would say one cycle) after which IGNORE_MOD_TAP_INTERRUPT is phased out and becomes the default, we still want want to keep it as a marker define. This would mean that the commit which removed it would have to be reverted. IGNORE_MOD_TAP_INTERRUPT_PER_KEY on the other hand is deprecated for good and will produce an error.

You're asking to remove the heart of the PR but okay

A compile-time switch enables HOLD_ON_OTHER_KEY_PRESS if no other tapping behavior is chosen, therefore we still need IGNORE_MOD_TAP_INTERRUPT. A message which announces the deprecation (with information) should be added to the compilation to hopefully catch as many users as possible:

I added the deprecation message in https://github.com/qmk/qmk_firmware/pull/15741/commits/d7a13ed404f8ccffb949a3b94f02594ec5ed0989 but I'm of the opinion that enabling HOLD_ON_THE_OTHER_KEY_PRESS in the absence of other explicitly enabled tap-or-hold modes is a bad idea. HOLD_ON_OTHER_KEY_PRESS applies globally to all dual-role keys, not just mod-taps. This means that forcing that option will change how layer-taps work and users who wish to retain the old tap-preferred/ignore-interrupt mode will have no way to do it.

Leaving it unconfigured will push QMK to enable HOLD_ON_OTHER_KEY_PRESS, enabling PERMISSIVE_HOLD isn't satisfactory either as it is a different tap-or-hold mode, and explicitly enabling HOLD_ON_OTHER_KEY_PRESS will be no different from the no-config out-of-the-box experience wherein the layer is triggered immediately on interrupts. There is no IGNORE_LAYER_TAP_INTERRUPT option that people can use, and adding such an option in this PR would be a step in the opposite direction...

we would have to add IGNORE_MOD_TAP_INTERRUPT_PER_KEY to the invalid linting rules [...]

Done in https://github.com/qmk/qmk_firmware/pull/15741/commits/654897b9d531faf0050ad6626360ebffe9f1f048

[...] and IGNORE_MOD_TAP_INTERRUPT to deprecated.

Done in https://github.com/qmk/qmk_firmware/pull/15741/commits/bfd5a6e85f185b4ad1b2bf9df5d1c7e7f938bd36. Should I keep the "value_type" field even if it's deprecated?

---

In light of the latest requested changes, I think it's fair to say that the PR had changed so much that @drashna's and @KarlK90's prior approvals are now invalid and should be "renewed".

Honestly, I wonder if keeping this PR focused on removing all instances of IGNORE_MOD_TAP_INTERRUPT and changing the default mod-tap behaviour wouldn't be better. Removing IGNORE_MOD_TAP_INTERRUPT_PER_KEY in favour of HOLD_ON_OTHER_KEY_PRESS_PER_KEY could be done in a new PR.

[sigprof]

HOLD_ON_OTHER_KEY_PRESS applies globally to all dual-role keys, not just mod-taps.

Actually now I remembered another reason why I did not like a global HOLD_ON_OTHER_KEY_PRESS — it also affects the behavior of keys like OSL() and OSM() (which many people don't even consider to be dual-role keys, but they actually are implemented like that, and using a global HOLD_ON_OTHER_KEY_PRESS would mean that those keys would revert to the hold behavior when typing quickly due to overlaps, and this may be annoying if a chain of one shot mods is needed). So a global HOLD_ON_OTHER_KEY_PRESS may be a really poor replacement for the old default, and we may need to have a special case option that applies only to MT() if we really want to preserve the original behavior while still removing IGNORE_MOD_TAP_INTERRUPT.