helix icon indicating copy to clipboard operation
helix copied to clipboard

Add find in next command for character pairs

Open thomasschafer opened this issue 1 year ago • 5 comments

Adds the ability to match inside/around the next matching character pair: for instance, with the cursor as below:

█ let x = [bar];

typing min[ would select the contents of the array (i.e. bar).

https://github.com/helix-editor/helix/assets/54135831/420993e0-08d5-4394-be68-068ed6b57a8f

I have only implemented matching in next matching characters (and not the remainder of matching options e.g. word, function etc.) as I wanted to confirm that my approach was okay - once there is agreement on an approach I can implement the remainder, either in this PR or separate PRs.

Partially resolves https://github.com/helix-editor/helix/discussions/10567

thomasschafer avatar Jul 10 '24 19:07 thomasschafer

See also https://matrix.to/#/!zMuVRxoqjyxyjSEBXc:matrix.org/$uaEuLHtpGnKb5lx33HLwLW1q7WPSzKvI4zXEo0BBQWU?via=matrix.org&via=tchncs.de&via=mozilla.org

kirawi avatar Jul 11 '24 01:07 kirawi

I think it would be useful to have a way to select the next pairing of a certain kind (or of any kind), but I believe this should belong under ]/[ instead and behave like ]f/[f for example. Putting this under m makes it complicated code-wise and adds more depth to the m menu which is already fairly complicated. Plus if you use n for "next" in that menu you can't use p since it's taken by paragraphs.

To start with it should be a pretty small and self-contained change to add ]m/[m commands that select the next/prev nearest pairing. I'm not totally sure about m for that keybinding but it's similar mm. We would also want to rename ]e/[e to next/prev "entry" rather than "pairing." Being able to select any pair like you can with mi<pair> will take larger changes - I believe we want to make some changes to the keymap structs so that we don't need to make ]/[ single commands the way mi and ma are.

the-mikedavis avatar Jul 11 '24 13:07 the-mikedavis

I think it would be useful to have a way to select the next pairing of a certain kind (or of any kind), but I believe this should belong under ]/[ instead and behave like ]f/[f for example. Putting this under m makes it complicated code-wise and adds more depth to the m menu which is already fairly complicated. Plus if you use n for "next" in that menu you can't use p since it's taken by paragraphs.

To start with it should be a pretty small and self-contained change to add ]m/[m commands that select the next/prev nearest pairing. I'm not totally sure about m for that keybinding but it's similar mm. We would also want to rename ]e/[e to next/prev "entry" rather than "pairing." Being able to select any pair like you can with mi<pair> will take larger changes - I believe we want to make some changes to the keymap structs so that we don't need to make ]/[ single commands the way mi and ma are.

The first part makes sense, but I'm just wondering regarding the second bit - how would the [/] prefix work with selecting a specific character, e.g. selecting in the next {} pair? Something like ]i{? I know you said it would require a larger refactor so we will want to implement this separately, but I'm curious about what the end result would look like.

thomasschafer avatar Jul 11 '24 19:07 thomasschafer

I'm also thinking that we would drop the i/a part and use "around" like the other goto_next_<textobject> commands, so it would be ]{ for example rather than ]i{. The jump gets you to the textobject and then you can use mi/ma to refine the selection.

Currently there isn't a way to write a command that accepts any character. select_textobject uses an on-next-key callback to decide which textobject it's going to act on. It's a little hacky though and the downside is that you must hardcode the keybindings within, like the w in miw, and you can't bind a key to do miw directly - you would have to use a macro (https://github.com/helix-editor/helix/issues/1595).

One solution would be to refactor all of the goto_next_<textobject>/goto_prev_<textobject> commands to be one command that uses an on-next-key callback. I'd really like to avoid that though so it doesn't inherit all the problems mi and ma have. I drafted out some changes for formalizing "fallback" commands that can accept a character and refactoring mi/ma menus to be regular commands here: https://github.com/the-mikedavis/helix/compare/425fee957c566ea425983b6786246942905abf3d...bf9dd58fc4bb49c78378a8a6bc276aeb80ca88b8

the-mikedavis avatar Jul 11 '24 19:07 the-mikedavis

I'm also thinking that we would drop the i/a part and use "around" like the other goto_next_<textobject> commands, so it would be ]{ for example rather than ]i{. The jump gets you to the textobject and then you can use mi/ma to refine the selection.

Not sure how ergonomic this is - for instance, if you wanted to select inside the next | pair, then ]|mi| wouldn't work as (at least currently) mi| doesn't work when the cursor is on a |. Even selecting inside a pair where this would work, e.g. {/}, would require something like ]{mi{, which feels a little clunky, not only because of the number of keypresses but also the repetition of the desired character ({ in this case) - you're no better off in this case than just using f{mi{ which is already possible. For this reason, I wonder if it's better to allow both ]{ and ]i{, where the former selects around (as you mentioned) but the latter selects inside?

Currently there isn't a way to write a command that accepts any character. select_textobject uses an on-next-key callback to decide which textobject it's going to act on. It's a little hacky though and the downside is that you must hardcode the keybindings within, like the w in miw, and you can't bind a key to do miw directly - you would have to use a macro (#1595).

One solution would be to refactor all of the goto_next_<textobject>/goto_prev_<textobject> commands to be one command that uses an on-next-key callback. I'd really like to avoid that though so it doesn't inherit all the problems mi and ma have. I drafted out some changes for formalizing "fallback" commands that can accept a character and refactoring mi/ma menus to be regular commands here: the-mikedavis/[email protected]

Useful to know, thank you for sharing!

thomasschafer avatar Jul 11 '24 21:07 thomasschafer

@the-mikedavis sorry to ping on this, just wondering if you had thoughts on my question above about the addition of ]i| alongside ]|?

thomasschafer avatar Jul 17 '24 18:07 thomasschafer

Hi there, i also wanted to bring up the solution someone pointed out on reddit: https://www.reddit.com/r/HelixEditor/comments/1e5ip31/comment/ldmb6cl/?utm_source=share&utm_medium=web3x&utm_name=web3xcss&utm_term=1&utm_content=share_button

To me it feels like the perfect addition to the current behaivor

  1. Currently using mi( does nothing if its not inside of a parentheses pair → why not using it to just jump to the next found pair
  2. This would solve the ] topic where the problems with inside/outside and any character would have to be solved

Im just a newbie, part-time nvim user and hyped helix user but to my limited understanding this would be a nice feature

EDIT:

AND i forgot to shout out a HUGE HUGE THANK YOU to the people taking the time and creating PRs

SHU-red avatar Jul 17 '24 19:07 SHU-red

Hi there, i also wanted to bring up the solution someone pointed out on reddit: https://www.reddit.com/r/HelixEditor/comments/1e5ip31/comment/ldmb6cl/?utm_source=share&utm_medium=web3x&utm_name=web3xcss&utm_term=1&utm_content=share_button

To me it feels like the perfect addition to the current behaivor

  1. Currently using mi( does nothing if its not inside of a parentheses pair → why not using it to just jump to the next found pair
  2. This would solve the ] topic where the problems with inside/outside and any character would have to be solved

I think that automatically jumping if not already inside matching characters is confusing, as you might not think that you're inside a matching pair and so attempt to jump ahead, but actually you are and so select a wider region than expected. Being explicit should require no extra keypresses (]i{ vs mi{ based on the discussion above), but has no risk of confusion because of the explicit choice.

thomasschafer avatar Jul 17 '24 19:07 thomasschafer

Being explicit should require no extra keypresses (]i{ vs mi{ based on the discussion above), but has no risk of confusion because of the explicit choice.

Nothing to complain

SHU-red avatar Jul 17 '24 19:07 SHU-red

I agree, nuanced behavior like that can be confusing especially when you're working with multiple selections.

I'd like to avoid the i if we can to avoid making ] too nested. Textobjects in ]/[ typically use "around" but there's also "movement" which is sometimes captured as the same thing as "inside" if "around" doesn't make sense for a textobject in ]/[. For example some languages use it for arguments/parameters so that ]a doesn't include the ,. We could reason that the "movement" for pair characters is the same as "inside". Then if you want to go around you can use mam after moving.

So ]m (if we go with m for the "any pair" keybinding) would select inside the next paren/bracket/etc. pair.

the-mikedavis avatar Jul 17 '24 19:07 the-mikedavis

I agree, nuanced behavior like that can be confusing especially when you're working with multiple selections.

I'd like to avoid the i if we can to avoid making ] too nested. Textobjects in ]/[ typically use "around" but there's also "movement" which is sometimes captured as the same thing as "inside" if "around" doesn't make sense for a textobject in ]/[. For example some languages use it for arguments/parameters so that ]a doesn't include the ,. We could reason that the "movement" for pair characters is the same as "inside". Then if you want to go around you can use mam after moving.

So ]m (if we go with m for the "any pair" keybinding) would select inside the next paren/bracket/etc. pair.

Sounds good, thanks for clarifying!

thomasschafer avatar Jul 18 '24 13:07 thomasschafer

I will work on a new PR based on the implementation discussed here

thomasschafer avatar Jul 18 '24 13:07 thomasschafer

I will work on a new PR based on the implementation discussed here

Comment the new PR here when you have it ;)

adriangalilea avatar Jul 18 '24 16:07 adriangalilea

New PR here: https://github.com/helix-editor/helix/pull/11260

thomasschafer avatar Jul 21 '24 21:07 thomasschafer