Skip to content

[Core] Replace Tapping Force Hold feature with Quick Tap Term#17007

Merged
KarlK90 merged 11 commits intoqmk:developfrom
filterpaper:quick_tap_term
Dec 12, 2022
Merged

[Core] Replace Tapping Force Hold feature with Quick Tap Term#17007
KarlK90 merged 11 commits intoqmk:developfrom
filterpaper:quick_tap_term

Conversation

@filterpaper
Copy link
Member

@filterpaper filterpaper commented May 5, 2022

Description

The current TAPPING_FORCE_HOLD setting is a binary feature. This change replaces it with QUICK_TAP_TERM that allows user to fine tune double-tap timing to activate auto-repeat on hold-tap keys.

QUICK_TAP_TERM matches TAPPING_TERM as default and includes per key support with get_quick_tap_term() function. Parity setting to TAPPING_FORCE_HOLD is #define QUICK_TAP_TERM 0

Types of Changes

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

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

@filterpaper filterpaper mentioned this pull request May 5, 2022
Closed
11 tasks
@zvecr zvecr added the breaking_change Changes that need to wait for a version increment label May 5, 2022
@zvecr zvecr changed the base branch from master to develop May 5, 2022 02:35
@filterpaper filterpaper force-pushed the quick_tap_term branch 12 times, most recently from 7037b65 to 12b2599 Compare May 5, 2022 06:00
@filterpaper
Copy link
Member Author

filterpaper commented May 5, 2022 •
edited
Loading

@KarlK90 I had to make these changes to retro_tapping/test_tapping.cpp for the unit test to pass this PR. Does the parameter name ANewTapWithinTappingTermIsBuggy mean what it says?

precondition
precondition reviewed May 5, 2022
View reviewed changes
Copy link
Contributor

@precondition precondition left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you intentionally avoid implementing an equivalent to ZMK's global quick-tap because it is out-of-scope and in order to keep the PR small or is that planned in the future? I think the name "quick tap" only makes sense in the context of its global version.

@filterpaper
Copy link
Member Author

filterpaper commented May 5, 2022

Do you intentionally avoid implementing an equivalent to ZMK's global quick-tap because it is out-of-scope and in order to keep the PR small or is that planned in the future? I think the name "quick tap" only makes sense in the context of its global version.

quick-tap has existed for a while, and global-quick-tap was just added less than a month ago. The latter will not be in this PR because porting it isn't that easy from what I can tell.

@filterpaper filterpaper force-pushed the quick_tap_term branch 6 times, most recently from 78b7cf0 to 48abf9a Compare May 7, 2022 04:17
@KarlK90
Copy link
Member

KarlK90 commented May 7, 2022

@KarlK90 I had to make these changes to retro_tapping/test_tapping.cpp for the unit test to pass this PR. Does the parameter name ANewTapWithinTappingTermIsBuggy mean what it says?

The changes are apparently not needed anymore? I can't tell you much about the test itself though, it was added by @fredizzimo a "long time" ago and I just ported it.

@drashna drashna requested a review from a team May 11, 2022 11:08
@precondition
Copy link
Contributor

precondition commented May 12, 2022

Works as expected on my Polilla (STM32F042)

@filterpaper
Copy link
Member Author

filterpaper commented Nov 28, 2022

@filterpaper with the merge of #17281 this PR unfortunately has conflicts and needs a rebase.

@KarlK90 Thanks for reviewing again, rebased as requested.

@KarlK90
Copy link
Member

KarlK90 commented Nov 29, 2022 •
edited
Loading

@filterpaper you're rebase unfortunately doesn't contain the changes you did earlier to action_tapping.c and some usages of TAPPING_FORCE_HOLD and TAPPING_FORCE_HOLD_PER_KEY sneaked into the code base in user code as well. I'll merge your PR ASAP afterwards. Thank you!

@filterpaper
Copy link
Member Author

filterpaper commented Dec 9, 2022

@filterpaper you're rebase unfortunately doesn't contain the changes you did earlier to action_tapping.c and some usages of TAPPING_FORCE_HOLD and TAPPING_FORCE_HOLD_PER_KEY sneaked into the code base in user code as well. I'll merge your PR ASAP afterwards. Thank you!

@KarlK90 Rebased and tested against current develop branch—confirmed that everything is working correctly.

@KarlK90 KarlK90 force-pushed the quick_tap_term branch 2 times, most recently from 462532f to b99d9c5 Compare December 12, 2022 12:43
KarlK90
KarlK90 approved these changes Dec 12, 2022
View reviewed changes
Copy link
Member

@KarlK90 KarlK90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now! Thanks.

@KarlK90 KarlK90 merged commit cbabc8d into qmk:develop Dec 12, 2022
@KarlK90 KarlK90 mentioned this pull request Dec 12, 2022
Merged
14 tasks
@filterpaper filterpaper deleted the quick_tap_term branch December 13, 2022 06:30
@filterpaper filterpaper mentioned this pull request Dec 15, 2022
Merged
14 tasks
omikronik pushed a commit to omikronik/qmk_firmware that referenced this pull request Jan 22, 2023
@filterpaper @KarlK90 @omikronik
[Core] Replace Tapping Force Hold feature with Quick Tap Term (qmk#17007
8b75734 
)

* Replace Tapping Force Hold feature with Quick Tap Term

* Replace keyboard level TAPPING_FORCE_HOLD with QUICK_TAP_TERM 0

* Deprecate force hold in info_config.json

* Before and after quick tap term unit tests

* Quick tap unit tests iteration

* Keymap config.h correction

* Remove TAPPING_FORCE_HOLD_PER_KEY macros that were missed

* Add two more test cases for quick tap

* Replace TAPPING_FORCE_HOLD with QUICK_TAP_TERM in configs qmk#2

* Replace TAPPING_FORCE_HOLD_PER_KEY with QUICK_TAP_TERM_PER_KEY in configs qmk#2

* Add function declaration for get_quick_tap_term

Co-authored-by: Stefan Kerkmann <karlk90@pm.me>
@Hakari-R Hakari-R mentioned this pull request Jul 31, 2024
Closed
@m-dwork m-dwork mentioned this pull request Aug 27, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@drashna drashna drashna approved these changes

@KarlK90 KarlK90 KarlK90 approved these changes

+1 more reviewer

@precondition precondition precondition approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

breaking_change Changes that need to wait for a version increment core documentation keyboard keymap via Adds via keymap and/or updates keyboard for via support

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@filterpaper @KarlK90 @precondition @0xcharly @drashna @fredizzimo @tzarc @zvecr