Fix 806 regression

[jacekkopecky]

Fixes #806 and #826 and #829.
I don't know how to add specs for the mouse movements but they are fixed too. @maxbrunsfeld please advise if you would like those specs and know how to do them.

[maxbrunsfeld]

@jacekkopecky Could you explain why we need specific code for handling mouse dragging, rather than treating it like any other means of updating selections?

I thought the problem was just that we prevent cursors from moving to the end of the line, even if they have non-empty selections.

[jacekkopecky]

Here's what I think happens on mouse dragging in normal mode:

1. mouseDown after the end of the line, cursor moves to the end of the line and there's no selection
2. mouseMove, cursor moves and selection is started and grows
   The issue is that without watching the mouse, we don't know in step 1 that we shouldn't move the cursor - hence the behaviour shown in the very helpful animated gif in Last character on line is skipped when selecting upward #806.

[maxbrunsfeld]

@jacekkopecky Can you verify that this fixes the insert-newline-above and join-lines commands in normal mode? I would think that there would still be problems, because other commands outside of vim-mode might need to move the cursor to the end of the line, but won't be able to set the @processing flag. I could be wrong though; let me know.

[jacekkopecky]

@maxbrunsfeld yes, I've just verified it.

The @processing flag is there so that vim-mode can put the cursor immediately where it belongs after vim-mode commands, but it doesn't reposition the cursor while those commands are working.

For non-vim-mode commands, we notice something has happened with the onDidChangeCursorPosition handler, but we only do something about the cursor position after the current tick (that's the _.debounce(..., 0) so the foreign commands are done by then.

[jacekkopecky]

it means, though, that a command like cmd-<right> puts the cursor at the end of the line, then drawing happens, and only then the cursor is put where it belongs and redrawn. $ doesn't suffer this issue, and vim-mode could very easily have cmd-<right> mapped to the same command as $.

[jacekkopecky]

@maxbrunsfeld the last comment removes the short cursor flicker on native commands such as cmd-<right>. Still uncertain?

[maxbrunsfeld]

Hooking into the TextEditorComponent's rendering cycle is an interesting solution, but it's reaching pretty far into private code, and it creates a coupling between the VimState and the rendering layer that I'd like to avoid.

I think that instead, we should hook into the lifecycle of Atom's commands. Basically, we'll only need to adjust the cursor position at two times: after mouse dragging ends (exactly the way you have it now), and after each command runs. Then I don't think we'll need to use onDidChangeSelectionRange, onDidChangeCursorPosition, or onDidAddCursor to adjust the cursor position, and I don't think we'll need the @processing variable on VimState.

In the above atom/atom pull request, I've added a method atom.commands.onDidDispatch that we can use for this. This will be available in Atom v1.0.8.

[jacekkopecky]

Because we don't have a good currentTarget in the did-dispatch event (see https://github.com/atom/atom/pull/8496/files#diff-22aaebf7af3d31bbf37d33248ec7af1cR253) we won't know that the event was for our editor. We might run the cursor position and selection check for every editor on every command, but that doesn't feel right.

[maxbrunsfeld]

I did a little work on this, and it seemed like checking event.target is @editorElement was sufficient.

EDIT: oops, @editorElement, not @editor

[jacekkopecky]

got it, that should work, thanks

[jacekkopecky]

I now got rid of all the cursor/selection change listeners (which required a few spec changes), but it works now (with my recent enough clone of Atom, anyway).

[maxbrunsfeld]

It looks like you can remove the if @mode is 'normal' here, since ::ensureCursorsWithinLine internally guards that the mode is normal.

[jacekkopecky]

quite right; in fact, now it's the check in ensureCursorIsWithinLine that's no longer necessary so I removed that one instead.

[maxbrunsfeld]

❤️ This is looking really good. Really excited to get this 🚢ed once Atom 1.0.8 is out.

[chibicode]

@maxbrunsfeld @jacekkopecky I've been following this thread for a while. Thank you for your hard work!

[jacekkopecky]

Thank you for your kind words! Oops, just remembered that I have a check for existence of onDidDispatch that shouldn't be there; can remove it in an hour.

[maxbrunsfeld]

Actually @jacekkopecky I would say leave that in there for a little while. Maybe put a TODO comment to remove it. That way, users w/ old versions of Atom won't get exceptions when they upgrade vim-mode; they just won't have the cursor adjusted.

[jacekkopecky]

Hmmm, I'll have to check the amount of breakage that causes in 1.0.7. If it's usable, you're right.

[jacekkopecky]

Yup, seems usable all right

[maxbrunsfeld]

✨

[chibicode]

Yay! Thank you!