Revise to allow combining multiple same-hand mods.

This commit revises Chordal Hold as described in discussion in
#24560 (comment)

1. In "RCTL_T(KC_A)↓, RSFT_T(KC_C)↓, RCTL_T(KC_A)↑" before the tapping
   term, RCTL_T(KC_A) is settled as tapped.
2. In "RCTL_T(KC_A)↓, RSFT_T(KC_C)↓, RSFT_T(KC_C)↑", both RCTL_T(KC_A)
   and RSFT_T(KC_C) are settled as tapped.
3. In "RCTL_T(KC_A)↓, RSFT_T(KC_C)↓, KC_U↓" (all keys on the same side),
   both RCTL_T(KC_A) and RSFT_T(KC_C) are settled as tapped.
4. In "RCTL_T(KC_A)↓, RSFT_T(KC_C)↓, LSFT_T(KC_T)↓", with the third key
   on the other side, we allow Permissive Hold or Hold On Other Keypress
   to decide how/when to settle the keys.
5. In "RCTL_T(KC_A)↓, RSFT_T(KC_C)↓" held until the tapping term, the
   keys are settled as held.

1–3 provide same-hand roll protection. 4–5 are for combining multiple
same-hand modifiers.

I've updated the unit tests and have been running it on my keyboard, for
a few hours so far, and all seems good. I really like this scheme. It
allows combining same-side mods, yet it also has roll protection on
streaks. For me, this feels like Achordion, but clearly better streak
handling and improved responsiveness.

docs/tap_hold.md

@@ -587,86 +587,6 @@ As shown in the last line above, you may use
 `get_chordal_hold_default(tap_hold_record, other_record)` to get the default tap
 vs. hold decision according to the opposite hands rule.
 
-If you use home row mods, you may want to produce a hotkey like Ctrl+Shift+V by
-holding Ctrl and Shift mod-taps on one hand while tapping `KC_V` with the other
-hand, say:
-
-- `RCTL_T(KC_K)` Down
-- `RSFT_T(KC_L)` Down (on the same hand as `RCTL_T(KC_K)`)
-- `KC_V` Down
-- `KC_V` Up
-- `RCTL_T(KC_K)` Up
-- `RSFT_T(KC_L)` Up
-
-However, supposing `RCTL_T(KC_K)` and `RSFT_T(KC_L)` are on the same hand,
-Chordal Hold by default considers `RCTL_T(KC_K)` tapped, producing "`kV`"
-instead of the desired Ctrl+Shift+V. 
-
-To address this, `get_chordal_hold()` may be defined to allow chords between any
-pair of mod-tap keys with
-
-```c
-bool get_chordal_hold(uint16_t tap_hold_keycode, keyrecord_t* tap_hold_record,
-                      uint16_t other_keycode, keyrecord_t* other_record) {
-    // Allow hold between any pair of mod-tap keys.
-    if (IS_QK_MOD_TAP(tap_hold_keycode) && IS_QK_MOD_TAP(other_keycode)) {
-        return true;
-    }
-
-    // Otherwise defer to the opposite hands rule.
-    return get_chordal_hold_default(tap_hold_record, other_record);
-}
-```
-
-Or to allow one-handed chords of specific mod-taps but not others, use:
-
-```c
-bool get_chordal_hold(uint16_t tap_hold_keycode, keyrecord_t* tap_hold_record,
-                      uint16_t other_keycode, keyrecord_t* other_record) {
-    switch (tap_hold_keycode) {
-        case RCTL_T(KC_K):  
-            if (other_keycode == RSFT_T(KC_L)) {
-                // Allow hold in "RCTL_T(KC_K) down, RSFT_T(KC_L) down".
-                return true;
-            }
-            break;
-
-        case RSFT_T(KC_L):
-            if (other_keycode == RCTL_T(KC_K)) {
-                // Allow hold in "RSFT_T(KC_L) down, RCTL_T(KC_K) down".
-                return true;
-            }
-            break;
-    }
-    // Otherwise defer to the opposite hands rule.
-    return get_chordal_hold_default(tap_hold_record, other_record);
-}
-```
-
-Above, two exceptions are defined, one where `RCTL_T(KC_K)` is pressed first and
-another where `RSFT_T(KC_L)` is held first, such that Ctrl+Shift+V could be done
-by holding the mod-taps in either order. For yet finer control, you could choose
-to define an exception for one order but not the other:
-
-```c
-bool get_chordal_hold(uint16_t tap_hold_keycode, keyrecord_t* tap_hold_record,
-                      uint16_t other_keycode, keyrecord_t* other_record) {
-    switch (tap_hold_keycode) {
-        case RCTL_T(KC_K):  
-            if (other_keycode == RSFT_T(KC_L)) {
-                // Allow hold in "RCTL_T(KC_K) down, RSFT_T(KC_L), down".
-                return true;
-            }
-            break;
-
-        // ... but RSFT_T(KC_L) is considered tapped in 
-        // "RSFT_T(KC_L) down, RCTL_T(KC_K) down".
-    }
-    // Otherwise defer to the opposite hands rule.
-    return get_chordal_hold_default(tap_hold_record, other_record);
-}
-```
-
 
 ## Retro Tapping
 

quantum/action_tapping.c

@@ -49,7 +49,7 @@ __attribute__((weak)) bool get_permissive_hold(uint16_t keycode, keyrecord_t *re
 }
 #    endif
 
-#    ifdef CHORDAL_HOLD
+#    if defined(CHORDAL_HOLD)
 extern const char chordal_hold_layout[MATRIX_ROWS][MATRIX_COLS] PROGMEM;
 
 #        define REGISTERED_TAPS_SIZE 8
@@ -65,11 +65,23 @@ static int8_t registered_tap_find(keypos_t key);
 static void registered_taps_del_index(uint8_t i);
 /** Logs the registered_taps array for debugging. */
 static void debug_registered_taps(void);
-/** Processes and pops buffered events until the first tap-hold event. */
-static void waiting_buffer_process_regular(void);
 
-static bool is_one_shot(uint16_t keycode) {
-    return IS_QK_ONE_SHOT_MOD(keycode) || IS_QK_ONE_SHOT_LAYER(keycode);
+/** \brief Finds which queued events should be held according to Chordal Hold.
+ *
+ * In a situation with multiple unsettled tap-hold key presses, scan the queue
+ * up until the first release, non-tap-hold, or one-shot event and find the
+ * lastest event in the queue that settles as held according to
+ * get_chordal_hold().
+ *
+ * \return Index of the first tap, or equivalently, one past the latest hold.
+ */
+static uint8_t waiting_buffer_find_chordal_hold_tap(void);
+
+/** Processes queued events up to and including `key` as tapped. */
+static void waiting_buffer_chordal_hold_taps_until(keypos_t key);
+
+static bool is_mt_or_lt(uint16_t keycode) {
+    return IS_QK_MOD_TAP(keycode) || IS_QK_LAYER_TAP(keycode);
 }
 #    endif // CHORDAL_HOLD
 
@@ -190,15 +202,15 @@ void action_tapping_process(keyrecord_t record) {
 bool process_tapping(keyrecord_t *keyp) {
     const keyevent_t event = keyp->event;
 
-#    ifdef CHORDAL_HOLD
+#    if defined(CHORDAL_HOLD)
     if (!event.pressed) {
         const int8_t i = registered_tap_find(event.key);
         if (i != -1) {
             // If a tap-hold key was previously settled as tapped, set its
             // tap.count correspondingly on release.
             keyp->tap.count = 1;
             registered_taps_del_index(i);
-            ac_dprintf("CHORDAL_HOLD: Found tap release for [%d]\n", i);
+            ac_dprintf("Found tap release for [%d]\n", i);
             debug_registered_taps();
         }
     }
@@ -251,6 +263,25 @@ bool process_tapping(keyrecord_t *keyp) {
                     // enqueue
                     return false;
                 }
+#    if defined(CHORDAL_HOLD)
+                else if (is_mt_or_lt(tapping_keycode) && !event.pressed && waiting_buffer_typed(event) && !get_chordal_hold(tapping_keycode, &tapping_key, get_record_keycode(keyp, false), keyp)) {
+                    // Key release that is not a chord with the tapping key.
+                    // Settle the tapping key and any other pending tap-hold
+                    // keys preceding the press of this key as tapped.
+
+                    ac_dprintf("Tapping: End. Chord considered a tap\n");
+                    tapping_key.tap.count = 1;
+                    registered_taps_add(tapping_key.event.key);
+                    process_record(&tapping_key);
+                    tapping_key = (keyrecord_t){0};
+
+                    waiting_buffer_chordal_hold_taps_until(event.key);
+                    debug_registered_taps();
+                    debug_waiting_buffer();
+                    // enqueue
+                    return false;
+                }
+#    endif      // CHORDAL_HOLD
                 /* Process a key typed within TAPPING_TERM
                  * This can register the key before settlement of tapping,
                  * useful for long TAPPING_TERM but may prevent fast typing.
@@ -268,6 +299,22 @@ bool process_tapping(keyrecord_t *keyp) {
                     // clang-format on
                     ac_dprintf("Tapping: End. No tap. Interfered by typing key\n");
                     process_record(&tapping_key);
+
+#    if defined(CHORDAL_HOLD)
+                    uint8_t first_tap = waiting_buffer_find_chordal_hold_tap();
+                    ac_dprintf("first_tap = %u\n", first_tap);
+                    if (first_tap < WAITING_BUFFER_SIZE) {
+                        for (; waiting_buffer_tail != first_tap; waiting_buffer_tail = (waiting_buffer_tail + 1) % WAITING_BUFFER_SIZE) {
+                            ac_dprintf("Processing [%u]\n", waiting_buffer_tail);
+                            process_record(&waiting_buffer[waiting_buffer_tail]);
+                        }
+                    }
+
+                    waiting_buffer_chordal_hold_taps_until(event.key);
+                    debug_registered_taps();
+                    debug_waiting_buffer();
+#    endif // CHORDAL_HOLD
+
                     tapping_key = (keyrecord_t){0};
                     debug_tapping_key();
                     // enqueue
@@ -325,47 +372,46 @@ bool process_tapping(keyrecord_t *keyp) {
                         tapping_key.tap.interrupted = true;
 
 #    if defined(CHORDAL_HOLD)
-                        if (!is_one_shot(tapping_keycode) && !get_chordal_hold(tapping_keycode, &tapping_key, get_record_keycode(keyp, false), keyp)) {
+                        if (is_mt_or_lt(tapping_keycode) && !get_chordal_hold(tapping_keycode, &tapping_key, get_record_keycode(keyp, false), keyp)) {
                             // In process_action(), HOLD_ON_OTHER_KEY_PRESS
                             // will revert interrupted events to holds, so
                             // this needs to be set false.
                             tapping_key.tap.interrupted = false;
 
-                            ac_dprintf("Tapping: End. Chord considered a tap\n");
-                            tapping_key.tap.count = 1;
-                            registered_taps_add(tapping_key.event.key);
-                            debug_registered_taps();
-                            process_record(&tapping_key);
-                            tapping_key = (keyrecord_t){0};
-
-                            // Process regular keys in the waiting buffer.
-                            waiting_buffer_process_regular();
+                            if (!is_tap_record(keyp)) {
+                                ac_dprintf("Tapping: End. Chord considered a tap\n");
+                                tapping_key.tap.count = 1;
+                                registered_taps_add(tapping_key.event.key);
+                                debug_registered_taps();
+                                process_record(&tapping_key);
+                                tapping_key = (keyrecord_t){0};
+                            }
                         } else
-#    endif
+#    endif // CHORDAL_HOLD
                             if (TAP_GET_HOLD_ON_OTHER_KEY_PRESS
 #    if defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT)
                                 // Auto Shift cannot evaluate this early
                                 // Retro Shift uses the hold action for all nested taps even without HOLD_ON_OTHER_KEY_PRESS, so this is fine to skip
                                 && !(MAYBE_RETRO_SHIFTING(event, keyp) && get_auto_shifted_key(get_record_keycode(keyp, false), keyp))
 #    endif
                             ) {
-                            // Settle the tapping key as *held*, since
-                            // HOLD_ON_OTHER_KEY_PRESS is enabled for this key.
-                            ac_dprintf("Tapping: End. No tap. Interfered by pressed key\n");
-                            process_record(&tapping_key);
+                                // Settle the tapping key as *held*, since
+                                // HOLD_ON_OTHER_KEY_PRESS is enabled for this key.
+                                ac_dprintf("Tapping: End. No tap. Interfered by pressed key\n");
+                                process_record(&tapping_key);
 
-#    ifdef CHORDAL_HOLD
-                            if (waiting_buffer_tail != waiting_buffer_head && is_tap_record(&waiting_buffer[waiting_buffer_tail])) {
-                                tapping_key = waiting_buffer[waiting_buffer_tail];
-                                // Pop tail from the queue.
-                                waiting_buffer_tail = (waiting_buffer_tail + 1) % WAITING_BUFFER_SIZE;
-                            } else
-#    endif
-                            {
-                                tapping_key = (keyrecord_t){0};
+#    if defined(CHORDAL_HOLD)
+                                if (waiting_buffer_tail != waiting_buffer_head && is_tap_record(&waiting_buffer[waiting_buffer_tail])) {
+                                    tapping_key = waiting_buffer[waiting_buffer_tail];
+                                    // Pop tail from the queue.
+                                    waiting_buffer_tail = (waiting_buffer_tail + 1) % WAITING_BUFFER_SIZE;
+                                } else
+#    endif // CHORDAL_HOLD
+                                {
+                                    tapping_key = (keyrecord_t){0};
+                                }
+                                debug_tapping_key();
                             }
-                            debug_tapping_key();
-                        }
                     }
                     // enqueue
                     return false;
@@ -660,16 +706,39 @@ static void debug_registered_taps(void) {
     ac_dprintf("}\n");
 }
 
-static void waiting_buffer_process_regular(void) {
-    for (; waiting_buffer_tail != waiting_buffer_head; waiting_buffer_tail = (waiting_buffer_tail + 1) % WAITING_BUFFER_SIZE) {
-        if (is_tap_record(&waiting_buffer[waiting_buffer_tail])) {
-            break; // Stop once a tap-hold key event is reached.
+static uint8_t waiting_buffer_find_chordal_hold_tap(void) {
+    keyrecord_t *prev         = &tapping_key;
+    uint16_t     prev_keycode = get_record_keycode(&tapping_key, false);
+    uint8_t      first_tap    = WAITING_BUFFER_SIZE;
+    for (uint8_t i = waiting_buffer_tail; i != waiting_buffer_head; i = (i + 1) % WAITING_BUFFER_SIZE) {
+        keyrecord_t   *cur         = &waiting_buffer[i];
+        const uint16_t cur_keycode = get_record_keycode(cur, false);
+        if (!cur->event.pressed || !is_tap_record(prev) || !is_mt_or_lt(prev_keycode)) {
+            break;
+        } else if (get_chordal_hold(prev_keycode, prev, cur_keycode, cur)) {
+            first_tap = i; // Track one index past the latest hold.
         }
-        ac_dprintf("waiting_buffer_process_regular: processing [%u]\n", waiting_buffer_tail);
-        process_record(&waiting_buffer[waiting_buffer_tail]);
+        prev         = cur;
+        prev_keycode = cur_keycode;
     }
+    return first_tap;
+}
 
-    debug_waiting_buffer();
+static void waiting_buffer_chordal_hold_taps_until(keypos_t key) {
+    while (waiting_buffer_tail != waiting_buffer_head) {
+        keyrecord_t *record = &waiting_buffer[waiting_buffer_tail];
+        ac_dprintf("waiting_buffer_chordal_hold_taps_until: processing [%u]\n", waiting_buffer_tail);
+        if (is_tap_record(record)) {
+            record->tap.count = 1;
+            registered_taps_add(record->event.key);
+        }
+        process_record(record);
+        waiting_buffer_tail = (waiting_buffer_tail + 1) % WAITING_BUFFER_SIZE;
+
+        if (KEYEQ(key, record->event.key) && record->event.pressed) {
+            break;
+        }
+    }
 }
 #    endif // CHORDAL_HOLD
 

tests/tap_hold_configurations/chordal_hold/default/config.h

@@ -0,0 +1,21 @@
+/* Copyright 2022 Vladislav Kucheriavykh
+ * Copyright 2024 Google LLC
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include "test_common.h"
+#define CHORDAL_HOLD

tests/tap_hold_configurations/chordal_hold/default/test.mk

@@ -0,0 +1,17 @@
+# Copyright 2022 Vladislav Kucheriavykh
+# Copyright 2024 Google LLC
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+INTROSPECTION_KEYMAP_C = test_keymap.c

tests/tap_hold_configurations/chordal_hold/default/test_keymap.c

@@ -0,0 +1,22 @@
+// Copyright 2024 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "quantum.h"
+
+const char chordal_hold_layout[MATRIX_ROWS][MATRIX_COLS] PROGMEM = {
+    {'L', 'L', 'L', 'L', 'L', 'R', 'R', 'R', 'R', 'R'},
+    {'L', 'L', 'L', 'L', 'L', 'R', 'R', 'R', 'R', 'R'},
+    {'*', 'L', 'L', 'L', 'L', 'R', 'R', 'R', 'R', 'R'},
+    {'L', 'L', 'L', 'L', 'L', 'R', 'R', 'R', 'R', 'R'},
+};