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

Conversation

jacekkopecky
Copy link
Contributor

Currently, motions would individually ensure that cursor doesn't end up after the last character of a line. This fails in a number of circumstances, e.g. with mouse clicks to move/add a cursor, and with specs that setCursorBufferPosition to put the cursor where it should never be.

This PR reacts to any cursor movement with TextEditor's onDidChangeCursorPosition and any cursor addition with onDidAddCursor and makes sure the cursor never goes where it doesn't belong.

The PR also fixes some tests that expected the cursor after the last character in command mode, and removes some outdated spec comments.

@jacekkopecky
Copy link
Contributor Author

This PR also fixes #2, as far as I can tell. Yay for fixing the longest-outstanding issue of vim-mode. 8-)

@bronson
Copy link
Contributor

bronson commented Jul 17, 2015

This is terrific! It's nice when the fix also deletes code scattered all over the place.

Minor conflicts with #591. Both should be merged.

@jacekkopecky
Copy link
Contributor Author

fixes #579

@@ -654,6 +662,16 @@ class VimState
text = @getRegister(name)?.text
@editor.insertText(text) if text?

ensureCursorIsWithinLine: (cursor) =>
return if @processing or @mode is 'visual' or @mode is 'insert'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain a bit about why this @processing variable is needed? Without looking into this too deeply, I would think that we could just return unless @mode is 'normal'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some motions, esp. with counts, invoke several Atom motions, each notifies us about cursor movement, and they all can happen in normal mode. We don't want to interfere until that's done.

But since it needs explanation, it could be written clearer or better; suggestions welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But 'not "normal"' should be OK instead of two comparisons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's in the new commit, which also adds more @processing 8-)

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to interfere until that's done.

I see. I think this is a fine solution actually.

@maxbrunsfeld
Copy link
Contributor

Very good idea.

@maxbrunsfeld
Copy link
Contributor

Ok great! Congratulations on closing issue #2.

maxbrunsfeld pushed a commit that referenced this pull request Jul 24, 2015
use onDidChangeCursorPosition for keeping cursor in line
@maxbrunsfeld maxbrunsfeld merged commit c384079 into atom:master Jul 24, 2015
@jacekkopecky jacekkopecky deleted the ensure-cursor-on-line branch August 7, 2015 15:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants