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

[Fixed] Reverse selection in normal mode #3124

Closed
wants to merge 3 commits into from
Closed

[Fixed] Reverse selection in normal mode #3124

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 10, 2018

What this PR does / why we need it: This takes care of problems faced during reverse selection where only the first letter of the codeblock used to be selected and the rest was ignored.

Which issue(s) this PR fixes #1951

Special notes for your reviewer: This seemed to be an issue with way getText() works on normal mode where it starts from the back and tries to move up finding where the start is and if it cannot find the start it returns only the first character as we began from the Right of stop.
The fix implemented just emulates a forward select when backward selection is made by the user and effectively works like a forward select.

Prateek Nayak added 2 commits October 10, 2018 17:48
Given the reverse selection only selected the first character but forward selection worked fine, we change a reverse selection into a forward selection at the background so that everything works fine.
Possible cause of the bug: getText() probably started from the right of the stop and then moved backward until it find's start but if it cannot do so, it just selects the first character as can be seen in the case of backward selection.
@ghost ghost changed the title Added separate condition for reverse select Reverse selection in normal mode Oct 10, 2018
@ghost ghost changed the title Reverse selection in normal mode [Fixed] Reverse selection in normal mode Oct 11, 2018
@jpoon jpoon self-requested a review November 1, 2018 05:08
@@ -1010,8 +1010,10 @@ class CommandOverrideCopy extends BaseCommand {
.map(range => {
const start = Position.EarlierOf(range.start, range.stop);
const stop = Position.LaterOf(range.start, range.stop);
return vimState.editor.document.getText(new vscode.Range(start, stop.getRight()));
})
return start < stop
Copy link
Member

Choose a reason for hiding this comment

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

The docs for EarlierOf state:

 /**
   * Returns which of the 2 provided Positions comes earlier in the document.
   */

so start should always be >= stop.

What's the case when start < stop?

Copy link
Member

@jpoon jpoon left a comment

Choose a reason for hiding this comment

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

Thanks for your PR. Sorry for the delay in the review.

@ghost ghost closed this Nov 1, 2018
@ghost
Copy link
Author

ghost commented Nov 1, 2018

I think i read it wrong, and thought EarlierOf returned the position from where the selection started. Sorry for the inconvenience due to my misunderstanding.

This pull request was closed.
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.

1 participant