Do not enable PERMISSIVE_HOLD when TAPPING_TERM exceeds 500ms #15674

precondition · 2021-12-30T22:58:29Z

Description

Do not enable PERMISSIVE_HOLD-like behaviour when TAPPING_TERM ≥ 500ms.

This is a poorly documented obscure "feature" that's entirely unnecessary and does more harm than good. When people are testing dual-role keys like MT(), they need to play around with the different tap-hold option and tweak their tapping term in order to limit misfires. A lot of times, people enable IGNORE_MOD_TAP_INTERRUPT because this is the option that is the most restrictive when it comes to selecting the hold action (desirable for a person using home row mods for example) but because they're not used to it, they get a lot of misfires. In which case, they're advised to increase the tapping term.

However, some users crank it way above 500ms "just to be safe" and make it very hard to accidentally trigger a hold. That's when QMK "helpfully" enables permissive hold (despite the user not having enabled the setting in config.h) under their nose. Now, when testing, the user encounters even more misfires than before because permissive hold activates an additional way to trigger holds. This only serves to frustrate the users and push them away from exploiting the full capacities of the advanced tap-hold mechanics that QMK offers.

Finally, this PR is not a loss of functionality because one only needs to add #define PERMISSIVE_HOLD to their config.h to get the behaviour if they so desire. Furthermore, you can expect this PR to free up about 55 bytes in keymaps that don't enable PERMISSIVE_HOLD. (Everything counts on AVR)

Types of Changes

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

drashna

code change looks good. But I'm not sure that this is for the best.

precondition · 2022-05-05T14:10:59Z

keyboards/helix/pico/keymaps/mtei/config.h:#define TAPPING_TERM 300
keyboards/helix/pico/keymaps/mtei/config.h:#define PERMISSIVE_HOLD
keyboards/helix/pico/keymaps/mtei/config.h:/* when TAPPING_TERM >= 500 same effect PERMISSIVE_HOLD.
keyboards/helix/pico/keymaps/mtei/config.h:   see tmk_core/common/action_tapping.c */
--
keyboards/helix/rev2/keymaps/five_rows/config.h:#define TAPPING_TERM 300
keyboards/helix/rev2/keymaps/five_rows/config.h:#define PERMISSIVE_HOLD
keyboards/helix/rev2/keymaps/five_rows/config.h:/* when TAPPING_TERM >= 500 same effect PERMISSIVE_HOLD.
keyboards/helix/rev2/keymaps/five_rows/config.h:   see tmk_core/common/action_tapping.c */
--
keyboards/helix/rev3_5rows/keymaps/five_rows/config.h:#define TAPPING_TERM 300
keyboards/helix/rev3_5rows/keymaps/five_rows/config.h:#define PERMISSIVE_HOLD
keyboards/helix/rev3_5rows/keymaps/five_rows/config.h:/* when TAPPING_TERM >= 500 same effect PERMISSIVE_HOLD.
keyboards/helix/rev3_5rows/keymaps/five_rows/config.h:   see tmk_core/common/action_tapping.c */
--
layouts/community/ergodox/adam/config.h:
layouts/community/ergodox/adam/config.h:#undef TAPPING_TERM
layouts/community/ergodox/adam/config.h:#define TAPPING_TERM 300 //At 500 some bad logic takes hold
layouts/community/ergodox/adam/config.h:#define IGNORE_MOD_TAP_INTERRUPT

Can I also remove these (would-be-)outdated comments?

KarlK90

Other then the rebase this LGTM! Thanks.

quantum/action_tapping.c

KarlK90 · 2022-06-24T09:37:18Z

Can I also remove these (would-be-)outdated comments?

Yes, please remove them. This doesn't need confirmation from the owners as it just rectifies the situation.

KarlK90 · 2022-06-24T09:42:52Z

One more request:

❯ rg "TAPPING_TERM [5-9]\d\d"
users/wanleg/config.h
14:#define TAPPING_TERM 700

keyboards/1k/keymaps/tap_dance/config.h
6:#define TAPPING_TERM 500

keyboards/idobao/id67/keymaps/thewerther/config.h
20:#define TAPPING_TERM 500

keyboards/handwired/onekey/config.h
44:#define TAPPING_TERM 500

keyboards/handwired/ergocheap/config.h
49:#define TAPPING_TERM 500

keyboards/handwired/uthol/rev3/config.h
59:#define TAPPING_TERM 500

keyboards/xiudi/xd002/keymaps/tap_dance/config.h
3:#define TAPPING_TERM 500

keyboards/1upkeyboards/sweet16/keymaps/ridingintraffic/config.h
5:#define TAPPING_TERM 500

keyboards/keebio/quefrency/keymaps/bjohnson/config.h
29:#define TAPPING_TERM 500

keyboards/thevankeyboards/minivan/keymaps/halvves/config.h
3:#define TAPPING_TERM 505

keyboards/thevankeyboards/minivan/keymaps/belak/config.h
6:#define TAPPING_TERM 500

keyboards/flehrad/bigswitch/keymaps/333fred/config.h
26:#define TAPPING_TERM 500

keyboards/zfrontier/big_switch/config.h
53:#define TAPPING_TERM 500

layouts/community/66_ansi/xyverz/config.h
3:#define TAPPING_TERM 600 // ms

❯ rg "TAPPING_TERM \d\d\d\d"
keyboards/woodkeys/bigseries/1key/keymaps/dudeofawesome/config.h
21:#define TAPPING_TERM 1000

keyboards/massdrop/alt/keymaps/favorable-mutation/config.h
24:#define TAPPING_TERM 1000

users/hvp/config.h
18:#define LONG_TAPPING_TERM 1000

These configs should get #define PERMISSIVE_HOLD as they expected (?) or at least use this behavior.

filterpaper · 2022-06-24T09:46:02Z

Do we typically have to manage user config/keymaps for breaking changes?

KarlK90 · 2022-06-24T10:01:43Z

Do we typically have to manage user config/keymaps for breaking changes?

I would say that QMK should not break user space if it is possible. So if there is a trivial backwards compatible conversion it should be done in the PR.

TAPPING_TERM

precondition · 2022-06-24T10:11:06Z

Yes, please remove them. This doesn't need confirmation from the owners as it just rectifies the situation.

Solved in 9167f41

These configs should get #define PERMISSIVE_HOLD as they expected (?) or at least use this behavior.

Solved in f3e9fa7

~/qmk_firmware on tap_term_500 *3 ⨤7                                                                                                
❯ grep -c PERMISSIVE_HOLD users/wanleg/config.h keyboards/1k/keymaps/tap_dance/config.h keyboards/idobao/id67/keymaps/thewerther/config.h keyboards/handwired/onekey/config.h keyboards/handwired/ergocheap/config.h keyboards/handwired/uthol/rev3/config.h keyboards/xiudi/xd002/keymaps/tap_dance/config.h keyboards/1upkeyboards/sweet16/keymaps/ridingintraffic/config.h keyboards/keebio/quefrency/keymaps/bjohnson/config.h keyboards/thevankeyboards/minivan/keymaps/halvves/config.h keyboards/thevankeyboards/minivan/keymaps/belak/config.h keyboards/flehrad/bigswitch/keymaps/333fred/config.h keyboards/zfrontier/big_switch/config.h layouts/community/66_ansi/xyverz/config.h keyboards/woodkeys/bigseries/1key/keymaps/dudeofawesome/config.h keyboards/massdrop/alt/keymaps/favorable-mutation/config.h users/hvp/config.h
users/wanleg/config.h:1
keyboards/1k/keymaps/tap_dance/config.h:1
keyboards/idobao/id67/keymaps/thewerther/config.h:1
keyboards/handwired/onekey/config.h:1
keyboards/handwired/ergocheap/config.h:1
keyboards/handwired/uthol/rev3/config.h:1
keyboards/xiudi/xd002/keymaps/tap_dance/config.h:1
keyboards/1upkeyboards/sweet16/keymaps/ridingintraffic/config.h:1
keyboards/keebio/quefrency/keymaps/bjohnson/config.h:1
keyboards/thevankeyboards/minivan/keymaps/halvves/config.h:1
keyboards/thevankeyboards/minivan/keymaps/belak/config.h:1
keyboards/flehrad/bigswitch/keymaps/333fred/config.h:1
keyboards/zfrontier/big_switch/config.h:1
layouts/community/66_ansi/xyverz/config.h:1
keyboards/woodkeys/bigseries/1key/keymaps/dudeofawesome/config.h:1
keyboards/massdrop/alt/keymaps/favorable-mutation/config.h:1
users/hvp/config.h:1

KarlK90

Thanks for quickly implementing the changes.

quantum/action_tapping.c

KarlK90

All test are green now. This is good to go.

precondition · 2022-06-24T10:46:54Z

It also passes all the new tap hold configuration unit tests I had added in #15741 so that's extra re-insurance

)

github-actions bot added core documentation translation labels Dec 30, 2021

precondition force-pushed the tap_term_500 branch from b008d85 to 32522a0 Compare December 30, 2021 23:40

drashna requested a review from a team January 2, 2022 08:08

precondition force-pushed the tap_term_500 branch from 1cbbe50 to 840ed16 Compare March 7, 2022 18:50

drashna approved these changes Mar 8, 2022

View reviewed changes

drashna requested a review from a team March 8, 2022 04:22

precondition force-pushed the tap_term_500 branch from 840ed16 to 404dbc2 Compare April 17, 2022 13:30

KarlK90 requested changes Jun 24, 2022

View reviewed changes

quantum/action_tapping.c Show resolved Hide resolved

quantum/action_tapping.c Show resolved Hide resolved

Do not enable PERMISSIVE_HOLD when TAPPING_TERM exceeds 500ms

21f12cd

precondition added 2 commits June 24, 2022 12:03

Remove outdated comments about tapping term ≥ 500

9167f41

Backwards compatibility: add PERMISSIVE_HOLD to users with ≥500ms

f3e9fa7

TAPPING_TERM

precondition force-pushed the tap_term_500 branch from 404dbc2 to f3e9fa7 Compare June 24, 2022 10:10

github-actions bot added keyboard keymap labels Jun 24, 2022

KarlK90 approved these changes Jun 24, 2022

View reviewed changes

Fix ifdef spaghetti of parentheses

9b6b203

KarlK90 requested changes Jun 24, 2022

View reviewed changes

quantum/action_tapping.c Outdated Show resolved Hide resolved

KarlK90 approved these changes Jun 24, 2022

View reviewed changes

KarlK90 merged commit 3b9e186 into qmk:develop Jun 24, 2022

0xcharly pushed a commit to Bastardkb/bastardkb-qmk that referenced this pull request Jul 4, 2022

Do not enable PERMISSIVE_HOLD when TAPPING_TERM exceeds 500ms (qmk#15674

c209b09

)

precondition mentioned this pull request Aug 12, 2022

Bug17281 - Retain momentary layers until the end of tapping #17282

Merged

14 tasks

precondition deleted the tap_term_500 branch October 5, 2022 07:56

nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023

Do not enable PERMISSIVE_HOLD when TAPPING_TERM exceeds 500ms (qmk#15674

b3cbdfa

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not enable PERMISSIVE_HOLD when TAPPING_TERM exceeds 500ms #15674

Do not enable PERMISSIVE_HOLD when TAPPING_TERM exceeds 500ms #15674

precondition commented Dec 30, 2021 •

edited

Loading

drashna left a comment

precondition commented May 5, 2022 •

edited

Loading

KarlK90 left a comment

KarlK90 commented Jun 24, 2022 •

edited

Loading

KarlK90 commented Jun 24, 2022 •

edited

Loading

filterpaper commented Jun 24, 2022

KarlK90 commented Jun 24, 2022

precondition commented Jun 24, 2022 •

edited

Loading

KarlK90 left a comment

KarlK90 left a comment

precondition commented Jun 24, 2022

Do not enable PERMISSIVE_HOLD when TAPPING_TERM exceeds 500ms #15674

Do not enable PERMISSIVE_HOLD when TAPPING_TERM exceeds 500ms #15674

Conversation

precondition commented Dec 30, 2021 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

drashna left a comment

Choose a reason for hiding this comment

precondition commented May 5, 2022 • edited Loading

KarlK90 left a comment

Choose a reason for hiding this comment

KarlK90 commented Jun 24, 2022 • edited Loading

KarlK90 commented Jun 24, 2022 • edited Loading

filterpaper commented Jun 24, 2022

KarlK90 commented Jun 24, 2022

precondition commented Jun 24, 2022 • edited Loading

KarlK90 left a comment

Choose a reason for hiding this comment

KarlK90 left a comment

Choose a reason for hiding this comment

precondition commented Jun 24, 2022

precondition commented Dec 30, 2021 •

edited

Loading

precondition commented May 5, 2022 •

edited

Loading

KarlK90 commented Jun 24, 2022 •

edited

Loading

KarlK90 commented Jun 24, 2022 •

edited

Loading

precondition commented Jun 24, 2022 •

edited

Loading