Skip to content
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

Merged
merged 44 commits into from
Dec 5, 2023
Merged

keyboard: Use parser for key sequences #2051

merged 44 commits into from
Dec 5, 2023

Conversation

pokey
Copy link
Member

@pokey pokey commented Nov 21, 2023

Checklist

  • I have added tests
  • [-] I have updated the docs and cheatsheet (keeping this somewhat under wraps while we experiment
  • I have not broken the cheatsheet
  • [-] Refactor delete mapping to just issue token for delete action
  • Extract partial parameters
  • [-] Add railroad to docs?

@pokey pokey force-pushed the pokey/keyboard-parser branch 10 times, most recently from d5e5fc4 to a311e68 Compare November 24, 2023 12:38
@@ -28,7 +28,7 @@ trim_trailing_whitespace = false
[Makefile]
indent_style = tab

[**/vendor/**]
[**/{vendor,generated}/**]
Copy link
Member Author

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

@pokey pokey marked this pull request as ready for review November 24, 2023 12:46
@pokey pokey requested review from josharian and AndreasArvidsson and removed request for AndreasArvidsson November 24, 2023 12:46
@pokey
Copy link
Member Author

pokey commented Nov 24, 2023

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

Base automatically changed from pokey/support-mapping-from-key-to-vscode-command-in-cursorless-keyboard-mode to main November 24, 2023 12:53
@pokey pokey force-pushed the pokey/keyboard-parser branch from 43f419e to 6d8c39e Compare November 24, 2023 12:56
@pokey pokey force-pushed the pokey/keyboard-parser branch from 25533ae to 1bc2b31 Compare November 24, 2023 13:02
Copy link
Member Author

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

Copy link
Member Author

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

@pokey
Copy link
Member Author

pokey commented Dec 4, 2023

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.

Copy link
Collaborator

@josharian josharian left a 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)


// We handle multi-key sequences by repeatedly awaiting a single keypress
// until they've pressed something in the map.
while (values.length !== 1) {
Copy link
Member Author

@pokey pokey Dec 5, 2023

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

Copy link
Collaborator

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.

Copy link
Member Author

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?

Copy link
Member Author

@pokey pokey Dec 5, 2023

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

Copy link
Collaborator

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.

@pokey pokey added this pull request to the merge queue Dec 5, 2023
Merged via the queue into main with commit 7a51850 Dec 5, 2023
14 checks passed
@pokey pokey deleted the pokey/keyboard-parser branch December 5, 2023 15:37
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.

3 participants