helix icon indicating copy to clipboard operation
helix copied to clipboard

Add a simple amp-like jump command

Open pascalkuthe opened this issue 9 months ago • 35 comments

Closes #274 Closes #510 Closes #5340

This is a simple amp-like jumpmode that @the-mikedavis and I have been toying around with for a while. When discussing/starting to review #5340 we found the PR more complex than we would like for this feature (a diff above 1k loc). We were uncomfortable with how closely it follows hop and the subsequent debate about emulating the million other jumpmode plugins out there. There are many different approaches to jump commands, and we cannot maintain them all in core. Instead, we want to pick a single opinionated choice that we feel fits well with the rest of the editor.

While testing that PR we made the following observations:

  • character based jumps (which require a character before tying a label) don't really hold their water, you can use f for simple cases and / (which we want to improve further) for complex cases. Compared to a jumpmode these commands work with multicursors.
  • word base jumps can be useful for quickly repositioning the primary cursor.
  • a simple implementation like in AMP without dimming, color changes and complex letter assignment based on distance, ... was not only a lot simpler to implement but also more predictable/less disruptive

Based on these observations we decided to mostly follow amps implementation. This PR also has a more correct implementation fixing issues I found with the original PR (labels would appear on single char words, punction and would include leading whitespace, ..).

This PR contains a cherry pick from #6417, depending on which PR is merged first I will rebase the other PR and drop that commit

pascalkuthe avatar Nov 21 '23 01:11 pascalkuthe

I was testing it and I didn't see any issue so far on the current implementation. Nice work.

I have a question, would it be possible to work in all visible buffers? So we could go to a word in another split window?

danillos avatar Nov 21 '23 20:11 danillos

I have been testing it and it works brilliantly, thanks!

David-Else avatar Nov 21 '23 20:11 David-Else

Should the default value for jump-label-alphabet be reordered to prefer QWERTY home-row characters first?

Omnikar avatar Nov 22 '23 00:11 Omnikar

Should the default value for jump-label-alphabet be reordered to prefer QWERTY home-row characters first?

We generally don't try to tailor our keymap to qwerty (many people use different keymaps). Using the alphabet is a pretty reasonal default IMO.

I have set the setting to my (colmak) homerow plus some keys that are easily reachabke. What exactly those are is highly individual.

pascalkuthe avatar Nov 22 '23 14:11 pascalkuthe

Should the default value for jump-label-alphabet be reordered to prefer QWERTY home-row characters first?

We generally don't try to tailor our keymap to qwerty (many people use different keymaps). Using the alphabet is a pretty reasonal default IMO.

I have set the setting to my (colmak) homerow plus some keys that are easily reachabke. What exactly those are is highly individual.

Alphabet is certainly a good default, but having a config option to prefer the home row keys of QUERTY / Colmeak / Dvorak would be next level awesome :)

I have set the setting to my (colmak) homerow plus some keys

How is this possible? Cheers!

David-Else avatar Nov 22 '23 14:11 David-Else

just add this to your config (under editor, its also documented in this PR):

jump-label-alphabet = "eirsaotndhfuywgm(cplbjvkx)z_"

That example is for my personal layout (colmak) where brackets and underscore are on the base layer. Basically the letters that come first are used first so e, i r and s endup close to cursor (and for me used most often). You may also completely exclude some hard to reach letters (like q in my case)

pascalkuthe avatar Nov 22 '23 14:11 pascalkuthe

@pascalkuthe You are amazing, you thought of everything. Sorry I didn't see that in the PR and wasted your time.

David-Else avatar Nov 22 '23 14:11 David-Else

Should the default value for jump-label-alphabet be reordered to prefer QWERTY home-row characters first?

We generally don't try to tailor our keymap to qwerty (many people use different keymaps).

Doesn't having hjkl as default directional controls already do that?

Omnikar avatar Nov 23 '23 03:11 Omnikar

hjkl is a historical artifact, not intentional bias towards qwerty

the-mikedavis avatar Nov 23 '23 08:11 the-mikedavis

Would it be too difficult to add in the character search mode as well? One case where the word base jumps can still be a bit difficult to use is when you want to jump to the beginning of a portion of code that has a lot of symbols, such as jumping to the beginning of the array at ["--lsp"] (which gets changed to ["--cnp"] in the screenshot):

image

Or jumping to the open bracket or generics of a function:

image

~~It also can't jump to single characters, but I'm not sure that would actually be an issue in practice:~~ (edit: this was explicitly mentioned as intended behavior)

Another nice to have would be jumping into the middle of identifiers, such as words separated by underscores like in these functions: image but there doesn't seem to be an easy way to implement this.

Or maybe character search should be another PR to keep this one less cluttered.

qwerty01 avatar Nov 25 '23 01:11 qwerty01

@qwerty01 I think the recommendation for that is to just use search (/ and ?)

Omnikar avatar Nov 25 '23 02:11 Omnikar

/ and f would work well for spots that are easily searchable, but braces and other non-word characters are usually not very searchable since they're 1 character and would have lots of matches. With the current solution, it would probably be finding some other spot on the line and then doing some other key combinations to get to the spot you want to, but that kinda defeats the purpose of amp jumps, which is looking at the spot you want to go and typing the keys that pop up.

qwerty01 avatar Nov 25 '23 04:11 qwerty01

Well you could also type characters before and/or after the brace you're trying to jump to when using /.

Omnikar avatar Nov 25 '23 04:11 Omnikar

Oh quick aside, found an issue when scrolling while jump mode is active, only the visible screen at the time of enabling jump mode gives jump options, so if you scroll while jump mode is active, there aren't more options to jump:

scrolling

Also probably a use case that won't come up often but figured I'd mention it.

EDIT: The limits are explicitly set on line 5882 and 5896, removing them doesn't cause any noticeable performance issues, but still leaves large files mostly unmarked since we run out of jump labels.

EDIT2: Mistyping a jump code can lead to jumping off the screen if it corresponds to a location that was marked off the screen, so it might be best if the current functionality is kept, or maybe exiting jump mode on scroll would be better.

qwerty01 avatar Nov 25 '23 05:11 qwerty01

Yeah, I think we should clear the jump labels / exit jumping mode when the viewport changes (scrolling, resizing, etc.). It's only meant to jump to locations that are visible/in-view.

I have a question, would it be possible to work in all visible buffers? So we could go to a word in another split window?

Currently it's a regular motion so like w/b/e or text-objects it only works on the current view (window). IMO jumping across views is not necessary to optimize for - I think switching views and then jumping within the view would work well enough.

the-mikedavis avatar Nov 25 '23 10:11 the-mikedavis

Yeah, I think we should clear the jump labels / exit jumping mode when the viewport changes (scrolling, resizing, etc.). It's only meant to jump to locations that are visible/in-view.

I have a question, would it be possible to work in all visible buffers? So we could go to a word in another split window?

Currently it's a regular motion so like w/b/e or text-objects it only works on the current view (window). IMO jumping across views is not necessary to optimize for - I think switching views and then jumping within the view would work well enough.

yeah I fixed this by simply dismissing any pending (or pseudo pending) keys whenever a mouse button is pressed (also during bracketed paste). I think mouse buttons behaving the same as other keys in that regard just makes sense. I think this will fix quite a few similar bugs.

pascalkuthe avatar Nov 30 '23 23:11 pascalkuthe

We generally don't try to tailor our keymap to qwerty (many people use different keymaps).

I don't have strong opinions here, but I wouldn't mind ordering the default to use the qwerty homerow. I also don't use qwerty but the majority of the users do and it would make the default config slightly more efficient for them (the rest of us will remap anyway).

archseer avatar Dec 05 '23 01:12 archseer

I think a feature worth considering is jumping while adding the target word as a new selection, similarly to what n in select mode does.

postsolar avatar Dec 07 '23 12:12 postsolar

We might want to consider that as a larger effort of https://github.com/helix-editor/helix/issues/5672. It might be useful to add selections rather than extend in some other cases like textobjects and tree-sitter motions. We could have a separate mode for it so that select mode always extends and the new mode always adds.

In any case though that should be left for future work rather than added to this PR

the-mikedavis avatar Dec 07 '23 14:12 the-mikedavis

This seems to implement a "hop.nvim" jump mechanism (2 random chars). Will this also include a "leap.nvim"/ "lightspeed.nvim" jump mechanism (2 predictable chars + eventual 3rd)? Gauging from github and forums such as reddit, the "leap" strategy seems more popular and is implemented in the previous PR #5340 . If this is intended to be part of the core, I would suggest to also include the seemingly more popular approach.

  • Update: If the cited statement below of "character based jumps" refers to the "leap" strategy, I would disagree that f or / solve the problem. They always involve more keystrokes, since with f one needs to place the cursor in the correct line beforehand, using the motion keys, and / requires you to press n over and over again.

character based jumps (which require a character before tying a label) don't really hold their water, you can use f for simple cases and / (which we want to improve further) for complex cases. Compared to a jumpmode these commands work with multicursors.

brunoherrick avatar Dec 14 '23 17:12 brunoherrick

Could you elaborate on the behavior you're looking for? I'm not sure anything like that is implemented in #5340. The character based jumps like with g/ from #5340 I think would be ok to add as a follow-up to this PR as long as it only operates on the current view and re-uses most of the code introduced here.

the-mikedavis avatar Dec 15 '23 00:12 the-mikedavis

@the-mikedavis exactly, the behavior you referenced via g/ from the previous PR #5340 is the one I tried to describe. In particular, after compiling that PR branch, one needs to put the following in the config.toml:

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

The advantage of this approach is that it is more natural, since before jumping you already know the chars you want to jump to, instead of typing 2 random chars. The disadvantage is that it may require an additional 3rd char for disambiguation. Hopefully it is possible to reuse the code from this PR and implement both types of jumps, as these two seem to be the most popular approaches for blazing fast movement.

  • Note: Regarding the popularity, besides the Github stars for each project and posts on reddit comparing both of the approaches, we also have this small poll in reddit. The family of jump techniques leap + lightspeed + sneak are slightly more popular compared to the family easymotion + hop.

https://reddit.com/r/neovim/comments/x9lsdh/which_jump_plugin_are_you_using_now/ https://www.reddit.com/poll/x9lsdh

  1. Sneak, Lightspeed, Leap: ( 9.2% + 22.3% + 15.9% = 47.4% )
  2. EasyMotion, Hop: ( 12.3% + 34.4% = 46.7% )

There is also a 3rd method which, although less known, is praised in a few comments. It is "pounce.nvim" and is similar to "leap"/ "lightspeed" since you also type the chars of the word you are currently looking at, but it is fuzzy meaning it can be a little more flexible. I would argue this one is a bit niche already, albeit interesting.

brunoherrick avatar Dec 15 '23 03:12 brunoherrick

Re: how it acts with regards to selections. An interesting implementation has been recently tried out in https://github.com/phaazon/hop.kak. What I found really elegant and interesting is that it's extensible as in it only acts as a "selections filter" so by using selections registers the user is free to build their own command, be it one adding a new cursor, or extending current selection, or something else. What exactly gets selected (e.g. words, or lines, or whatever else) is also easily controllable by the user. I think something like this would be awesome to have in Helix

postsolar avatar Dec 15 '23 13:12 postsolar

Ah right, I had forgotten that the number of characters to search is configurable in #5340. That reminds me why I'm not enthusiastic about adding character based jumping or jump features in general to core.

What I don't like about #5340 is the size of the feature: it's adds a lot of code (four new modules, state-machine-like code, cosmetics like dimming), a new configuration section under editor.jump-mode, and basically adds a new "mode" to the editor (where this PR uses established machinery: just cx.on_next_key). I'm hesitant to add jumping to core at all because there are so many methods of doing it: there are many existing plugins and each plugin has a opinions on how to jump. I'm imagining us ending up with a lot of code in core covering multiple methods of jumping and/or behavior configuration like number of characters because we're trying to satisfy all the ways people think a jump feature should work.

So if we add jumping at all I'd like to keep it very simple like this Amp-like jump and leave everything else to plugins.

the-mikedavis avatar Dec 15 '23 15:12 the-mikedavis

Tested it and it works beautifully. Do you, guys, have a document/wiki that describes what a proper reviewer should do and how to become one?

yerlaser avatar Jan 28 '24 21:01 yerlaser

You don't need any permissions to review PRs, feel free to leave review comments on open PRs. We don't have an exact procedure for becoming a maintainer / gaining triage or write priviledges if that's what you mean. This PR isn't really waiting on review though: the changes look good but it'd be nice to get some input from @semin-park (see https://github.com/helix-editor/helix/pull/8875#pullrequestreview-1743457571). He's been inactive for quite some time though so I think it would be ok to go ahead with this (\cc @archseer)

the-mikedavis avatar Jan 30 '24 16:01 the-mikedavis

For what it's worth: I've been using this for a few months and have not encountered any issues with it.

dvic avatar Feb 05 '24 14:02 dvic

One minor bug I encountered: the last line in the buffer has an invalid number of anchors unless it's new-line-terminated:

rust, Ok is not reachable if there's no new line (not formatted correctly just for the demo purpose):

toml, "all" is not reachable if there's no new line:

[editor.whitespace]
render = "all"

js, const, foo, and bar aren't reachable:

image image
export const foo = (bar) => 42;

kdziamura avatar Feb 08 '24 11:02 kdziamura

I can't reproduce that

pascalkuthe avatar Feb 08 '24 12:02 pascalkuthe

Tried it myself and couldn't reproduce it either.

yerlaser avatar Feb 08 '24 14:02 yerlaser