Skip to content
This repository was archived by the owner on Apr 6, 2018. It is now read-only.

Fixed yank cursor bug#417

Closed
isaachess wants to merge 2 commits into
atom:masterfrom
isaachess:yankCursor
Closed

Fixed yank cursor bug#417
isaachess wants to merge 2 commits into
atom:masterfrom
isaachess:yankCursor

Conversation

@isaachess
Copy link
Copy Markdown
Contributor

  • When yanking, there was a bug that made the cursor move to the very end of the yank selection, rather than moving the cursor to the beginning of the selection like vim does. After the yank, cursor now moves to start of selection.
  • Also, @editor.getSelection() is deprecated, so we are now using editor.getLastSelection().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rather than just measure the screen position, we get the range of the selection and use that to check the selection position.

@isaachess
Copy link
Copy Markdown
Contributor Author

I see that the Travis CI build failed, but I can't find anything in the spec files that mention Yank. Can you point me in the right direction of where I need to update those?

@isaachess
Copy link
Copy Markdown
Contributor Author

I have updated the tests so this now passes.

This functionality still does not work exactly like vim, but I personally feel it is close to vim, and a better "not-like-vim" than we had before.

Now whenever you yank anything, the cursor moves to the very beginning of that selection. This is how vim works, except in the special cases of shift+Y, y y, y j, etc. In those cases, vim leaves the cursor in its original position, but my fix moves it to the start of the line (i.e., the start of the selection).

However, while it used to work correctly in those special cases, it worked incorrectly in every other yank. So I personally this fix, while not perfect, is better than where we were.

Thoughts? I'll understand if you don't want to merge this in. But let's not let the "perfect be the enemy of the good," right? 🚀

@isaachess
Copy link
Copy Markdown
Contributor Author

Can I get a review/comment on this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this be selection.getText() since you already have the selection variable?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Never mind, I'm assuming that line 145 may have changed the selection so you would need to re-grab it?

@kevinsawicki
Copy link
Copy Markdown
Contributor

Can you elaborate a bit more which cases this fixes? And were those cases covered by new specs?

It looks like all the spec changes have FIXME comments so I just want to clarify whether this changed behavior is tested in the cases where it is working correctly in this PR, thanks

@isaachess
Copy link
Copy Markdown
Contributor Author

As for which cases it fixes, please see my bullet points on the pull request. The reason FIXME is there is because it does not yet work exactly like VIM, but works (in my opinion) better than what we had before. But yes, the new functionality is all in the specs.

@maxbrunsfeld
Copy link
Copy Markdown
Contributor

@isaachess - as far as I can tell, your change fixes the cursor position for the following operations:

  • y in visual mode
  • yiw in normal mode

and it breaks the cursor position for line-wise yanks.

I'm ok with merging this and then re-fixing the behavior for line-wise yanks, but would you mind adding tests for the cases that you fixed? It looks like your only changes to the specs were adding FIXMEs for the cases that you broke. Thanks for surfacing these bugs.

@bronson
Copy link
Copy Markdown
Contributor

bronson commented Jul 4, 2015

Looks like the problems addressed by this PR are completely solved in 0.54.0. At least, viwy and yiw jump to the beginning of the selection (even when selecting in reverse), and yy yj etc leave the cursor in place. Linewise works like Vim too.

@maxbrunsfeld
Copy link
Copy Markdown
Contributor

Thanks @bronson. Closing this out.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants