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 find in next command for character pairs #11124

Conversation

thomasschafer
Copy link
Contributor

@thomasschafer thomasschafer commented Jul 10, 2024

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

Screen.Recording.2024-07-10.at.19.46.23.mov

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 #10567

@kirawi kirawi added the A-command Area: Commands label Jul 10, 2024
archseer
archseer previously approved these changes Jul 11, 2024
@archseer archseer dismissed their stale review July 11, 2024 00:06

Need to do a full pass

@kirawi
Copy link
Member

kirawi commented Jul 11, 2024

@the-mikedavis
Copy link
Member

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.

@thomasschafer
Copy link
Contributor Author

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.

@the-mikedavis
Copy link
Member

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 (#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/helix@425fee9...bf9dd58

@thomasschafer
Copy link
Contributor Author

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!

@kirawi kirawi added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 17, 2024
@thomasschafer
Copy link
Contributor Author

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

@SHU-red
Copy link

SHU-red commented Jul 17, 2024

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

@thomasschafer
Copy link
Contributor Author

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.

@SHU-red
Copy link

SHU-red commented Jul 17, 2024

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

@the-mikedavis
Copy link
Member

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.

@thomasschafer
Copy link
Contributor Author

thomasschafer commented Jul 18, 2024

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

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

@adriangalilea
Copy link

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

Comment the new PR here when you have it ;)

@thomasschafer
Copy link
Contributor Author

New PR here: #11260

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants