VSCodeVim / Vim

:star: Vim for Visual Studio Code
http://aka.ms/vscodevim
MIT License
13.93k stars 1.32k forks source link

Insert-mode selections do not work #479

Closed tobia closed 8 years ago

tobia commented 8 years ago

What did you do?

Select some text while in insert mode, for example with the mouse or Shift+arrow.

Then type something that operates on the text (that works in vanilla VS Code) for example ( to surround with parentheses.

What did you expect to happen?

I expected the editor to go into some sort of Insert-mode selection, more similar to regular VS Code selection that Vim modes, where I could type into the selection to replace it, or type (, [, {, ', ", backtick, etc. to surround the selection with pairs of characters.

What happened instead?

The editor went straight into Vim visual mode, where those keys don't do anything.

Technical details:

rebornix commented 8 years ago

@tobia just one stupid question, this is not related to Vim, right? It's all about how we reuse VS Code functionality in Insert mode?

tobia commented 8 years ago

@rebornix It's a condition where this extension removes functionality that was present in vanilla VS Code (modeless selection + keys to wrap in parentheses) without providing a replacement.

tobia commented 8 years ago

It also breaks snippets that have variable parts, unless I'm missing something.

killercup commented 8 years ago

I've also had problems with that. There are some vim plugins to surround a selection with braces/quotes/etc, and it would be great if there was a nice way to do this in vscode as well.

That being that, I'd also be happy with an option to not enter visual mode when selecting text with the cursor in insert mode. That may sound crazy, as there will be two different "select text" modes, but it would be a way to preserve more built-in vscode behaviour.

rebornix commented 8 years ago

@killercup your suggestion about two "select text" modes makes sense, we need to balance between Vim and VS Code somehow. I'll give it a thorough thinking about this thing then put more info here.

tobia commented 8 years ago

Vim itself has this mode, at least in some of its GUI flavors. It's entered by selecting with the mouse or with shift+arrows while in insert mode, is labeled -- (insert) VISUAL -- and it goes back to insert mode when exited or acted upon, not to normal mode.

See for example http://stackoverflow.com/questions/16203524/vims-insert-visual-mode

everpointer commented 8 years ago

I kind of have the same problem, when using snippets with variable label. When snippet tab stops at label variable, it turned into visual mode which broke the snippets operation. I keep uninstalling vim extension just because I can't use snippet well.

johnfn commented 8 years ago

@everpointer ahh, that is definitely an issue. will definitely see if we can get snippets working for you!

rebornix commented 8 years ago

@johnfn I suppose https://github.com/Microsoft/vscode/issues/8093 covers this issue, right?

johnfn commented 8 years ago

Nah, this one requires no additional API changes.

Just ensure we're not in insert mode before going into visual mode when the selection changes.

rebornix commented 8 years ago

Then this fix would be easy.

johnfn commented 8 years ago

Yup! (Like most fixes 😉 )

killercup commented 8 years ago

Just fiddled with the code a bit to see if I can fix this (this prevents me from using this extension).

Adding this._vimState.currentMode !== ModeName.Insert && here prevents it from going to visual mode when selecting stuff in insert mode.

But: This does not do the "replace/wrap" thing vscode usually does. If I write "lorem ipsum", select the "ipsum" part and press "(", it just adds a "(" after the selected text (or "()" in some languages, e.g. JavaScript).

(Sorry, I don't have time to dig into this any deeper right now.)

rebornix commented 8 years ago

@killercup The reason that replace doesn't work after you tweak the mode switching code is due to this piece of code https://github.com/VSCodeVim/Vim/blob/543b6c106044a5fe0e9243ee11004a684f5919f1/src/actions/actions.ts#L588

await TextEditor.insert(char, vimState.cursorPosition);

      vimState.cursorStartPosition = Position.FromVSCodePosition(vscode.window.activeTextEditor.selection.start);
      vimState.cursorPosition = Position.FromVSCodePosition(vscode.window.activeTextEditor.selection.start);

We just insert new chars to current cursor (the end of selection if the selection is forward) as above. So a proper fix is replacing the selection in Insert Mode instead of inserting in Insert Mode if there is any selection.

Then it looks like a good fix @killercup ?

killercup commented 8 years ago

Okay, replacing that else branch with

const selection = vscode.window.activeTextEditor.selection;
let cursorPosition = Position.FromVSCodePosition(vscode.window.activeTextEditor.selection.start);
if (selection.isEmpty) {
  await TextEditor.insert(char, vimState.cursorPosition);
} else {
  await TextEditor.replace(selection, char);
}

vimState.cursorStartPosition = Position.FromVSCodePosition(cursorPosition);
vimState.cursorPosition = Position.FromVSCodePosition(cursorPosition);

makes it overwrite the selection content. Sadly, the cursor is then just before inserted char.

Something like this did not work (but should be easy for someone who actually knows what they are doing):

cursorPosition = cursorPosition.translate(0, char.length)
await vscode.commands.executeCommand("cursorMove", {
  to: cursorPosition,
  inSelectionMode: false,
  noOfLines: 0
});

The other missing thing is that this does not wrap the selection when used with something like "(" or "[". Ideally, we would be able to just fall back into the default vscode behaviour when in insert mode. I don't know if that's easy to do, though.

rebornix commented 8 years ago

The reason that the cursor is before the inserted chars is because your code make it so

let cursorPosition = Position.FromVSCodePosition(vscode.window.activeTextEditor.selection.start);
...
vimState.cursorPosition = Position.FromVSCodePosition(cursorPosition);

See, you are assigining the position of the start of previous selection after replacement, which is just before the inserted chars. I suppose TextEditor.insert and TextEditor.replace can hanlde the cursor position perfectly?

killercup commented 8 years ago

Yes, I'm stupid. Using this after inser/replace (and using the end of the selection!) works nicely: const cursorPosition = vscode.window.activeTextEditor.selection.end;

Just the wrapping behaviour missing now. Do you want me to make a PR for this (and maybe make this a configuration option)? I'll probably struggle a bit more with this when writing tests :)

rebornix commented 8 years ago

@killercup just send out the PR, really look forward to it! You can just file one with WIP prefix and add test cases later, besides, our test infra is intuitive and straight forward, you can pick them up quickly :)

killercup commented 8 years ago

@rebornix opened #544, will try to work more on this later. (If you want to pick this up instead, feel free to do so! I won't have much time this week.)

everpointer commented 8 years ago

New release without solving this problem...Keep waiting.

johnfn commented 8 years ago

@everpointer please thumbs up issues that personally affect you. The way we prioritize things is by upvote count.

You can also help with the current WIP PR :) Everyone over here works on this plugin in their spare time. (Well, except for @rebornix I guess :P)

everpointer commented 8 years ago

Sorry, I understand. Thumb up issues means create a new issue? Not sure these github workflow and currently not able to contribute the code. Thanks to the reply.

johnfn commented 8 years ago

Click on the emoji plus face on the top of the first post and click on the thumbs up there.

I did it to your post :)

everpointer commented 8 years ago

Got it, thanks.

edthedev commented 8 years ago

:+1: VsCode snippets are a great feature.

If we make it configureable, I would encourage not breaking VsCode snippets to be the default, and then the the more vim-like behavior to be a configuration setting. Although either way is probably fine, if it makes it into the ReadMe.

johnfn commented 8 years ago

Pretty sure I just fixed this on master. Give it a whirl and see if there's any cases I missed.