Add VS Code keys and improve keybinding documentation #4392
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
carlos-zamora
requested changes
Jan 29, 2020
| ### Accepted Modifiers and Keys | ||
|
|
||
| #### Modifiers | ||
| `Ctrl+`, `Shift+`, `Alt+` |
There was a problem hiding this comment.
Should we add a note here...
- You can bind a command to a key-combo without modifiers, but that's not a good idea...
- We recommend not binding to
ctrl + <key>because it may conflict with common keybindings from command line applications. (i.e.: CTRL+C should not be bound to stuff willy nilly, or else you won't be able to stop processes)
There was a problem hiding this comment.
I'm not sure if this is necessary?
ghost
added
Needs-Author-Feedback
carlos-zamora
requested changes
Jan 29, 2020
doc/cascadia/SettingsSchema.md
Outdated
| | find | Open the search dialog box. | | | | | ||
| | increaseFontSize | Make the text larger by one delta. | `delta` | integer | Amount of size increase per command invocation. | | ||
| | moveFocus | Focus on a different pane depending on direction. | `direction` | `left`, `right`, `up`, `down` | Direction which the focus will move. | | ||
| | newTab | Create a new tab. | - `commandLine`<br>- `startingDirectory`<br>- `tabTitle`<br>- `index`<br>- `profile` | - string<br>- string<br>- string<br>- integer<br>- string | - Executable run within the tab.<br>- Directory in which the tab will open.<br>- Title of the new tab.<br>- Profile that will open based on its position in the dropdown (starting at 0).<br>- Profile that will open based on its GUID or name. | |
There was a problem hiding this comment.
Can we mention that by default, we open a tab with the default profile. Also, same problem as above: which args are required and what do they default to. This one is probably the most complicated one.
There was a problem hiding this comment.
IIRC, none of these args are required.
doc/cascadia/SettingsSchema.md
Outdated
| | scrollUp | Move the screen up. | | | | | ||
| | scrollUpPage | Move the screen up a whole page. | | | | | ||
| | scrollDownPage | Move the screen down a whole page. | | | | | ||
| | splitPane | Separate the active pane into two. | - `split`<br>- `commandLine`<br>- `startingDirectory`<br>- `tabTitle`<br>- `index`<br>- `profile` | - `vertical`, `horizontal`, `auto`<br>- string<br>- string<br>- string<br>- integer<br>- string | - How the pane will split. `auto` will split in the direction that provides the most surface area.<br>- Executable run within the pane.<br>- Directory in which the pane will open.<br>- Title of the tab when the new pane is focused.<br>- Profile that will open based on its position in the dropdown (starting at 0).<br>- Profile that will open based on its GUID or name. | |
There was a problem hiding this comment.
Separate the active pane into two.
False.
Also, this will have similar problems to newTab
ghost
added
the
Needs-Author-Feedback
ghost
removed
the
Needs-Author-Feedback
zadjii-msft
requested changes
Jan 31, 2020
ghost
added
the
Needs-Author-Feedback
zadjii-msft
added
Area-Settings
Co-Authored-By: Mike Griese <migrie@microsoft.com>
ghost
removed
the
Needs-Author-Feedback
Co-Authored-By: Mike Griese <migrie@microsoft.com>
Co-Authored-By: Mike Griese <migrie@microsoft.com>
Co-Authored-By: Mike Griese <migrie@microsoft.com>
Co-Authored-By: Mike Griese <migrie@microsoft.com>
Co-Authored-By: Mike Griese <migrie@microsoft.com>
Co-Authored-By: Mike Griese <migrie@microsoft.com>
carlos-zamora
approved these changes
Feb 4, 2020
| | find | Open the search dialog box. | | | | | ||
| | increaseFontSize | Make the text larger by one delta. | `delta` | integer | Amount of size increase per command invocation. | | ||
| | moveFocus | Focus on a different pane depending on direction. | `direction`* | `left`, `right`, `up`, `down` | Direction in which the focus will move. | | ||
| | newTab | Create a new tab. Without any arguments, this will open the default profile in a new tab. | 1. `commandLine`<br>2. `startingDirectory`<br>3. `tabTitle`<br>4. `index`<br>5. `profile` | 1. string<br>2. string<br>3. string<br>4. integer<br>5. string | 1. Executable run within the tab.<br>2. Directory in which the tab will open.<br>3. Title of the new tab.<br>4. Profile that will open based on its position in the dropdown (starting at 0).<br>5. Profile that will open based on its GUID or name. | |
There was a problem hiding this comment.
Include format to put GUID in there? Not sure if it's "{...}" or "xxx-...-xxx". Minor but the schema doesn't cover this, surprisingly.
| | scrollUp | Move the screen up. | | | | | ||
| | scrollUpPage | Move the screen up a whole page. | | | | | ||
| | scrollDownPage | Move the screen down a whole page. | | | | | ||
| | splitPane | Halve the size of the active pane and open another. Without any arguments, this will open the default profile in the new pane. | 1. `split`*<br>2. `commandLine`<br>3. `startingDirectory`<br>4. `tabTitle`<br>5. `index`<br>6. `profile` | 1. `vertical`, `horizontal`, `auto`<br>2. string<br>3. string<br>4. string<br>5. integer<br>6. string | 1. How the pane will split. `auto` will split in the direction that provides the most surface area.<br>2. Executable run within the pane.<br>3. Directory in which the pane will open.<br>4. Title of the tab when the new pane is focused.<br>5. Profile that will open based on its position in the dropdown (starting at 0).<br>6. Profile that will open based on its GUID or name. | |
There was a problem hiding this comment.
I like the "[-]" and "[|]" example. I think it's in the schema. But that would be a nice to have for 1 anyways.
5 tasks
ghost
requested review from
miniksa
approved these changes
Feb 4, 2020
DHowett-MSFT
approved these changes
Feb 4, 2020
| buffer += pair.first; | ||
| serializedSuccessfully = true; | ||
| foundKey = true; | ||
| break; |
There was a problem hiding this comment.
i'm glad you made the serialization more expensive honestly - we shouldn't be using it often. parsing happens all the dang time.
|
Format dat code! |
|
THANK YOU for this merged PR! I was the author of related #3140 - I am so excited to see this shipped! 😄 Thanks, all! |
|
🎉 Handy links: |
ghost
mentioned this pull request
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.
Summary of the Pull Request
Add keybinding keys to be compatible with VS Code conventions: https://code.visualstudio.com/docs/getstarted/keybindings#_accepted-keys
Improve keybinding documentation because we desperately need it. :)
(Documentation will definitely be iterated on, just wanted to get this PR rolling.)
References
#3809
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
Tested the new keys