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

Disable Vim Mode in Debug Repl #723

Merged
merged 6 commits into from
Sep 14, 2016
Merged

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented Sep 6, 2016

Yay! We love PRs! 🎊

Please include a description of your change & check your PR against this list, thanks:

  • Commit message has a short title & issue references
  • Each commit does a logical chunk of work.
  • It builds and tests pass (e.g gulp tslint)

This PR demonstrates the idea of disabling Vim keybindings in Debug Repl as Debug Repl is now inside the Editor Repl. It doesn't mean we have to merge this in, just want to show team how it works and what's the catch.

@jpoon
Copy link
Member

jpoon commented Sep 6, 2016

Damn...that is a lot of if/else statements we need for this :(

@rebornix
Copy link
Member Author

rebornix commented Sep 6, 2016

@jpoon yup, that's for other Code team members to see the dirty workaround with current Code behavior. It's real dirty.

@johnfn
Copy link
Member

johnfn commented Sep 6, 2016

Can you do all of these just once inside keyPressed in modeHandler? Or is there something I'm missing?

@jpoon
Copy link
Member

jpoon commented Sep 6, 2016

Can you do all of these just once inside keyPressed in modeHandler?

That's what I was thinking too. But, with the way the code is now, it shows how bad of a hack it is and VSCode should really fix it on their end

@rebornix
Copy link
Member Author

rebornix commented Sep 6, 2016

@johnfn As we catch Code's keystrokes like Backspace, I don't know how to delegate back this keystroke to Code. If that's possible, then the solution is quite clean somehow. I may need to do some searching or maybe you guys already have ideas.

@jpoon
Copy link
Member

jpoon commented Sep 6, 2016

I thought all keystrokes go through...

  async handleKeyEvent(key: string): Promise<Boolean> {

@johnfn
Copy link
Member

johnfn commented Sep 6, 2016

it shows how bad of a hack it is and VSCode should really fix it on their end

If @rebornix's solution is really the only way to go about this, IMO it shows more that we architected our own code poorly. :/

(And by we I mean "I". I never really bothered to refactor extension.ts because I just wanted to pretend it didn't exist. :P)

@jpoon
Copy link
Member

jpoon commented Sep 7, 2016

Related: #666

@rebornix
Copy link
Member Author

@jpoon @johnfn now the code looks more reasonable :) We are going to mitigate this issue (like disabling debug panel from editor pane in extension host) this sprint but now this fix can get rid of the issue real quick, even though the additional inDebugRepl check is still dirty.

@rebornix rebornix changed the title [WIP] Disable Vim Mode in Debug Repl Disable Vim Mode in Debug Repl Sep 12, 2016
},
{
"key": "up",
"command": "extension.vim_up",
"when": "editorTextFocus && vim.mode != 'Insert Mode'"
"when": "editorTextFocus && vim.mode != 'Insert Mode' && !vim.debugInput"
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to move this check to a more central location? Maybe inside 'handlekey'?

Copy link
Member Author

@rebornix rebornix Sep 13, 2016

Choose a reason for hiding this comment

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

The catch is we can't delegate back modifiers like ctrl to Code, that's also why we need to put vim.mode != 'Insert Mode' here :(

I remembered you commented on similar issues on Code's issue list?

Copy link
Member

Choose a reason for hiding this comment

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

Ah right. Brain fart.

Change is ugly :( but LGTM.

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait. Cause these ctrl modifiers are configured they should already go through handlekeys right?

@rebornix rebornix merged commit 3dc5f10 into VSCodeVim:master Sep 14, 2016
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.

3 participants