feat: add per-action require-prior-idle override for tap-hold

[malpern]

Summary

Adds a per-action (require-prior-idle <ms>) option to all tap-hold variants, allowing individual actions to override the global tap-hold-require-prior-idle defcfg value.

Addresses jtroo#1967.

Motivation

With the global tap-hold-require-prior-idle from jtroo#1960, users can't mix tap-hold behaviors — home-row mods benefit from idle detection, but layer-tap keys (e.g., tab/number-layer) need immediate activation even during fast typing.

Usage

(defcfg tap-hold-require-prior-idle 150)
(defalias
  ;; HRMs inherit global 150ms — no override needed
  a (tap-hold 200 200 a lmet)
  s (tap-hold 200 200 s lalt)
  ;; Layer key disables idle detection for immediate activation
  nav (tap-hold 200 200 tab (layer-toggle nav) (require-prior-idle 0))
  ;; Or enable per-action without a global setting
  d (tap-hold-release 200 200 d lctl (require-prior-idle 150))
)

Design

Follows the (keyword value) option convention established by tap-hold-opposite-hand. The option name is require-prior-idle (no tap-hold- prefix since it's already inside a tap-hold action).

• (require-prior-idle 0) — disables for this action even when a global threshold is set
• (require-prior-idle N) — enables/overrides with N ms threshold
• Omitted — falls back to global defcfg value

Keyberon: Added require_prior_idle: Option<u16> to HoldTapAction. The existing idle check in do_action() uses the per-action value when present, falling back to the global tap_hold_require_prior_idle.

Parser: Shared parse_tap_hold_options() helper scans for known option keywords in trailing list expressions. count_trailing_options() identifies option lists by checking the first atom against a keyword allowlist, so actions with list-type positional params (e.g., (layer-toggle nav)) aren't misidentified as options.

Test coverage

6 sim tests: per-action override shrinks threshold, per-action disable overrides global, per-action enables without global, mixed actions (issue jtroo#1967 use case), works with tap-hold-release, works with tap-hold-opposite-hand.

7 parser tests: valid value parses, zero parses, non-numeric rejected, unknown option rejected, duplicate rejected, works with tap-hold-press, works with tap-hold-release.

Supported variants

Works with all tap-hold variants: tap-hold, tap-hold-press, tap-hold-release, tap-hold-press-timeout, tap-hold-release-timeout, tap-hold-release-keys, tap-hold-release-tap-keys-release, tap-hold-except-keys, tap-hold-tap-keys, and tap-hold-opposite-hand.

🤖 Generated with Claude Code

[claude]

Code Review

This is a well-motivated feature with a clean design. The core logic in layout.rs is minimal and correct. A few observations below:

---

Correctness / Potential Issues

Inconsistent use of n_positional vs. hardcoded slice offsets

In parse_tap_hold_timeout, the options slice correctly uses n_positional:

let opts = parse_tap_hold_options(&ac_params[n_positional..], s)?;

But in parse_tap_hold, parse_tap_hold_keys, and parse_tap_hold_keys_trigger_tap_release, hardcoded offsets are used:

let opts = parse_tap_hold_options(&ac_params[4..], s)?;  // parse_tap_hold
let opts = parse_tap_hold_options(&ac_params[5..], s)?;  // parse_tap_hold_keys
let opts = parse_tap_hold_options(&ac_params[6..], s)?;  // parse_tap_hold_keys_trigger_tap_release

These happen to be correct since n_positional was just validated to equal those values, but using n_positional consistently throughout would be cleaner and more maintainable.

count_trailing_options edge case with misleading error

If a user's hold-action is a list that starts with require-prior-idle (e.g., (tap-hold 200 200 a (require-prior-idle))), count_trailing_options would classify it as an option, causing n_positional to be 3 and triggering: "tap-hold expects 4 items after it, got 3". This would be confusing since the true problem is the invalid hold-action. In practice this is an extremely unlikely input, but a comment acknowledging the assumption (that require-prior-idle is not a valid action name) would be helpful.

---

Code Duplication

The require-prior-idle parsing in defhands.rs duplicates the logic from parse_tap_hold_options() (length check, parse_u16 call, error message). Consider extracting just the per-option parsing into a small shared helper, or at minimum a shared constant for the error message format string. Right now, if the option syntax changes, it needs updating in two places.

---

Minor Nits

• parse_tap_hold_options uses a local seen_require_prior_idle: bool for duplicate detection, while defhands.rs uses seen_options: HashSet<&str>. The bool approach is fine for a single option, but if more options are added to TapHoldOptions in the future, the HashSet pattern from defhands.rs scales better.

• The !matches!(ac_params.len(), 6) that was replaced in parse_tap_hold_keys_trigger_tap_release with n_positional != 6 is a nice cleanup.

---

Performance

No concerns. The added require_prior_idle.unwrap_or(self.tap_hold_require_prior_idle) is a trivially cheap Option<u16> unwrap on the struct, adding zero meaningful overhead in the hot-path decision.

---

Test Coverage

Good overall. The 6 sim tests cover the key behavioral cases (override shrinks threshold, override disables, enables without global, mixed actions, tap-hold-release, opposite-hand). One gap: the parser tests cover tap-hold, tap-hold-press, and tap-hold-release, but not tap-hold-press-timeout, tap-hold-release-timeout, tap-hold-release-keys, or tap-hold-tap-keys — all of which are listed as supported. Adding at least one smoke test per parse path (parse_tap_hold_timeout and parse_tap_hold_keys) would close this gap.

---

Summary

The design is solid — Option<u16> with None = fall back to global and Some(0) = disabled is idiomatic and easy to reason about. The main actionable items are:

1. Use n_positional consistently for the options slice in all parse functions.
2. Deduplicate the require-prior-idle parsing logic between tap_hold.rs and defhands.rs.
3. Add parser tests for parse_tap_hold_timeout and parse_tap_hold_keys paths.

[claude]

Code Review

This is a well-designed and cleanly implemented feature. The count_trailing_options approach for distinguishing positional parameters from option lists is smart — it correctly handles variants like tap-hold-release-keys where the positional hold parameter is also a list. Here are my observations:

---

Bug / Correctness Issues

None found. The core logic change in layout.rs is correct:

let idle_threshold = require_prior_idle.unwrap_or(self.tap_hold_require_prior_idle);
if idle_threshold > 0 { ... }

Option<u16> with None = fall back to global, Some(0) = explicitly disabled, Some(n) = use n — this three-way distinction requires the current type and the sentinel 0 correctly handles the disable case.

---

Design / Maintenance Concerns

1. Inconsistent duplicate detection pattern between parse_tap_hold_options and defhands.rs

defhands.rs uses a single HashSet<&str> with a generic check before the match (line 126), which scales to any number of options automatically. parse_tap_hold_options uses a per-option boolean flag (seen_require_prior_idle). As more options are added to parse_tap_hold_options in the future, someone will need to remember to add a new flag for each one. Consider adopting the HashSet pattern for consistency:

// Instead of separate bool flags:
let mut seen_opts: HashSet<&str> = HashSet::default();
// ...
if !seen_opts.insert(kw) {
    bail_expr!(&option[0], "duplicate option '{}'", kw);
}

2. count_trailing_options uses linear scan

TAP_HOLD_OPTION_KEYWORDS.contains(&kw) is O(n) over the keyword list. Fine for one keyword, but as options grow a HashSet would be more appropriate. Low priority for now.

3. HoldTapAction struct is not #[non_exhaustive]

Adding require_prior_idle to the public HoldTapAction struct is a breaking change for any downstream code constructing the struct with literal syntax (as evidenced by the ~25 require_prior_idle: None additions in the existing tests). If keyberon is intended as an embeddable library (not just kanata-internal), adding #[non_exhaustive] to HoldTapAction would protect against this in the future. If it's internal-only, no action needed.

---

Test Coverage

4. Redundant sim test

per_action_require_prior_idle_mixed_actions (lines 873–892 in tap_hold_tests.rs) has an identical config and simulation sequence to per_action_require_prior_idle_disable_overrides_global. The comment describes testing the "issue jtroo#1967 use case" of mixing HRM keys (using global) with layer-tap keys (per-action override), but the actual test doesn't have an HRM key in the layout — both tests only have a plain a key and a tap-hold d key.

A true mixed test would include two tap-hold keys: one relying on the global threshold and one with a per-action override, with both exercised in sequence to confirm they behave differently.

5. Missing parser test for tap-hold-opposite-hand with the new option

The sim test (per_action_require_prior_idle_with_opposite_hand) covers the runtime behavior, but there's no parser unit test for tap-hold-opposite-hand analogous to the other per_action_require_prior_idle_on_* parser tests. Since tap-hold-opposite-hand has a distinct parser path (defhands.rs), a parser test would give explicit coverage of that code path.

6. Missing parser test for tap-hold-release-tap-keys-release

The PR lists this variant as supported and it's handled by parse_tap_hold_keys_trigger_tap_release, but there's no parser test for it (unlike the other variants).

---

Minor / Nits

• The error message in parse_tap_hold_options says "expected option list, e.g. \(require-prior-idle 150)`"` — this is helpful. Consider updating it when future options are added.
• The TapHoldOptions struct with #[derive(Default)] is forward-looking and clean — adding a new option field later is a one-liner.

---

Summary

The core implementation is correct and the overall approach is solid. The main items worth addressing before merging:

• (Medium) The redundant sim test (macOS: Improve TCC permission UX (error messages, --check-permissions, .app bundle docs) #4) — easy to fix and improves confidence in the actual use case
• (Low) Missing parser test for tap-hold-opposite-hand (feat: thread device identity for per-device switch conditions #5) and tap-hold-release-tap-keys-release (feat: name-based device matching and defdevice config #6)
• (Low) Inconsistent duplicate detection pattern (feat: TapHoldReason tracing for HRM decisions #1) — not a bug today, but a future maintenance trap

Great work on the defcfg.adoc documentation update — the examples clearly explain all three cases (override, disable, enable-without-global).

🤖 Generated with Claude Code

[claude]

Code Review

Overall this is a well-designed, minimal PR. The feature addresses a real user need and the implementation is clean. Here are my findings:

---

Hot Path (keyberon/src/layout.rs) ✅

The critical change is exactly one line:

let idle_threshold = require_prior_idle.unwrap_or(self.tap_hold_require_prior_idle);

This is exactly right — cheap Option::unwrap_or, no branching overhead, no allocations. No performance concerns here.

---

Correctness: count_trailing_options disambiguation

The approach of scanning trailing expressions from the end and checking their first atom against TAP_HOLD_OPTION_KEYWORDS is sound. However, it creates a two-pass design: count_trailing_options runs first to count options, then parse_tap_hold_options re-parses the same expressions. If the two ever diverge (e.g., count_trailing_options counts something that parse_tap_hold_options later rejects), you'd get a confusing arity error instead of the real error. It's a minor fragility worth noting.

A single-pass alternative would be to attempt parsing from the end speculatively, but the current approach is simple and correct for the current option set.

---

Issue: defhands.rs doesn't use the shared helper

parse_tap_hold_opposite_hand in defhands.rs has its own manual option-parsing loop and calls parse_require_prior_idle_option directly, rather than delegating to parse_tap_hold_options. This creates two maintenance surfaces:

// defhands.rs — manual handling
"require-prior-idle" => {
    require_prior_idle = Some(tap_hold::parse_require_prior_idle_option(...)?);
}

vs.

// tap_hold.rs — shared helper
pub(crate) fn parse_tap_hold_options(...) -> Result<TapHoldOptions> { ... }

When a second option is added to TapHoldOptions, it would need to be added in defhands.rs separately. Consider having parse_tap_hold_opposite_hand collect its own option names and then fall through to parse_tap_hold_options for the shared ones, or restructure so defhands.rs always calls the shared helper for tap-hold generic options.

---

Minor: Hardcoded valid-options string

bail_expr!(
    &option[0],
    "unknown tap-hold option '{}'. \
    Valid options: require-prior-idle",
    kw
),

This string will drift as more options are added. Since TAP_HOLD_OPTION_KEYWORDS already exists, consider:

bail_expr!(
    &option[0],
    "unknown tap-hold option '{}'. Valid options: {}",
    kw,
    TAP_HOLD_OPTION_KEYWORDS.join(", ")
);

Same applies to the error string in defhands.rs.

---

Minor: Memory size of HoldTapAction

Option<u16> is 4 bytes (2 bytes value + 1 byte discriminant + 1 byte padding). Since HoldTapAction instances are stored in the arena allocator (s.a.sref()), the cost is per-action at parse time, not per-keypress at runtime. This is acceptable, but worth knowing.

If memory ever becomes a concern, Option<NonZeroU16> fits in 2 bytes. The current semantics (Some(0) = disabled, None = use global) would need slight adjustment, but the logical distinction is clean and doesn't warrant a change now.

---

Missing parser test: tap-hold-tap-keys

The PR description lists tap-hold-tap-keys as supported, and the implementation covers it via parse_tap_hold_keys, but there's no corresponding parser test. tap-hold-release-keys and tap-hold-except-keys are tested (both route through the same function), so this isn't a bug — but a test for completeness would be consistent with the rest of the suite.

---

Test quality: simulation tests ✅

The sim tests are well-structured. Particularly good:

• per_action_require_prior_idle_mixed_actions directly reproduces the issue Feature request: Per action require_prior_idle override jtroo/kanata#1967 scenario
• per_action_require_prior_idle_disable_overrides_global verifies Some(0) semantics explicitly
• Test comments explain why the expected output is correct, not just what it is

One suggestion: the comment on per_action_require_prior_idle_overrides_global says "60ms gap > 50ms per-action threshold → normal hold", which is correct but the direction of reasoning is easy to misread. The idle check fires prior_idle_tap = true only when ticks_since_occurrence <= threshold. So 60 > 50 means the check does NOT fire → normal hold. Worth clarifying in the comment.

---

Summary

Area	Assessment
Hot-path runtime change	✅ Minimal and correct
Parser design	✅ Correct; minor fragility noted
defhands.rs integration	⚠️ Shared helper not used — future maintenance risk
Error messages	⚠️ Hardcoded strings will drift
Test coverage	✅ Solid; tap-hold-tap-keys parser test missing
Documentation	✅ Clear examples, well-placed in config.adoc
API design	✅ Consistent with existing tap-hold-opposite-hand option convention

The two items worth addressing before merge are the defhands.rs maintenance risk and the hardcoded valid-options strings. Everything else is minor.

[claude]

Code Review

This is a well-designed, well-tested feature. The implementation is clean and the hot-path change is minimal. I want to also correct some factual errors in the prior three reviews left by earlier Claude runs, since they may cause unnecessary churn.

---

Corrections to Prior Reviews

Review 1 claims: hardcoded slice offsets like &ac_params[4..], &ac_params[5..] are used instead of n_positional.
Actually: All four call sites consistently use &ac_params[n_positional..]. This is correct as-written. No change needed.

Review 1 claims: parse_tap_hold_options uses a per-option boolean flag for duplicate detection.
Actually: It uses HashSet<&str> — exactly the scalable pattern recommended. No change needed.

Review 2 claims: per_action_require_prior_idle_mixed_actions is a redundant sim test that "only has a plain a key".
Actually: The config has two tap-hold keys — @a (tap-hold-press relying on the global 150ms threshold) and @d (tap-hold-press with per-action override 0). Case 1 and Case 2 exercise them independently in the same config. This is exactly the jtroo#1967 mixed scenario.

Review 2 claims: there's no parser test for tap-hold-opposite-hand.
Actually: per_action_require_prior_idle_on_tap_hold_opposite_hand exists in defcfg.rs and covers that path.

---

Genuine Issues

1. tap-hold-release-tap-keys-release missing from documentation list

The variant is listed as supported in the PR description and has both a parser test and a sim test, but the docs/config.adoc supported-variants list omits it:

- Works with all `tap-hold` variants: `tap-hold`, `tap-hold-press`,
  `tap-hold-release`, `tap-hold-press-timeout`, `tap-hold-release-timeout`,
  `tap-hold-release-keys`, `tap-hold-except-keys`, `tap-hold-tap-keys`,
  and `tap-hold-opposite-hand`.

tap-hold-release-tap-keys-release should be added here.

2. defhands.rs will diverge when new options are added (medium priority)

parse_tap_hold_options and the option loop in parse_tap_hold_opposite_hand are separate code paths. When a second option is added to TapHoldOptions, it must be added to defhands.rs manually. Today the shared parse_require_prior_idle_option helper reduces the duplication, but the match arm itself still lives in both places. A comment calling this out explicitly would at minimum prevent future maintainers from missing the second location.

3. Hardcoded valid-options strings will drift (low priority)

TAP_HOLD_OPTION_KEYWORDS exists but isn't used in error messages:

// tap_hold.rs
"unknown tap-hold option '{}'. Valid options: require-prior-idle"

// defhands.rs  
"Valid options: timeout, same-hand, neutral, unknown-hand, neutral-keys, require-prior-idle"

When a new option is added, the keyword slice gets updated but the error messages won't unless explicitly remembered. For tap_hold.rs where TAP_HOLD_OPTION_KEYWORDS is already defined, consider:

bail_expr!(
    &option[0],
    "unknown tap-hold option '{}'. Valid options: {}",
    kw,
    TAP_HOLD_OPTION_KEYWORDS.join(", ")
);

---

What's Working Well

• Hot path: require_prior_idle.unwrap_or(self.tap_hold_require_prior_idle) is exactly one Option<u16> unwrap — no allocations, no extra branches. Zero concern.
• Option<u16> semantics: None = use global, Some(0) = explicitly disabled, Some(n) = use n. Clean three-way distinction that's consistent with how tap_hold_require_prior_idle: u16 already uses 0 for "disabled".
• count_trailing_options design: scanning from the end and stopping at the first non-option correctly handles list-type positional params (e.g., (b) in tap-hold-release-keys) without false positives.
• Test coverage: the 6 sim tests cover the key behavioral cases including the motivating use case. The 10+ parser tests cover every supported variant individually.

---

Summary

Item	Severity
tap-hold-release-tap-keys-release missing from docs list	Minor
defhands.rs option loop diverges from shared helper	Medium
Hardcoded valid-options strings	Low

The first item is a one-word doc fix. The second and third are maintenance concerns that don't affect correctness today. Overall the PR is solid and ready to merge once the doc omission is addressed.