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

jj cursor position fix for #1418 #2366

Merged
merged 8 commits into from
Feb 19, 2018
Merged

jj cursor position fix for #1418 #2366

merged 8 commits into from
Feb 19, 2018

Conversation

dderg
Copy link
Contributor

@dderg dderg commented Feb 7, 2018

Which issue(s) this PR fixes
fixes #1418

Special notes for your reviewer: tried to make test, but tests are made in the way that jj not actually passed to editor.

@Chillee
Copy link
Member

Chillee commented Feb 8, 2018

I think @jpoon recently added functionality for testing remappings. Have you tried the method here?

@dderg
Copy link
Contributor Author

dderg commented Feb 8, 2018

it tests that keys are replaced with correct keys, but that is not the case

@dderg
Copy link
Contributor Author

dderg commented Feb 9, 2018

anyway, im using vim with my fix for 3 days now, everything is ok

@dderg
Copy link
Contributor Author

dderg commented Feb 10, 2018

What is stopping us from merging this pr?

@AlexArendsen
Copy link

+1 I'd love to see this merged in too.

@Chillee
Copy link
Member

Chillee commented Feb 11, 2018

@prog666 Sorry, I didn't immediately merge this because I was confused what the error was (I think partially because the repro in the issue linked isn't related to the title of this PR). After playing around with it a bit though, I see what the issue was.

LGTM!

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 @prog666. I'm on vacation so haven't been on top of these issues/PRs. I left a few comments.

@@ -376,9 +376,9 @@ export class Position extends vscode.Position {
* Gets the position one to the left of this position. Does not go up line
* breaks.
*/
public getLeft(): Position {
public getLeft(count: number = 1): Position {
if (!this.isLineBeginning()) {
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 update this check to ensure that we don't go past the beginning of the line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's no check for getRight()

@@ -376,9 +376,9 @@ export class Position extends vscode.Position {
* Gets the position one to the left of this position. Does not go up line
Copy link
Member

Choose a reason for hiding this comment

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

Please update this comment.

@dderg
Copy link
Contributor Author

dderg commented Feb 15, 2018

@jpoon done

@jpoon
Copy link
Member

jpoon commented Feb 18, 2018

On vacation in Africa at the moment. We'll be back to hotel tomorrow and I can take a look then. Otherwise @Chillee can merge assuming my comments were addressed.

@Chillee
Copy link
Member

Chillee commented Feb 19, 2018

Looks like both of jpoon's comments have been addressed. I'm gonna merge this. Thanks!

@Chillee Chillee merged commit 630b882 into VSCodeVim:master Feb 19, 2018
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.

Wrong cursor position entering normal mode
4 participants