Fix visual mode bugs#1304to#1308 #1322
Conversation
… make the change if in visual mode
…aech/Vim into FixVisualModeBugs#1304to#1308
|
Not ignored ;) he is still on vacation |
|
Haha 👍 |
src/actions/actions.ts
Outdated
| return await new DeleteOperator(this.multicursorIndex).run(vimState, position.getLineBegin(), position.getLineEnd()); | ||
| if (vimState.currentMode === ModeName.Visual) { | ||
| return await new DeleteOperator(this.multicursorIndex).run(vimState, | ||
| vimState.lastVisualSelectionStart.getLineBegin(), |
There was a problem hiding this comment.
Why is it lastVisualSelection? Is the current selection not what we want?
lastSelection was intended for commands like gv that are triggered after you stopped selecting entirely and did something else, so I just wanna make sure this is correct.
There was a problem hiding this comment.
Like ideally you should be able to just access the current selection?
There was a problem hiding this comment.
I try it out tomorrow. Since i am not very familiar with the code, i just search with intellisense for possible members/function, i could use 😃
There was a problem hiding this comment.
Which properties you want to use?
I can find a current selection member in vimState.
Using TextEditor.getSelection() does not work.
There was a problem hiding this comment.
vimState.cursorStartPosition and vimState.cursorPosition. Sorry I didn't make that more clear and you had to hunt around.
|
Sorry... i worked at another company in the last month and did not have much time. I get to it in 2 weeks :) |
|
Great! Will leave it open then! |
|
I merged the current master branch into mine. But the action.ts file does no longer exist. |
|
@xlaech it was split between a few files here https://github.com/VSCodeVim/Vim/tree/master/src/actions I would advise searching for the actions you want with |
|
There you go :) I was very confused by the merge... so i just wrote the whole fix from scratch. I also came to the conclusion, that the whole put command might need some refactoring in the future. At the moment, I think it is responsible for 2 many things. Please provide feedback. |
test/mode/modeVisual.test.ts
Outdated
| @@ -810,4 +810,136 @@ suite("Mode Visual", () => { | |||
| }); | |||
|
|
|||
| }); | |||
| }); | |||
|
|
|||
| suite("search works in visual mode", () => { | |||
There was a problem hiding this comment.
why are these 2 tests here? I think they were already merged into master, probably an artifact from your merge? :)
There was a problem hiding this comment.
I just copied the current file from master in the main repo and rewrote the tests. I thought they were added by you guys. If they are not needed i can delete them.
There was a problem hiding this comment.
But if they are in master already they shouldn't show up in the diff
There was a problem hiding this comment.
It was just an artifact of an unresolved merge conflict.
|
So is this good to go now? |
Fixed the bugs in Visual mode:
Did not fix #1306. See Diskussion.
Please intensively review #1308, since this was the most difficult one and i am still not sure if i have forgotten any edge cases. The tests run thought 👍