search

jtroo / kanata

Improve keyboard comfort and usability with advanced customization
GNU Lesser General Public License v3.0
2.92k stars 124 forks source link

Bug(?): Difficulties with consecutive modified input-keys in sequence key-lists #956

Closed antler5 closed 6 months ago

antler5 commented 6 months ago

Requirements

Describe the bug

I believe that sequences such as (defseq seq-name (a S-b S-c)) are intended to be valid when written as such and typed without lifting LSFT between b and c. In practice, sequences with consecutive shifted characters don't activate like that. I demonstrate this by defining two sequences, aBa => x and aBB => y, and show that the latter always falls back to the former. This behavior is reflected in the online simulator.

If the current behavior is intended it just needs to be explicit, but IMO it shouldn't matter if we press and release modifiers in-between letters in sequences either (as long as we continue within sequence-timeout). This would conflict with the use of a modifier as a dedicated key in a sequence, and you never know what people (like me?) will want, so I'm open to semantics and/or configuration options supporting both possibilities -- just something to have in mind while patching for this issue.

Aside: I initially believed that enabling visible-backspaced consistently caused LSFT to be left Down, as I observed that behavior and had to escape out of it with lctl+spc+esc even after resetting to master. I realized while writing the report that it made sense, because when visible-backspaced isn't enabled LSFT probably gets eaten anyways, so of course it would only surface then -- but now I can't reproduce it??? So this will be the only aside about it until I figure out what was going on. I do still have logs, but the setup was different.

I've added my own debug statements to elucidate the issue, which is related to whether modifiers are removed from Sequence TrieKey's after being encoded into unused KeyCode bits at both parse- and run-time. They are encoded with a Press of shift explicitly preceding each shifted key, but at run-time users expect to be able to press shift once and hold it. This currently results in an invalid sequence, or in this case, a fall-back to another sequence with only the initial shift-press. Since the mods are encoded into the KeyCode they should probably just be removed from Sequence keys entirely, but I'm not sure if this should happen in parse_action_atom, in the Sequence-specific code, how it might effect other contexts, or why the modifier backtracking exists in the first-place (Chesterton fence).

~I don't know why LSFT isn't released when visible-backspaced is enabled, though it occurs to me as I type this that it's probably never Press'ed in the first place otherwise.~

Relevant kanata config

(defcfg sequence-timeout 2000)

(defseq ex (S-b   b))
(defseq wy (S-b S-b))

(deffakekeys ex (macro x))
(deffakekeys wy (macro y))

(defsrc
  lsft a    b c rsft)
(deflayer base
  lsft sldr b c rsft)

To Reproduce

The online simulator can demonstrate this. Here we input aBb, aBB (holding shift), and aBB (with two shifts), observing the outputs x (correct), x (erroneous), and y (correct).

https://jtroo.github.io?data=BQEw...qggA

~But (if I'm reading correctly) it does not reflect that LSFT remains held when visible-backspaced is enabled.~

Expected behavior

What Should Happen

aBbc => xc
aBBc => yc

What Actually Happens

Without visible-backspaced:

aBbc => xc
aBBc => xc

~With visible-backspaced:~

aBbc => XC
aBBc => XC
;; + LSFT is never released.

Kanata version

kanata 1.6.0-prerelease-4 (d029299)

Debug logs

Without visible-backspaced:

Input aBb: Correctly prints x.

32:38.53045 [DEBUG] (1) kanata_parser::cfg: parsing sequence key-list: [S-b, b]
32:38.53052 [DEBUG] (1) kanata_parser::cfg: registered new sequence: [32810, 32816, 48]
32:38.53057 [DEBUG] (1) kanata_parser::cfg: parsing sequence key-list: [S-b, S-b]
32:38.53064 [DEBUG] (1) kanata_parser::cfg: registered new sequence: [32810, 32816, 32810, 32816]
[...]
32:42.91310 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_A, value: Press }
32:42.91333 [DEBUG] (3) kanata_state_machine::kanata: entering sequence mode
32:42.97450 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_A, value: Release }
32:43.09406 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_LEFTSHIFT, value: Press }
32:43.09427 [DEBUG] (3) kanata_state_machine::kanata: sequence got: LShift
32:43.09439 [DEBUG] (3) kanata_state_machine::kanata: current sequence: [32810]
32:43.22954 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_B, value: Press }
32:43.22971 [DEBUG] (3) kanata_state_machine::kanata: sequence got: B
32:43.22978 [DEBUG] (3) kanata_state_machine::kanata: current sequence: [32810, 32816]
32:43.28615 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_B, value: Release }
32:43.28631 [DEBUG] (3) kanata_state_machine::kanata: key release   B
32:43.28641 [DEBUG] (3) kanata_state_machine::oskbd::linux: send to uinput: InputEvent { time: SystemTime { tv_sec: 0, tv_nsec: 0 }, kind: Key(KEY_B), value: 0 }
32:43.34977 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_LEFTSHIFT, value: Release }
32:43.34991 [DEBUG] (3) kanata_state_machine::kanata: key release   LShift
32:43.34997 [DEBUG] (3) kanata_state_machine::oskbd::linux: send to uinput: InputEvent { time: SystemTime { tv_sec: 0, tv_nsec: 0 }, kind: Key(KEY_LEFTSHIFT), value: 0 }
32:43.54483 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_B, value: Press }
32:43.54498 [DEBUG] (3) kanata_state_machine::kanata: sequence got: B
32:43.54503 [DEBUG] (3) kanata_state_machine::kanata: current sequence: [32810, 32816, 48]
32:43.54509 [DEBUG] (3) kanata_state_machine::kanata: sequence complete; tapping fake key
32:43.54516 [DEBUG] (3) kanata_state_machine::kanata: key release   B
32:43.54520 [DEBUG] (3) kanata_state_machine::oskbd::linux: send to uinput: InputEvent { time: SystemTime { tv_sec: 0, tv_nsec: 0 }, kind: Key(KEY_B), value: 0 }
32:43.54645 [DEBUG] (3) kanata_state_machine::kanata: key press     X
32:43.54655 [DEBUG] (3) kanata_state_machine::oskbd::linux: send to uinput: InputEvent { time: SystemTime { tv_sec: 0, tv_nsec: 0 }, kind: Key(KEY_X), value: 1 }
32:43.54779 [DEBUG] (3) kanata_state_machine::kanata: key release   X
32:43.54786 [DEBUG] (3) kanata_state_machine::oskbd::linux: send to uinput: InputEvent { time: SystemTime { tv_sec: 0, tv_nsec: 0 }, kind: Key(KEY_X), value: 0 }
32:43.59984 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_B, value: Release }

Input aBB: Erroneously prints x.

26:42.08346 [DEBUG] (1) kanata_parser::cfg::alloc: freeing allocations of length 0
26:42.08325 [DEBUG] (1) kanata_parser::cfg: parsing sequence key-list: [S-b, b]
26:42.08398 [DEBUG] (1) kanata_parser::cfg: registered new sequence: [32810, 32816, 48]
26:42.08353 [DEBUG] (1) kanata_parser::cfg: parsing sequence key-list: [S-b, S-b]
26:42.08399 [DEBUG] (1) kanata_parser::cfg: registered new sequence: [32810, 32816, 32810, 32816]
[...]
26:50.68246 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_A, value: Press }
26:50.68269 [DEBUG] (3) kanata_state_machine::kanata: entering sequence mode
26:50.74365 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_A, value: Release }
26:50.86443 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_LEFTSHIFT, value: Press }
26:50.86465 [DEBUG] (3) kanata_state_machine::kanata: sequence got: LShift
26:50.86472 [DEBUG] (3) kanata_state_machine::kanata: current sequence: [32810]
26:51.04731 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_B, value: Press }
26:51.04750 [DEBUG] (3) kanata_state_machine::kanata: sequence got: B
26:51.04757 [DEBUG] (3) kanata_state_machine::kanata: current sequence: [32810, 32816]
26:51.10808 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_B, value: Release }
26:51.10822 [DEBUG] (3) kanata_state_machine::kanata: key release   B
26:51.10828 [DEBUG] (3) kanata_state_machine::oskbd::linux: send to uinput: InputEvent { time: SystemTime { tv_sec: 0, tv_nsec: 0 }, kind: Key(KEY_B), value: 0 }
26:51.31445 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_B, value: Press }
26:51.31467 [DEBUG] (3) kanata_state_machine::kanata: sequence got: B
26:51.31474 [DEBUG] (3) kanata_state_machine::kanata: current sequence: [32810, 32816, 32816]
26:51.31482 [DEBUG] (3) kanata_state_machine::kanata: attempting to backtrack
26:51.31488 [DEBUG] (3) kanata_state_machine::kanata: found alternative sequence
26:51.31492 [DEBUG] (3) kanata_state_machine::kanata: sequence complete; tapping fake key
26:51.31501 [DEBUG] (3) kanata_state_machine::kanata: key release   LShift
26:51.31505 [DEBUG] (3) kanata_state_machine::oskbd::linux: send to uinput: InputEvent { time: SystemTime { tv_sec: 0, tv_nsec: 0 }, kind: Key(KEY_LEFTSHIFT), value: 0 }
26:51.31512 [DEBUG] (3) kanata_state_machine::kanata: key release   B
26:51.31516 [DEBUG] (3) kanata_state_machine::oskbd::linux: send to uinput: InputEvent { time: SystemTime { tv_sec: 0, tv_nsec: 0 }, kind: Key(KEY_B), value: 0 }
26:51.31633 [DEBUG] (3) kanata_state_machine::kanata: key press     X
26:51.31638 [DEBUG] (3) kanata_state_machine::oskbd::linux: send to uinput: InputEvent { time: SystemTime { tv_sec: 0, tv_nsec: 0 }, kind: Key(KEY_X), value: 1 }
26:51.31649 [DEBUG] (3) kanata_state_machine::kanata: key release   X
26:51.31654 [DEBUG] (3) kanata_state_machine::oskbd::linux: send to uinput: InputEvent { time: SystemTime { tv_sec: 0, tv_nsec: 0 }, kind: Key(KEY_X), value: 0 }
26:51.38932 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_B, value: Release }
26:51.63131 [DEBUG] (3) kanata_state_machine::kanata: process recv ev KeyEvent { code: KEY_LEFTSHIFT, value: Release }

~With visible-backspaced:~

~▸ Details~

Operating system

Linux (Ubuntu-based)

Additional context

I've been poking around re: #950, doing awful things that shouldn't be up-streamed, and finally hit a bug that wasn't my fault :p

The parser seems to have a good pile of tests, but I don't see a place to put eg. a Simulator file for post-parsing tests?

antler5 commented 6 months ago

Aside: I initially believed that enabling visible-backspaced consistently caused LSFT to be left Down, as I observed that behavior and had to escape out of it with lctl+spc+esc even after resetting to master. I realized while writing the report that it made sense, because when visible-backspaced isn't enabled LSFT probably gets eaten anyways, so of course it would only surface then -- but now I can't reproduce it??? So this will be the only aside about it until I figure out what was going on.

Diffed the logs. What happened was that I was using RSFT to trigger the sequence initially, and the problem disappeared because I started using LSFT instead, which masks the issue by triggering a manual Release. If I trigger the sequence with RSFT, I can again observe that LSFT is Press'ed from before/during the sequence-bound macro's execution (causing unintended caps) until I manually tap it. I tried making the same swap in the simulator, but this bug does not appear to be reflected there.

I haven't checked to see what the status quo and history is (love the historical issues/discussions), but IMO modifiers should be released before macros are executed, suppressed, and restored if still held -- the burden should be on the user to define multiple macros / sequences if they want eg. abbr => abbreviation, Abbr => Abbreviation, and ABBR => ABBREVIATION.

I'm not making a separate issue yet because these are basically my working notes and I think it's likely that the same code is at fault, but this is definitely a separate (and way less preference-based) bug that could warrant it's own issue if it ends up leading to another part of the code-base.

jtroo commented 6 months ago

You need to wrap the two b in parens:

Sample

jtroo commented 6 months ago

As an aside the design of sequence chords is here: https://github.com/jtroo/kanata/blob/main/docs/sequence-adding-chords-ideas.md

I should probably make sure all the useful bits out of that document make it into config.adoc.

antler5 commented 6 months ago

Awesome, thanks! I thought I'd have seen anything like that by now, crawling the web of code around parse_mod_prefix; must be tunnel vision. Heh, still not working for my config but now I know it's on my end; I'll go find where grouping is implemented.

And my apologies, all these beautiful docs and examples and I haven't given them a through read-through; I see how this stemmed from the Press-first approach. Thank you for taking the time! <3

That just leaves the RSFT behavior, which does persist. If we haven't got anything up our sleeves for that one, I'll be rooting around at least a little more (before (hopefully) reaching set-and-forget stability), and won't let it go un-grokked. Thx!

IMO it shouldn't matter if we press and release modifiers in-between letters in sequences either

parens

Oh, that means (S-(b b)), and (S-b S-b)) are/can-be different -- I'm sure I can tweak that into matching my pref, and really, I'm just getting what I asked for :p

the burden should be on the user to define multiple macros / sequences

force users to be clear about what they mean and define the extra permutations, if they want them

hehe, exactly -- and I'm sure the macros'll be everything I could hope for c:

jtroo commented 6 months ago

The parser seems to have a good pile of tests, but I don't see a place to put eg. a Simulator file for post-parsing tests?

This might be what you're looking for: https://github.com/jtroo/kanata/tree/main/src/tests/sim_tests