diff options
| author | Pascal Getreuer <50221757+getreuer@users.noreply.github.com> | 2025-04-22 00:59:49 -0700 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2025-04-22 09:59:49 +0200 |
| commit | 73e2ef486ab7acf3763695cb3a3cf691451cdff3 (patch) | |
| tree | 42fa19653935e7e2c7cc720accaf5ba17641d141 /quantum | |
| parent | b5f8f4d6a201e6a67d1ee483fe6d3327d98cb052 (diff) | |
[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')
| -rw-r--r-- | quantum/action.c | 3 | ||||
| -rw-r--r-- | quantum/action_tapping.c | 105 | ||||
| -rw-r--r-- | quantum/action_tapping.h | 3 |
3 files changed, 79 insertions, 32 deletions
diff --git a/quantum/action.c b/quantum/action.c index eb0dbc7022..dd82c9ec99 100644 --- a/quantum/action.c +++ b/quantum/action.c | |||
| @@ -281,6 +281,9 @@ void process_record(keyrecord_t *record) { | |||
| 281 | if (IS_NOEVENT(record->event)) { | 281 | if (IS_NOEVENT(record->event)) { |
| 282 | return; | 282 | return; |
| 283 | } | 283 | } |
| 284 | #ifdef FLOW_TAP_TERM | ||
| 285 | flow_tap_update_last_event(record); | ||
| 286 | #endif // FLOW_TAP_TERM | ||
| 284 | 287 | ||
| 285 | if (!process_record_quantum(record)) { | 288 | if (!process_record_quantum(record)) { |
| 286 | #ifndef NO_ACTION_ONESHOT | 289 | #ifndef NO_ACTION_ONESHOT |
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 @@ | |||
| 6 | #include "action_tapping.h" | 6 | #include "action_tapping.h" |
| 7 | #include "action_util.h" | 7 | #include "action_util.h" |
| 8 | #include "keycode.h" | 8 | #include "keycode.h" |
| 9 | #include "quantum_keycodes.h" | ||
| 9 | #include "timer.h" | 10 | #include "timer.h" |
| 10 | 11 | ||
| 11 | #ifndef NO_ACTION_TAPPING | 12 | #ifndef NO_ACTION_TAPPING |
| @@ -102,10 +103,10 @@ __attribute__((weak)) bool get_hold_on_other_key_press(uint16_t keycode, keyreco | |||
| 102 | # endif | 103 | # endif |
| 103 | 104 | ||
| 104 | # if defined(FLOW_TAP_TERM) | 105 | # if defined(FLOW_TAP_TERM) |
| 105 | static uint32_t last_input = 0; | 106 | static uint32_t flow_tap_prev_time = 0; |
| 106 | static uint16_t prev_keycode = KC_NO; | 107 | static uint16_t flow_tap_prev_keycode = KC_NO; |
| 107 | 108 | ||
| 108 | uint16_t get_flow_tap_term(uint16_t keycode, keyrecord_t *record, uint16_t prev_keycode); | 109 | static bool flow_tap_key_if_within_term(keyrecord_t *record); |
| 109 | # endif // defined(FLOW_TAP_TERM) | 110 | # endif // defined(FLOW_TAP_TERM) |
| 110 | 111 | ||
| 111 | static keyrecord_t tapping_key = {}; | 112 | static keyrecord_t tapping_key = {}; |
| @@ -157,19 +158,6 @@ void action_tapping_process(keyrecord_t record) { | |||
| 157 | } | 158 | } |
| 158 | } | 159 | } |
| 159 | if (IS_EVENT(record.event)) { | 160 | if (IS_EVENT(record.event)) { |
| 160 | # if defined(FLOW_TAP_TERM) | ||
| 161 | const uint16_t keycode = get_record_keycode(&record, false); | ||
| 162 | // Track the previous key press. | ||
| 163 | if (record.event.pressed) { | ||
| 164 | prev_keycode = keycode; | ||
| 165 | } | ||
| 166 | // If there is no unsettled tap-hold key, update last input time. Ignore | ||
| 167 | // mod keys in this update to allow for chording multiple mods for | ||
| 168 | // hotkeys like "Ctrl+Shift+arrow". | ||
| 169 | if (IS_NOEVENT(tapping_key.event) && !IS_MODIFIER_KEYCODE(keycode)) { | ||
| 170 | last_input = timer_read32(); | ||
| 171 | } | ||
| 172 | # endif // defined(FLOW_TAP_TERM) | ||
| 173 | ac_dprintf("\n"); | 161 | ac_dprintf("\n"); |
| 174 | } | 162 | } |
| 175 | } | 163 | } |
| @@ -252,22 +240,8 @@ bool process_tapping(keyrecord_t *keyp) { | |||
| 252 | // into the "pressed" tapping key state | 240 | // into the "pressed" tapping key state |
| 253 | 241 | ||
| 254 | # if defined(FLOW_TAP_TERM) | 242 | # if defined(FLOW_TAP_TERM) |
| 255 | const uint16_t keycode = get_record_keycode(keyp, false); | 243 | if (flow_tap_key_if_within_term(keyp)) { |
| 256 | if (is_mt_or_lt(keycode)) { | 244 | return true; |
| 257 | const uint32_t idle_time = timer_elapsed32(last_input); | ||
| 258 | uint16_t term = get_flow_tap_term(keycode, keyp, prev_keycode); | ||
| 259 | if (term > 500) { | ||
| 260 | term = 500; | ||
| 261 | } | ||
| 262 | if (idle_time < 500 && idle_time < term) { | ||
| 263 | debug_event(keyp->event); | ||
| 264 | ac_dprintf(" within flow tap term (%u < %u) considered a tap\n", (int16_t)idle_time, term); | ||
| 265 | keyp->tap.count = 1; | ||
| 266 | registered_taps_add(keyp->event.key); | ||
| 267 | debug_registered_taps(); | ||
| 268 | process_record(keyp); | ||
| 269 | return true; | ||
| 270 | } | ||
| 271 | } | 245 | } |
| 272 | # endif // defined(FLOW_TAP_TERM) | 246 | # endif // defined(FLOW_TAP_TERM) |
| 273 | 247 | ||
| @@ -582,6 +556,13 @@ bool process_tapping(keyrecord_t *keyp) { | |||
| 582 | return true; | 556 | return true; |
| 583 | } else if (is_tap_record(keyp)) { | 557 | } else if (is_tap_record(keyp)) { |
| 584 | // Sequential tap can be interfered with other tap key. | 558 | // Sequential tap can be interfered with other tap key. |
| 559 | # if defined(FLOW_TAP_TERM) | ||
| 560 | if (flow_tap_key_if_within_term(keyp)) { | ||
| 561 | tapping_key = (keyrecord_t){0}; | ||
| 562 | debug_tapping_key(); | ||
| 563 | return true; | ||
| 564 | } | ||
| 565 | # endif // defined(FLOW_TAP_TERM) | ||
| 585 | ac_dprintf("Tapping: Start with interfering other tap.\n"); | 566 | ac_dprintf("Tapping: Start with interfering other tap.\n"); |
| 586 | tapping_key = *keyp; | 567 | tapping_key = *keyp; |
| 587 | waiting_buffer_scan_tap(); | 568 | waiting_buffer_scan_tap(); |
| @@ -809,6 +790,66 @@ static void waiting_buffer_process_regular(void) { | |||
| 809 | # endif // CHORDAL_HOLD | 790 | # endif // CHORDAL_HOLD |
| 810 | 791 | ||
| 811 | # ifdef FLOW_TAP_TERM | 792 | # ifdef FLOW_TAP_TERM |
| 793 | void flow_tap_update_last_event(keyrecord_t *record) { | ||
| 794 | // Don't update while a tap-hold key is unsettled. | ||
| 795 | if (waiting_buffer_tail != waiting_buffer_head || (tapping_key.event.pressed && tapping_key.tap.count == 0)) { | ||
| 796 | return; | ||
| 797 | } | ||
| 798 | const uint16_t keycode = get_record_keycode(record, false); | ||
| 799 | // Ignore releases of modifiers and held layer switches. | ||
| 800 | if (!record->event.pressed) { | ||
| 801 | switch (keycode) { | ||
| 802 | case MODIFIER_KEYCODE_RANGE: | ||
| 803 | case QK_MOMENTARY ... QK_MOMENTARY_MAX: | ||
| 804 | case QK_LAYER_TAP_TOGGLE ... QK_LAYER_TAP_TOGGLE_MAX: | ||
| 805 | # ifndef NO_ACTION_ONESHOT // Ignore one-shot keys. | ||
| 806 | case QK_ONE_SHOT_MOD ... QK_ONE_SHOT_MOD_MAX: | ||
| 807 | case QK_ONE_SHOT_LAYER ... QK_ONE_SHOT_LAYER_MAX: | ||
| 808 | # endif // NO_ACTION_ONESHOT | ||
| 809 | # ifdef TRI_LAYER_ENABLE // Ignore Tri Layer keys. | ||
| 810 | case QK_TRI_LAYER_LOWER: | ||
| 811 | case QK_TRI_LAYER_UPPER: | ||
| 812 | # endif // TRI_LAYER_ENABLE | ||
| 813 | return; | ||
| 814 | case QK_MODS ... QK_MODS_MAX: | ||
| 815 | if (QK_MODS_GET_BASIC_KEYCODE(keycode) == KC_NO) { | ||
| 816 | return; | ||
| 817 | } | ||
| 818 | break; | ||
| 819 | case QK_MOD_TAP ... QK_MOD_TAP_MAX: | ||
| 820 | case QK_LAYER_TAP ... QK_LAYER_TAP_MAX: | ||
| 821 | if (record->tap.count == 0) { | ||
| 822 | return; | ||
| 823 | } | ||
| 824 | break; | ||
| 825 | } | ||
| 826 | } | ||
| 827 | |||
| 828 | flow_tap_prev_keycode = keycode; | ||
| 829 | flow_tap_prev_time = timer_read32(); | ||
| 830 | } | ||
| 831 | |||
| 832 | static bool flow_tap_key_if_within_term(keyrecord_t *record) { | ||
| 833 | const uint16_t keycode = get_record_keycode(record, false); | ||
| 834 | if (is_mt_or_lt(keycode)) { | ||
| 835 | const uint32_t idle_time = timer_elapsed32(flow_tap_prev_time); | ||
| 836 | uint16_t term = get_flow_tap_term(keycode, record, flow_tap_prev_keycode); | ||
| 837 | if (term > 500) { | ||
| 838 | term = 500; | ||
| 839 | } | ||
| 840 | if (idle_time < 500 && idle_time < term) { | ||
| 841 | debug_event(record->event); | ||
| 842 | ac_dprintf(" within flow tap term (%u < %u) considered a tap\n", (int16_t)idle_time, term); | ||
| 843 | record->tap.count = 1; | ||
| 844 | registered_taps_add(record->event.key); | ||
| 845 | debug_registered_taps(); | ||
| 846 | process_record(record); | ||
| 847 | return true; | ||
| 848 | } | ||
| 849 | } | ||
| 850 | return false; | ||
| 851 | } | ||
| 852 | |||
| 812 | // By default, enable Flow Tap for the keys in the main alphas area and Space. | 853 | // By default, enable Flow Tap for the keys in the main alphas area and Space. |
| 813 | // This should work reasonably even if the layout is remapped on the host to an | 854 | // This should work reasonably even if the layout is remapped on the host to an |
| 814 | // alt layout or international layout (e.g. Dvorak or AZERTY), where these same | 855 | // alt layout or international layout (e.g. Dvorak or AZERTY), where these same |
diff --git a/quantum/action_tapping.h b/quantum/action_tapping.h index 2af000ad73..0cf4aa1200 100644 --- a/quantum/action_tapping.h +++ b/quantum/action_tapping.h | |||
| @@ -166,6 +166,9 @@ bool is_flow_tap_key(uint16_t keycode); | |||
| 166 | * @return Time in milliseconds. | 166 | * @return Time in milliseconds. |
| 167 | */ | 167 | */ |
| 168 | uint16_t get_flow_tap_term(uint16_t keycode, keyrecord_t *record, uint16_t prev_keycode); | 168 | uint16_t get_flow_tap_term(uint16_t keycode, keyrecord_t *record, uint16_t prev_keycode); |
| 169 | |||
| 170 | /** Updates the Flow Tap last key and timer. */ | ||
| 171 | void flow_tap_update_last_event(keyrecord_t *record); | ||
| 169 | #endif // FLOW_TAP_TERM | 172 | #endif // FLOW_TAP_TERM |
| 170 | 173 | ||
| 171 | #ifdef DYNAMIC_TAPPING_TERM_ENABLE | 174 | #ifdef DYNAMIC_TAPPING_TERM_ENABLE |
