[vim] Reimplementation of Vim key mappings

[mightyguava]

This is a reimplementation of Vim key mappings. The reasons why I felt a reimplementation was necessary are listed in a previous pull request by @jankeromnes at #973 (comment). I'd be happy to go into more detail if anyone is interested. Sorry about the long pull request but there are a lot of changes and a lot to discuss. Thank you in advance for taking the time to read this.

Summary

This reimplementation has almost reached feature parity with the original, with some divergence in features. Overall the code is cleaner and more maintainable, which should make future additions significantly less time consuming and less error prone.

I opened this as a pull request because I would like to ask for advice from the community on where to go from here, and see if this can be added to the main repository so that others can use it and contribute. While the reimplementation has not reached full feature parity, there are only a few missing features and I'm not sure when I'll get to implement them. There are also many behavioral fixes and a few new features that would be useful to existing users.

Discussion

I am usually against having 2 parallel implementations, and would greatly appreciate any opinions on how we can get the features merged. However, due to the reasons above I think in the short term the best solution would be to have both available, maybe with this as a separate branch?

While investigating how to implement visual mode, I saw that setCursor() clears the selection (let me know if I am mistaken). Would it be possible to add to the exported CodeMirror API a version of setCursor() that does not reset to selection, and/or methods to expand the existing selection? Or do these APIs already exist? If they don't I'd be happy to add them with some assistance.

To support additional key-to-key mappings, like mapping 'Left' to 'j' and 'Y' to 'y$', the Vim keymap would need to expose some API functions to let users define the mapping. This could also be a feature in codemirror.js but it seems to be more of a Vim specific feature. I currently am exporting a Vim instance object as a property of CodeMirror. Is that fine or should we work out a different way?

Functional differences from original

This is not a complete list, only major points are listed. Also the list of supported keys on top of vim.js is a bit out of date and I may have missed a few things while looking through the code.

Additional features not in original

• Support for marks
• Support for registers
• Respects orthogonality between motions and operators (Should be less buggy because motions and operators execute independently. Also, newly implemented motions should work with existing operators without additional changes, and vice versa. )
• A unified keymap with no nesting (plus a trivial insert mode keymap), compared to multiple prefix keymaps in the original implementation

Yet to be implemented

• Support for text object manipulation (I am not familiar with this feature at all and it seems relatively complex, so might not be the best person to implement it. Also seems to be a more advanced feature than visual mode so I'd like to do that first.)
• Ctrl-F, Ctrl-B (they would be easy to do for normal mode, but in order to make them visual mode compatible, need a way to get the position in the next page without moving the cursor)
• Ctrl-N, Ctrl-P, arrow keys, space, and backspace (don't believe these are commonly used over h/j/k/l, though I might be wrong, and I want to implement them via key-to-key mappings instead duplicating them in the main keymap)

Next steps

The immediate next steps I see for the reimplementation, in any order, are:

• add unit tests
• visual mode
• key-to-key (and key-to-command) mappings, like mapping Y to y$ (this is an important vim feature that would be very difficult to do in the original, but should be pretty simple in the reimplementation)
• text objects

[marijnh]

I agree that the current vim mode is starting to look messy. I'd like contributors to the old code (@jankeromnes, @xLanny, @misfo, @mightyguava, @chris-morgan, @mattsacks) to take a look, verify that their favorite features still work, maybe help extend this to cover missing features. It's hard for me as a non-vim user to interpret this code, but at a glance it does look clean. If it's viable to switch to this as the one vim mode in the distribution, that'd be fine with me.

[jankeromnes]

The code does look cleaner, and I like the orthogonality between motions and operators (even if code duplication was kind of avoided in current implementation). However, it's 1016 lines versus 897 lines and it doesn't yet reach full feature parity.

I gave it a quick try:

Missing features (not exhaustive):

• Arrow navigation in default mode
• Repetition for Y, D
• Nothing happens on : (would be nice to have :%s// some day) or caw

Still missing in both implementations:

• Nothing happening on * or /
• No repetition on inserts

What do you mean exactly by "support for marks"? The previous implementation has a map of marks.

Also, no need for a setCursor() method that doesn't reset the selection, just use setSelection(old, new).

I also don't like side-by-side implementations, and maybe a branch is a bit overkill since your repo already holds the concurrent implementation. Personally I don't mind the new implementation, since it has some obvious benefits, but I'd like to see it come closer to feature parity before merging it. Also, a visual mode would help get my vote.

[mightyguava]

Thanks for the comments! Looks like priority-wise for me, key-to-key mapping first and then move on to visual mode. Will wait for more comments here before trying to decide on whether to merge. If anyone can help this reach feature parity or add other improvements please feel free to. I'd really appreciate it.

Replies to @jankeromnes

• Arrow navigation - as stated above, I plan to implement this with a user configurable key-to-key mapping. Map arrow keys to hjkl. Same for backspace, space, ctrl-n, ctrl-p, etc.
• Didn't know Y, D can repeat. Will add
• The current implementation's ":" only supports line numbers, which is covered by gg and G. I felt Ex mode would be a step after visual mode.
• Support for marks: I saw the map for marks in the existing code but couldn't get it to work. Just investigated a bit and the existing mark implementation has yankTillMark and deleteTillMark (without the prefix " or `), while the new implementation implements it as an orthogonal motion. Also, still can't figure out how to activate them in the existing implementation.
• Verbosity: Yep the code is longer, but for good reason. I avoided long one-liners and added { } and line breaks for most control conditions, which expands the code a lot but the gain in readability is more important. New code respects an 80 char limit with the exception of the key maps. The register implementation is also almost 100 lines.

[mightyguava]

Alright repetition on Y and D are done, arrow keys and more are mapped. Repeat on insert mode is hard because it would require Vim to hook into CodeMirror's keyhandling, or have Vim take over insert mode. Neither sounds like a good approach. I could probably do * and / if that'll get your vote to merge :)

[mattsacks]

Couple of things with regards to this branch:

1. I am completely against having two implementations of the Vim keymappings in CodeMirror. It doesn't need to be any harder to maintain. If you're going to refactor the entire file for better extensibility in the future (an idea that I totally support), you can't just go halfway and ask for it to be brought in anyway.
2. Marks (and effectively, registers) are available in the current Vim implementation.
3. Text-objects are one of the first things Vim users test when gauging a "vim-mode" in another editor. I don't think it's okay to lack support for even the most basic text-objects (ones that don't span multiple lines).

About the implementation:

1. I like the defaultKeyMapping style. Personal thoughts:
   • Maybe try to split some things out onto new lines? Just a pet peeve.
   • keyToKeyMapping isn't necessary. You should facilitate whatever mapping system you implement with some defaults.
2. Okay, still reading the code. Definitely split stuff out onto new lines. { return; } can just be return; too.
3. You have setMark and setRegister but no getMark and getRegister.

Besides that, it's a lot of code to comb through - but it all looks great for the most part. There's just some key things you need to address before this needs to be considered as an actual replacement of the current implementation. Thanks for all your work on this.

sidenote: @marijnh, is use 'strict'; necessary/preferred? I've heard rumors of it slowing down JavaScript execution due to the environment it needs to consider (as apart from the default, not strict).

[mightyguava]

Re: @mattsacks

1. The implementation has nearly reached feature parity, with many improvements, and it is quite an exaggeration to call it halfway. I completely agree that it is bad to replace the current code with something that doesn't have full feature parity, but please understand that my time is limited and I have been doing this by myself, working on what I thought was most important. A large part of the reason why this pull request exists is to ask for comments on what is important and what should be done before replacing the existing implementation. Another part is to ask for your help!
2. I haven't been able to figure out how to trigger marks in the current implementation, even with some cursory investigation of the code, so I assumed it was not functional. It's quite possible that I may just not know about another way to use it? Please let me know so I can add the functionality.
3. I have been implementing in the order here: http://whileimautomaton.net/2008/11/vimm3/operator, which lists text objects last. I have also never used it in the years I used Vim. Maybe I'm missing out. Will attempt to port over the code in the current implementation, but will not attempt a rewrite until after visual mode if it's not compatible. Also, anyone is free to contribute on what they feel is important.

About the implementation:

1. Wrapping it into the 80 char limit sounds good (might actually go with 100 char). That's what I originally had but it ended up getting harder to parse than keeping each entry on a single line.
2. As for keyToKeyMapping, I originally planned to do what you wrote, but keeping a single map ended up being much simpler and I couldn't convince myself to implement arrow keys, etc, without some kind of key to key mapping. A full mapping system would come after text objects and visual mode, but I will definitely keep this in mind.
3. Will split stuff onto new lines, got a bit lazy. I am completely against changing { return; } to return; however, pet peeve of mine? :)
4. getRegister is a method on registerController, getMark is not implemented since I'm just storing handles to CodeMirror bookmarks. Could you clarify the benefits of exposing getMark and getRegister to the keyMap?

Thank you very much for reading the code over. I realize it is very long, but hopefully it wasn't difficult reading (that was the whole point of reimplementing). I will work on the key points you brought up.

I'm fine with getting rid of use 'strict';, but I'd like some kind of automated code style enforcement. Maybe adding JSLint to the Travis build?

[mattsacks]

Sorry, halfway or half-implemented is oversimplifying the effort put into this branch. I apologize.

But honestly, text-objects are one of the most novel features of Vim and give it a lot of power. I think you're missing out on making them lower priority than visual mode (which I think will have a lot more complexity to it than text-objects, but maybe not).

80 chars is more standard of a text-width than 100.

Marks are currently implemented using the 'm' prefix, but I'm not sure if you can actually jump to them yet. It's just an object that stores the current line (and should store the current cursor) number.

[marijnh]

Keep the 'use strict'! There is no reason not to use that.

Also, I personally think an 80 chars limit is archaic. It requires a lot of effort being put into code formatting, and the benefits are dubious.

[mightyguava]

Oops was trying to post a comment. Chose 80 char line limit in the end because that way I can fit a 3 way merge on my screen. Also can fit 2 files side-by-side on my laptop, which is very convenient. 100 is just a bit too long.

[mightyguava]

Text objects have been ported over. Found tons of bugs while testing, but the only new one is a cursor jumping bug when I try da{ on the entire function block in vim2.html. Everything in the trace looks right up to the operation() function in CodeMirror. It seems like CodeMirror is adding 2 extra spaces after deletion. Anyways don't think it's a huge issue and will probably investigate more after visual mode. @marijnh do you know why the Chrome debugger can't step out of the operation function?

Also implemented Ctrl-f and Ctrl-b, though I would really prefer if there was a way to get a page down position without actually moving the cursor, so I don't have to do the ugly hack I have now.

Feature parity has been reached!

[ghost]

A few bugs I ran into in a few minutes of testing :

• [cyd]iw operates on one less character than a word
• [cyd]iW appears to operate on nothing at all
• The tag text object doesn't work, I'm not sure if it was working in the old implementation or not
• If % happens when the cursor is not on a paired character, it should search forward to the next paired character and work as usual
• Searching (/ and ?) doesn't work
• : (ex mode) doesn't work, it only worked for line numbers in the old implementation but does nothing now

Most of those aren't major with the exception of the word/WORD text objects (matt is right, text objects are one of the first things vim users look for) and none of them should be hard to fix. I'll hop on them in a few days if no one beats me to it.

[mightyguava]

Thanks for testing this @xLanny

I added [cyd][ai]W, but the off-by-one error is still there for inner words (and also reproduces in the old implementation). The problem is probably that the "inclusive" option does not behave correctly. It should get fixed automatically if text objects triggers are rewritten to use the same keyMap system as the rest of file uses. I practically did a copy and paste from the old implementation on this and had the not had the time to rewrite. It wouldn't be a hard fix if you change inclusion in the text objects, but a better fix would be to move to the defaultKeyMap. I have comments in the file if you are interested and have time. If not then a quick fix is fine too and I will do the rewrite after figuring out visual mode. Thanks for volunteering!

The remaining things you mentioned are preserved from the existing behavior, with the exception of ex mode. I left it out on purpose because I haven't thought of a good way to handle ex mode mappings yet and how to properly process ex mode commands. This should probably be a longer discussion, so maybe we can move that into a separate issue? Not sure what the standard way to separate out topics of discussion on GitHub is.

Finally, here's a summary of text object bugs found so far. All but the cursor jumping are carried over from the old implementation:

• No repeats on word objects
• Off-by-1 error when operating on i[wW]
• [cyd][ai]w does not properly expand backward when symbols are matched rather than alphanumerics
• Exception is thrown if [cyd][ai][[{(] is triggered outside the scope of the outermost brace
• ]}) are not implemented
• Tag text object is not implemented
• Cursor jumping when doing ca{ on the function scope braces on the demo page (I was unable to track this down, cursor positions look fine up to control is given back to CodeMirror, and no other operations are triggered on the Vim side)

[mightyguava]

A basic, mostly functional visual mode has been implemented! I have to say, it's pretty cool.

[mightyguava]

Are there objections toward replacing the old Vim that I haven't covered?

[jankeromnes]

The new implementation is 54K (21K minified) versus 31K (14K minified), but I guess the additional minified 7K are a good tradeoff for the improvements and new features.

Thanks a lot for the visual mode, it's still not perfect (there is an off-by-1 error when you try to x text, and D should remove entire lines in visual mode instead of acting like in normal mode) but it's a feature I was really looking forward to.

I didn't find any other problems, so if @marijnh, @mattsacks, @xLanny, @misfo and @chris-morgan don't have further objections, I think we could go for the new implementation. Thanks for all the good work!

EDIT: Does the new implementation respect the behavior described in #997 ?

[mightyguava]

I agree visual mode isn't perfect. There are many many more features in Vim, and including quite a few things in visual mode, that I haven't gotten to yet. I will try to cover the more important ones over time.

The off-by-1 error was actually because I forgot to turn on "showCursorWhenSelecting", so the fat cursor wasn't showing up. The fat-cursor in CodeMirror always shows on the character after the current cursor index. In Vim's visual mode, the fat-cursor shows on the character at the current cursor index. So what the Vim keymap does is always include the character after the cursor, even though it's not in the selection. It should feel natural now.

D, X, and Y are fixed.

The Ctrl-[ behavior change has been integrated.

[ghost]

Hmm, is there a reason that visual mode is implemented in vim.js as opposed to vim2.js?

[mightyguava]

I replaced vim.js with vim2.js. It seemed the consensus was that we didn't want 2 alternate implementations at the same time, which is why I tried so hard to reach feature parity. I moved it over now since it seemed ready for merging.

[ghost]

Ahh, I see. Well it's got the green light to merge from me in that case.

[marijnh]

I'll merge this in a few days. If anyone wants to do a detailed test drive and/or fix minor bugs they find, now is the time.

[mightyguava]

I'm going to be adding more unit tests over the next few days, but won't be doing a test drive due to the obvious conflict of interest... I also welcome anyone to test and find/fix bugs.

[marijnh]

Please take #1010 and #1011 into account.

[mightyguava]

Accounted for #1010, replied to #1011.

[jankeromnes]

@wesalvaro Haha sorry about that, I selfishly implemented the visual mode behavior before there was a visual mode, because it was the one I was using the most. I think this was fixed a while ago, and both behaviors should be correct in the new implementation anyway. Funny how small details sometimes get more unnerving than bigger problems.

[mightyguava]

D in the new implementation should, in normal mode, delete to end of line, and is repeatable. In visual mode, it should delete the entire selection linewise, and is not repeatable.

[marijnh]

Merged (in squashed form) as 206afdf (and equivalent on the v3 branch).