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

Enhanced jump mode (based on #3791) #5340

Closed
wants to merge 4 commits into from

Conversation

semin-park
Copy link

@semin-park semin-park commented Dec 29, 2022

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.
Now that #5420 is merged, this is rebased on top of master :D

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

Improvements

The improvements are:

  1. Jump mode #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

jump.mp4

Growing selection

jump-growing-selection.mp4

@Omnikar
Copy link
Contributor

Omnikar commented Dec 30, 2022

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.

helix-term/src/commands.rs Outdated Show resolved Hide resolved
@semin-park
Copy link
Author

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 semin-park force-pushed the jump-rebase branch 4 times, most recently from 8493944 to e5d621f Compare December 31, 2022 09:26
@semin-park
Copy link
Author

semin-park commented Dec 31, 2022

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.

@the-mikedavis the-mikedavis added C-enhancement Category: Improvements A-helix-term Area: Helix term improvements S-waiting-on-pr Status: This is waiting on another PR to be merged first labels Dec 31, 2022
otavioschwanck
otavioschwanck previously approved these changes Jan 13, 2023
@cd-a
Copy link
Contributor

cd-a commented Jan 14, 2023

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 semin-park force-pushed the jump-rebase branch 4 times, most recently from 124a74b to f042feb Compare January 16, 2023 06:56
@semin-park
Copy link
Author

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 rework positioning/rendering and enable softwrap/virtual text #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.

@semin-park semin-park force-pushed the jump-rebase branch 2 times, most recently from c358d82 to 13d3f71 Compare January 16, 2023 15:07
@QuinnFreedman
Copy link

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
Copy link
Author

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
Copy link

QuinnFreedman commented Jan 22, 2023

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
Copy link

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

@YaLTeR
Copy link
Contributor

YaLTeR commented Jul 5, 2023

Thanks for making this! EasyMotion-style two-character jump is one of my favorite ways of getting around, and this is close enough.

Found a bug: if I activate the actions from Space-? then they won't reset on Esc or other button press, which can cause all kinds of glitched output.

@evanrichter
Copy link
Contributor

Found a bug: if I activate the actions from Space-? then they won't reset on Esc or other button press, which can cause all kinds of glitched output.

confirmed here too. looks like jump mode doesn't work at all from the Space-? menu for me. The jump targets highlight with characters, text is dimmed, but you can't actually jump anywhere. If you activate jump mode through gw and either jump somewhere or ESC, then normal behavior resumes.

@the-mikedavis
Copy link
Member

That behavior has already been mentioned: #5340 (comment)

It's a problem with the way the command palette works rather than a problem with the feature here.

@semin-park
Copy link
Author

Sorry, fixed the lint issue just now. Could you run it again?

@SecretPocketCat
Copy link

I really love this, hope this gets merged ASAP so that I don't have to build from src 🙂

One little thing I encountered is that line 26 in my config collapses when entering jump mode shifting all lines that follow which is quite disorienting
config.zip
Not sure what I did there as replacing the empty line negates the issue, but I figured I might as well share the repro.

Similar shift happens when there are too many tags (e.g. matching a very common char like double quotes in the config) but I can't really imagine a way around that.

@archseer archseer mentioned this pull request Aug 25, 2023
3 tasks
@dvic
Copy link
Contributor

dvic commented Aug 30, 2023

@semin-park this PR is awesome, thank you for your hard work 🙏! Is it a lot of work to rebase/ merge master in this pr? I'm not familiar with the code base but I could give it a try and open a pull request on your branch.

@fromZahir2Aleph
Copy link

@semin-park this PR is awesome, thank you for your hard work 🙏! Is it a lot of work to rebase/ merge master in this pr? I'm not familiar with the code base but I could give it a try and open a pull request on your branch.

I have merged it using 'three-way merge editor' in VS Code. Works the same way as before merging so far. And I am practically a noob even at Git.

@Omnikar
Copy link
Contributor

Omnikar commented Aug 30, 2023

@semin-park this PR is awesome, thank you for your hard work 🙏! Is it a lot of work to rebase/ merge master in this pr? I'm not familiar with the code base but I could give it a try and open a pull request on your branch.

I have rebased it here: semin-park#1

@tmpm697
Copy link

tmpm697 commented Aug 30, 2023

Really need this feature as I like https://github.com/ggandor/leap.nvim a lot, reduce so much keystrokes.

@ntzm
Copy link

ntzm commented Aug 31, 2023

One minor issue I've found is that modifiers are removed from dimmed text. In your example videos the italic comments are kept as italic when they are dimmed. However testing locally this does not seem to be the case.

@vaheqelyan

This comment was marked as spam.

@semin-park
Copy link
Author

Oh sorry for the late reply, I'll get back to resolving the conflicts maybe this or next week.

Been quite busy lately haha

@conways-glider
Copy link

Is this still being worked on? If not, what would it take to get this across the line?

@yerlaser
Copy link
Contributor

Is this still being worked on? If not, what would it take to get this across the line?

It needs to be reviewed and core contributors currently don't have time for this.
In the meantime, if you're impatient, you can try helix-ext as it incorporates many such PRs which didn't make it to the Helix proper.

@SecretPocketCat
Copy link

Is this still being worked on? If not, what would it take to get this across the line?

I'd venture a guess this's not gonna be merged since #8875 exists. A solution like this will have to wait for the plugin system whenever that drops.

@Drazhar
Copy link

Drazhar commented Jan 19, 2024

Is this still being worked on? If not, what would it take to get this across the line?

It needs to be reviewed and core contributors currently don't have time for this. In the meantime, if you're impatient, you can try helix-ext as it incorporates many such PRs which didn't make it to the Helix proper.

Can you give a link to Helix-ext? Sounds interesting, but I couldn’t find it.

@icecreammatt
Copy link

Is this still being worked on? If not, what would it take to get this across the line?

It needs to be reviewed and core contributors currently don't have time for this. In the meantime, if you're impatient, you can try helix-ext as it incorporates many such PRs which didn't make it to the Helix proper.

Can you give a link to Helix-ext? Sounds interesting, but I couldn’t find it.

https://github.com/omentic/helix-ext

@yerlaser
Copy link
Contributor

Is this still being worked on? If not, what would it take to get this across the line?

It needs to be reviewed and core contributors currently don't have time for this. In the meantime, if you're impatient, you can try helix-ext as it incorporates many such PRs which didn't make it to the Helix proper.

Can you give a link to Helix-ext? Sounds interesting, but I couldn’t find it.

You already have the link :)

I tried that myself some time ago and I like the features very much, the jump-mode, color-coded brackets and indents.
But, it did crash on me a couple of times and so I switched back to the proper Helix because I didn't want to loose my data.
Your mileage may vary, though.

@James-Firth
Copy link

Excited when there's time for this to be reviewed!

Not to derail or scope creep this PR, this could be a future feature on top of this jump mode? Just want to mention the idea here in case it would require a deep rework if delayed:

It would be great if you could also use jump mode to place a new cursor for those tricky multi-cursor moments.

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 C-enhancement Category: Improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.