Skip to content

Auto-pop originating oneshot layer when executing oneshot actions - #12

Open
argenkiwi wants to merge 2 commits into
mainfrom
bugfix/oneshot-pop-originating-layer
Open

argenkiwi wants to merge 2 commits into
mainfrom
bugfix/oneshot-pop-originating-layer

Conversation

@argenkiwi

Copy link
Copy Markdown
Owner

Summary

When a key is pressed from within an active one-shot layer (oneshot_depth > 0 on the originating layer) and triggers an Op::Oneshot* action (such as oneshotm), the originating layer is now automatically deactivated and popped before activating the target one-shot layer.

Problem

Previously, Op::Oneshot* actions did not pop or clear any active one-shot layers. When activating a one-shot macro layer (e.g. c+space = oneshot(macros)) and then pressing a key mapped to oneshotm(shift, macro(dot space)), the [macros] layer remained active with oneshot_depth == 1. A subsequent keypress (e.g. a) was then mistakenly resolved against [macros] (executing a = oneshotm(shift, macro(S-1 space)) producing ! ) instead of falling back to [main] with Shift (A).

Solution

  • Check if self.layer_state[layer].oneshot_depth > 0 on the originating layer index during Op::Oneshot | Op::OneshotM | Op::OneshotK and Op::OneshotMulti.
  • If so, call self.deactivate_layer(output, layer) and decrement self.layer_state[layer].oneshot_depth -= 1.
  • Sequential one-shot modifier chaining on base layers (e.g. tapping oneshot(shift) then oneshot(control) on [main]) remains fully functional and continues to aggregate modifiers without popping each other.
  • Bumped version to 0.3.5.

Tests

  • Added unit tests:
    • test_oneshotm_inside_oneshot_layer_pops_originating_layer
    • test_oneshot_modifier_chaining_aggregates
  • All 95 tests pass.

When a key is pressed from within an active oneshot layer (oneshot_depth > 0
on the originating layer) and triggers an Op::Oneshot* action (such as
oneshotm), deactivate and pop the originating layer before activating the
target oneshot layer.

This prevents the previous oneshot layer from lingering active and shadowing
subsequent base layer keys, while preserving sequential oneshot modifier
chaining when tapping modifier oneshots from the base layer.

Bump version to 0.3.5.
@argenkiwi

Copy link
Copy Markdown
Owner Author

Automated code review

9 findings in src/keyboard_impl/actions.rs. 7 are real bugs/cleanup I'll push fixes for shortly; 2 are existing/intentional design with no code change planned.

Bugs / cleanup to be fixed

actions.rs:65Op::Layer/Op::LayerM never pops the originating oneshot layer.
The exact bug this PR fixes still reproduces when a plain momentary-layer key (not a oneshot-family key) is pressed from within an armed oneshot layer. Repro: capslock = oneshot(macros), and inside [macros], n = layer(nav). Tap capslock (arms macros), then hold n. n resolves against macros and hits Op::Layer, which just calls activate_layer with no pop-check — macros stays active forever.

actions.rs:196Op::Overload's press branch also never pops the originating oneshot layer.
The fix only covers the four Oneshot-family ops, not Overload/Layer/Toggle. Repro: capslock = oneshot(macros), inside [macros], f = overload(fnav, esc). Tap capslock, hold f long enough to activate fnavmacros remains latched active after fnav releases.

actions.rs:291 — the pop-check only decrements oneshot_depth by 1 / calls deactivate_layer once.
If the originating layer's oneshot_depth is already >1 (e.g. tapping the same oneshot key twice before it's consumed), the layer remains active/bound after the "fix" runs. Repro: tap capslock twice (arms oneshot_depth[macros]=2), then press dot — the pop-check deactivates once (2->1, active still >0), so macros is still active and the next key still wrongly resolves against it. Fix: a while oneshot_depth > 0 { deactivate; depth -= 1 } loop, same pattern as clear_oneshot, scoped to the originating layer only.

actions.rs:289 — the pop-check trusts a layer value that can be stale when replayed via pending_overload/pending_timeout.
OverloadState/TimeoutState capture only a layer index (dl, see overload.rs:75, timeout_handler.rs:49/58) and replay it into execute_descriptor an arbitrary amount of time later — long enough for an unrelated oneshot(x) key elsewhere to legitimately arm/consume a latch on the same layer index in between. The pop-check would mis-pop (or wrongly skip) that unrelated latch. Fix planned: snapshot the originating layer's oneshot_depth at the point dl is captured, thread it through execute_descriptor as oneshot_snapshot: Option<u8>, and only pop if the live depth still matches the snapshot at replay time.

actions.rs:289 (and the OneshotMulti arm below) — duplicated pop-check logic.
The 8-line "pop the originating oneshot layer" block is copy-pasted into two match arms instead of centralized — which is exactly why Op::Layer/Op::Overload never got it. Will extract into a shared pop_originating_oneshot_layer helper in layer.rs and call it from all four press branches.

actions.rs:283play_macro_init_async (and the nested-descriptor dispatch) runs before the new pop-check.
Output for this press is produced while the originating oneshot layer is still nominally active. Will reorder so the pop-check runs first, before any macro/nested side effects.

actions.rs:317update_mods(output, -1, 0) was moved out of the else branch to run unconditionally on release.
This duplicates the mod computation already done by the press-time update_mods(output, layer, 0) in the common (latched) path, and wasn't needed for the pop-check fix. Will revert to only running in the else (cancel) branch.

No code change planned (existing/intentional design)

actions.rs:298update_mods(output, layer, 0) excludes layer from the mod aggregation by index regardless of its active-count, same pattern used everywhere else a press excludes its own originating layer (e.g. Op::KeySequence at line 16). Not introduced or worsened by this PR.

actions.rs:289 — the bounds guard (layer >= 0 && (layer as usize) < self.config.layers.len()) is intentional: unlike Op::Swap's dl (always valid), layer here can legitimately be -1 (top-level, no originating oneshot layer), so the guard is load-bearing, not dead code. Will keep it, centralized into the shared helper.

Fix commits incoming on this branch shortly.

Code review of the previous commit found the auto-pop fix incomplete and
partly incorrect:

- Op::Layer/Op::LayerM and Op::Overload could also be triggered from within
  an active oneshot layer but never popped it, so the same lingering-layer
  bug still reproduced through those ops.
- The pop only decremented oneshot_depth/active by 1, so a layer armed twice
  before being consumed (e.g. double-tapping the oneshot key) stayed active
  after the "fix" ran.
- The pop logic was duplicated between the Oneshot-family and OneshotMulti
  arms instead of centralized, which is exactly why Layer/Overload missed it.
- The pop ran after any macro/nested-descriptor side effects instead of
  before them.
- pending_overload/pending_timeout replay a layer index (`dl`) captured at
  press time into execute_descriptor an arbitrary amount of time later; the
  pop-check had no way to tell a stale replay from a fresh one, so it could
  mis-pop an unrelated oneshot latch armed/consumed on the same layer index
  in the meantime.

Extract a single pop_originating_oneshot_layer helper (fully clearing
oneshot_depth via a while loop, same pattern as clear_oneshot) and call it
from all four press branches that can be reached from within an active
oneshot layer. Guard it with an oneshot_depth snapshot captured alongside
`dl` in OverloadState/TimeoutState, threaded through execute_descriptor as
oneshot_snapshot: Option<u8> -- None for every synchronous/immediate call,
Some(snapshot) for the two deferred-replay call sites -- so a stale replay
only pops if the live depth still matches what was captured at press time.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@argenkiwi

Copy link
Copy Markdown
Owner Author

Pushed a fix for all 7 addressed findings in d85f75d — summary:

  • Op::Layer/Op::LayerM and Op::Overload now pop the originating oneshot layer too (previously only the Oneshot-family ops did).
  • The pop fully clears oneshot_depth/active via a loop, so a layer armed more than once before being consumed no longer lingers.
  • Consolidated into a single pop_originating_oneshot_layer helper (layer.rs) called from all four press branches, instead of duplicated inline logic.
  • The pop now runs before any macro/nested-descriptor side effects in the Oneshot-family arms.
  • Reverted the incidental unconditional update_mods call on oneshot release back to the else (cancel) branch only.
  • Guarded the pop-check against stale pending_overload/pending_timeout replay: OverloadState/TimeoutState now snapshot the originating layer's oneshot_depth at capture time, and the pop only proceeds if the live depth still matches at replay time.

Added 4 regression tests covering each of these. All 98 tests pass, cargo clippy -- -D warnings clean.

The two "no code change" findings (mod-aggregation exclusion, bounds guard) still stand as noted above — existing/intentional design, not touched by this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant