Refactor Vim mode

[vkbo]

Summary:

This PR refactors the Vim mode code command processing logic.

Related Issue(s):

Reviewer's Checklist:

• The header of all files contain a reference to the repository license
• The overall test coverage is increased or remains the same as before
• All tests are passing
• All linting checks are passing and the style guide is followed
• Documentation (as docstrings) is complete and understandable
• Only files that have been actively changed are committed

[vkbo]

@idealemu, I've refactored the logic a little, and removed some function calls. Could you check that the functionality wasn't affected? I noticed no change, but I'm not a Vim expert 😅 (tests still pass).

I also added the switch to Preferences. I had to change how the Vim mode is initiated because now it can change dynamically at runtime when the user changes the setting, but it wasn't that tricky to do.

Edit: I'll update the docs later. I need to rewrite the text a little, so I'll need a review on that too from you to make sure I don't change the meaning.

[idealemu]

@idealemu, I've refactored the logic a little, and removed some function calls. Could you check that the functionality wasn't affected? I noticed no change, but I'm not a Vim expert 😅 (tests still pass).

At first glance it seems good. I'll try it a bit longer to see if anything comes up with regular usage.

I also added the switch to Preferences. I had to change how the Vim mode is initiated because now it can change dynamically at runtime when the user changes the setting, but it wasn't that tricky to do.

Super ! As a side note, vim mode can be quite confusing for people who don't know vim and would enable it just to see. You can always disable it if you don't understand, but one approach I've seen other software use (Joplin I think ?) was to have a little box that pops up asking if you are sure, and asking you to input the vim command that allows you to close and exit vim without saving as a proof that you know what you are doing (Hint: it's :q! ). I don't know if this would be a good idea here.

Edit: I'll update the docs later. I need to rewrite the text a little, so I'll need a review on that too from you to make sure I don't change the meaning.

If you want me to check them too please just ping me 😄

[vkbo]

Thanks!

Yeah, I was wondering if the "Enable Vim mode" setting should have some more info around it. I was also wondering if it should be moved to its own section in Preferences so it stands out more. Do you think you would like to add more app-level commands at some point too? So that there may be more settings associated with Vim Mode?

I think adding a pop-up box that tells you the risks of enabling Vim mode should be enough. It may be a good idea. At least the first time you enable it.

[idealemu]

Thanks!

Yeah, I was wondering if the "Enable Vim mode" setting should have some more info around it. I was also wondering if it should be moved to its own section in Preferences so it stands out more.

My guess is that if there is a warning box that says something like "Hey are you sure about this ? Enter how to quit vim without saving as proof", people who know would do it, and people who don't would be spared confusion, and people who are curious would look it up and hence find what it is all about: no need to bunch to much information in the settings when you can 'prompt' users to look it up.

Do you think you would like to add more app-level commands at some point too? So that there may be more settings associated with Vim Mode?

I don't think so ? Vim is well known for being entirely configurable so if we start providing some settings for certain aspects then it will be a weird in between. I think based on what is associated with vim this is either a case of zero customisation, or full customisation.
Some examples of this in the wild are:

• Joplin vim mode -> just a keyboard layout essentially (similar to what we have done here)
• IdeaVim (plugin for IntelliJ IDEs) -> vim mode, but you can also point it to your vimrc config file. In that case I assume it needs a full vimscript or lua engine and needs to be built a lot different then just a bunch of key-mappings.

And I don't think we would want the latter in novelWriter 😄

I think adding a pop-up box that tells you the risks of enabling Vim mode should be enough. It may be a good idea. At least the first time you enable it.

That should do the trick for now 👍

[idealemu]

Another optimisation I just thought (if this is the right branch for that) of would actually be the order of the IFs based on what commands are more likely to be input.
Ex:
"hjkl"are the most basic movement keys and are used all the time, but "de" for example is much less rarely used so no need to check that if every time when we could return early instead.

[vkbo]

Yeah, putting them in order is probably more optimal. However, this is not really performance critical since the user isn't actively typing when entering the commands, so a longer delay is tolerable and quite likely not noticeable anyway.

The critical part of the editor is the amount of processing happening as you type. A very good typing speed is about a character per 100 ms, and even in Python you can do a lot in 100 ms. You can wait even longer when it's an interaction like passing a command.

Now, I don't like to waste cycles, which is why I though it made sense to extract the command from the VimState class only once, but once that is done, I expect the if checks themselves are very fast. Still, sorting them in most likely order doesn't hurt. But as I said, I doubt it will make a noticeable difference.

[vkbo]

I've moved the Vim mode setting to a new "Features" section of Preferences.

I think that will make it easier to add more Vim settings if needed, and make them separate enough that they stand out.

When toggling the setting from off to on, and clicking "Save", a dialog box will show up asking "Are you sure you want to enable Vim mode? This changes how the editor accepts input.". If you select "No", it will not enable it.

Thoughts?

On a sidenote, I've been sick with the flu for a week and a half, so I haven't been able to keep my own schedule for the 2.8 release. Hopefully I will get the RC1 release ready this weekend.

[vkbo]

I've also updated the Vim docs. You can see a preview here: https://github.com/vkbo/novelWriter/blob/refactor_vim_mode/docs/source/features/vim_mode.rst

Let me know if it's OK or needs any changes.

[vkbo]

One final question: How do you want to be credited for the Vim mode contribution? I prefer to credit people by name, but it is not required.

[idealemu]

When toggling the setting from off to on, and clicking "Save", a dialog box will show up asking "Are you sure you want to enable Vim mode? This changes how the editor accepts input.". If you select "No", it will not enable it.

Thoughts?

That seems good enough for me 👍 Super !

I've also updated the Vim docs. You can see a preview here: https://github.com/vkbo/novelWriter/blob/refactor_vim_mode/docs/source/features/vim_mode.rst

Let me know if it's OK or needs any changes.

Perfect ! Now it will just need to reflect any future changes, but if I (or another) makes those we can just edit the existing rst.

One final question: How do you want to be credited for the Vim mode contribution? I prefer to credit people by name, but it is not required.

I can be credited by name 😄 I can't really remember why I switched my Github account to use an emu instead, but I will update my display name on my profile so you can just take it from there 👍

[vkbo]

I can be credited by name 😄 I can't really remember why I switched my Github account to use an emu instead, but I will update my display name on my profile so you can just take it from there 👍

Cool. Then I think we're good to go!

Although there is one remaining issue I just thought of. This will cause problems:

novelWriter/novelwriter/guimain.py

Lines 1275 to 1284 in 746ebb7

	def _keyPressEscape(self) -> None:
	"""Process an escape keypress in the main window."""
	if CONFIG.vimMode:
	self.docEditor.setVimMode(nwVimMode.NORMAL)
	return
	if self.docEditor.searchVisible():
	self.docEditor.closeSearch()
	elif SHARED.focusMode:
	SHARED.setFocusMode(False)

When Vim mode is active, it will always capture the escape here, and the regular functionality to close the search box if it is active, or exit focus mode, will no longer work. Could this if check be changed to only cancel Vim mode if it is not set to NORMAL?

I'd like to rewrite it to:

    @pyqtSlot()
    def _keyPressEscape(self) -> None:
        """Process an escape keypress in the main window."""
        if self.docEditor.searchVisible():
            self.docEditor.closeSearch()
        elif self.docEditor.isVimEscapable():
            self.docEditor.setVimMode(nwVimMode.NORMAL)
        elif SHARED.focusMode:
            SHARED.setFocusMode(False)

and add this in the doc editor:

    def isVimEscapable(self) -> bool:
        """Check if Escape keypress should be processed for Vim mode."""
        return CONFIG.vimMode and self._vim.mode != nwVimMode.NORMAL

Do you see any issues?

[idealemu]

Could this if check be changed to only cancel Vim mode if it is not set to NORMAL?

You need to be able to press ESC to reach NORMAL mode when not in it (be it in INSERT or VISUAL/V-LINE mode), so you shouldn't ever need to press ESC within NORMAL mode.

I just checked vim behaviour, and pressing ESC while in NORMAL mode, only does one thing: cancel/interrupt an ongoing sequence of keys. Although inputting a sequence that is not valid seems to do the same.

Now you normal typing is done in INSERT mode, so if ESC needs to be e.g interact with autocomplete e.x for tag selector, then that might be a conflict.

[vkbo]

The default behaviour of Esc is to close the search box. Which it still does if the search box is active in the first branch of the "if". But if we also need to use Esc when in NORMAL mode, then maybe the code should be reverted to the old behaviour and Esc only work for Vim mode switching iv Vim mode is enabled. Basically the way you had it before.

It's not a big deal that Esc cannot close search or exit focus mode like it does normally. There are other ways to do that when Vim mode is enabled.

So, basically:

    @pyqtSlot()
    def _keyPressEscape(self) -> None:
        """Process an escape keypress in the main window."""
        if CONFIG.vimMode:
            # Esc key is only used for Vim commands
            self.docEditor.setVimMode(nwVimMode.NORMAL)
        elif self.docEditor.searchVisible():
            self.docEditor.closeSearch()
        elif SHARED.focusMode:
            SHARED.setFocusMode(False)

[vkbo]

The auto-complete menu captures the Esc key before it reaches the main window, so closing the auto-complete menu works fine also in Vim mode, so that's not a problem.

[idealemu]

The auto-complete menu captures the Esc key before it reaches the main window, so closing the auto-complete menu works fine also in Vim mode, so that's not a problem.

That seems to be what I've experienced when using tags.

I'd like to rewrite it to:

    @pyqtSlot()
    def _keyPressEscape(self) -> None:
        """Process an escape keypress in the main window."""
        if self.docEditor.searchVisible():
            self.docEditor.closeSearch()
        elif self.docEditor.isVimEscapable():
            self.docEditor.setVimMode(nwVimMode.NORMAL)
        elif SHARED.focusMode:
            SHARED.setFocusMode(False)

I mean this could be ok ? If a search windows is visible then it takes precedent over mode switching, but what I want to avoid is the bug that a lot of vim mode implementations have where you have to press ESC twice to get it to register as the "vim ESC", which is very workflow breaking for the user.

To be fair I would need to try this change to see if it actually affects things in a bad way, I don't think I am getting the full picture just from reading the snippet for this change

[vkbo]

The way isVimEscapable is implemented means it doesn't call setVimMode if it is already in NORMAL mode. However, if it is supposed to reset the command if it's in NORMAL mode, then this will never occur.

I guess I can let closing the search box take precedence, like in the code above, but change the second condition back to check if vimMode is enabled like you did in your implementation. That means that when Vim mode is on, it can never reach the last if condition where it exits focus mode. That may actually be wise because if you do use Vim mode and are in Focus mode, hitting Esc once too many will exit Focus mode, which may be annoying. You can always exit Focus mode anyway by pressing F8, so it isn't important.

I think I will make this change, and just leave it like that. Then you can test it a bit with this in mind when I make the RC1 release, and we can make changes before the final release if necessary.

[idealemu]

The way isVimEscapable is implemented means it doesn't call setVimMode if it is already in NORMAL mode. However, if it is supposed to reset the command if it's in NORMAL mode, then this will never occur.

It does in real vim, but I don't think I particularly cared about this in the implementation, so you are correct that it doesn't do that here. It might by default do it when in VISUAL mode because pressing ESC will cancel your current selection I think.

I think I will make this change, and just leave it like that. Then you can test it a bit with this in mind when I make the RC1 release, and we can make changes before the final release if necessary.

Sounds good ! I will do some writing with it and see if anything is annoying, otherwise I think it should be fine as you suggested.