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

Fix #426 #445

Merged
merged 5 commits into from
Jul 15, 2016
Merged

Fix #426 #445

merged 5 commits into from
Jul 15, 2016

Conversation

arussellk
Copy link
Contributor

This binds ctrl+[ to the escape command.

I removed an existing CommandOpenBracket. This was only used by the test and couldn't be accessed when actually using the editor.
I also had to add a "hack" alongside the "<c-r>" remap in modeHandler.ts.

We need something like this in the tests:

editor.pressKeys("ctrl+[");

so that we can ensure the ctrl+ mappings from package.json are working properly.

@arussellk
Copy link
Contributor Author

I'll look into why this failed and fix it. I knew my first pull request would be a bit rocky...

@rebornix
Copy link
Member

LGTM. The test failure has nothing to do with your code, thanks for your contribution, it's really neat!

@@ -505,6 +505,7 @@ export class ModeHandler implements vscode.Disposable {

async handleKeyEvent(key: string): Promise<Boolean> {
if (key === "<c-r>") { key = "ctrl+r"; } // TODO - temporary hack for tests only!
if (key === "<ctrl-[>") { key = "<esc>"; } // TODO - temporary hack because package.json maps ctrl+[ to the escape command
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think we don't need this line and the corresponding test as well because <ctrl-[> will only be passed in by test code, right? VS Code will ensure it will translate ctrl-[ to vim_esc command and we don't need test this feature in our code, what do you think?

Copy link
Contributor Author

@arussellk arussellk Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. <ctrl-[> is only in test code.

I would like to have a test to verify that pressing ctrl+[ results in the right action, but perhaps a comment in the test to reference package.json is adequate. What do you think?

Something like this:

test("can be activated", async () => {
  let activationKeys = ['<esc>'];
  // ctrl+[ is mapped to the vim_esc command by package.json
...
 });

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be sure to remove this line too.

@arussellk
Copy link
Contributor Author

I have ctrl+[ working the way I think you want it. Let me know if something else needs to change.

Sorry about all the commits - I don't know how to squash them with a remote involved. This seems to indicate that, as admins, you can fix it: https://help.github.com/articles/about-pull-request-merge-squashing/

@jpoon
Copy link
Member

jpoon commented Jul 15, 2016

Nope, no option to auto-squash, likely cause it's out of date with master. You should be able to squash them using git rebase -i HEAD~5 followed by a git push --force

@johnfn
Copy link
Member

johnfn commented Jul 15, 2016

This PR looks excellent. Let's squash and merge this bad boy!

@johnfn johnfn merged commit db168e3 into VSCodeVim:master Jul 15, 2016
@johnfn
Copy link
Member

johnfn commented Jul 15, 2016

Actually, I was able to squash it myself. How awesome is that? I guess I'm just cooler than you, @jpoon. 😉

Thanks for the contribution, @arussellk!

@jpoon
Copy link
Member

jpoon commented Jul 15, 2016

Hmm. Did GH give you the option to squash after you push that big red merge button?

@johnfn
Copy link
Member

johnfn commented Jul 15, 2016

Yeah, I clicked the drop down and there was an option to squash.

@arussellk arussellk deleted the Fix#426 branch July 15, 2016 12:38
ascandella added a commit to ascandella/Vim that referenced this pull request Jul 16, 2016
Looks like this was accidentally removed in VSCodeVim#445
jpoon pushed a commit that referenced this pull request Jul 16, 2016
Looks like this was accidentally removed in #445
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.

4 participants