diff options
author | Pascal Getreuer <[email protected]> | 2025-04-22 00:59:49 -0700 |
---|---|---|
committer | GitHub <[email protected]> | 2025-04-22 09:59:49 +0200 |
commit | 73e2ef486ab7acf3763695cb3a3cf691451cdff3 (patch) | |
tree | 42fa19653935e7e2c7cc720accaf5ba17641d141 /quantum/action_tapping.c | |
parent | b5f8f4d6a201e6a67d1ee483fe6d3327d98cb052 (diff) | |
download | qmk_firmware-73e2ef486ab7acf3763695cb3a3cf691451cdff3.tar.gz qmk_firmware-73e2ef486ab7acf3763695cb3a3cf691451cdff3.zip |
[Bug][Core] Fix for Flow Tap: fix handling of distinct taps and timer updates. (#25175)
* Flow Tap bug fix.
As reported by @amarz45 and @mwpardue, there is a bug where if two
tap-hold keys are pressed in distinct taps back to back, then Flow Tap
is not applied on the second tap-hold key, but it should be.
In a related bug reported by @NikGovorov, if a tap-hold key is held
followed by a tap of a tap-hold key, then Flow Tap updates its timer on
the release of the held tap-hold key, but it should be ignored.
The problem common to both these bugs is that I incorrectly assumed
`tapping_key` is cleared to noevent once it is released, when actually
`tapping_key` is still maintained for `TAPPING_TERM` ms after release
(for Quick Tap). This commit fixes that. Thanks to @amarz45, @mwpardue,
and @NikGovorov for reporting!
Details:
* Logic for converting the current tap-hold event to a tap is extracted
to `flow_tap_key_if_within_term()`, which is now invoked also in the
post-release "interfered with other tap key" case. This fixes the
distinct taps bug.
* The Flow Tap timer is now updated at the beginning of each call to
`process_record()`, provided that there is no unsettled tap-hold key
at that time and that the record is not for a mod or layer switch key.
By moving this update logic to `process_record()`, it is conceptually
simpler and more robust.
* Unit tests extended to cover the reported scenarios.
* Fix formatting.
* Revision to fix @NikGovorov's scenario.
The issue is that when another key is pressed while a layer-tap hasn't
been settled yet, the `prev_keycode` remembers the keycode from before
the layer switched. This can then enable Flow Tap for the following key
when it shouldn't, or vice versa.
Thanks to @NikGovorov for reporting!
This commit revises Flow Tap in the following ways:
* The previous key and timer are both updated from `process_record()`.
This is slightly later in the sequence of processing than before, and
by this point, a just-settled layer-tap should have taken effect so
that the keycode from the correct layer is remembered.
* The Flow Tap previous key and timer are updated now also on key
release events, except for releases of modifiers and held layer
switches.
* The Flow Tap previous key and timer are now updated together, for
simplicity. This makes the logic easier to think about.
* A few additional unit tests, including @NikGovorov's scenario as
"layer_tap_ignored_with_disabled_key_complex."
Diffstat (limited to 'quantum/action_tapping.c')
-rw-r--r-- | quantum/action_tapping.c | 105 |
1 files changed, 73 insertions, 32 deletions
diff --git a/quantum/action_tapping.c b/quantum/action_tapping.c index 312c639169..3e391d1526 100644 --- a/quantum/action_tapping.c +++ b/quantum/action_tapping.c @@ -6,6 +6,7 @@ #include "action_tapping.h" #include "action_util.h" #include "keycode.h" +#include "quantum_keycodes.h" #include "timer.h" #ifndef NO_ACTION_TAPPING @@ -102,10 +103,10 @@ __attribute__((weak)) bool get_hold_on_other_key_press(uint16_t keycode, keyreco # endif # if defined(FLOW_TAP_TERM) -static uint32_t last_input = 0; -static uint16_t prev_keycode = KC_NO; +static uint32_t flow_tap_prev_time = 0; +static uint16_t flow_tap_prev_keycode = KC_NO; -uint16_t get_flow_tap_term(uint16_t keycode, keyrecord_t *record, uint16_t prev_keycode); +static bool flow_tap_key_if_within_term(keyrecord_t *record); # endif // defined(FLOW_TAP_TERM) static keyrecord_t tapping_key = {}; @@ -157,19 +158,6 @@ void action_tapping_process(keyrecord_t record) { } } if (IS_EVENT(record.event)) { -# if defined(FLOW_TAP_TERM) - const uint16_t keycode = get_record_keycode(&record, false); - // Track the previous key press. - if (record.event.pressed) { - prev_keycode = keycode; - } - // If there is no unsettled tap-hold key, update last input time. Ignore - // mod keys in this update to allow for chording multiple mods for - // hotkeys like "Ctrl+Shift+arrow". - if (IS_NOEVENT(tapping_key.event) && !IS_MODIFIER_KEYCODE(keycode)) { - last_input = timer_read32(); - } -# endif // defined(FLOW_TAP_TERM) ac_dprintf("\n"); } } @@ -252,22 +240,8 @@ bool process_tapping(keyrecord_t *keyp) { // into the "pressed" tapping key state # if defined(FLOW_TAP_TERM) - const uint16_t keycode = get_record_keycode(keyp, false); - if (is_mt_or_lt(keycode)) { - const uint32_t idle_time = timer_elapsed32(last_input); - uint16_t term = get_flow_tap_term(keycode, keyp, prev_keycode); - if (term > 500) { - term = 500; - } - if (idle_time < 500 && idle_time < term) { - debug_event(keyp->event); - ac_dprintf(" within flow tap term (%u < %u) considered a tap\n", (int16_t)idle_time, term); - keyp->tap.count = 1; - registered_taps_add(keyp->event.key); - debug_registered_taps(); - process_record(keyp); - return true; - } + if (flow_tap_key_if_within_term(keyp)) { + return true; } # endif // defined(FLOW_TAP_TERM) @@ -582,6 +556,13 @@ bool process_tapping(keyrecord_t *keyp) { return true; } else if (is_tap_record(keyp)) { // Sequential tap can be interfered with other tap key. +# if defined(FLOW_TAP_TERM) + if (flow_tap_key_if_within_term(keyp)) { + tapping_key = (keyrecord_t){0}; + debug_tapping_key(); + return true; + } +# endif // defined(FLOW_TAP_TERM) ac_dprintf("Tapping: Start with interfering other tap.\n"); tapping_key = *keyp; waiting_buffer_scan_tap(); @@ -809,6 +790,66 @@ static void waiting_buffer_process_regular(void) { # endif // CHORDAL_HOLD # ifdef FLOW_TAP_TERM +void flow_tap_update_last_event(keyrecord_t *record) { + // Don't update while a tap-hold key is unsettled. + if (waiting_buffer_tail != waiting_buffer_head || (tapping_key.event.pressed && tapping_key.tap.count == 0)) { + return; + } + const uint16_t keycode = get_record_keycode(record, false); + // Ignore releases of modifiers and held layer switches. + if (!record->event.pressed) { + switch (keycode) { + case MODIFIER_KEYCODE_RANGE: + case QK_MOMENTARY ... QK_MOMENTARY_MAX: + case QK_LAYER_TAP_TOGGLE ... QK_LAYER_TAP_TOGGLE_MAX: +# ifndef NO_ACTION_ONESHOT // Ignore one-shot keys. + case QK_ONE_SHOT_MOD ... QK_ONE_SHOT_MOD_MAX: + case QK_ONE_SHOT_LAYER ... QK_ONE_SHOT_LAYER_MAX: +# endif // NO_ACTION_ONESHOT +# ifdef TRI_LAYER_ENABLE // Ignore Tri Layer keys. + case QK_TRI_LAYER_LOWER: + case QK_TRI_LAYER_UPPER: +# endif // TRI_LAYER_ENABLE + return; + case QK_MODS ... QK_MODS_MAX: + if (QK_MODS_GET_BASIC_KEYCODE(keycode) == KC_NO) { + return; + } + break; + case QK_MOD_TAP ... QK_MOD_TAP_MAX: + case QK_LAYER_TAP ... QK_LAYER_TAP_MAX: + if (record->tap.count == 0) { + return; + } + break; + } + } + + flow_tap_prev_keycode = keycode; + flow_tap_prev_time = timer_read32(); +} + +static bool flow_tap_key_if_within_term(keyrecord_t *record) { + const uint16_t keycode = get_record_keycode(record, false); + if (is_mt_or_lt(keycode)) { + const uint32_t idle_time = timer_elapsed32(flow_tap_prev_time); + uint16_t term = get_flow_tap_term(keycode, record, flow_tap_prev_keycode); + if (term > 500) { + term = 500; + } + if (idle_time < 500 && idle_time < term) { + debug_event(record->event); + ac_dprintf(" within flow tap term (%u < %u) considered a tap\n", (int16_t)idle_time, term); + record->tap.count = 1; + registered_taps_add(record->event.key); + debug_registered_taps(); + process_record(record); + return true; + } + } + return false; +} + // By default, enable Flow Tap for the keys in the main alphas area and Space. // This should work reasonably even if the layout is remapped on the host to an // alt layout or international layout (e.g. Dvorak or AZERTY), where these same |