feat: add tap-hold-keys action with named key list options

[malpern]

Summary

• Adds new tap-hold-keys action with named optional list parameters: (tap-on-press ...), (tap-on-press-release ...), (hold-on-press ...)
• Addresses Feature request: combined tap-hold-press + tap-hold-release-tap-keys-release jtroo/kanata#1985 — allows specific keys to trigger hold immediately on press, while keeping PermissiveHold behavior for unlisted keys
• Design follows jtroo's guidance to use named list params instead of more positional args

Changes

• parser/src/cfg/list_actions.rs — new TAP_HOLD_KEYS constant
• parser/src/cfg/mod.rs — dispatch to new parser
• parser/src/cfg/tap_hold.rs — parser for named list options
• parser/src/cfg/custom_tap_hold.rs — closure with tap/hold-on-press logic
• src/tests/sim_tests/tap_hold_tests.rs — 6 new sim tests
• docs/config.adoc + cfg_samples/kanata.kbd — docs and example

Test plan

• All 222 sim tests pass (cargo test --features=simulated_output)
• cargo check clean
• CI passes on this PR

🤖 Generated with Claude Code

[claude]

Code Review: tap-hold-keys

This is a clean, well-structured addition that follows the existing codebase patterns. The design (named list options instead of positional args) is the right call and matches jtroo's guidance. Tests cover the main scenarios. Below are my findings, grouped by priority.

---

🐛 Potential Logic Issue — tap-on-press-release key falls through to PermissiveHold

In custom_tap_hold.rs, when a key is in tap-on-press-release but hasn't been released yet, the code falls through to the PermissiveHold check at the bottom of the loop body:

if keys_tap_on_press_release.iter().copied().any(|j2| j2 == j) {
    let target = Event::Release(i, j);
    if queued.clone().copied().any(|q| q.event() == target) {
        return (Some(WaitingAction::Tap), false);
    }
    // *** falls through here if not yet released ***
}
// PermissiveHold check (also checks Release(i, j))
let target = Event::Release(i, j);
if queued.clone().copied().any(|q| q.event() == target) {
    return (Some(WaitingAction::Hold), false);
}

The PermissiveHold check is also looking for Release(i, j), so if the tap-on-press-release key hasn't been released, it won't trigger Hold either — the fallthrough is harmless for that key. But consider this queue:

[Press(B), Press(C), Release(C)]  // B is in tap-on-press-release, C is unlisted

• Iteration on Press(B): B in list, no release yet → fall through → PermissiveHold check for Release(B) → not found → skip
• Iteration on Press(C): not in any list → PermissiveHold for Release(C) → found → triggers Hold

So pressing a tap-on-press-release key followed by a fully-tapped unlisted key triggers Hold. This may be the intended PermissiveHold fallback behavior, but it's worth a test and a clarifying comment, since the interaction is non-obvious. The existing custom_tap_hold_release_trigger_tap_release has the same fallthrough pattern — is it also intentional there?

---

📋 Missing Tests

Priority ordering when a key appears in multiple lists

The docstring documents the priority: tap-on-press > hold-on-press > tap-on-press-release. This is code-path-specific behavior that should be covered by a test.

tap-on-press-release key with a concurrent unlisted key press+release

As described above — the interaction with PermissiveHold for concurrent unlisted keys is untested and may surprise users.

Parser error cases

None of the tests exercise the error paths (wrong number of positional args, duplicate option, empty key list). Since the error messages are hand-written, it's easy to accidentally break them.

---

🔁 Code Duplication — Option Counting

parse_tap_hold_keys_named_lists inlines the trailing-option-counting logic:

let n_opts = {
    let mut count = 0;
    for expr in ac_params.iter().rev() {
        if let Some(list) = expr.list(s.vars()) {
            if let Some(kw) = list.first().and_then(|e| e.atom(s.vars())) {
                if TAP_HOLD_KEYS_OPTION_KEYWORDS.contains(&kw) {
                    count += 1;
                    continue;
                }
            }
        }
        break;
    }
    count
};

This is structurally identical to count_trailing_options (line 82–96 in tap_hold.rs), which takes TAP_HOLD_OPTION_KEYWORDS. A small refactor — count_trailing_options(ac_params, s, keywords: &[&str]) — would eliminate the duplication.

---

🧹 Readability — parse_key_list_from_option

The try_fold with nested closures and ?? is harder to follow than necessary:

option[1..].iter().try_fold(vec![], |mut keys, key| {
    key.atom(s.vars())
        .map(|a| -> Result<()> {
            keys.push(str_to_oscode(a).ok_or_else(|| {
                anyhow_expr!(key, "string of a known key is expected")
            })?);
            Ok(())
        })
        .ok_or_else(|| {
            anyhow_expr!(key, "string of a known key is expected, found list instead")
        })??;
    Ok(keys)
})

A simple for loop would be clearer and more idiomatic here, matching the style used elsewhere in this file.

---

⚡ Minor Inconsistency — Pre-converting to u16 at allocation

In the existing custom_tap_hold_release, keys are stored as OsCode and converted with .map(u16::from) on each loop iteration (line 58). The new custom_tap_hold_keys pre-converts to u16 at allocation time, which is a slight win on the hot path. Worth noting as intentional (and potentially worth backporting to the other closures), or reverting for consistency.

---

ℹ️ Minor — No Abbreviated Form (TAP_HOLD_KEYS_A)

All other tap-hold-*-keys variants have a unicode-shorthand alias (TAP_HOLD_EXCEPT_KEYS_A, TAP_HOLD_TAP_KEYS_A). tap-hold-keys doesn't. If this is intentional (these aliases seem to be for internal use / test compatibility), it's fine — just worth documenting why.

---

✅ What's Working Well

• The named-list API design is a clear improvement over adding more positional arguments
• seen_options deduplication check is a nice touch
• require-prior-idle support is correctly wired through
• Test coverage for all three list types individually, combined, and with require-prior-idle
• The option-counting approach from the end of params is robust against edge cases
• Docs and sample config are clear

Overall this is solid work. The main asks before merge would be: add the missing priority/concurrent-key tests, and consider the fallthrough behavior comment/test. The duplication and readability items can be done in a follow-up.

[claude]

Code Review: tap-hold-keys

Overall this is a clean, well-scoped addition. The design follows existing patterns in the codebase, the tests cover the main scenarios, and the docs/sample are present. A few issues worth addressing before merge:

---

Bugs / Correctness

None found. The core logic in custom_tap_hold_keys is correct. The three-way priority (tap-on-press → hold-on-press → tap-on-press-release → PermissiveHold fallback) works as intended, and the PermissiveHold fallback correctly handles keys that appear in the tap-on-press-release list but haven't yet been released (falls through to the same release check, which also won't fire until the release arrives, so no spurious early decision).

---

Code Quality

Vec::from_iter(...) → .collect() (custom_tap_hold.rs, three occurrences)

// Current — non-idiomatic
let keys_tap_on_press = a.sref_vec(Vec::from_iter(
    keys_tap_on_press.iter().copied().map(u16::from),
));

// Preferred
let keys_tap_on_press = a.sref_vec(
    keys_tap_on_press.iter().copied().map(u16::from).collect(),
);

parse_key_list_from_option — double ?? is hard to read (tap_hold.rs)

The try_fold with nested closures and ?? at the end is unusual in this codebase. The same logic is clearer as a plain loop:

fn parse_key_list_from_option(...) -> Result<Vec<OsCode>> {
    if option.len() < 2 {
        bail_expr!(...);
    }
    let mut keys = Vec::new();
    for key in &option[1..] {
        let a = key
            .atom(s.vars())
            .ok_or_else(|| anyhow_expr!(key, "string of a known key is expected, found list instead"))?;
        keys.push(
            str_to_oscode(a)
                .ok_or_else(|| anyhow_expr!(key, "string of a known key is expected"))?,
        );
    }
    Ok(keys)
}

---

API / Consistency

Missing ASCII alias constant (list_actions.rs)

Every other tap-hold-* variant has an ASCII alias (e.g. TAP_HOLD_RELEASE_KEYS_A = "tap⬓↑keys", TAP_HOLD_TAP_KEYS_A = "tap⬓tapkeys"). TAP_HOLD_KEYS has none. Users relying on the shorthand alias pattern will find this inconsistent. A constant like TAP_HOLD_KEYS_A = "tap⬓keys" (or similar) and a corresponding entry in is_list_action + parse_action_list dispatch would round this out.

---

Test Coverage

Leftover debugging comment in a test (tap_hold_tests.rs, tap_hold_keys_tap_on_press_release)

// tap-on-press-release key pressed but not released: no early decision,
// falls through to PermissiveHold which triggers hold on press+release
// Wait, it IS in tap-on-press-release list so it checks for release.
// If not released, falls through to PermissiveHold check which also needs release.
// So just pressing without releasing: no decision until timeout.

The "Wait, it IS in..." lines read as in-progress thinking. The final sentence is the correct explanation — just keep that one (or rewrite for clarity).

Missing test: key in multiple lists

The priority order (tap-on-press > hold-on-press > tap-on-press-release) is documented in the custom_tap_hold_keys comment but never exercised. A test that puts the same key in two lists and asserts which one wins would validate both the implementation and document the behavior for future maintainers.

---

Documentation

PermissiveHold semantics in docs (config.adoc)

For any other key, activates $hold-action when the key is pressed and released (PermissiveHold behavior).

"The key is pressed and released" could be misread as "you press and release the tap-hold key itself." It's actually "another key is pressed AND released while this tap-hold is pending." A small clarification:

For any other key: if that key is pressed and released while this key is waiting, $hold-action is triggered early (PermissiveHold behavior).

---

Summary

Area	Status
Core logic / correctness	✅ No bugs found
Style (Vec::from_iter, try_fold)	Minor cleanup needed
API consistency (alias constant)	Missing — worth adding
Test comment cleanup	Minor
Test for multi-list priority	Missing
Docs clarity	Minor wording improvement

The feature itself is solid and fills a real gap. The above are mostly polish items; the alias constant is the one I'd consider a functional gap since every other variant has one.

[claude]

Code Review: tap-hold-keys

(Note: there are two prior review comments from a previous session. This is a fresh review of the current diff.)

The implementation is clean and well-structured. The named-list API is the right design call. Tests cover the main paths well. Below are the key issues I found.

---

Bug: tap-on-press-release key + concurrent unlisted key triggers Hold unexpectedly

In custom_tap_hold.rs, when a key B is in tap-on-press-release but hasn't yet been released, the code falls through to the PermissiveHold check for that same press event. That's fine — PermissiveHold checks Release(B), which isn't in the queue yet.

But on the next loop iteration, if an unlisted key C appears as Press(C) with Release(C) already in the queue, PermissiveHold fires and returns Hold.

Queue: [Press(B), Press(C), Release(C)]
// B is in tap-on-press-release, C is unlisted

// Press(B): tap-on-press-release → Release(B) not found → fall through
//           PermissiveHold      → Release(B) not found → skip
// Press(C): not in any list
//           PermissiveHold      → Release(C) found → return Hold  ← surprising!

A user with (tap-on-press-release b) presses B then quickly taps C → gets Hold, not Tap. This is almost certainly not the intended behavior: the presence of B in the tap-on-press-release list implies the user wants to keep typing freely until B is actually released.

Suggested fix: when a key is in tap-on-press-release and hasn't been released yet, continue the loop without falling through to PermissiveHold:

if keys_tap_on_press_release.iter().copied().any(|j2| j2 == j) {
    let target = Event::Release(i, j);
    if queued.clone().copied().any(|q| q.event() == target) {
        return (Some(WaitingAction::Tap), false);
    }
    continue; // waiting for release — don't let unlisted keys trigger Hold
}

At minimum, this needs a test:

// tap-on-press-release key is held while an unlisted key is tapped
let result = simulate(cfg, "d:a t:20 d:b d:c u:c t:100").to_ascii();
// Expected: ??? — document whatever the intended behavior is

---

Missing Test: hold-on-press > tap-on-press-release priority

The docstring documents priority as:

tap-on-press > hold-on-press > tap-on-press-release > PermissiveHold

The tap_hold_keys_priority_order test only covers tap-on-press > hold-on-press. The hold-on-press > tap-on-press-release case is undocumented. A key in both lists should trigger Hold — add a test to lock in this behavior.

---

Missing: ASCII alias constant

Every other tap-hold-* variant has a compact alias:

• TAP_HOLD_EXCEPT_KEYS_A = "tap⬓⤫keys"
• TAP_HOLD_TAP_KEYS_A = "tap⬓tapkeys"

TAP_HOLD_KEYS has none. If these are user-facing (they appear in is_list_action), users relying on the pattern will find this inconsistent. Even if they're internal-only, the pattern should be followed for consistency.

---

Code Duplication: option-counting logic

The n_opts block in parse_tap_hold_keys_named_lists is structurally identical to count_trailing_options (which already exists in tap_hold.rs). The only difference is the keyword set. Extracting:

fn count_trailing_options(ac_params: &[SExpr], s: &ParserState, keywords: &[&str]) -> usize

would eliminate the duplication and let both sites share the logic.

---

What's Working Well

• Named-list API is clearly superior to more positional args
• seen_options deduplication is a nice defensive touch
• require-prior-idle is correctly threaded through
• .collect() and plain for loop in parse_key_list_from_option are idiomatic ✓
• Tests cover all three list types individually and in combination
• Docs and sample config are clear and accurate
• The tap_hold_keys_no_options test confirming PermissiveHold fallback behavior is excellent

---

Summary

Item	Priority
tap-on-press-release + concurrent unlisted key triggers Hold	High — likely a bug
Test for hold-on-press > tap-on-press-release priority	Medium
Missing ASCII alias constant	Medium
Option-counting duplication	Low (cleanup)

[claude]

Code Review: feat: add tap-hold-keys action with named key list options

Overall this is a well-structured addition. The named-list API is a clean design, error messages are user-friendly, and the 6 sim tests give solid coverage of the happy paths. A few things worth addressing before merging:

---

🐛 Missing test: PermissiveHold fallback while tap-on-press-release key is still held

The current tests for tap-on-press-release cover:

• press + release → tap ✓
• press + hold past timeout → hold ✓

But there is no test for the third scenario: a non-listed key is pressed and released while the tap-on-press-release key is still held. Because the PermissiveHold fallback applies to all queued events (including the tap-on-press-release key's ongoing press), this case would trigger hold via the non-listed key's press+release cycle. Whether that's intended or surprising is worth documenting and testing:

;; b is tap-on-press-release; c is unlisted (PermissiveHold)
;; d:a d:b d:c u:c → hold fires because c completes a press+release
let result = simulate(cfg, "d:a t:20 d:b t:5 d:c t:5 u:c t:100").to_ascii();
// expected: dn:Y ...  (hold via PermissiveHold on c, not on b)

---

🔧 Code duplication: inline option-counting duplicates count_trailing_options

parse_tap_hold_keys_named_lists inlines its own option-counting loop rather than calling the existing count_trailing_options. The only difference is the keyword set used. A simple fix is to make count_trailing_options accept a keyword slice:

// before
fn count_trailing_options(ac_params: &[SExpr], s: &ParserState) -> usize {
    // hard-codes TAP_HOLD_OPTION_KEYWORDS
}

// after
fn count_trailing_options(ac_params: &[SExpr], s: &ParserState, keywords: &[&str]) -> usize {
    // uses `keywords` parameter
}

All existing callers pass TAP_HOLD_OPTION_KEYWORDS; parse_tap_hold_keys_named_lists would pass TAP_HOLD_KEYS_OPTION_KEYWORDS. This also means the constant TAP_HOLD_KEYS_OPTION_KEYWORDS is used in only one place right now, which is a code smell.

---

⚠️ Inconsistent key-type storage across custom_tap_hold_* functions

Existing functions (e.g. custom_tap_hold_release, custom_tap_hold_tap_keys) store &'static [OsCode] and convert to u16 at comparison time:

let keys = a.sref_vec(Vec::from_iter(keys.iter().copied())); // stores OsCode
// ...
keys.iter().copied().map(u16::from).any(|j2| j2 == j)       // converts on use

The new custom_tap_hold_keys pre-converts to u16 and stores &'static [u16]:

let keys_tap_on_press = a.sref_vec(keys_tap_on_press.iter().copied().map(u16::from).collect());
// ...
keys_tap_on_press.iter().copied().any(|j2| j2 == j)

The new approach is actually better (conversion done once, cheaper per keystroke on the hot path). But the inconsistency could confuse a future contributor looking at both patterns side-by-side. Either apply the better pattern consistently to the other functions, or add a brief comment explaining why.

---

📄 Priority ordering not in user-facing docs

The docstring on custom_tap_hold_keys documents key-in-multiple-lists priority:

tap-on-press > hold-on-press > tap-on-press-release > PermissiveHold

This should also appear in docs/config.adoc — users composing (hold-on-press b) and (tap-on-press-release b) for the same key won't find this in the help text.

---

🚫 No alias constant for tap-hold-keys

Other actions have compact Unicode aliases registered (e.g. TAP_HOLD_TAP_KEYS_A = "tap⬓tapkeys", TAP_HOLD_EXCEPT_KEYS_A = "tap⬓⤫keys"). TAP_HOLD_KEYS has none. This may be intentional (it's a newer/different kind of action), but if the project convention is to add aliases for all tap-hold variants, it's missing here.

---

💡 Minor: warn or degrade gracefully when no key lists are provided

When all three lists are empty (e.g. (tap-hold-keys 200 200 x y) with no options), the new function creates three empty sref_vec slices and a closure that behaves identically to tap-hold-release (pure PermissiveHold). The existing tap_hold_keys_no_options test confirms this works, but:

1. A parse-time warning ("no key lists specified; this is equivalent to tap-hold-release") would help users who accidentally omit list options.
2. Alternatively, detect the no-op case and emit HoldTapConfig::PermissiveHold directly instead of a Custom closure — avoids allocating the closure and three empty vecs.

Neither of these is critical for correctness.

---

✅ What's well done

• Named option lists are the right API design — positional key lists would have been messy for 3 independent groups.
• seen_options: HashSet<&str> deduplication catches user config mistakes early with a clear error.
• The require-prior-idle integration works without needing new plumbing.
• Error messages include examples (e.g. \(tap-on-press a b c)``).
• Tests cover all individual lists, their combination, priority ordering, and require-prior-idle — that's thorough.

The implementation is solid. The main ask before landing is a test (or explicit doc note) for the PermissiveHold-while-tap-on-press-release-is-held scenario, and collapsing the duplicated option-counting logic.

[claude]

Code Review: tap-hold-keys (fresh pass)

Note: there are already several review comments from previous sessions on this PR. This review tries to add fresh perspective and resolve open questions.

---

Clarification: the tap-on-press-release + unlisted-key interaction is correct

Prior reviews flagged this as a potential bug:

pressing a tap-on-press-release key B, then fully tapping an unlisted key C, triggers Hold via the PermissiveHold path

This is actually correct, intended behavior, not a bug. The contract is:

• Listed keys in tap-on-press-release: only trigger tap if that specific key is fully tapped
• Unlisted keys: PermissiveHold applies (any complete press+release triggers hold)

So d:a d:b d:c u:c → Hold is the right result. The documentation says "For any other key: if that key is pressed and released while this key is waiting, $hold-action is triggered early (PermissiveHold behavior)." That's exactly what happens.

That said, it's worth adding a test to pin this behavior explicitly:

// b is tap-on-press-release, c is unlisted → PermissiveHold on c triggers hold
let result = simulate(cfg, "d:a t:20 d:b t:5 d:c t:5 u:c t:100").to_ascii();
assert_eq!("t:30ms dn:Y ...", result);

---

Bug: priority order in docstring doesn't match code for tap-on-press-release vs PermissiveHold

The custom_tap_hold_keys docstring says:

Priority: tap-on-press > hold-on-press > tap-on-press-release > PermissiveHold

But look at what happens when a key is in tap-on-press-release and an unlisted key triggers PermissiveHold first in the queue. For example:

queue: [Press(C), Release(C), Press(B)]  // C unlisted, B in tap-on-press-release

• Iteration on Press(C): not in any list → PermissiveHold finds Release(C) → returns Hold immediately
• Press(B) is never reached

The priority ordering only holds within a single press event. Across the queue, earlier-processed events can override later ones regardless of which list they're in. The docstring is misleading — it should describe per-key-event priority, not a global queue priority.

---

Performance: queued.clone() called up to twice per press event

// In the tap-on-press-release branch:
if queued.clone().copied().any(|q| q.event() == target) { ... }
// In the PermissiveHold branch:
if queued.clone().copied().any(|q| q.event() == target) { ... }

Two iterator clones + two linear scans per press event. For a 3-key-deep queue this is ~6 comparisons, which is fine. For pathological cases (hundreds of buffered events), this is O(n²). Given kanata's design keeps queues very short, this is acceptable — but worth a comment so future contributors don't add more clones without thinking.

---

Confirmed correct: timeout_action: *hold_action

The HoldTapAction uses hold_action for both hold and timeout_action, consistent with all other tap-hold-* variants. ✓

---

Still open from prior reviews (most important)

1. Refactor count_trailing_options to accept a keyword slice — the inlined option-counting loop is the most substantive code-quality issue. The fix is straightforward and removes the code duplication cleanly.

2. Add Unicode alias (TAP_HOLD_KEYS_A) for consistency with TAP_HOLD_TAP_KEYS_A, TAP_HOLD_EXCEPT_KEYS_A, etc.

3. Document priority ordering in docs/config.adoc — users composing multiple overlapping key lists won't find the priority rules in the help text.

---

Overall

The implementation is correct and well-tested. The most important pre-merge asks are:

• A test for the unlisted-key PermissiveHold scenario (documents intended behavior)
• The count_trailing_options refactor (removes ~15 lines of duplication)
• Fix/clarify the per-key vs. across-queue priority docstring

The rest are polish items that could land as follow-ups.