helix
helix copied to clipboard
helix-editor
Jump mode
Supersedes #1162. Relevant: #274, #510.
This PR depends on virtual text (#417); my commits are currently based on top of that PR. As a result, note that there's a fair chance your config won't work if you check out this branch to test, since the virtual text PR was last rebased several months ago.
As of now, the implementation of this PR takes one keypress after the command is invoked, and then annotates all instances of that key that are on screen and after the cursor with key sequences that can then be typed to jump to that location.
However, I am considering the idea of instead annotating jumps to tree-sitter syntax nodes immediately when the command is invoked, saving a keypress at the cost of some jump precision. What are you all's thoughts on that?
To do:
- [x] Allow the colors of the jump annotations to be themed.
- [x] Allow jumping to locations before the cursor.
- [x] Add compatibility with select mode.
https://user-images.githubusercontent.com/79615262/189510972-9ce08e25-0544-4edb-84e5-d2a9055334ba.mp4
I ran the video at half the speed and still didn't get what keys are being pressed :) But, in any case, thanks for the feature, it will be a big upgrade in usability when shipped. With regard to your question, I would love to see the first option to remain as a fallback. I have to work with certain filetypes that don't have tree-sitter support and I miss so many features, even autocompletion of very long words, like internationalization :). So, over-reliance on tree-sitter hurts usability a lot.
Thanks for the effort. As mentioned in the relevant issues, there are many ways to implement this feature. Some people want the behaviour of Hop.vim others like leap.nvim more. I think the most important thing is to build a solid foundation, so every labeling mechanism can be implemented to give users maximum choice.
I think the most important thing is to build a solid foundation, so every labeling mechanism can be implemented to give users maximum choice.
Fortunately, that's already how the code in this PR is laid out; finding the locations to jump to and labelling and going through with the jumping are separate, although they exist within the same function right now.
I ran the video at half the speed and still didn't get what keys are being pressed :)
Each jump in the video is 4 to 5 keystrokes:
-
gj, which is what I currently have bound to starting jump mode - The key for the character being searched
- The 1 to 2 key jump label at the location to jump to
With regard to your question, I would love to see the first option to remain as a fallback. I have to work with certain filetypes that don't have tree-sitter support and I miss so many features, even autocompletion of very long words, like internationalization :). So, over-reliance on tree-sitter hurts usability a lot.
Ah right I see what you mean. Allowing the jump location calculation mechanism to be configured brings up the question of what exactly the configuration options should be, so I'll definitely need some input on that front.
My favorite implementation is in Amp.rs: https://amp.rs/docs/usage/#jump-mode As you can see it works on words and labels every element with a two-character label. This allows to reference up to 676 elements which is normally sufficient to reference all words in a viewport. I assume that tokenization into words is done by helix itself without tree-sitter. And it saves time because you only need to look up the label immediately after the keybinding without first looking up for the search character.
My favorite implementation is in Amp.rs: https://amp.rs/docs/usage/#jump-mode As you can see it works on words and labels every element with a two-character label. This allows to reference up to 676 elements which is normally sufficient to reference all words in a viewport. I assume that tokenization into words is done by helix itself without tree-sitter. And it saves time because you only need to look up the label immediately after the keybinding without first looking up for the search character.
It looks like Amp exclusively uses 2-character jump labels; I have this PR set up to allow jump labels to be as long as is necessary, although that length would usually not exceed 2 and in practicality never exceed 3. The important thing though is that different jump labels have different lengths; farther jump locations use longer labels.
As I mentioned on Matrix, I came up with a formula to mimic Easymotion's behaviour of dynamically calculating how many keys it needs to use to prefix multikey jumps, and how many can be used for single key jumps.
I'm wondering if we should provide two jump mode keybinds: one to jump to words for faster jumping, and one that takes an extra search keypress for precision jumping.
Unfortunately, my knowledge of the helix codebase (and Rust, as well) is still insufficient to work on this, so I can only suggest. I would say whichever way it's easier for you to implement is the best. Because, I think many people are eagerly awaiting for some implementation of this feature. It can be improved afterwards based on feedback of the majority.
I would say whichever way it's easier for you to implement is the best.
Well, one is already implemented in this PR, and the other I actually implemented in the original jump mode PR (#1162), I'd just have to combine them.
Although first, I'd like to get a maintainer's feedback on this configurability / two jump mode keybinds aspect.
Maybe we could have 3 separate commands:
- one that jumps based on typed characters
- one that jumps based on tree-sitter
- one that tries tree-sitter first, but if there's no language on the doc, then fall back to typed chars
The latter could be the one that is bound in the default key bindings.
Maybe we could have 3 separate commands:
one that jumps based on typed characters
one that jumps based on tree-sitter
one that tries tree-sitter first, but if there's no language on the doc, then fall back to typed chars
The latter could be the one that is bound in the default key bindings.
How about two commands:
- One which tries tree-sitter but falls back to words
- One which searches a typed character
I don't see a reason to separate "tree-sitter" from "tree-sitter with fallback".
Also, with tree-sitter or word-wise jumping, should we select the destination syntax node/word?
Yeah, two commands sounds good to me too. Personally I think selecting the whole word/node makes the most sense.
I've pushed an initial implementation of word-wise jumping (no tree-sitter support yet) that selects the word that was jumped to. I've additionally made the character search jump (now bound to gJ) extend while in select mode.
Should a new theme key be added for the jump labels? What should it be called?
I haven't given this a proper look yet but I think it could be cleaner to create and merge jump label highlights in the same way we currently create & merge diagnostics and cursors: https://github.com/helix-editor/helix/blob/03612174ee4cef23217b5adf415ced4a851b4a44/helix-term/src/ui/editor.rs#L120-L135 and https://github.com/helix-editor/helix/blob/03612174ee4cef23217b5adf415ced4a851b4a44/helix-term/src/ui/editor.rs#L262-L302. It wouldn't need any changes to the rendering code and would be able to use a new theme key very easily.
I do think this should be themable π. I don't have strong opinions on the theme key name though. ui.jump.label or something similar as a placeholder?
Great that you're working on this further! Like one of the commenters, I'm open to the most user-efficient implementation getting preferential treatment, even if it differs from what I'm used to.
Personally my sense was that populating the screen with keys close to j/k has the lowest mental and mechanical overhead, compared to e.g. first typing the specific letter one aims for. But I didn't test the alternatives for a sufficiently long time.
About the tree-sitter integration idea⦠with all the syntax nodes that can be and usually are present, I'm not sure how to approach choosing jump locations, or if we should even be pursuing integrating tree-sitter with jump mode at all. I think I need commentary from someone who's more familiar with working with tree-sitter.
A tree-sitter version of this would be interesting. There would need to be autocomplete for the node names but even then it would be potentially confusing because each tree-sitter grammar uses its own node names which are almost never documented and sometimes have odd naming conventions.
It would be a fun experiment but I would definitely leave it out of this PR
EDIT: I don't think it would be really possible to do it with jump labels because they would be overlapping most of the time. So instead I was thinking of something like search or select, which is different from this feature
A tree-sitter version of this would be interesting. There would need to be autocomplete for the node names but even then it would be potentially confusing because each tree-sitter grammar uses its own node names which are almost never documented and sometimes have odd naming conventions.
It would be a fun experiment but I would definitely leave it out of this PR
EDIT: I don't think it would be really possible to do it with jump labels because they would be overlapping most of the time. So instead I was thinking of something like search or select, which is different from this feature
Alright, then in that case, I think all that's really left for this PR is adding theming support and getting #417 merged. I'm excited! π
#417 has been rebased on master, so I've rebased this PR on that. I'll rebase again once #417 gets merged.
My concern is, what if #417 takes forever to get merged? Is there a plan B :)
My concern is, what if #417 takes forever to get merged? Is there a plan B :)
You can always pull someone's fork locally and use that. Or exercise some patience. I don't think it makes sense to completely change an implementation to avoid waiting on a proper review.
Jump labels can now be themed using ui.jump.single and ui.jump.multi. If absent, the code falls back to the red and yellow shown in the video.
I also fixed a bug that caused jumps near the cursor to be missing if there were enough total jump labels to have 3-key ones.
The checklist I have in the first comment in complete now, so this PR should be ready to be reviewed.
I have to work with certain filetypes that don't have tree-sitter support and I miss so many features, even autocompletion of very long words, like internationalization :).
Interesting, didn't thought about that. I just tried it out, yup not good experience. I picked the following paragraphs from the analects (original text, non-translated, non-explained https://lunyu.5000yan.com/).
With gj, looks like the jump for this jumps to the wrong characters too, it assumes all characters are 1 character width.
With gJ<ctrl-space>zi1<ctrl-space>p (the character is supposed to be ε but not sure why the jump characters appearing randomly), and worse case is it didn't even jump to the correct position, I guess it's due to the jump characters are being displayed assuming all characters being searched are single-width characters, that's why it just display the jump characters on the screen and when I press it's wrong. Most likely it's the same for unicode. And combining this with IME, the number of characters I have press to get this work is likely not worth it.
Can double check where the jumps are located against the first screen then you probably noticed some issues. Although not all users may use CJK characters, but I think even emojis might be affected.
It seems like the jump label location calculation is malfunctioning for multibyte characters. I'll see what I can do.
I also think there's a bug where the jump labels will stop being generated if a line that extends off screen is reached, because it thinks offscreen means off the bottom/top of the screen.
I just did some testing and it seems like the cursor actually jumps to the correct locations, but the labels are in the wrong locations. In my code, I calculate the text annotation positions by using byte_to_line and line_to_byte on the jump locations, which I now know are correct. I did some more testing and I think the issue might be a bug in those two functions I mentioned.
Oh, it turns out I just needed to use char_to_line and line_to_char instead.
Given the plans to change the virtual text strategy (https://github.com/helix-editor/helix/pull/417#issuecomment-1303910195) will it be a big lift to rework this PR? I'd really love this functionality π
Given the plans to change the virtual text strategy (#417 (comment)) will it be a big lift to rework this PR? I'd really love this functionality π
Well it would depend on how different the interface to the virtual text functionality ends up being.