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

Expand selection to whole word when pressing * #6046

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

magopian
Copy link
Contributor

@magopian magopian commented Feb 18, 2023

Closes #6045

This is an attempt at adding the functionality discussed in this issue (and earlier on matrix).

I'm open to suggestions, I've barely learned Rust, and it was a long time ago and seldom used it, so i'm very... rusty.

Please let me know if this is useful, relevant, and how it could be improved!

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.

I like this change and the implementation looks reasonable to me too as its just lifted from goto_file 👍

Searching for a single char with * seems like a nieche use-case: *n it not much shorter than /[char]<ret> or f[char].
Automatically using the word under the cursor is consistent with gf and can be super useful.

However this behavior change should be documented in keymap.md .
Currently it says:

Use current selection as the search pattern

Wit this PR something like the following would help:

Use current selection as the search pattern, if only a single char is selected the current word (miW) is used instead

@pascalkuthe pascalkuthe added E-easy Call for participation: Experience needed to fix: Easy / not much C-enhancement Category: Improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 18, 2023
@magopian
Copy link
Contributor Author

Thanks for the feedback, I've updated the keymap.md file accordingly 👍

@magopian magopian requested a review from pascalkuthe February 18, 2023 18:42
pascalkuthe
pascalkuthe previously approved these changes Feb 18, 2023
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.

LGTM now 👍

@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 Feb 18, 2023
@firstrow
Copy link

nice improvement. going to test it locally. currently was using this config:

"*" = ["move_prev_word_start", "move_next_word_end", "search_selection", "global_search"]

@ubitux
Copy link

ubitux commented Apr 18, 2023

I think it makes sense to encapsulate the word between \b to prevent overmatching

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 was initially opposed to this behavior (along these lines: #6045 (comment)) but with the special registers from #6985 you can search for the current selection exactly with /<C-r>.. So I think this would be a nice improvement. I left a few small comments and pushed up a merge with master since there were some changes from the special registers PR that conflicted

primary,
textobject::TextObject::Inside,
count,
false,
Copy link
Member

Choose a reason for hiding this comment

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

Passing false here makes this look for the surrounding "word" rather than the "WORD". I think that's an ok behavior but the change in book/src/keymap.md should mention miw instead of miW. Also see

'w' => textobject::textobject_word(text, range, objtype, count, false),
'W' => textobject::textobject_word(text, range, objtype, count, true),

Some("ifoobar::baz<ret>bar::crux<esc>k3blC*"), // k3b places the cursor on the first letter of 'foobar', then move one to the right for good measure, then adds a cursor on the line below
Some(&|app| {
assert!(
// The selections don't seem to be ordered, so we have to test for the two possible orders.
Copy link
Member

Choose a reason for hiding this comment

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

I believe what's happening here is that collecting into a hashmap might change the order. The selections do have a deterministic order

Copy link
Contributor

Choose a reason for hiding this comment

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

The source code could add the itertools crate and then use unique on the iterator instead of collecting twice. unique is stable.

@the-mikedavis
Copy link
Member

the-mikedavis commented Feb 10, 2024

I believe Kakoune also adds \b in some cases with *. I think it would be a nice improvement to move our * closer to Kakoune's behavior - though that should be a follow-up PR

@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-command Area: Commands and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 10, 2024
@NikitaRevenco
Copy link
Contributor

If there's anything that needs to be done to merge this, I'm happy to help!

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-easy Call for participation: Experience needed to fix: Easy / not much 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.

Expand to word boundary if we are at a single char selection when pressing *
7 participants