-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Refactor Remapper #4735
Refactor Remapper #4735
Conversation
Refactor the Remapper and ModeHandler to allow better remapping experience. It will allow to remap operator keys, motion keys and multiple keys when the first key could be handled. Should fix the following issues (maybe more): VSCodeVim#4674 VSCodeVim#4464 VSCodeVim#3988 VSCodeVim#3768 VSCodeVim#3742 VSCodeVim#2975 VSCodeVim#2955 VSCodeVim#2234 VSCodeVim#2041 VSCodeVim#1870 VSCodeVim#1821 VSCodeVim#1579 VSCodeVim#1398 Needs more testing.
Travis tests have failedHey @berknam, Node.js: 12if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; fi
npm test
TravisBuddy Request Identifier: aba8ded0-8ce2-11ea-a7f4-9bc5993473fa |
- Rename variables to better describe their use - Fix some bugs with potential remaps that weren't ambiguous remaps - Add debugging logs to the new mechanisms - Fixed timeout being called twice when there was still a potential remap after the first key, even though the user had already waited - Remove unused variable
This Pull Request started out as a proof of concept, however I think that now it is working pretty well and could be added. I will add some tests for the new ambiguous maps. But when it comes to the timeouts my idea was to create a remapping, set the 'timeout' time in config, send the keys of the remapping, check that it hasn't changed anything, create a timeout with the 'timeout' config plus an offset (to make sure it finishes) to wait for the remapping to be handled and applied, and then check the final result. If someone has a better idea I would really appreciate. Some things to help understand the changes:
If someone could test this and give me some feedback I would really appreciate! |
Fantastic, thanks @berknam for the impactful changes and detailed explanation! I'll take a look ASAP. |
@@ -52,7 +52,8 @@ export class BaseAction { | |||
public doesActionApply(vimState: VimState, keysPressed: string[]): boolean { | |||
if ( | |||
this.mustBeFirstKey && | |||
vimState.recordedState.commandWithoutCountPrefix.length > keysPressed.length | |||
(vimState.recordedState.commandWithoutCountPrefix.length > keysPressed.length || | |||
vimState.recordedState.operator) |
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 needed because the actions that must be first key can't come after an operator and since I'm resetting the commandList on modeHandler
runAction
function the commandWithoutCountPrefix
might still have the same length so we need to check if there is no operator.
This looks great. If you could post a vsix I would install and test that. |
@sqlkoala what's your email address? |
Hi, its
...
got it.
…On 07.05.2020 20:48, Jason Fields wrote:
@sqlkoala <https://github.com/sqlkoala> what's your email address?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4735 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHH2F5KJWXK255MKWEPFRLDRQL7AJANCNFSM4MHEHBKA>.
|
Ah, actually gmail won't let me send a |
My first feedback: One other thing: I switch j and k -- dont ask why ;-)
|
Thanks @sqlkoala for the feedback. The issues you found happen because VSCodeVim doesn't have an Operator Pending Mode. When it would be in operator mode in vim/nvim VSCodeVim is still in normal mode, so all remaps from normal mode apply and it waits for the possible 'w' after 'dd'. I could try to introduce the Operator Pending Mode with this PR, because I think this new remapper makes it easier to implement. But I don't know if it should be done in this PR or a different one? @J-Fields could you maybe give me a hint here? Also there is still an issue with waiting twice for the timeout in some situations on the version you got, that I found now when making the tests for this PR. I will push the fix ASAP. |
- The previous fix only worked for ambiguous remaps, now works for all - Fix the remapping being changed, now we make a copy of remapping when adding the timeout ending key
Travis tests have failedHey @berknam, Node.js: 12if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; fi
npm test
TravisBuddy Request Identifier: 70c903c0-91ed-11ea-9e78-458f7b3d5a30 |
Travis tests have failedHey @berknam, Node.js: 12if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; fi
npm test
TravisBuddy Request Identifier: d3e5de90-91ef-11ea-9e78-458f7b3d5a30 |
to prevent any timing issue related to the performance of the testing machine
Travis tests have failedHey @berknam, Node.js: 12if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; fi
npm test
TravisBuddy Request Identifier: ae2c5bf0-91f1-11ea-9e78-458f7b3d5a30 |
@J-Fields The conflicts are resolved. |
Thanks 🙂 |
|
Yes. I know about this. It is on my todo list. Thats only a visual issue because it is no longer on OperatorPendingMode but we need to call the I've been thinking of splitting the updateView function in multiple functions like I will push a fix for that as well later after all the review, that should be a simple fix. |
Cool, easier to fix if it's just the view being out of sync with the real state
I've thought about this as well. I think it probably makes sense to split all the view management logic into its own class (an instance of which would be held by |
There is already an // There was no action run yet but we still want to update the view to be able
// to show the potential remapping keys being pressed, the `"` character when
// waiting on a register key or the `?` character and any following character
// when waiting on digraph keys. The 'oldWaitingForAnotherActionKey' is used
// to call the updateView after we are no longer waiting keys so that any
// existing overlapped key is removed.
if (
(this.vimState.currentMode === Mode.Insert || this.vimState.currentMode === Mode.Replace) &&
(this.vimState.recordedState.bufferedKeys.length > 0 ||
this.vimState.recordedState.waitingForAnotherActionKey ||
oldWaitingForAnotherActionKey !== this.vimState.recordedState.waitingForAnotherActionKey)
) {
await this.updateView(this.vimState);
} Right now it's just a matter of adding the check for a changed |
@berknam Whenever you've got a chance, fix up the handful of nitpicks I had and push again. After a skim it all looks pretty reasonable, I'll try to go back and re-read the more complicated bits tomorrow and do some manual testing. Thanks again, I'm super excited to get this merged! |
- Change 'handleKeyAsAnAction' to return true when an action is run and false otherwise so that we can prevent calling updateView twice just to update some decorations
Finally merged! 🙌 Thanks again, @berknam! |
@J-Fields Thank you for taking the time to go through this. This really became a beefy PR! 😅 Feel free to assign to me any new issue from If we start seeing a lot of people coming with issues that are not really issues but just a case of them being used to the way it worked before, maybe we can then create and pin an issue explaining the change. |
So excited for this. As a colemak user the existing remapping system was so buggy as to be unusable and I eventually gave up on it and went back to neovim. As soon as there is a new release with this remapping code (any idea when that might be?) I'll be trying this out. |
Aiming for some time this week, not sure exactly when. Still dogfooding and trying to tie up a few more loose ends before then |
Hi @J-Fields, wondering if there are any updates in regards to a release containing this PR? |
@J-Fields any news? |
@sangonzal @akuseru1 Sorry - been busy with other things recently. Please see this comment. |
Hi, |
@grozan Why would you use |
@berknam I've always used
This is working fine with 1.16.0. Is there is a workaround, or another syntax, to come back to normal mode at the end? |
The reason this fails now is because we introduced a way of stopping infinite recursive mapping loops like you can do in Vim. In Vim you would use the key <C-c> but since some users might not allow that key to be handled by VSCodeVim we added the <Esc> key. This is what is happening to you, because you are doing a recursive remap and then we receive and "Escape" key like if it was pressed by the user it tries to force stop the remap. The correct way you should do your remap above should be: "vim.visualModeKeyBindings": [
{
"before": ["<leader>","c","y"],
"after": ["<Esc>"],
"commands": [
"editor.action.commentLine",
"editor.action.copyLinesDownAction",
"editor.action.commentLine",
]
}
] Bear in mind, though, that the action from "after" runs first and then the actions from commands run. In this case that is not an issue, but if you have a situation where you need to escape after all commands finish running then you have to use a "NonRecursive" map like this: "vim.visualModeKeyBindingsNonRecursive": [
{
"before": ["<leader>","c","y"],
"commands": [
"editor.action.commentLine",
"editor.action.copyLinesDownAction",
"editor.action.commentLine",
"extension.vim_escape"
]
}
] |
@berknam thanks a lot, it's clear now |
What this PR does / why we need it:
Refactor the Remapper and ModeHandler to allow better remapping experience.
It will allow to remap operator keys, motion keys and multiple keys when the first key could be handled.
It also implements the timeoutlen and OperatorPendingMode.
Which issue(s) this PR fixes
Should fix the following issues (maybe more):
fixes #5125
fixes #5084
fixes #5067
fixes #5057
fixes #5016
fixes #4991
fixes #4928
fixes #4883
fixes #4756
fixes #4674
fixes #4563
fixes #4353
fixes #4532
fixes #4530
fixes #4464
fixes #4236
fixes #4118
fixes #4057
fixes #3988
fixes #3373
fixes #3768
fixes #3742
fixes #3413
fixes #3171
fixes #3086 (except for the SelectMode, maybe a new issue with this feature request should be opened)
fixes #3082
fixes #2975
fixes #2955
fixes #2897 (although this one will still be missing the language mapping that is in Todo)
fixes #2466
fixes #2234
fixes #2041
fixes #1883
fixes #1870
fixes #1835
fixes #1821
fixes #1579
fixes #1398
fixes #1261
There was an issue with timings after 'v' visual mode, that I don't know exactly what caused but this PR fixed it. Like when you pressed 'v' to enter visual mode and then pressed a remapped key too quickly it wouldn't work.
There is also a lot of old issues (back when there still was a 'otherModesKeyBindings') that would probably be fixed by this (I think some of them are already fixed for a long time but no one closed them).
I'm still looking up the issues list when I have time and I will continue to add to this list.
Special notes for your reviewer:
Needs more testing. I don't really know how we should implement tests for the timeout part of the remap.I've created a new way of making tests with remaps that allows to create a few tests all with the same remaps by using an array of steps with the keypresses and end result of each step.
If someone could test this out and give me some feedback I would really appreciate. I think this refactor could solve alot of issues and also brings VSCodeVim closer to Vim when it comes to remapping experience.
Todo:
<silent>, <nowait>, <buffer> and <special>
arguments. Currently the arguments are ignored, but the mapping is done. I think the only arguments that make sense for us is the<nowait>
and maybe the<silent>
but I don't think this is needed for now, if someone wants it they can open an issue and then we'll see. The mappings with<expr> or <Plug>
will not be allowed. (Refs .vimrc support #463 )map ab abcd
will execute 'a' command and insert 'bcd'. The 'ab' in the {rhs} will not be mapped again.map x y
withmap y x
. It still does not catchmap g wg
, because thew
is used before the next mapping is done.<leader>s) -> f)i<space><esc>
when you press<leader>s)
and there is no)
to be found it shouldn't insert any space.fill= 10I=<Esc>
,fill- 10I-<Esc>
and<leader>f fill
when you press<leader>f
it remaps tofill
and waits for another key or timeout if you then pressx
it it handles thefill
and if it fails to find 'i' then the 2 'l's are not executed but the user pressed 'x' is still executed.<C-c>
but since the user might stop ctrl keys from being used I decided to implement<C-c>
and<Esc>
. Example: if you remap<leader>sp -> i"<esc>la",<space><esc>l<leader>sp
to split a word like thishello -> "h", "e", "l", "l", "o",
you can stop it from continuing to other words by pressing<C-c>
or<Esc>
."
after pressing<C-r>
and?
after pressing<C-k>
in the same way- [ ] Implement operatorPendingMode Modifiers 'v' and 'V'Right now this one won't be easy to do because it would require changing a lot of the existing actions since currently there is no notion ofinclusive/exclusive
in VSCodeVim, there is only the notion oflinewise/characterwise
. After this PR gets merged we can create an issue for this to see if it is something really needed or not- [ ] Implement language mapping (I'm postponing this one for later as well. I think right now the most important thing is to get this PR merged and then the one which implements the plugins with remaps. And after that we can think about adding this one. This PR is already too broad so lets not make it even worse.'lm[ap]'
and'ln[oremap]'
) to allow mapping characters after the commands that accept characters likef, F, t, T, r
or the surround and easymotion target keys. (This is not the langmap setting!)This should make life easier for anyone using a different keyboard layout like Dvorak or different input modes. (Refs #2234 and #2217 )