Ikcelaks / qmk_sequence_transform

Sequence-Transform is a user library for QMK that enables a rich declarative ruleset for transforming a sequence of keypresses into any output you would like.
Apache License 2.0
6 stars 3 forks source link

[BUG] Unhandled error on sequence token being present in the transform string #63

Open QKekos opened 7 months ago

QKekos commented 7 months ago

So if user anyhow puts a token into the transform string - manually or with the help of \1 and regex group, containing sequence token, doesn't matter, generator crashes with default python stack trace, which is not really helpful to user

QKekos commented 7 months ago

also would be actually nice to let users put tokens in the transform string so generator could've unpack them to the actual strings, like a fallback buffer

Ikcelaks commented 7 months ago

It is an error to have sequence tokens in the completion string (the part of the transform that isn't cancelled out by being part of the common prefix of the expanded sequence and transform).

We should detect this and exit with a clear error instead of crashing though

QKekos commented 7 months ago

It happens even if you have it in the cancelling out part of the rule:

c★           ⇒ con
c★f          ⇒ c★fig

crashes

Also I'm against keeping error there, we can actually parse and replace all the tokens in completion strings, like it doesn't really matter for manual placement, but not being able to use them in regex groups - ⎵(con)?sq★ ⇒ \1sequence, is not nice, like I can't put c☆ there having ⎵c☆ ⇒ con rule

kamih commented 7 months ago

We have to print an error if there's any seq token left in the completion string. For backspace rules there's no way for the user to add backspaces in the transform to explain what they really want. For example, they can't write c*l -> c*<ol for cool, < here being a backspace. So transforms in the rule definitions just shouldn't have seq tokens at all IMO. they should just be the fully transformed string, and the parser figures out what it needs to do for that rule to work with other rules (which it already does).

Ikcelaks commented 7 months ago

It happens even if you have it in the cancelling out part of the rule:

c★           ⇒ con
c★f          ⇒ c★fig

crashes

Also I'm against keeping error there, we can actually parse and replace all the tokens in completion strings, like it doesn't really matter for manual placement, but not being able to use them in regex groups - ⎵(con)?sq★ ⇒ \1sequence, is not nice, like I can't put c☆ there having ⎵c☆ ⇒ con rule

The c* doesn't cancel out, because the sequence gets expanded before cancelling happiness. So conf gets compared to c*fig and we end up trying to backspace the times and completing with *fig and * is unprintable.

Sequence tokens should basically never be in the transform. ** -> *n is a trick that relies on the fact that there isn't a rule that matches on just *. The user should not generally write rules this way.

The intention is that the sequence is the exact sequence of key presses, and the transform is the full output. Rules that use the fallback buffer break the first rule, but the second rule can't be broken in general.

QKekos commented 7 months ago

Sequence tokens should basically never be in the transform.

And I'm saying that that's wrong, like sure, that's how the code works now, but not how it should work, like just the generator expansion like we have for regexes, actual trie won't actually store any tokens in completion string