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 <felix.kuehling@gmail.com>

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)
+    TAP_DEFINE_KEYCODE;
-    uint16_t tapping_keycode = get_record_keycode(&tapping_key, false);
-#    endif
 
     // if tapping
     if (IS_TAPPING_PRESSED()) {
-        // clang-format off
+        if (WITHIN_TAPPING_TERM(event) || MAYBE_RETRO_SHIFTING(event)) {
-        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 (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)
+                        IS_RELEASED(event) && waiting_buffer_typed(event) &&
-#        ifdef PERMISSIVE_HOLD_PER_KEY
+                        TAP_GET_PERMISSIVE_HOLD
-                            && get_permissive_hold(tapping_keycode, &tapping_key)
-#        elif defined(PERMISSIVE_HOLD)
-                            && true
-#        endif
                     )
                     // 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)
+                    || (TAP_GET_RETRO_TAPPING &&
-                    || (
-#            ifdef RETRO_TAPPING_PER_KEY
-                        get_retro_tapping(tapping_keycode, &tapping_key) &&
-#            endif
                         (
                             // Rolled over the two keys.
-                            (
+                            (tapping_key.tap.interrupted == true && (
-                                (
+                                (TAP_IS_LT && TAP_GET_HOLD_ON_OTHER_KEY_PRESS) ||
-                                    false
+                                (TAP_IS_MT && !TAP_GET_IGNORE_MOD_TAP_INTERRUPT)
-#            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
                             )
                             // 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 (TAP_GET_HOLD_ON_OTHER_KEY_PRESS) {
-#        if defined(HOLD_ON_OTHER_KEY_PRESS_PER_KEY)
-                        if (get_hold_on_other_key_press(tapping_keycode, &tapping_key))
-#        endif
-                        {
                             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) || MAYBE_RETRO_SHIFTING(event)) {
-        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 (event.pressed) {
                 if (IS_TAPPING_RECORD(keyp)) {
-//#    ifndef TAPPING_FORCE_HOLD
+                    if (!TAP_GET_TAPPING_FORCE_HOLD && !tapping_key.tap.interrupted && tapping_key.tap.count > 0) {
-#    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) {
                         // 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;

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);
+}