Enhanced jump mode (based on #3791)

[semin-park]

What does this PR do?

~~Took #3791, rebased on top of master and modified the jump logic for word based jumps.~~ Now it's based on #5420.

Disclaimer: the logic was heavily influenced by hop.nvim and the visuals (RGB values) were directly copied.

Improvements

The improvements are:

1. #3791 takes a set of characters (to be used in jumping), calculates an arbitrary cutoff point and then uses keys[..cutoff] for single-key-press jumps and the remaining keys[cutoff..] for multi-key-press jumps. This logic works, but it produces too many unnecessary 3 character jumps even when we don't need them. The formula for how many <= 2 key-press-jumps we can generate with this logic is: keys[..cutoff].len() + keys[cutoff..].len().pow(2). Assuming 26 characters and cutoff of 10, the result is 10 + (26-10)^2 = 266 jump locations. Any more than this will end up with some jumps requiring three key presses or more.
   In this PR, I'm starting with all one character jumps and expand the trie only when necessary. This means that we can always produce the most optimized character sequences, and thus the arbitrary cutoff point doesn't have to be calculated anymore. Now the number of <= 2 key-press jumps we can represent is simply 26^2 = 676. Practically, I have never had to deal with any three character sequences, since usually the number of jump locations on my screen is less than 400.

2. The previous logic used movement::move_prev_word_start to get the word boundary, but it didn't check what kind of character is underneath the cursor. This meant that many jump locations were too close to each other, so there used to be a logic that checks whether the current jump location is too close to the previous jump location, and abandons the current jump location if it is too close. In this PR, I'm reducing the number of jump locations by only generating jump targets for words that satisfy char_is_word (i.e., alphanumeric + _). Usually the place we want to edit is an identifier (function name, parameter name etc), so I feel this approach is more ergonomic.

3. Some visual enhancements, such as dimming the screen so that the jump locations can be seen more easily, and using slightly different colors in case of a multi character sequences, so that the first key can be seen more easily. This UI part is just reproducing what hop.nvim does in helix.

How to proceed

I know that virtual text support is still WIP and this PR is going to become stale. But I wanted to put this up here just so that it could be incorporated in the future work.

With regards to how to work further on this, I'm completely open to whatever works. I can take this further after #5008 gets merged; or if @Omnikar wants to push this through, I am happy with that too after we talk this through.

(Updated) Examples

Normal use case

https://user-images.githubusercontent.com/30904140/210132132-5d114b33-fd80-4ab4-bf23-bf5950102239.mp4

Growing selection

https://user-images.githubusercontent.com/30904140/210132227-8007c9a1-3a3c-4087-bf75-286cc04c1b27.mp4

[Omnikar]

These are some nice improvements, I'll do a full review when I get a chance.

From a quick test it looks like it's missing the selection behaviours from #3791; i.e. extend mode support and selecting the jumped-to word when using word jump.

[semin-park]

These are some nice improvements, I'll do a full review when I get a chance.

From a quick test it looks like it's missing the selection behaviours from #3791; i.e. extend mode support and selecting the jumped-to word when using word jump.

Oh I totally missed this behavior, I'll add them back in. Thanks for the comment, much appreciated!

[semin-park]

Fixed the comments and added selection extending behavior for word based jumps as well. The videos in the original comment have been updated as well for quick grasp of the current behavior.

I tried to reorganize the code into:

1. sequence.rs generates the key sequences for jump targets. It doesn't know anything about the screen or the document that we're dealing with.
2. locations.rs generates the jump targets by reading the buffer in the current screen.
3. score.rs sorts the jump targets based on some scoring function.
4. annotate.rs has helper functions such as generating the actual text annotations understood by helix-view, dimming and clearing the view.

[cd-a]

Looks great. Would it be possible to have the dimming as an optional setting? It's personal preference, and I myself find that very distracting while working, and prefer just highlights with a background color.

[semin-park]

Since virtual text support is going to come from #5420, I modified the annotation logic to use #5420 instead of #417. The diffs are:

• Use #5420 for virtual text support (annotation logic changed)
• Penalize targets that are further away in the y-axis, in order to prefer targets closer to the current row
• Color for jump characters are now configurable. The added theme parameters are ui.virtual.jump.single, ui.virtual.jump.multi.first and ui.virtual.jump.multi.rest. These parameters can be assigned different colors in the theme toml file.
• Took @dahmc's suggestion and made dimming optional. Jump mode will still dim the view by default, but this behavior can be overridden with

// config.toml
[editor]
dim-during-jump = false

Jump mode seems to be a bit unstable in the presence of other virtual text, so I'll debug further and report back.

[QuinnFreedman]

Is this currently working? I tried it out, but when I try to enter jump mode it hangs the whole editor. There is no error log and eventually I have to just close the terminal window.

[semin-park]

Huh that's odd, could you tell me your configuration and your setup, so that I can try to reproduce? I am using this daily and I didn't mean to break anything..

[QuinnFreedman]

After doing a little debugging, it looks like the issue was from the theme I was using (the built-in monokai pro theme). Switching back to the default theme works fine. I installed this fork over the top of my main branch helix install, so I'm not sure which version the theme came from. I would guess it's just missing the colors for jump mode (maybe we could fallback to defaults or something in that case)

Here is the offending theme if you are interested: monokai_pro.zip

[QuinnFreedman]

Also, a very minor bug: it seems like word-wise jump ignores the last word in the document if there is no trailing newline

[pascalkuthe]

That was a known issue with #5420. Rebasing on the newest changes will fix the problem

[semin-park]

Ah I see, thanks for the debugging :) I rebased this PR on top of #5420 so hopefully this issue is gone now :) But one thing that I noticed in your theme file is that it's lacking these values:

"ui.virtual.jump.single" = { fg = "#ff007c", modifiers = ["bold"] }
"ui.virtual.jump.multi.first" = { fg = "#00dfff", modifiers = ["bold"] }
"ui.virtual.jump.multi.rest" = "#2b8db3"

I added them into helix/runtime/themes/monokai_pro.toml in this PR, so you might want to use that if you want your jump labels to be colored correctly

[QuinnFreedman]

Thanks! that worked. Everything seems to be working as expected now

[QuinnFreedman]

On the subject of themes, would you consider adding a customizable style for the background text in jump mode? (Or does that already exist?) It seems like it's mapped to the comment style which looks a little weird since my comments are italic

[QuinnFreedman]

One other quick bug: in sort_jump_targets, cursor is in screen coordinates but positions are in document coordinates. This means that if you jump from deeper in a document, all the single-character jump tags will be at the top of the screen.

I'm happy to make a PR to address this if you want. Should be a one-line fix

[semin-park]

One other quick bug: in sort_jump_targets, cursor is in screen coordinates but positions are in document coordinates. This means that if you jump from deeper in a document, all the single-character jump tags will be at the top of the screen.

I'm happy to make a PR to address this if you want. Should be a one-line fix

Oh thanks for the bug report! The bug must have sneaked in when I last pushed my commits. Fixed it and pushed again :)

On the subject of themes, would you consider adding a customizable style for the background text in jump mode? (Or does that already exist?) It seems like it's mapped to the comment style which looks a little weird since my comments are italic

The background is currently set to the comment style, but I can make it configurable that falls back to the comment style if no applicable user config is found. I don't have time right now, but I can try changing this tonight 😄

[semin-park]

Added ui.virtual.jump.dim and set it to #666666 by default for all available helix themes.

[QuinnFreedman]

Thanks for your work on this! I can't wait for it to get merged into core

[jlebon]

This is great and works really well! I have a suggestion, but feel free to defer to later: is it possible to have Ctrl-c also work as a way to cancel jump mode after entering it (i.e. make it work just like Esc) ? This makes it similar to how Ctrl-c today can be used to exit most popup menus as an alternative to Esc. Currently, Ctrl-c is just treated as if "c" were typed which feels confusing.

[thomasgoulet]

Nice PR! Would love to have this be merged

Just wanted to mention that triggering the Jump mode commands from inside the command palette renders the virtual text but doesn't actually trigger the command / cancels it right away. This behavior also seems to affect the find_next_char, find_prev_char, etc... commands but they don't render any virtual text.

Only way to get the correct syntax highlight back is to run any of the Jump commands from a keybind or close the buffer.

[semin-park]

This is great and works really well! I have a suggestion, but feel free to defer to later: is it possible to have Ctrl-c also work as a way to cancel jump mode after entering it (i.e. make it work just like Esc) ? This makes it similar to how Ctrl-c today can be used to exit most popup menus as an alternative to Esc. Currently, Ctrl-c is just treated as if "c" were typed which feels confusing.

Thanks for the suggestion! I made jump mode only recognize characters when no modifier has been added to the input keys, and I think this should be reasonable for now, given that jump mode only works with ascii lowercase characters.

I personally hope that helix could alias Ctrl-c to ESC, so filed #5747 to ask about this.

[semin-park]

Nice PR! Would love to have this be merged

Just wanted to mention that triggering the Jump mode commands from inside the command palette renders the virtual text but doesn't actually trigger the command / cancels it right away. This behavior also seems to affect the find_next_char, find_prev_char, etc... commands but they don't render any virtual text.

Only way to get the correct syntax highlight back is to run any of the Jump commands from a keybind or close the buffer.

It looks like command palette creates a temporary Context when executing commands, so the context that jump mode sees is different from when it was launched directly from key binding. Asked about this in the matrix chat and was told that there are known bugs due to this (#4508, #5294) and works to address this (#5555).

But since this is going to be a large-scale change, I think you will have to use the key bindings for now :'(

[pascalkuthe]

With #5420 merged this can be rebased on master.

On a more personal note: Have you considered replacing the single char command with a double char command. That will require loooking at labels much less often. Especially if you make it directional and immidietly jump like leap

[Omnikar]

On a more personal note: Have you considered replacing the single char command with a double char command. That will require loooking at labels much less often. Especially if you make it directional and immidietly jump like leap

My personal use case of jump mode is to get to where I want to get to quickly, with minimal thought. Adding directionality to jump mode adds an extra thing you have to think about. Also, directional multichar jumping is starting to sound a lot like ordinary / ? searching.

[pascalkuthe]

On a more personal note: Have you considered replacing the single char command with a double char command. That will require loooking at labels much less often. Especially if you make it directional and immidietly jump like leap

My personal use case of jump mode is to get to where I want to get to quickly, with minimal thought. Adding directionality to jump mode adds an extra thing you have to think about. Also, directional multichar jumping is starting to sound a lot like ordinary / ? searching.

For the usecase of getting where I am going with minimal thought I love the word jump. I use it to just quickly get to the rough area where I want to edit.

The single char jump is too noisy for me. It places labels everywhere because many chars appear quite often. The usecase for single/double char jump for me are cases where I need a more precise jump where word boundaries don't cut it.

With two chars there are much less ambiguities and so often you end up at the right place right away or only habe a few labels to chose from. The directionality isn't that critical to that usecase. However K tend to think of it of a much more accurate f so that's whwre I am coming from with that. The case of I want to use f but there is a char in the way occurs so often to me and a command like I described solves it neatly. This usecase of a precise f would work especially well if the motion extends the selection (but I can also see that annoying people, maybe having a separate extend and goto variant of the command could work)

[semin-park]

Rebased on top of master, and made the number of characters to type before labelling configurable. The default value is still 1 just because I think that's the base case and I don't have a strong opinion on what should be the default, but you can override it like this:

[editor.jump-mode]
num-chars-before-label = 2

[pascalkuthe]

Rebased on top of master, and made the number of characters to type before labelling configurable. The default value is still 1 just because I think that's the base case and I don't have a strong opinion on what should be the default, but you can override it like this:

[editor.jump-mode]
num-chars-before-label = 2

neat, I thaught about that too but thaught that might be too much effort. Thanks for adding that :) Probably the best approach to accommodate various preferences. I think with very few well chosen config options like this one we could accommodate a wide range of preferences. I see that you already have an extending version of the commands which I could rebind to use in normal mode aswell, nice! Further option don't have to be part of the initial PR in my opinion either as this already covers lots of usecases. I will add this to my daily driver and test drive for a bit and do a more trough review in the future as I don't have time for that right now

[eulerdisk]

I think there will be many more "jump modes" in the future, other than WORD an N-CHAR, and the suggested keybinding is not fast enough for most people. Personally I will probably go for a single char activation, like the Enter key. (I have a Planck keyboard, the Enter key is under my right thumb, so for me Enter is FAST)

There's no a strong default solution, so maybe this feature should not deserve default keybindings.

[semin-park]

I think there will be many more "jump modes" in the future, other than WORD an N-CHAR, and the suggested keybinding is not fast enough for most people. Personally I will probably go for a single char activation, like the Enter key. (I have a Planck keyboard, the Enter key is under my right thumb, so for me Enter is FAST)

There's no a strong default solution, so maybe this feature should not deserve default keybindings.

I'm okay with removing the default key binding for jump mode, as long as it can be well communicated to the end users that such a thing exists. But I'm not quite sure what the best way is. If these motion plugins are used by the majority of people, I think having a default command in the default keymap is worth it for visibility (and they can look up what other commands / options are available).

But if not, I'm also okay with removing the default key bindings and adding a section in :tutor instead.

[pascalkuthe]

I think there will be many more "jump modes" in the future, other than WORD an N-CHAR, and the suggested keybinding is not fast enough for most people. Personally I will probably go for a single char activation, like the Enter key. (I have a Planck keyboard, the Enter key is under my right thumb, so for me Enter is FAST) There's no a strong default solution, so maybe this feature should not deserve default keybindings.

I'm okay with removing the default key binding for jump mode, as long as it can be well communicated to the end users that such a thing exists. But I'm not quite sure what the best way is. If these motion plugins are used by the majority of people, I think having a default command in the default keymap is worth it for visibility (and they can look up what other commands / options are available).

But if not, I'm also okay with removing the default key bindings and adding a section in :tutor instead.

I think the default bindings you came up with are nice. I like how intutive g/ and gw feel. They tie in with the existing w and / binding and are therefore easy to remeber and discover which is what we usually optimize for. If they are not short enough/not palced centrally enough for somebody then they can rebind in their personal config.

[semin-park]

Oh and I think it's worth mentioning that the current m-char search algorithm is brute force. It iterates all indices within the current view and tries to match the m-char string starting at each index. So the algorithm's worst-case complexity is O(mn). Under normal circumstances (= close to random characters), it should still be close to O(n), and even if not, m should be very small anyway. So practically this is still fast enough.

But if this needs to be further optimized, we could implement the KMP algorithm, although I would prefer to leave it as a low-priority future work.