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

Perform remapped commands when prefix by a number #1359

Merged
merged 5 commits into from
Mar 15, 2017

Conversation

bdauria
Copy link
Contributor

@bdauria bdauria commented Mar 4, 2017

This PR allows remapped commands to perform correctly when prefixed by a count.

The modified code makes sure that only the command uses its remapped version, and not the following text objects.

For instance, if the command l is remapped to c, and w to é performing: 2liw will still be translated to 2ciw and not 2cié (this is the current behaviour for normal, non-count commands).

The code uses keymappings defined in otherModesNonRecursive. Should recursive mappings be supported in this case as well ?

This should close the following issue: #1233

@johnfn
Copy link
Member

johnfn commented Mar 7, 2017

Not ignoring this, just waiting for some time to properly review it...

@bdauria
Copy link
Contributor Author

bdauria commented Mar 7, 2017

Sure, no worries take the time you need for this.

@johnfn
Copy link
Member

johnfn commented Mar 10, 2017

The main thing that I don't understand about this PR is that we now appear to be doing remapping in two different places. One in getRelevantAction, and one here: https://github.com/VSCodeVim/Vim/blob/master/src/mode/modeHandler.ts#L781

First off, does this assessment sound correct to you?

Second off, if that is true, then do you think you could make this PR such that all remapping happens in getRelevantAction? My gut sense tells me that is the place that remapping belongs.

@bdauria
Copy link
Contributor Author

bdauria commented Mar 10, 2017

@johnfn, yes, that sounds correct to me.

The thing that stopped me from implementing the feature in modeHandler.ts, is the limitation that the command has to be 1 char long. That basically means that it's already too late for remapping '2s' for instance.

I will try to move the mapping to happen in getRelevantAction instead, and make an update.

@johnfn
Copy link
Member

johnfn commented Mar 11, 2017

Thanks, @bdauria! If you have any trouble, feel free to ping me on Slack.

@bdauria
Copy link
Contributor Author

bdauria commented Mar 12, 2017

@johnfn I've pushed some changes. I basically reverted all the changes from the original PR and made the fix in ModeHandler instead.
The command passed to the different remappers will now be without the detected count prefix (see: b009d41).

as we discussed, moving the remapping part to getRelevantActions() would to big of a refactoring so not really in the scope of this PR.

@johnfn
Copy link
Member

johnfn commented Mar 15, 2017

Awesome. Thanks for the great work, @bdauria!

@johnfn johnfn merged commit 5119bca into VSCodeVim:master Mar 15, 2017
This pull request was closed.
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.

2 participants