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

Implement and document TAPPING_FORCE_HOLD_PER_KEY #7859

Merged

Conversation

ridingqwerty
Copy link
Contributor

Description

Inspired by @zk-phi
Implements a per-key TAPPING_FORCE_HOLD
Special thanks to @Curry for helping me suck less at git.

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

@ridingqwerty
Copy link
Contributor Author

I've added record to the per-key getter function at @drashna 's suggestion.

@drashna drashna requested a review from a team January 13, 2020 18:04
@ridingqwerty
Copy link
Contributor Author

Tested a bit further and confirmed that, in addition to the intended purpose of allowing one to double-tap to spam tap-hold keys not explicitly included by get_tapping_force_hold, this feature also doesn't break TT and friends.

@drashna
Copy link
Member

drashna commented Jan 17, 2020

"and friends" includes One Shot keys?

@ridingqwerty
Copy link
Contributor Author

ridingqwerty commented Jan 17, 2020

Correct, I am testing on my main branch which does not have this feature, and with vanilla TAPPING_FORCE_HOLD, TAPPING_TOGGLE 2, and ONESHOT_TAP_TOGGLE 2 defined, the following actions fail (as expected):

  • TT fails to lock layer
  • OSM fails to lock layer or mod state
  • Tap-key repeat on double-tap fails on tap-hold keys (LT, MT...)

I did the same tests on another branch with TAPPING_FORCE_HOLD_PER_KEY, all of the above features worked in the general case, but not for the specific keys that were whitelisted to have TAPPING_FORCE_HOLD enabled.

I'm not sure in what other ways Onshot keys would be affected by TAPPING_FORCE_HOLD but if there are other tests you can think of I'm open to trying them.

@ridingqwerty
Copy link
Contributor Author

Noticed a silly typo in tmk_core/common/action_tapping.c, a fallthrough check on whether or not IGNORE_TAPPING_FORCE_HOLD_PER_KEY was defined. Changed this to be TAPPING_FORCE_HOLD_PER_KEY as originally intended.

My testing didn't reveal the error because I wasn't defining both TAPPING_FORCE_HOLD_PER_KEY and TAPPING_FORCE_HOLD, just the per-key variant. I just happened to notice it when inspecting something else in tmk_core/common/action_tapping.c.

Retested and everything is fine.

@ridingqwerty ridingqwerty merged commit 95c24bb into qmk:master Jan 17, 2020
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Feb 12, 2020
* Implement and document TAPPING_FORCE_HOLD_PER_KEY

* Added "record" parameter to "get_tapping_force_hold"

* Correct typo -- remove 'IGNORE_' from 'IGNORE_TAPPING_FORCE_HOLD_PER_KEY'

Co-authored-by: GeorgeKoenig <[email protected]>
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Feb 14, 2020
* Implement and document TAPPING_FORCE_HOLD_PER_KEY

* Added "record" parameter to "get_tapping_force_hold"

* Correct typo -- remove 'IGNORE_' from 'IGNORE_TAPPING_FORCE_HOLD_PER_KEY'

Co-authored-by: GeorgeKoenig <[email protected]>
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Feb 21, 2020
* Implement and document TAPPING_FORCE_HOLD_PER_KEY

* Added "record" parameter to "get_tapping_force_hold"

* Correct typo -- remove 'IGNORE_' from 'IGNORE_TAPPING_FORCE_HOLD_PER_KEY'

Co-authored-by: GeorgeKoenig <[email protected]>
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Feb 26, 2020
* Implement and document TAPPING_FORCE_HOLD_PER_KEY

* Added "record" parameter to "get_tapping_force_hold"

* Correct typo -- remove 'IGNORE_' from 'IGNORE_TAPPING_FORCE_HOLD_PER_KEY'

Co-authored-by: GeorgeKoenig <[email protected]>
kylekuj pushed a commit to kylekuj/qmk_firmware that referenced this pull request Apr 21, 2020
* Implement and document TAPPING_FORCE_HOLD_PER_KEY

* Added "record" parameter to "get_tapping_force_hold"

* Correct typo -- remove 'IGNORE_' from 'IGNORE_TAPPING_FORCE_HOLD_PER_KEY'

Co-authored-by: GeorgeKoenig <[email protected]>
@uqs
Copy link

uqs commented Feb 12, 2022

I have a question on whether the logic here is reversed? The default is to return false, and the docs tell you to use this:

bool get_tapping_force_hold(uint16_t keycode, keyrecord_t *record) {
    switch (keycode) {
        case LT(1, KC_BSPC):
            return true;
        default:
            return false;
    }
}

But that doesn't work for me, I need to flip this around, so that backspace is held after double tapping, i.e. I have:

  bool get_tapping_force_hold(uint16_t keycode, keyrecord_t *record) {
      switch (keycode) {                                                                                                                                                                                                                           
          case LT(L_MOUSE, KC_TAB):
          case LT(L_NUM, KC_BSPC):
              return false;  // sic!
          default:
              return true;
      }
  }  

This works to use the layer, or tap the keycode, or have tab/backspace repeat after a double-tap-and-hold.

My config.h has this:

#define TAPPING_TOGGLE 2  // number of taps for a toggle-on-tap                                                                                                                                                                                    
#define TAPPING_TERM 170  // ms to trigger tap
// https://precondition.github.io/home-row-mods
#define TAPPING_FORCE_HOLD  // make tap-then-hold _not_ do key auto repeat
#define TAPPING_FORCE_HOLD_PER_KEY  // ... but do it for some!
#define HOLD_ON_OTHER_KEY_PRESS  // obsolete my LT_NUM_BSPC
#define HOLD_ON_OTHER_KEY_PRESS_PER_KEY  // ... but not for mod-taps!
#define IGNORE_MOD_TAP_INTERRUPT
#define PERMISSIVE_HOLD  // I don't think this works for me, hence I rolled my own implementation.

@sigprof
Copy link
Contributor

sigprof commented Feb 12, 2022

I have a question on whether the logic here is reversed?

The logic is not reversed, but the documentation of TAPPING_FORCE_HOLD and its per-key version is somewhat confusing.

The default behavior (if you don't have #define TAPPING_FORCE_HOLD, or if you have #define TAPPING_FORCE_HOLD_PER_KEY and your get_tapping_force_hold() returns false is like this:

  • If you tap the dual-role key, QMK sends a tap event for the “tap” keycode.
  • If you quickly press the dual-role key after tapping it, QMK will send a press event for the “tap” keycode too, so that the host will see the “tap” key as held down until you actually release the key.
  • If you actually need to invoke the “hold” action of the dual-role key after you just tapped it, you will need to wait until the tapping term expires before pressing the dual-role key again.

However, if you add #define TAPPING_FORCE_HOLD, or add #define TAPPING_FORCE_HOLD_PER_KEY and make your get_tapping_force_hold() returns true for some dual role key, the following will happen:

  • If you tap the dual-role key, QMK sends a tap event for the “tap” keycode — but QMK will also reset the state for the dual-role key, so that the next press will again be handled as if it was the first one.
  • If you quickly press the dual-role key after tapping it, QMK will start a tap/hold detection; then the key action will be determined according to the press duration, ignoring the fact that you tapped the key previously. So you can invoke the “hold” action of the dual-role key even after you just tapped the same key.
  • However, if you need to make the host see that you are holding the “tap” key assigned to the dual-role key, there is no way to do that in the “tapping force hold” mode. Some other behavior that relies on tap counting (e.g., TAPPING_TOGGLE) will also stop working.

So the “tapping force hold” mode name actually refers to forcing the “hold” action of the dual-role key to be used, not to the holding of the “tap” key.

@uqs
Copy link

uqs commented Feb 13, 2022

Thanks for the exhaustive explanation! So wouldn't one then rather use it for the home row mods, where you'd want the tap-then-hold to trigger first the tap, then the hold of the hold function (and not a repeated tap).

This seems to be roughly equivalent in 3min of testing:

config.h:
#define TAPPING_FORCE_HOLD_PER_KEY
// NO define TAPPING_FORCE_HOLD

keymap.c
bool get_tapping_force_hold(uint16_t keycode, keyrecord_t *record) {
    switch (keycode) {
          case KC_G_A:   // defined as home row mods on Colemak, i.e. LGUI_T(KC_A)
          case KC_A_R: 
          case KC_S_S: 
          case KC_C_T: 
          case KC_C_N: 
          case KC_S_E: 
          case KC_A_I:                                                                                
          case KC_G_O:
          case SFT_T(KC_SPC):
              return true;
          default:
              return false;
      }       
}

vs.

config.h:
#define TAPPING_FORCE_HOLD
#define TAPPING_FORCE_HOLD_PER_KEY

keymap.c:
bool get_tapping_force_hold(uint16_t keycode, keyrecord_t *record) {
      switch (keycode) {
          case LT(L_MOUSE, KC_TAB):
          case LT(L_NUM, KC_BSPC):
              return false;
          default:
              return true;
      }
}

I'll go with the latter config for now, as I'm used to not being able to have the repeated-tap on these keys anyway. And currently I can't backspace and then immediately enter a number from my L_NUM layer, as holding it will trigger even more backspace. Maybe that whole feature isn't for me? Or it might be best for layers that you use rarely, especially not during rapid character entry? Time for more experimentation!

BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* Implement and document TAPPING_FORCE_HOLD_PER_KEY

* Added "record" parameter to "get_tapping_force_hold"

* Correct typo -- remove 'IGNORE_' from 'IGNORE_TAPPING_FORCE_HOLD_PER_KEY'

Co-authored-by: GeorgeKoenig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants