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

Toggle Vim #1364

Closed
wants to merge 2 commits into from
Closed

Toggle Vim #1364

wants to merge 2 commits into from

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented Mar 6, 2017

#1055 Then you can disable Vim temporarily

cc @jpoon @xconverge @johnfn . I'm happy that we don't have too many dirty checks in code but our package.json is laughing at us.

src/globals.ts Outdated
@@ -9,4 +9,6 @@ export class Globals {
public static modeHandlerForTesting: any = undefined;

public static WhitespaceRegExp = new RegExp("^ *$");

public static active = true;
Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment so people know what this means

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed.

@@ -238,15 +242,54 @@ export async function activate(context: vscode.ExtensionContext) {
registerCommand(context, keybinding.command, () => handleKeyEvent(`${ bracketedKey }`));
}

registerCommand(context, 'toggleVim', async () => {
Globals.active = !Globals.active;
if (Globals.active) {
Copy link
Member

Choose a reason for hiding this comment

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

I imagine somewhere in here we want to do this for each mh we currently have?

Copy link
Member

Choose a reason for hiding this comment

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

Or does having it just here impact all mh correctly already?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch!

@xconverge
Copy link
Member

maybe want to clear out the status bar too? or put [vim-disabled] or something

@jpoon
Copy link
Member

jpoon commented Mar 7, 2017

this feels so dirty. do we really need/want this change? the fact that we can do it doesn't necessarily mean we should.

i personally would never use this and it seems sufficient to disable this through the extensions UI...

@johnfn
Copy link
Member

johnfn commented Mar 7, 2017

The one use case I know of is snippets, which we don't currently support. I've heard of people restarting their editor and disabling vim just to use a few snippets, which is kind of insane. With this and xconverges work making restarts unnecessary for config changes, I can see this being used by some people.

Agreed that it's not the idea solution for any individual problem we have though.

@xconverge
Copy link
Member

It is in VsVim, I have used the vsvim version on numerous occasions.

My use case is a coworker comes over and wants to explain something etc, as soon as they touch the keyboard, crazy stuff happens if they don't use vim. I personally will never use this for myself, it is only for that use case.

@johnfn
Copy link
Member

johnfn commented Mar 7, 2017

@xconverge I think what @jpoon is saying is that this doesn't offer much advantage over just disabling the extension in the sidebar.

@xconverge
Copy link
Member

Ok so what if I want to interrupt my coworker and type out a function?

I see what you guys are saying, but I don't think it is that dirty, and vscode is slow to disable/reload

@johnfn
Copy link
Member

johnfn commented Mar 7, 2017

He could just press i for you, hehe.

I'm ambivalent, but I'm never really against adding new features. 😉 (Oh, except useful ones, of course. ducks )

@xconverge
Copy link
Member

I have tried pressing i for coworkers, but then inevitably they go to copy paste or something and it just becomes silly for both of us

@johnfn
Copy link
Member

johnfn commented Mar 7, 2017

Haha and then it cuts off the last character and you get in a fighting match that ends in attempting to install spacemacs for 20 minutes?

@jpoon
Copy link
Member

jpoon commented Mar 7, 2017

My question is do people do this so often that they need a keyboard shortcut to save them the 2 seconds it would take to do through the UI?

Counter example to VSVim: atom/vim-mode#966

@rebornix's done the great work to implement it -- I'm fine to merge.

@rebornix
Copy link
Member Author

rebornix commented Mar 7, 2017

My use case is I'm trying to avoid reload VS Code as possible

  • VSCode source code is large and once Code is reloaded, it takes quite a while for TypeScript Language Service to warm up.
  • Tasks will be terminated when reloading Code
  • You can't reload as you are in a debugging session. etc, etc.

If I disable Vim for VS Code workspace, sometimes I forgot to bring it back as it's costly.

@jpoon It would be a good feature request to have a new handler called disableExtension in extension.ts but that requires thorough thinking. I'm not happy with the code either, it's nasty and I can wait for more ppl's feedback or request for this.

@Chillee
Copy link
Member

Chillee commented Apr 23, 2017

I think this would be useful. There are several cases where the plugin as it currently stands has very painful behavior to deal with (namely snippets and folds), and it would be very helpful to be able to disable it temporarily.

@jpoon
Copy link
Member

jpoon commented Apr 30, 2017

Dammit, I don't know how to git :(

I intended to resolve the conflicts, but ended up just creating a new brand-new branch. https://github.com/VSCodeVim/Vim/tree/toggle_vim

@rebornix
Copy link
Member Author

@jpoon i'll rebase and resolve the conflicts

@jpoon
Copy link
Member

jpoon commented Apr 30, 2017

I already did the work of resolving the conflicts here: https://github.com/VSCodeVim/Vim/tree/toggle_vim. We can create a new PR with that and close this one? Of you can try cherry-picking commits from there to your current branch.

Sorry, this was more complex than it needed to be.

@rebornix rebornix closed this May 1, 2017
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.

5 participants