Skip to content
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

Make IGNORE_MOD_TAP_INTERRUPT the default behaviour for mod-taps #20211

Merged
merged 9 commits into from
Apr 3, 2023

Conversation

precondition
Copy link
Contributor

@precondition precondition commented Mar 21, 2023

Description

This is part 2 of the project of removing IGNORE_MOD_TAP_INTERRUPT* to make it the default behaviour of mod-tap keys in order to align them with the rest of tap-hold keys and ease configuration that was started with PR #15741.

Details/Motivation (copied from part 1) 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, 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 planned ahead in Remove IGNORE_MOD_TAP_INTERRUPT_PER_KEY in favour of HOLD_ON_OTHER_KEY_PRESS_PER_KEY #15741 and already made a suite of unit tests called default_mod_tap. The associated config.h file included #define IGNORE_MOD_TAP_INTERRUPT and now I can simply remove that line and still make use of those same tests I had written)
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@drashna drashna requested a review from a team March 21, 2023 20:51
@github-actions github-actions bot added keyboard keymap via Adds via keymap and/or updates keyboard for via support labels Mar 21, 2023
@precondition
Copy link
Contributor Author

precondition commented Mar 21, 2023

The keyboard lint check fails because some of the keyboards are missing license headers in some of their files but this is irrelevant to this PR.

As for the QMK CI build, it gets discouraged when it sees the >100 keyboards it would need to compile and gives up.

The quick_tap tests for simple interactions between mod-tap keys assumed
the old HOLD_ON_OTHER_KEY_PRESS-like behaviour of mod-tap keys but
updating the tests to adapt to the new IGNORE_MOD_TAP_INTERRUPT-like
behaviour of modtaps would make them redundant with the same tests
already present in default_mod_tap, so they were simply removed instead.
@tzarc tzarc merged commit 1899793 into qmk:develop Apr 3, 2023
@precondition precondition deleted the imti_v2 branch April 3, 2023 09:35
@KarlK90
Copy link
Member

KarlK90 commented Apr 5, 2023

@precondition this PR completely flew under my radar, great that this is now the default behavior. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core documentation keyboard keymap translation via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants