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 commands for movement by subwords #8147

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

JeftavanderHorst
Copy link
Contributor

@JeftavanderHorst JeftavanderHorst commented Sep 2, 2023

This PR adds commands that enable movement within subwords that are snake_cased or camelCased, as previously proposed in discussion #5463. Subwords are delimited by underscores and by transitions from lowercase to uppercase.

The commands are named {move,extend}_{next,prev}_sub_word_{start,end}. This PR doesn't add any keybinds.

Example usage (| characters indicate cursor):

  • |f|oo bar baz -> move next sub word -> |foo |bar baz
  • |f|oo_bar_baz -> move next sub word -> |foo_|bar_baz
  • |f|ooBarBaz -> move next sub word -> |foo|BarBaz

Example keybinds:

[keys.normal]
W = 'move_next_sub_word_start'
B = 'move_prev_sub_word_start'
E = 'move_next_sub_word_end'

[keys.select]
W = 'extend_next_sub_word_start'
B = 'extend_prev_sub_word_start'
E = 'extend_next_sub_word_end'

@JeftavanderHorst
Copy link
Contributor Author

I notice many PRs have been marked with S-waiting-on-review but this one hasn't, is there anything I need to do to request a review?

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. A-command Area: Commands labels Feb 10, 2024
@shaleh
Copy link
Contributor

shaleh commented Apr 2, 2024

Can confirm this works as advertised. Thanks, I was literally making the same change and decided to look for a PR first.

@shaleh
Copy link
Contributor

shaleh commented Apr 2, 2024

My only concern is the widespread use of "char_is_word". I wonder in what other ways the definition of "word" is not matching "subword". Many of the places there is little context, just a solitary character.

Initially I thought Grapheme::is_word_boundary might be a good avenue but that is only used in doc formatting for some reason....

@JeftavanderHorst
Copy link
Contributor Author

It's been a while since I implemented this, so I can't remember whether I looked at char_is_word. There is at least one other definition of "word" that I didn't update, which is in text objects. Meaning miW is still Match inner WORD, instead of Match inner sub word. Sub word text objects are implemented in #8658 though, if you need them.

That being said, I've been using this branch in my daily driver since I created it, and so far everything works as expected, so I don't think char_is_word is too important. If the core devs are interested I can work on this as well, but for now, I tried to keep the diff as small as possible (except for the tests) so that I can easily keep up with master.

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

sorry it took a while to get to this. I think this is a pretty good implementation and I think this feature does carry its weight. I don't think we need to use subword in other places, char_is_word is to differentiate whitespace and special chars and usually have nothing to do with word segmentation.

@JeftavanderHorst
Copy link
Contributor Author

Friendly ping to @the-mikedavis to request a review, since this PR already got one approval and is relatively small (ignoring the tests)

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

I agree, this looks good! I've been thinking about how we should address this use-case for a while and I think this is the best option: adding commands lets you opt into the "subword" behavior and you can use the regular word behavior even at the same time as you show with the example keybindings. (As opposed to adding a config option that affects what we consider to be a word - that would be 'all or nothing')

@the-mikedavis the-mikedavis changed the title Movement by subwords Add commands for movement by subwords Aug 8, 2024
@the-mikedavis the-mikedavis merged commit 518425e into helix-editor:master Aug 8, 2024
6 checks passed
mxxntype pushed a commit to mxxntype/helix that referenced this pull request Aug 14, 2024
* Allow moving by subword
* Add tests for subword movement
GNUSheep pushed a commit to GNUSheep/helix that referenced this pull request Aug 19, 2024
* Allow moving by subword
* Add tests for subword movement
@ntzm
Copy link

ntzm commented Sep 2, 2024

What keybinds are people using with this feature? I really like it but I don't want to replace w, b, e, W, B, E with them

@sibouras
Copy link

sibouras commented Sep 2, 2024

probably prefix it with a key i don't use like "\\" = { w = "move_next_sub_word_start" }

@baarkerlounger
Copy link

Is there any intention to add default keybinds for this?

@the-mikedavis
Copy link
Member

No I don't see a reason to add default keybindings. The keymap is already pretty crowded

@baarkerlounger
Copy link

@the-mikedavis the benefits for me would be:

  1. Discoverability - it's harder to know this functionality exists without stumbling across this PR if it's not in the keymap
  2. Less configuration required - it would work out of the box, and remove the risk that the keys you choose to map it to are later used for something else in the default config.

I suppose it depends if you expect this to be something everybody uses or not

kyruzic pushed a commit to kyruzic/helix that referenced this pull request Sep 27, 2024
* Allow moving by subword
* Add tests for subword movement
@baldwindavid
Copy link

@ntzm - I've been prefixing with - and thinking of it as signifying the subtracted part of a word. In addition to the movement commands this minus mode allows for selecting the subword under cursor with another minus. Only issue is that - - at the start of a subword will select the subword before it, but easy enough to just use - e in that scenario. Would be nice if these could work like text objects and in match mode (i.e. match in subword), but this is good enough.

[keys.normal.minus]
"minus" = {label = "Select sub word", command = ["move_prev_sub_word_start", "move_next_sub_word_end"]}
"w" = "move_next_sub_word_start"
"b" = "move_prev_sub_word_start"
"e" = "move_next_sub_word_end"

[keys.select.minus]
"minus" = {label = "Select sub word", command = ["extend_prev_sub_word_start", "extend_next_sub_word_end"]}
"w" = "extend_next_sub_word_start"
"b" = "extend_prev_sub_word_start"
"e" = "extend_next_sub_word_end"

plul pushed a commit to plul/helix that referenced this pull request Oct 13, 2024
* Allow moving by subword
* Add tests for subword movement
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-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants