Basic keyboard features #2169
Merged
Basic keyboard features #2169
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pokey
force-pushed
the
pokey/basic-keyboard-features
branch
from
ce3c1ca to
6d92345
Compare
pokey
force-pushed
the
pokey/add-keyboard-special-target
branch
2 times, most recently
from
2becfef to
a82b28d
Compare
pokey
force-pushed
the
pokey/basic-keyboard-features
branch
2 times, most recently
from
18e3e3f to
73cc06b
Compare
pokey
force-pushed
the
pokey/add-keyboard-special-target
branch
2 times, most recently
from
17643e6 to
bdab379
Compare
pokey
force-pushed
the
pokey/basic-keyboard-features
branch
from
73cc06b to
8fad1fe
Compare
pokey
force-pushed
the
pokey/add-keyboard-special-target
branch
from
bdab379 to
e31466b
Compare
pokey
force-pushed
the
pokey/basic-keyboard-features
branch
from
8fad1fe to
9fc74ca
Compare
pokey
force-pushed
the
pokey/basic-keyboard-features
branch
2 times, most recently
from
8c675e3 to
35b1dd7
Compare
pokey
force-pushed
the
pokey/basic-keyboard-features
branch
from
35b1dd7 to
a9429d1
Compare
pokey
force-pushed
the
pokey/basic-keyboard-features
branch
from
f13d1c6 to
8e65c5e
Compare
|
ok @josharian I'm happy; have a look at my changes and make sure you are too 😊. The first commit should be a fairly unmolested version of your PR, so might be helpful to focus on the diff of the remaining commits |
|
@AndreasArvidsson when you get a minute, feel free to hide everything in keyboard dir and approve if you're happy with the remainder. I'll wait for @josharian's approval on the rest before merging |
josharian
approved these changes
Jan 15, 2024
| @@ -108,6 +108,8 @@ export default class KeyboardCommandsModal { | |||
|
|
|||
| modeOn = async () => { | |||
| if (this.isModeOn()) { | |||
| // Set target to current selection if we re-run mode on | |||
There was a problem hiding this comment.
don't follow this comment (can't even quite parse it)
There was a problem hiding this comment.
While trying to write a better comment, I decided I didn't like the way it was implemented. See https://github.com/cursorless-dev/cursorless/pull/2169/files#diff-9c79f806edb4ea294501b9810d847bd61a9cb17bcfe6930f91ca4cf062a36fc5 for my new solution
packages/cursorless-vscode/src/keyboard/grammar/getAcceptableTokenTypes.ts
Show resolved
Hide resolved
packages/cursorless-vscode/src/keyboard/grammar/grammarHelpers.ts
Outdated
Show resolved
Hide resolved
AndreasArvidsson
approved these changes
Jan 16, 2024
|
ok @josharian all good? |
|
LGTM but s/alread/already/ I assume there’s no VSCode event for when the cursor moves? (Thinking about future automation of keeping the selection in sync.) |
There is, but I'm not sure that's desirable |
|
Yeah, maybe not. |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 20, 2024
The original partial arg code constructed a partial arg where each field of the top-level command arg was either completely filled out or completely missing. Before #2169, that was fine, because the top-level commands were more specific, and thus their args had more detail and the command id itself told you more. For example, in `modifyTargetContainingScopeType`, you had a specific scope type that was missing, but you knew from command id that it was "containing". As of #2169, we do a lot more in the lower level captures. For example `modifyTargetContainingScopeType` and `modifyTargetEveryScopeType` are now just `modifyTarget`, which only has a `modifier` key at the top level, so you don't know until the very end what you're dealing with, because every modifier just looks like `{command: "modifyTarget", partialArg: {modifier: undefined}}`. This PR deeply reconstructs the partial target, so that you can get much more information. See the new test cases for examples of these partial arguments - Depends on #2169 ## Checklist - [x] Switch to `CURRENT` for default arg - [x] Remove unique queue file - [x] I have added [tests](https://www.cursorless.org/docs/contributing/test-case-recorder/) - [-] I have updated the [docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and [cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet) - [-] I have not broken the cheatsheet
bjaspan
pushed a commit
to bjaspan/cursorless
that referenced
this pull request
Jan 22, 2024
- Depends on cursorless-dev#2168 ## Checklist - [x] I have added [tests](https://www.cursorless.org/docs/contributing/test-case-recorder/) - [-] I have updated the [docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and [cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet) - [-] I have not broken the cheatsheet
bjaspan
pushed a commit
to bjaspan/cursorless
that referenced
this pull request
Jan 22, 2024
The original partial arg code constructed a partial arg where each field of the top-level command arg was either completely filled out or completely missing. Before cursorless-dev#2169, that was fine, because the top-level commands were more specific, and thus their args had more detail and the command id itself told you more. For example, in `modifyTargetContainingScopeType`, you had a specific scope type that was missing, but you knew from command id that it was "containing". As of cursorless-dev#2169, we do a lot more in the lower level captures. For example `modifyTargetContainingScopeType` and `modifyTargetEveryScopeType` are now just `modifyTarget`, which only has a `modifier` key at the top level, so you don't know until the very end what you're dealing with, because every modifier just looks like `{command: "modifyTarget", partialArg: {modifier: undefined}}`. This PR deeply reconstructs the partial target, so that you can get much more information. See the new test cases for examples of these partial arguments - Depends on cursorless-dev#2169 ## Checklist - [x] Switch to `CURRENT` for default arg - [x] Remove unique queue file - [x] I have added [tests](https://www.cursorless.org/docs/contributing/test-case-recorder/) - [-] I have updated the [docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and [cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet) - [-] I have not broken the cheatsheet
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist