Fixes #890, #1377: Selection (both visual/visualline) is very wonky with gj and gk #1600
Conversation
src/actions/actions.ts
Outdated
| @@ -3673,6 +3673,11 @@ abstract class MoveByScreenLine extends BaseMovement { | |||
| value: number = 1; | |||
|
|
|||
| public async execAction(position: Position, vimState: VimState): Promise<Position | IMovement> { | |||
| if (vimState.currentMode === ModeName.VisualLine) { | |||
There was a problem hiding this comment.
Could we have a quick comment here to say why this is necessary (or which issue it is fixing)?
There was a problem hiding this comment.
So the main issue is that we don't handle visualLine selections, due to the fact that our cursor must be at either start or stop (which as far as I can tell, works for every case but this one?). Well, I think that's true of IMovements at least.
However, in vim in visual line mode, doing gj on a wrapped line causes you to maintain the same highlighted area, however, your cursor is moved up one wrapped line.
However, we always snap the cursors at the beginning and the end of the selection to the end of the line (to maintain visual line mode). Thus, if we do a "gj" on a wrapped line, we are snapped back to our initial position, and we can't move down.
I don't think there's a good solution for this, so the workaround I came up with was to move by a "line" instead of a "wrappedLine" if you're doing visualLine movement.
This behavior doesn't exactly match vim, but I really doubt (I hope) that anybody relied on that feature of vim, since you can't select by screen lines anyways.
There was a problem hiding this comment.
Hmm, actually.
Maybe it is possible? The vscode.selection objects have a start, stop and active attribute. The problem (I think) is that the way we represent cursors only has a start and stop.
|
Again, thank you so much for the bevy of PRs recently. Much appreciated. :) |
src/actions/actions.ts
Outdated
| // and moving by screen line just snaps us back to the original position. | ||
| // Check PR #1600 for discussion. | ||
| if (vimState.currentMode === ModeName.VisualLine) { | ||
| if (this.movementType === "up" || this.movementType === "down") { |
There was a problem hiding this comment.
I'm not a fan of putting this filter here. Each non-abstract extended MoveByScreenLine defines its movementType, if the logic is only related to it, then we may want to put it in that extended class.
There was a problem hiding this comment.
Do you think it's better to define another class "MoveUpByScreenLineVisualLine"?
There was a problem hiding this comment.
The new commit should have done that.
Chillee
changed the title
I think this fixes all the visual selection related bugs I could find.