From 1cbd6edf9b6e377b6af48d37954ad16135594e39 Mon Sep 17 00:00:00 2001 From: Felix Kuehling <67731127+fxkuehl@users.noreply.github.com> Date: Mon, 28 Nov 2022 03:16:38 -0500 Subject: [PATCH] Bug17281 - Retain momentary layers until the end of tapping (#17282) * Make process_tapping more readable Move most #ifdefs into conditionally defined macros to make the logic easier to follow. * Retain momentary layers until the end of tapping This allows mod-tap and layer-tap keys on layers to behave as expected. Bug: https://github.com/qmk/qmk_firmware/issues/17281 * Add tests for delayed mod/layer release while tapping Mods and layer key release is delayed while tapping is in progress to ensure that the tap is registered with the modifier state and on the layer where the key was first pressed. Signed-off-by: Felix Kuehling --- quantum/action_tapping.c | 161 ++++++++++++++++++----------------- tests/basic/test_tapping.cpp | 69 +++++++++++++++ 2 files changed, 153 insertions(+), 77 deletions(-) diff --git a/quantum/action_tapping.c b/quantum/action_tapping.c index df3317ac05b2..b5386a5e1749 100644 --- a/quantum/action_tapping.c +++ b/quantum/action_tapping.c @@ -117,6 +117,66 @@ void action_tapping_process(keyrecord_t record) { } } +/* Some conditionally defined helper macros to keep process_tapping more + * readable. The conditional definition of tapping_keycode and all the + * conditional uses of it are hidden inside macros named TAP_... + */ +# if (defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT)) || defined(PERMISSIVE_HOLD_PER_KEY) || defined(TAPPING_FORCE_HOLD_PER_KEY) || defined(HOLD_ON_OTHER_KEY_PRESS_PER_KEY) +# define TAP_DEFINE_KEYCODE uint16_t tapping_keycode = get_record_keycode(&tapping_key, false) +# else +# define TAP_DEFINE_KEYCODE +# endif + +# if defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT) +# ifdef RETRO_TAPPING_PER_KEY +# define TAP_GET_RETRO_TAPPING get_retro_tapping(tapping_keycode, &tapping_key) +# else +# define TAP_GET_RETRO_TAPPING true +# endif +# define MAYBE_RETRO_SHIFTING(ev) (TAP_GET_RETRO_TAPPING && (RETRO_SHIFT + 0) != 0 && TIMER_DIFF_16((ev).time, tapping_key.event.time) < (RETRO_SHIFT + 0)) +# define TAP_IS_LT IS_LT(tapping_keycode) +# define TAP_IS_MT IS_MT(tapping_keycode) +# define TAP_IS_RETRO IS_RETRO(tapping_keycode) +# else +# define TAP_GET_RETRO_TAPPING false +# define MAYBE_RETRO_SHIFTING(ev) false +# define TAP_IS_LT false +# define TAP_IS_MT false +# define TAP_IS_RETRO false +# endif + +# ifdef PERMISSIVE_HOLD_PER_KEY +# define TAP_GET_PERMISSIVE_HOLD get_permissive_hold(tapping_keycode, &tapping_key) +# elif defined(PERMISSIVE_HOLD) +# define TAP_GET_PERMISSIVE_HOLD true +# else +# define TAP_GET_PERMISSIVE_HOLD false +# endif + +# ifdef HOLD_ON_OTHER_KEY_PRESS_PER_KEY +# define TAP_GET_HOLD_ON_OTHER_KEY_PRESS get_hold_on_other_key_press(tapping_keycode, &tapping_key) +# elif defined(HOLD_ON_OTHER_KEY_PRESS) +# define TAP_GET_HOLD_ON_OTHER_KEY_PRESS true +# else +# define TAP_GET_HOLD_ON_OTHER_KEY_PRESS false +# endif + +# ifdef IGNORE_MOD_TAP_INTERRUPT_PER_KEY +# define TAP_GET_IGNORE_MOD_TAP_INTERRUPT get_ignore_mod_tap_interrupt(tapping_keycode, &tapping_key) +# elif defined(IGNORE_MOD_TAP_INTERRUPT) +# define TAP_GET_IGNORE_MOD_TAP_INTERRUPT true +# else +# define TAP_GET_IGNORE_MOD_TAP_INTERRUPT false +# endif + +# ifdef TAPPING_FORCE_HOLD_PER_KEY +# define TAP_GET_TAPPING_FORCE_HOLD get_tapping_force_hold(tapping_keycode, &tapping_key) +# elif defined(TAPPING_FORCE_HOLD) +# define TAP_GET_TAPPING_FORCE_HOLD true +# else +# define TAP_GET_TAPPING_FORCE_HOLD false +# endif + /** \brief Tapping * * Rule: Tap key is typed(pressed and released) within TAPPING_TERM. @@ -125,24 +185,11 @@ void action_tapping_process(keyrecord_t record) { /* return true when key event is processed or consumed. */ bool process_tapping(keyrecord_t *keyp) { keyevent_t event = keyp->event; -# if (defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT)) || defined(PERMISSIVE_HOLD_PER_KEY) || defined(TAPPING_FORCE_HOLD_PER_KEY) || defined(HOLD_ON_OTHER_KEY_PRESS_PER_KEY) - uint16_t tapping_keycode = get_record_keycode(&tapping_key, false); -# endif + TAP_DEFINE_KEYCODE; // if tapping if (IS_TAPPING_PRESSED()) { - // clang-format off - if (WITHIN_TAPPING_TERM(event) -# if defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT) - || ( -# ifdef RETRO_TAPPING_PER_KEY - get_retro_tapping(tapping_keycode, &tapping_key) && -# endif - (RETRO_SHIFT + 0) != 0 && TIMER_DIFF_16(event.time, tapping_key.event.time) < (RETRO_SHIFT + 0) - ) -# endif - ) { - // clang-format on + if (WITHIN_TAPPING_TERM(event) || MAYBE_RETRO_SHIFTING(event)) { if (tapping_key.tap.count == 0) { if (IS_TAPPING_RECORD(keyp) && !event.pressed) { # if defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT) @@ -164,57 +211,31 @@ bool process_tapping(keyrecord_t *keyp) { * useful for long TAPPING_TERM but may prevent fast typing. */ // clang-format off -# if defined(PERMISSIVE_HOLD) || defined(PERMISSIVE_HOLD_PER_KEY) || (defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT)) else if ( ( - IS_RELEASED(event) && waiting_buffer_typed(event) -# ifdef PERMISSIVE_HOLD_PER_KEY - && get_permissive_hold(tapping_keycode, &tapping_key) -# elif defined(PERMISSIVE_HOLD) - && true -# endif + IS_RELEASED(event) && waiting_buffer_typed(event) && + TAP_GET_PERMISSIVE_HOLD ) // Causes nested taps to not wait past TAPPING_TERM/RETRO_SHIFT // unnecessarily and fixes them for Layer Taps. -# if defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT) - || ( -# ifdef RETRO_TAPPING_PER_KEY - get_retro_tapping(tapping_keycode, &tapping_key) && -# endif + || (TAP_GET_RETRO_TAPPING && ( // Rolled over the two keys. - ( - ( - false -# if defined(HOLD_ON_OTHER_KEY_PRESS) || defined(HOLD_ON_OTHER_KEY_PRESS_PER_KEY) - || ( - IS_LT(tapping_keycode) -# ifdef HOLD_ON_OTHER_KEY_PRESS_PER_KEY - && get_hold_on_other_key_press(tapping_keycode, &tapping_key) -# endif - ) -# endif -# if !defined(IGNORE_MOD_TAP_INTERRUPT) || defined(IGNORE_MOD_TAP_INTERRUPT_PER_KEY) - || ( - IS_MT(tapping_keycode) -# ifdef IGNORE_MOD_TAP_INTERRUPT_PER_KEY - && !get_ignore_mod_tap_interrupt(tapping_keycode, &tapping_key) -# endif - ) -# endif - ) && tapping_key.tap.interrupted == true + (tapping_key.tap.interrupted == true && ( + (TAP_IS_LT && TAP_GET_HOLD_ON_OTHER_KEY_PRESS) || + (TAP_IS_MT && !TAP_GET_IGNORE_MOD_TAP_INTERRUPT) + ) ) // Makes Retro Shift ignore [IGNORE_MOD_TAP_INTERRUPT's // effects on nested taps for MTs and the default // behavior of LTs] below TAPPING_TERM or RETRO_SHIFT. || ( - IS_RETRO(tapping_keycode) + TAP_IS_RETRO && (event.key.col != tapping_key.event.key.col || event.key.row != tapping_key.event.key.row) && IS_RELEASED(event) && waiting_buffer_typed(event) ) ) ) -# endif ) { // clang-format on debug("Tapping: End. No tap. Interfered by typing key\n"); @@ -224,13 +245,12 @@ bool process_tapping(keyrecord_t *keyp) { // enqueue return false; } -# endif /* Process release event of a key pressed before tapping starts * Without this unexpected repeating will occur with having fast repeating setting * https://github.com/tmk/tmk_keyboard/issues/60 */ else if (IS_RELEASED(event) && !waiting_buffer_typed(event)) { - // Modifier should be retained till end of this tapping. + // Modifier/Layer should be retained till end of this tapping. action_t action = layer_switch_get_action(event.key); switch (action.kind.id) { case ACT_LMODS: @@ -243,6 +263,16 @@ bool process_tapping(keyrecord_t *keyp) { if (action.key.mods && keyp->tap.count == 0) return false; if (IS_MOD(action.key.code)) return false; break; + case ACT_LAYER_TAP: + case ACT_LAYER_TAP_EXT: + switch (action.layer_tap.code) { + case 0 ...(OP_TAP_TOGGLE - 1): + case OP_ON_OFF: + case OP_OFF_ON: + case OP_SET_CLEAR: + return false; + } + break; } // Release of key should be process immediately. debug("Tapping: release event of a key pressed before tapping\n"); @@ -252,11 +282,7 @@ bool process_tapping(keyrecord_t *keyp) { // set interrupted flag when other key preesed during tapping if (event.pressed) { tapping_key.tap.interrupted = true; -# if defined(HOLD_ON_OTHER_KEY_PRESS) || defined(HOLD_ON_OTHER_KEY_PRESS_PER_KEY) -# if defined(HOLD_ON_OTHER_KEY_PRESS_PER_KEY) - if (get_hold_on_other_key_press(tapping_keycode, &tapping_key)) -# endif - { + if (TAP_GET_HOLD_ON_OTHER_KEY_PRESS) { debug("Tapping: End. No tap. Interfered by pressed key\n"); process_record(&tapping_key); tapping_key = (keyrecord_t){}; @@ -264,7 +290,6 @@ bool process_tapping(keyrecord_t *keyp) { // enqueue return false; } -# endif } // enqueue return false; @@ -357,27 +382,10 @@ bool process_tapping(keyrecord_t *keyp) { } } } else if (IS_TAPPING_RELEASED()) { - // clang-format off - if (WITHIN_TAPPING_TERM(event) -# if defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT) - || ( -# ifdef RETRO_TAPPING_PER_KEY - get_retro_tapping(tapping_keycode, &tapping_key) && -# endif - (RETRO_SHIFT + 0) != 0 && TIMER_DIFF_16(event.time, tapping_key.event.time) < (RETRO_SHIFT + 0) - ) -# endif - ) { - // clang-format on + if (WITHIN_TAPPING_TERM(event) || MAYBE_RETRO_SHIFTING(event)) { if (event.pressed) { if (IS_TAPPING_RECORD(keyp)) { -//# ifndef TAPPING_FORCE_HOLD -# if !defined(TAPPING_FORCE_HOLD) || defined(TAPPING_FORCE_HOLD_PER_KEY) - if ( -# ifdef TAPPING_FORCE_HOLD_PER_KEY - !get_tapping_force_hold(tapping_keycode, &tapping_key) && -# endif - !tapping_key.tap.interrupted && tapping_key.tap.count > 0) { + if (!TAP_GET_TAPPING_FORCE_HOLD && !tapping_key.tap.interrupted && tapping_key.tap.count > 0) { // sequential tap. keyp->tap = tapping_key.tap; if (keyp->tap.count < 15) keyp->tap.count += 1; @@ -389,7 +397,6 @@ bool process_tapping(keyrecord_t *keyp) { debug_tapping_key(); return true; } -# endif // FIX: start new tap again tapping_key = *keyp; return true; diff --git a/tests/basic/test_tapping.cpp b/tests/basic/test_tapping.cpp index 6ff9cfe22b2b..faf9b9fe9136 100644 --- a/tests/basic/test_tapping.cpp +++ b/tests/basic/test_tapping.cpp @@ -121,3 +121,72 @@ TEST_F(Tapping, ANewTapWithinTappingTermIsBuggy) { key_shift_hold_p_tap.release(); run_one_scan_loop(); } + +TEST_F(Tapping, TapA_CTL_T_KeyWhileReleasingShift) { + TestDriver driver; + InSequence s; + auto shift_key = KeymapKey(0, 7, 0, KC_LSFT); + auto mod_tap_hold_key = KeymapKey(0, 8, 0, CTL_T(KC_P)); + + set_keymap({shift_key, mod_tap_hold_key}); + + shift_key.press(); + // Shift is reported + EXPECT_REPORT(driver, (KC_LSFT)); + run_one_scan_loop(); + testing::Mock::VerifyAndClearExpectations(&driver); + + mod_tap_hold_key.press(); + // Tapping keys does nothing on press + EXPECT_NO_REPORT(driver); + run_one_scan_loop(); + + shift_key.release(); + // Releasing shift is delayed while tapping is in progress + EXPECT_NO_REPORT(driver); + run_one_scan_loop(); + testing::Mock::VerifyAndClearExpectations(&driver); + + mod_tap_hold_key.release(); + // Releasing mod-tap key reports the tap and releases shift + EXPECT_REPORT(driver, (KC_LSFT, KC_P)); + EXPECT_REPORT(driver, (KC_P)); + EXPECT_EMPTY_REPORT(driver); + run_one_scan_loop(); + testing::Mock::VerifyAndClearExpectations(&driver); +} + +TEST_F(Tapping, TapA_CTL_T_KeyWhileReleasingLayer) { + TestDriver driver; + InSequence s; + auto layer_key = KeymapKey(0, 7, 0, MO(1)); + auto trans_key = KeymapKey(1, 7, 0, KC_TRNS); + auto mod_tap_hold_key0 = KeymapKey(0, 8, 0, CTL_T(KC_P)); + auto mod_tap_hold_key1 = KeymapKey(1, 8, 0, CTL_T(KC_Q)); + + set_keymap({layer_key, trans_key, mod_tap_hold_key0, mod_tap_hold_key1}); + + layer_key.press(); + // Pressing the layer key does nothing + EXPECT_NO_REPORT(driver); + run_one_scan_loop(); + + mod_tap_hold_key1.press(); + // Tapping layer 1 mod-tap key does nothing on press + EXPECT_NO_REPORT(driver); + run_one_scan_loop(); + + layer_key.release(); + // Releasing layer is delayed while tapping is in progress + EXPECT_NO_REPORT(driver); + run_one_scan_loop(); + testing::Mock::VerifyAndClearExpectations(&driver); + + mod_tap_hold_key1.release(); + // Releasing mod-tap key reports the tap of the layer 1 key + // If delayed layer release is broken, this reports the layer 0 key + EXPECT_REPORT(driver, (KC_Q)); + EXPECT_EMPTY_REPORT(driver); + run_one_scan_loop(); + testing::Mock::VerifyAndClearExpectations(&driver); +}