-
-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
keyboard: Use parser for key sequences #2051
Conversation
d5e5fc4
to
a311e68
Compare
@@ -28,7 +28,7 @@ trim_trailing_whitespace = false | |||
[Makefile] | |||
indent_style = tab | |||
|
|||
[**/vendor/**] | |||
[**/{vendor,generated}/**] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We keep the parser generator output in source control because it's quite short, and includes types so we can type check it, and then imports work without needing a preprocessing step. We have a step in CI that ensures it's up to date, and it auto-updates when you run extension as it's quite quick to generate
ok @Kn0rk here's the parser idea in action. Seems to have worked out well I think. The most interesting file to have a look at is the grammar. I'm planning to record a video of adding a few new grammar rules to show how easy / powerful it is |
43f419e
to
6d8c39e
Compare
25533ae
to
1bc2b31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the generated grammar. note how it's fairly small, and we keep it in a directory called generated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kind of the punchline of this PR: the grammar that defines our modal keyboard interface
ok @josharian I believe I've addressed / responded to all your comments. Here's a diff from when you left your comments, in case helpful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a diff from when you left your comments, in case helpful.
Thanks for the diff! This LGTM. A few minor comments on the way, but as far as I'm concerned, nothing showstopping--don't block on further back and forth from me. (although i will of course continue to converse. :P)
packages/cursorless-vscode/src/keyboard/KeyboardCommandsModalLayer.ts
Outdated
Show resolved
Hide resolved
packages/cursorless-vscode/src/keyboard/sectionKeyMapExtractors.ts
Outdated
Show resolved
Hide resolved
|
||
// We handle multi-key sequences by repeatedly awaiting a single keypress | ||
// until they've pressed something in the map. | ||
while (values.length !== 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that checking this way means that not only can the user press any unique suffix, they can press any unique prefix. So you can either structure your maps to use leading differentiator or trailing differentiator and get the same benefit, eg:
leading:
{
"sf": "namedFunction",
"sl": "line",
"st": "token",
}
or trailing:
{
"fs": "namedFunction",
"ls": "line",
"ts": "token",
}
In either case, if the only thing the current layer is expecting is a scope, then these will shorten to f
, l
, and t
cc/ @josharian @Kn0rk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think(?) that’s good.
The only potential downside I see is that it is hard to mentally model, so minor changes in your bindings can impact other distant bindings.
But let’s just try it out and see. If that proves to be a problem, we have options: Can change approach, or build tooling to generate all possible keymaps, which can be inspected and diffed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, tho I think the suffix behaviour has the exact same problem, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, generating all possible keymaps isn't super easy, because it requires enumerating all possible paths through the parser, which I believe could be a challenge as Earley parsers support nondeterministic CFGs. In practice probably not too bad tho given our grammar size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/me waves hands and adopts Yiddish accent
enough worrying already, you’ll worry your mother to her grave. too many confused users? You should be so lucky.
Checklist