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

Pass in the keyrecord_t of the dual-role/tapping key when calling per-key tap hold functions #15938

Merged
merged 2 commits into from Feb 11, 2022

Conversation

precondition
Copy link
Contributor

@precondition precondition commented Jan 19, 2022

Description

[BUG FIX]

The second keyrecord_t* argument given to tap hold per key functions should refer to the same key as that given in the first uint16_t keycode argument. All the per-key functions called in bool process_tapping(keyrecord_t *keyp) use the same pattern : get_option(tapping_keycode, keyp) but from what I've gathered, keyp is not necessarily the tapping_key so it should instead be get_option(tapping_keycode, &tapping_key).

To illustrate my point:

Let the key on row=1, col=3 be MT(MOD_LSFT, KC_S) (hexadecimal keycode=0x6216 ) and the key on row=2, col=7 be KC_H.
Consider the following body for the get_tapping_term function.

uint16_t get_tapping_term(uint16_t keycode, keyrecord_t *record) {
    uprintf("0x%04X,%u,%u,%u,%u\n",
         keycode,
         record->event.key.row,
         record->event.key.col,
         get_highest_layer(layer_state),
         record->event.pressed
         );
    return TAPPING_TERM;
}  

If we try to capitalise H by holding down the S shift modtap, we can see the following appear in the printed output:

0x6216,1,3,0,1
0x6216,2,7,0,1
0x6216,1,3,0,1

The second printed line shows the keycode of the mod-tap but the keyrecord_t information of KC_H
Isn't keycode and record supposed to refer to the same underlying key event?

You can't even really exploit it that much, it seems. I thought that perhaps this might be useful to tweak the tapping term of a specific modtap if it is combined with another key but the following doesn't increase the tapping term for LSFT_T(KC_S) when its tap is interrupted by KC_H (row=2, col=7)

uint16_t get_tapping_term(uint16_t keycode, keyrecord_t *record) {
    uprintf("0x%04X,%u,%u,%u,%u\n",
         keycode,
         record->event.key.row,
         record->event.key.col,
         get_highest_layer(layer_state),
         record->event.pressed
         );
    switch (keycode) {
        case LSFT_T(KC_S):
            if (record->event.key.row == 2 && record->event.key.col == 7) {
                return 8000;
            } else {
                return TAPPING_TERM-10;
            }
        default:
            return TAPPING_TERM;
    }
};

As in, I most definitely do not need to be holding down LSFT_T(KC_S) for 8s before pressing KC_H in order to capitalise it.

When discussing this issue on the Discord server, sigprof also pointed out instances where the get_tapping_term function is called with a NULL second argument in core QMK code which presents the high risk of nullptr dereference at runtime if a user, for example, dereferences the record argument to fetch the keypos information in their per-key tap hold function(s). To avoid this problem, I replaced the NULLs by an empty keyrecord structure (&(keyrecord_t){}). Let me know if there's a better substitute that I can use.

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 have tested the changes and verified that they work and don't break anything (as well as I can manage).

@github-actions github-actions bot added the core label Jan 19, 2022
@drashna drashna requested a review from a team January 22, 2022 05:45
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.

The reasoning is sound from my POV and it compiles. Tested locally as well.

@KarlK90
Copy link
Member

KarlK90 commented Feb 2, 2022

@precondition On a second thought, could you add unit-tests to cover these changes. At least for the ones in action_tapping.c? Would tremendously help in refactoring later and ensuring that bugs like these won't slip in at a later point.

@zvecr zvecr merged commit 7148a69 into qmk:develop Feb 11, 2022
@zvecr zvecr added the bug label Feb 11, 2022
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.

None yet

4 participants