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

Add a simple amp-like jump command #8875

Merged
merged 4 commits into from Mar 23, 2024
Merged

Conversation

pascalkuthe
Copy link
Member

@pascalkuthe pascalkuthe commented Nov 21, 2023

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 pascalkuthe added C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer. A-command Area: Commands labels Nov 21, 2023
@pascalkuthe pascalkuthe force-pushed the amp-jump branch 2 times, most recently from 88c2975 to 1e48615 Compare November 21, 2023 01:28
book/src/keymap.md Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@danillos
Copy link
Contributor

danillos commented Nov 21, 2023

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?

@David-Else
Copy link
Contributor

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

@Omnikar
Copy link
Contributor

Omnikar commented Nov 22, 2023

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

archseer
archseer previously approved these changes Nov 22, 2023
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me but I'm not a jump mode user. I'd be interested in @semin-park's feedback as the original PR owner

@pascalkuthe
Copy link
Member Author

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.

@David-Else
Copy link
Contributor

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!

@pascalkuthe
Copy link
Member Author

pascalkuthe commented Nov 22, 2023

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)

@David-Else
Copy link
Contributor

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

@Omnikar
Copy link
Contributor

Omnikar commented Nov 23, 2023

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?

@the-mikedavis
Copy link
Member

hjkl is a historical artifact, not intentional bias towards qwerty

@qwerty01
Copy link

qwerty01 commented Nov 25, 2023

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.

@Omnikar
Copy link
Contributor

Omnikar commented Nov 25, 2023

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

@qwerty01
Copy link

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

@Omnikar
Copy link
Contributor

Omnikar commented Nov 25, 2023

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

@qwerty01
Copy link

qwerty01 commented Nov 25, 2023

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.

@the-mikedavis
Copy link
Member

the-mikedavis commented Nov 25, 2023

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.

@pascalkuthe
Copy link
Member Author

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.

@the-mikedavis the-mikedavis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 10, 2024
@wontem
Copy link

wontem commented Feb 14, 2024

Another quick question, is it necessary to have an anchor in the current cursor position (if you're already in the beginning of a word)?

Copy link

@James-Firth James-Firth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs are clear and the feature works great! (Just did my first build to test this out) :shipit:

EDIT: After using it for a bit I get why it's like that, this is better than what I expected! And thanks to trying to respond to a reply I've also answered my own concern about the colour support.

Is it intended to put the cursor at the end of the word you jump to and select the whole word? If so, working great!

Jumping in only after skimming (and not being familiar with amp) that's not what I expected (coming from EasyMotion in VSCode with vim bindings). I could see why you'd often want to act on the word you just jumped to so I can see the logic behind that behaviour!

Just have one comment/question about themes below

runtime/themes/everforest_dark.toml Outdated Show resolved Hide resolved
@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 8, 2024
@postsolar
Copy link
Contributor

postsolar commented Mar 9, 2024

I noticed that, unlike search_next and similar commands, goto_word does not respect selection orientation.

@pascalkuthe
Copy link
Member Author

It does in visual Mode just not on normal mode. It's intentional.

@postsolar
Copy link
Contributor

Got it, but what is the reasoning for this?

@the-mikedavis
Copy link
Member

See my comment above: #8875 (comment)

It matches the LSP location jumping features

@the-mikedavis the-mikedavis added this to the 24.03 milestone Mar 18, 2024
helix-core/src/graphemes.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@archseer archseer merged commit b46064b into helix-editor:master Mar 23, 2024
6 checks passed
@pascalkuthe pascalkuthe deleted the amp-jump branch March 23, 2024 11:47
@lucmos
Copy link

lucmos commented Mar 27, 2024

Thank you for this feature! It's working great 😄

Just a quick question -- Is it possible to "add" a selection to the word I am jumping to? Without extending the current selection as in visual mode, i.e., an easy way to add multiple cursors.

@the-mikedavis
Copy link
Member

There's some discussion about that above: #8875 (comment)

@giann
Copy link

giann commented Apr 3, 2024

Great feature however, the jump point have the same color as the word they're in for me:
Screenshot 2024-04-03 at 10 13 48
How can I fix this?

@pblkt
Copy link

pblkt commented Apr 3, 2024

It's likely that your theme doesn't define the ui.virtual.jump-label or the fallback ui.virtual.

To verify that this is indeed the issue, check that the labels in :theme default are highlighted as expected .

To fix, make sure your theme defines it by either creating and using a local one or opening a PR.

@giann
Copy link

giann commented Apr 3, 2024

That was it thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AMP like Jump Mode Better f à la clever-f and vim-sneak like jump motion