Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump mode #3791

Closed
wants to merge 27 commits into from
Closed

Jump mode #3791

wants to merge 27 commits into from

Conversation

Omnikar
Copy link
Contributor

@Omnikar Omnikar commented Sep 11, 2022

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:

  • Allow the colors of the jump annotations to be themed.
  • Allow jumping to locations before the cursor.
  • Add compatibility with select mode.
jump-mode.mp4

@Omnikar Omnikar mentioned this pull request Sep 11, 2022
2 tasks
@yerlaser
Copy link
Contributor

yerlaser commented Sep 11, 2022

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.

@eulerdisk
Copy link

eulerdisk commented Sep 11, 2022

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.

@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 11, 2022

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.

@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 11, 2022

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:

  1. gj, which is what I currently have bound to starting jump mode
  2. The key for the character being searched
  3. 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.

@yerlaser
Copy link
Contributor

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.

@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 11, 2022

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.

@yerlaser
Copy link
Contributor

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.

@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 11, 2022

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.

@dead10ck
Copy link
Member

dead10ck commented Sep 11, 2022

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.

@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 11, 2022

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".

@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 11, 2022

Also, with tree-sitter or word-wise jumping, should we select the destination syntax node/word?

@dead10ck
Copy link
Member

Yeah, two commands sounds good to me too. Personally I think selecting the whole word/node makes the most sense.

@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 11, 2022

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.

@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 11, 2022

Should a new theme key be added for the jump labels? What should it be called?

@the-mikedavis
Copy link
Member

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:

let highlights = Self::doc_syntax_highlights(doc, view.offset, inner.height, theme);
let highlights = syntax::merge(highlights, Self::doc_diagnostics_highlights(doc, theme));
let highlights: Box<dyn Iterator<Item = HighlightEvent>> = if is_focused {
Box::new(syntax::merge(
highlights,
Self::doc_selection_highlights(
editor.mode(),
doc,
view,
theme,
&editor.config().cursor_shape,
),
))
} else {
Box::new(highlights)
};
and
pub fn doc_diagnostics_highlights(
doc: &Document,
theme: &Theme,
) -> Vec<(usize, std::ops::Range<usize>)> {
use helix_core::diagnostic::Severity;
let get_scope_of = |scope| {
theme
.find_scope_index(scope)
// get one of the themes below as fallback values
.or_else(|| theme.find_scope_index("diagnostic"))
.or_else(|| theme.find_scope_index("ui.cursor"))
.or_else(|| theme.find_scope_index("ui.selection"))
.expect(
"at least one of the following scopes must be defined in the theme: `diagnostic`, `ui.cursor`, or `ui.selection`",
)
};
// basically just queries the theme color defined in the config
let hint = get_scope_of("diagnostic.hint");
let info = get_scope_of("diagnostic.info");
let warning = get_scope_of("diagnostic.warning");
let error = get_scope_of("diagnostic.error");
let r#default = get_scope_of("diagnostic"); // this is a bit redundant but should be fine
doc.diagnostics()
.iter()
.map(|diagnostic| {
let diagnostic_scope = match diagnostic.severity {
Some(Severity::Info) => info,
Some(Severity::Hint) => hint,
Some(Severity::Warning) => warning,
Some(Severity::Error) => error,
_ => r#default,
};
(
diagnostic_scope,
diagnostic.range.start..diagnostic.range.end,
)
})
.collect()
}
. 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?

@adrian5
Copy link
Contributor

adrian5 commented Sep 11, 2022

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.

@kirawi kirawi added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. S-waiting-on-pr Status: This is waiting on another PR to be merged first A-helix-term Area: Helix term improvements labels Sep 13, 2022
@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 15, 2022

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.

@the-mikedavis
Copy link
Member

the-mikedavis commented Sep 16, 2022

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

@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 16, 2022

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! 🎉

@pickfire
Copy link
Contributor

pickfire commented Sep 29, 2022

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/).

Screenshot_20220929_234158

With gj, looks like the jump for this jumps to the wrong characters too, it assumes all characters are 1 character width.

Screenshot_20220929_234309

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.

Screenshot_20220929_234631

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.

@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 29, 2022

It seems like the jump label location calculation is malfunctioning for multibyte characters. I'll see what I can do.

@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 29, 2022

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.

@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 29, 2022

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.

@Omnikar
Copy link
Contributor Author

Omnikar commented Sep 29, 2022

Oh, it turns out I just needed to use char_to_line and line_to_char instead.

@Omnikar Omnikar marked this pull request as ready for review October 5, 2022 21:43
@heliostatic
Copy link
Contributor

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 😀

@Omnikar
Copy link
Contributor Author

Omnikar commented Nov 28, 2022

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.

@matklad
Copy link
Contributor

matklad commented Dec 7, 2022

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

I've tried (way too) many jump plugins in various editors, so far the best logic I've found is from https://github.com/msafi/xvsc/tree/master/findJump.

The idea is to combine "type word to search" and "type trigger letter to jump". I think the algorithm is roughly like this:

  • given a query string like fo
  • highlight n closest matches too the cursor position
  • draw "jump" characters next to the matches such that there's no conflicts
  • if the next typed char is a jump char, do the jump.
  • otherwise (eg, another o), narrow the set of potential matches to those starting with foo and at the same time extend the set (as the n matches might be further from the cursor).
  • this narrow and extend determines the n and trigger characters: if the query string is fo, we can only use characters which can't complete fo as jump characters.

This achieves the following:

  • "near" jumps are generally just two letters (this can subsume fFtT)
  • arbitrary far jumps are possible, and require just a single jump letter
  • human user always know what to type next. It's not "type char, look what next two chars I need to type, type some unusual combination". It's "start typing the thing I am looking for, type jump char once that shows up and I register that"

@yerlaser
Copy link
Contributor

yerlaser commented Dec 8, 2022

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

I've tried (way too) many jump plugins in various editors, so far the best logic I've found is from https://github.com/msafi/xvsc/tree/master/findJump.

The idea is to combine "type word to search" and "type trigger letter to jump". I think the algorithm is roughly like this:

* given a query string like `fo`

* highlight `n` closest matches too the cursor position

* draw "jump" characters next to the matches such that there's no conflicts

* if the next typed char is a jump char, do the jump.

* otherwise (eg, another `o`), _narrow_ the set of potential matches to those starting with `foo` and at the same time _extend_ the set (as the n matches might be further from the cursor).

* this narrow and extend determines the `n` and trigger characters: if the query string is `fo`, we can only use characters which can't complete `fo` as jump characters.

This achieves the following:

* "near" jumps are generally just two letters (this can subsume `fFtT`)

* arbitrary far jumps are possible, and require just a single jump letter

* human user always know what to type next. It's not "type char, look what next two chars I need to type, type some unusual combination". It's "start typing the thing I am looking for, type jump char once that shows up and I register that"

I thought like this too until I tried https://amp.rs/
Don't judge by the editor itself, it has limited features.
But, it has an awesome jump mode: https://amp.rs/docs/usage/#jump-mode
I'd invite you to try it for a few minutes and I expect you'll immediately appreciate it.
Once you get used to it, your muscle memory will refuse anything else :)
You type f and then type the highlighted characters.

@matklad
Copy link
Contributor

matklad commented Dec 8, 2022

I did try amp roughly when it was released. I find findJump’s approach better:

  • I can jump to any symbol on the screen, not just word start.
  • Common case of near jump requires just two characters.
  • Typing two “random” characters is harder than typing start of the search and a single random character.

@eulerdisk
Copy link

I agree with @matklad.
The AceJump approach (which inspired findJump) is the only one that got me whan I was an IntelliJ user. It doesn't "break the flow" like the other n-char methods.

@pythoneer
Copy link

pythoneer commented Dec 8, 2022

Not quite sure if i follow this findJump/AceJump approach. Its always a little hard to figure out short gif/videos without trying it out yourself.

So if i have a text and started the "jump mode" and typed the first chars to where i want to jump like

random text
with <x>[acti]on
and a little
bit of <y>[acti]vity
to see how this
works in,
you know, <z>[acti]on

typed: acti

the text parts that would match my acti would be highlighted with a maker in front of it (x, y, z). Those are chars that will never follow in the matched chars (like o and v) typing one of those makers would jump there and typing either o or v would narrow down the possibilities. If i got that right – that is really cool to be honest. Because typing random sequences of letters that are littered across the screen is harder than just typing the first (lets say 3) letters of the actual word you want to jump to and that would narrow it down quite dramatically and its what you already have in mind and is most likely a "natural" word that is not awkward to type like a random"jy" or "kq" that you actively need to think about because you might not have typed that combination for a long time if not ever.

@yerlaser
Copy link
Contributor

yerlaser commented Dec 8, 2022

And that is the problem.
With the approach you describe you need to do two mental exercises:

  1. Find the right search character to type and the right amount of them. Bear in mind that the place you want to jump to can be a bunch of special characters making this non-trivial.
  2. After the spots are highlighted you need to lookup the label and type it.

While in amp.rs approach you leave out the first exercise entirely.
Your eyes are already at the place you want to jump to. You turn on the jump mode and immediately see a two-letter label that you need to type to jump to it.

@Omnikar
Copy link
Contributor Author

Omnikar commented Dec 23, 2022

This PR already contains two different jump mode behaviours: One takes a search character first as described in the original PR comment (bound to gJ), and the other immediately annotates word starts as has been described (bound to gj).

@kevyuu
Copy link

kevyuu commented Apr 3, 2023

Hi, I found that the jump to word behaviour will hang if I press it in the middle of the file instead of the start of the file. Here is a file that I try on. It is just a single cpp file. And it will include "\t" as jump point as well.
msaa_sample.zip

@archseer
Copy link
Member

Replaced by #5340

@archseer archseer closed this Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-pr Status: This is waiting on another PR to be merged first S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.