[vim keymap] Visual mode rewrite and other changes

[metatheos]

My second batch of changes, containing the remaining additions from my local setup. Should also fix some problems discussed in Issue #1246.

Changes

1. New moveByLinesToFirstNonWhiteSpaceCharacter, used to implement the motions + (move {repeat} lines down, then to the first non-whitespace-character on that line), - (same for up) and _ (same as + but with repeat reduced by 1, so with a repeat of 1 it behaves like ^).
2. Some commands require the user to input a character to operate on. Previously pressing any handled special key (like Enter, Space or Down) would pass it on as string (see Issue vim mode 'r <Enter>' keeps replace mode active #1246). These keys are now converted to a printable character if possible and rejected otherwise.
3. If there is text selected, r{character} replaces all individual characters in the selection by {character}, while leaving linebreaks intact. Special case as by vim help is \n as {character}, which creates only one linebreak for an r operation.
4. Visual mode is significantly changed, using some of the properties of CodeMirror v3. Notes below.

DIY-Example: Inputting ggOTitle<Esc>yypVr= (with <Esc> being the escape-key) should now behave just as it does in vim.

Notes on moveByLinesToFirstNonWhiteSpaceCharacter

I'm on the fence about whether to keep moveByLinesToFirstNonWhiteSpaceCharacter or merge it with moveToFirstNonWhiteSpaceCharacter. Merging means there would be one less motion, but instead one with an if at the start picking between the two code paths. I also find the name to be obscenely long. @mightyguava what would you prefer here?

Notes on visual mode (selection)

Vim's cursor is not between characters but on one. This also has implications on the selection model. For the beginning of the selection the left side of the cursor is used, while for the end it is the right side. This requires sel.to to be incremented by 1 from the position CodeMirror's setSelection would place it at. Even more of a special case is linewise selection. Here sel.from is moved to the first character of the line, while sel.to is moved behind the last one.

CodeMirror as it is doesn't seem to require sel.from and sel.to to be either sel.anchor or sel.head. I'm not sure if I should expect this special case to be stable and especially whether I really want you to maintain this rather exotic use-case. I am replicating setSelection's behavior for now in an own function (vim.js:1511) and adapt the values at the end. From my testing it seemed to work out quite well. Obviously it cannot deal with folding yet, since I cannot access skipAtomic from my code (a hacky workaround would be calling setCursor(curEnd) or something similar before the custom operation). The only other problem I ran into is how CodeMirror restores selection on undo, which is a separate issue I'll look into as well. Also of course replicating internal code is really bad, I'll try to think of a way that doesn't need much change to the API but still allows me to get the core-code out of my setSelection.

I made one change to codemirror.css. The reason is that setting showCursorWhenSelecting doesn't help if the cursor is covered by the selection (selection has z-index set to 1).

[marijnh]

Great!

@mightyguava I'd like you to glance over this again (if you don't have time for these reviews, let me know, I'll stop notifying you.)

[mightyguava]

Sorry for the delay. I was away for the long weekend. The new visual mode looks vastly superior to the previous version. Thanks!

If you want to merge moveByLinesToFirstNonWhiteSpaceCharacter, I suggest merging with moveByLines instead, and add an additional argument for which character the cursor lands on the line. It would also make a bit more structural sense and probably less duplication.

Regarding the custom selection function. I strongly agree that that changing CodeMirror's internal state here is bad. A rule I followed implementing vim.js was to not change CodeMirror's internal state in any way. This made some parts of the logic a bit convoluted, but let vim.js be much more future proof. As you said, a better solution would be to refactor CodeMirror's selection API a bit to make it work for the vim use case. Maybe changing setSelection to accept a sel object with anchor, head, from, and to? Defer to @marijnh here.

[metatheos]

If you want to merge moveByLinesToFirstNonWhiteSpaceCharacter, I suggest merging with moveByLines instead

Now I feel stupid. I'll do just that. Any idea how I could improve the handling of 0-based repeat on _, while I'm at it? I don't like how I introduce motionArgs.repeatOffset just for that.

Also: would you mind if I rename textObjectManipulation to textObjectRange and invert its inclusive-parameter, renaming it to inner? I found these names rather confusing as they are.

As for the detection for mouse interaction, I've restored that for now, but I hope I can take a stab at solving #1084 the way @marijnh suggests soon. This might result in some tools to handle various mouse-related issues and also help with solving #1038.

[mightyguava]

Now I feel stupid. I'll do just that. Any idea how I could improve the handling of 0-based repeat on _, while I'm at it? I don't like how I introduce motionArgs.repeatOffset just for that.

Sorry, don't have a better idea. I find _ weird by itself, and don't see how you could not special case it.

Also: would you mind if I rename textObjectManipulation to textObjectRange and invert its inclusive-parameter, renaming it to inner? I found these names rather confusing as they are.

Sure. The text object manipulation motion is a bad hack I threw in to carry over the functionality from the original vim.js implementation. Any changes to clean it up is fine by me.

As for the detection for mouse interaction, I've restored that for now, but I hope I can take a stab at solving #1084 the way @marijnh suggests soon. This might result in some tools to handle various mouse-related issues and also help with solving #1038.

Sounds great. I didn't do it earlier because the fix would be v3 specific and somewhat involved. At this point, keeping vim.js v2 compatible is moot.

[metatheos]

Any changes to clean it up is fine by me.

I just looked through it while trying to improve cw, which led me to looking into aw and iw. At first "Manipulation" confused me a bit, I think "Limits" gives a better idea what it is there for. Also inclusive has a special meaning in vim that is -- as far as I can tell -- unrelated to the use there, so inner might be more descriptive. I had a patch to improve aw and iw but just noticed that it still needs some tweaks to behavior around the end of lines. The actual fix for cw seems to be even more tricky.

I merged the two motions, and added two small fixes in another patch. One was an oversight by me (gj and gk are not considered linewise in vim) and the other one was t walking back if there was no such character on the line (because it could not tell whether it got an actual coordinate or an error-value).

Since you mention v2, how relevant is support for the version? I'm not too familiar with how CodeMirror is used by others so I hope I'm not ignorantly trampling on anything here.

[marijnh]

I don't have a clear grasp of what the selection problem you're having is like, exactly. Just an idea -- would a beforeSelectionChange event, similar to the beforeChange event added this week, which allows you to check and adjust selection changes as they are made, be helpful for you? If not, please explain the nature of the problem or point me to a previous explanation.

[marijnh]

Should I merge the patches here as they stand, by the way, or is there going to be more adjustment?

[metatheos]

The idea of beforeSelectionChange sounds really interesting, maybe that would work even better than an extended api for setSelection().

I don't have a clear grasp of what the selection problem you're having is like, exactly.

As I've written in the notes on visual mode, there are a few differences between vim-selection and the default selection handling. The short version: it covers at least one additional character at the end, and sometimes more in both directions (moving sel.from and sel.to without changing sel.anchor or sel.head).

You can also try the vim-demo if you prefer that, press v to activate visual mode and move around using hjkl/arrow keys to select text. You'll notice that in the current version on the homepage the selection does not include the character under the cursor, while in my version the selection expands under the cursor as well. One notable special situation is that on pressing v, initially sel.to is set to sel.head+1, while sel.anchor and sel.head are still in the same position, which would make setSelection() return early (in the homepage-version there is some flipping of head and anchor to avoid that and I could work around that by moving the cursor before setting selection). Another thing to try is V for linewise visual mode, which now allows to move the cursor freely within the selected lines (so sel.from and sel.to are only moved whenever sel.head changes lines).

While what I do works whenever I am actively setting selection, a few areas are out of reach this way (namely click/drag and undo). The vim-keymap does not register for any events at this point, but I'm planning to try your suggestion from #1084 soon. What I like about beforeSelectionChange is that it might allow me to cover both cases with common code. Do you think it should be safe to modify sel.from and sel.to in general and especially during beforeSelectionChange? In that case I would call the normal setSelection() (which I now emulate instead) and do my final touches in the event handler.

As for the merge it depends on whether @mightyguava likes the current state and the way I implemented his suggestions. Especially he might not want the workaround of reimplementing setSelection() to stay in, which I would understand.

[schanzer]

+1 vote for something like beforeSelectionChange - that definitely has uses on my end as well.

[0b10011]

👍 for beforeSelectionChange -- Would be useful in my project.

[mightyguava]

If it would be safe to modify sel.from and sel.to in a beforeSelectionChange event, without changing sel.anchor and sel.head, then I agree it could make for a great solution.

As for the merge it depends on whether @mightyguava likes the current state and the way I implemented his suggestions. Especially he might not want the workaround of reimplementing setSelection() to stay in, which I would understand.

Ah didn't know you were waiting on me for this. If @marijnh will be adding beforeSelectionChange, then I would rather the code to emulate setSelection to not be merged. As I said before my concern is forward compatibility with possible future changes to CodeMirror's selection model (for example, multi selections #778). I don't mean to make it hard on you but I'd rather not chance breaking visual mode completely down the road. Thanks for bearing with me.

[marijnh]

If it would be safe to modify sel.from and sel.to in a beforeSelectionChange event, without changing sel.anchor and sel.head, then I agree it could make for a great solution.

There's an invariant that from/to always correspond to anchor/head (from to the lesser one, to to the greater). This invariant is not going away.

[metatheos]

There's an invariant that from/to always correspond to anchor/head (from to the lesser one, to to the greater). This invariant is not going away.

Ok, that kicks my approach out of the window. I understand that you don't want to risk stability issues you'd have to deal with though, I'll see if I can get my changes to visual mode out again so the remaining ones can be merged.

[metatheos]

@mightyguava are you ok with how the patch looks right now? In that case I'd suggest a merge as

[vim keymap] better r, t, new +/-/_
- handling special keys better for commands requiring a character
- r can operate on selection
- fixed: t moves back if character not found
- +/-/_ via extension of moveByLines motion

Also I'd like you to take a look at d776f74 if you're interested. There I played around with making the selection right but the (now hidden) cursor wrong.

[marijnh]

I've merged the changes as they stand.

(I somehow missed the horrible things you were doing in your setSelection replacement before. Such an approach is not acceptable. All selection changes are supposed to go through CodeMirror's internal setSelection, and poking at the selection from outside is bound to create all kinds of inconsistencies.)

[metatheos]

You're right, the original commit would have better just been an use-case. I should have opened a normal Issue first to ask whether sel.from/sel.to values not being a permutation of sel.anchor/sel.head is something CodeMirror is designed to handle. If it was, I still could have made a pull-request with proper api-access to the values. Sorry about that.