Add missing window keys (<C-w><C-[hjklovq]>) #2600
Merged
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
jpoon
requested changes
May 4, 2018
There was a problem hiding this comment.
Thanks @tyru. Changes here make sense. If you don't mind, it'd be great if you could re-organize these ctrl+<key> alphabetically to make it easier to read. If you could also delete any configs that are binding to more than one key combo (e.g. ctrl+w l) that would be awesome.
package.json
Outdated
| { | ||
| "key": "ctrl+h", | ||
| "command": "extension.vim_navigateCtrlH", | ||
| "when": "editorTextFocus && vim.active && vim.use<C-w> && !inDebugRepl" |
package.json
Outdated
| { | ||
| "key": "ctrl+l", | ||
| "command": "extension.vim_navigateCtrlL", | ||
| "when": "editorTextFocus && vim.active && vim.use<C-w> && !inDebugRepl" |
package.json
Outdated
| { | ||
| "key": "ctrl+j", | ||
| "command": "extension.vim_navigateCtrlJ", | ||
| "when": "editorTextFocus && vim.active && vim.use<C-w> && !inDebugRepl" |
package.json
Outdated
| "key": "ctrl+j", | ||
| "command": "extension.vim_navigateCtrlJ", | ||
| "when": "editorTextFocus && vim.active && vim.use<C-w> && !inDebugRepl" | ||
| }, | ||
| { | ||
| "key": "ctrl+w k", |
There was a problem hiding this comment.
Hmm, don't think these keybindings are removed. We should only be binding to one single key combo here so any keys with ctrl+<key> <key> don't belong here.
jpoon
reviewed
May 4, 2018
package.json
Outdated
| { | ||
| "key": "ctrl+k", | ||
| "command": "extension.vim_navigateCtrlK", | ||
| "when": "editorTextFocus && vim.active && vim.use<C-w> && !inDebugRepl" |
jpoon
reviewed
May 4, 2018
package.json
Outdated
| { | ||
| "key": "ctrl+q", | ||
| "command": "extension.vim_winCtrlQ", | ||
| "when": "editorTextFocus && vim.active && vim.use<C-w> && !inDebugRepl" |
jpoon
reviewed
May 4, 2018
package.json
Outdated
| { | ||
| "key": "ctrl+v", | ||
| "command": "extension.vim_winCtrlV", | ||
| "when": "editorTextFocus && vim.active && vim.use<C-w> && !inDebugRepl" |
Should I sort only added keys, or all keys in |
Travis tests have failedHey @tyru, Node.js: 8npm test --silent; |
|
If you could do all the |
jpoon
reviewed
May 4, 2018
package.json
Outdated
| @@ -330,11 +335,6 @@ | |||
| "command": "list.collapse", | |||
| "when": "listFocus && !inputFocus" | |||
| }, | |||
| { | |||
| "key": "g g", | |||
There was a problem hiding this comment.
We need this one. g g will directly call a vscode command so we need this here. mind putting it back?
There was a problem hiding this comment.
After removing this, gg seems to work.
I'm new to VSCode extension development and I'm not used to package.json and what should I write to contributes.commands. so please correct me if I'm wrong.
But unlike other VSCode extensions, I guess it's enough to lists only one key sequences here to send to VSCodeVim's state machine.
The state machine processes keys one by one, and fire the command if key sequence is matched.
|
Hey @tyru, TravisCI finished with status |
Okay, I'll try it :) |
tyru
changed the title
|
Sorted |
Travis tests have failedHey @tyru, Node.js: 8npm test --silent; |
Sent #2609. But I found another problem. Vim/test/mode/modeInsert.test.ts Line 209 in 16a2348 Removing this line passes the test, however. |
tyru
changed the title
|
Test should pass on CI though? We need that test as we have has special logic to delete that matching The tests already have a mechanism to override the "vim" specific configurations (https://github.com/VSCodeVim/Vim/blob/master/test/testConfiguration.ts), maybe we need to do something similar for the global settings. |
jpoon
reviewed
May 4, 2018
package.json
Outdated
| "key": "ctrl+[", | ||
| "command": "extension.vim_ctrl+[", | ||
| "when": "editorTextFocus && vim.active && vim.use<C-[> && !inDebugRepl" | ||
| "key": "ctrl+k", |
There was a problem hiding this comment.
I think we can merge the two ctrl+k to
{
"key": "ctrl+k",
"command": "extension.vim_ctrl+k",
"when": "editorTextFocus && vim.active && vim.use<C-k> && !inDebugRepl"
},
There was a problem hiding this comment.
Thanks. how about duplicate ctrl+h and ctrl+j?
In the same way, I should merge like this?
{
"key": "ctrl+h",
"command": "extension.vim_ctrl+h",
"when": "editorTextFocus && vim.active && vim.use<C-h> && !inDebugRepl"
},
{
"key": "ctrl+j",
"command": "extension.vim_ctrl+j",
"when": "editorTextFocus && vim.active && vim.use<C-j> && !inDebugRepl"
},
There was a problem hiding this comment.
Oh yeah, I didn't spot those duplicates. That looks good.
jpoon
reviewed
May 4, 2018
package.json
Outdated
| "key": "ctrl+a", | ||
| "command": "extension.vim_ctrl+a", | ||
| "when": "editorTextFocus && vim.active && vim.use<C-a> && !inDebugRepl" | ||
| "key": "ctrl+v", |
There was a problem hiding this comment.
I think we can merge these two ctrl+v as well:
{
"key": "ctrl+v",
"command": "extension.vim_ctrl+v",
"when": "editorTextFocus && vim.active && vim.use<C-v> && !inDebugRepl"
},
jpoon
reviewed
May 4, 2018
| }, | ||
| { | ||
| "key": "g g", |
There was a problem hiding this comment.
I'm fairly certain we need this. When the explorer window is selected, pressing gg will take you to the first item in the list.
There was a problem hiding this comment.
Oooops I'm sorry... entirely I didn't care the explorer window and the when condition`.
Will restore it later.
|
Thanks for sorting the |
Ah, okay, forgot about that 😅 |
|
Okay, fixed problems 🎉 |
Travis tests have failedHey @tyru, Node.js: 8npm test --silent; |
jpoon
approved these changes
May 5, 2018
|
Thanks @jpoon for merging, now this branch is all green. All my branches are left except refactoring |
This was referenced May 5, 2018
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.
What this PR does / why we need it
Some commands are even implemented but only keymappings are missing.
This PR makes it to work just to add the keymappings to invoke the implementation.
Which issue(s) this PR fixes
Fixes #2582, #2467