Add ability to match in next and previous char pairs (2) #11695
Conversation
|
Note: the first commit is essentially just the changes linked in the comment here. I left the two |
the-mikedavis
added
A-keymap
|
Bumping this 🙏 |
|
If I jump to a function, would this patch still support jumping to next and previous function? |
Yes, that still works as it does currently on master |
|
@the-mikedavis apologies for the ping, wondering if you'd mind taking a look at this? 🙏 |
|
I do want to take a look at this soon but I'm about to be away for a while. I have a note written down to give this a look once I'm back in a 2ish weeks. (I'm in favor of the feature, I just haven't looked at the changes yet) |
helix-core/src/surround.rs
Outdated
| @@ -172,6 +178,17 @@ pub fn find_nth_pairs_pos( | |||
|
|
|||
| let (open, close) = get_pair(ch); | |||
| let pos = range.cursor(text); | |||
| let (pos, n) = match find_type { | |||
| FindType::Surround(n) => (pos, n), | |||
| FindType::Next(n) => match search::find_nth_next(text, open, pos, n) { | |||
There was a problem hiding this comment.
Using plaintext search routines isn't right here. This should be based on tree-sitter. The plaintext search gets thrown off by string literals.
In general I think reusing the surround function won't work too well here. The surround function mostly reuses other functions (match_bracket:: and search::) to find pairs and only adds logical necessary for surround. For forwards/reverse search you are looking for a pair fist and then a single (simpler) call to match_bracket/search would suffice.
I would probably be ok with not implementing the tree-sitter based forward/reverse search for now but I would definitly want to see this as a seperate function (likely directly within textobject.rs).
There was a problem hiding this comment.
Sorry not sure I'm fully following - mi{ doesn't use treesitter as far as I can tell, so would we want ]{ to behave any differently?
Also, I decided to go with the option of finding the first char within a pair and then using textobject_pair_surround as this allows selection of ranges between characters where the direction is not clear from the bracket e.g. between | characters. ]| works fine to jump between pairs of | in something like foo|bar|baz|, but that wouldn't work if first searching for the | and then calling one of the match_brackets functions. I'm sure I could overcome this by making a bunch of changes in match_brackets but I thought that would add a lot of code - do you think that option would be preferable, or is there a different approach I'm missing?
kirawi
added
the
S-waiting-on-author
thomasschafer
force-pushed
the
tschafer-add-go-to-next-inner-matching-chars-2
branch
from
97910ca to
66f0f38
Compare
|
Bump on this? 🙏 |
|
this is going to be a big quality of life improvement, appreciate it! |
|
can't wait for this ngl |
@the-mikedavis sorry for another ping - will you have a chance to look at this PR? 🙏 |
|
At first I was a bit confused as to why this feature wasn't bound to the |
| @@ -154,14 +154,20 @@ fn find_nth_closest_pairs_plain( | |||
| Err(Error::PairNotFound) | |||
| } | |||
|
|
|||
| pub enum FindType { | |||
There was a problem hiding this comment.
It looks like each of these enum variants are storing the count, right? Let's separate the enum from the count and pass that as a separate parameter even if it means adding #[allow(clippy::too_many_arguments)] for some functions
There was a problem hiding this comment.
Sounds good, done
helix-term/src/keymap.rs
Outdated
There was a problem hiding this comment.
The fallback commands change will be a regression for the ability to remap mi/ma - currently you can remap those but with the fallback commands change you would be unable to remap the fallback behavior until we expose it in keymap configuration (i.e. in TOML). We should expose more of the keymap type anyways and allow remapping the names of minor modes and whether or not they are sticky. I'm not sure it's a straightforward change though since we want to revisit and potentially further refactor minor modes for #12281
There was a problem hiding this comment.
Not sure I'm fully understanding - I've tried remapping with specific keys e.g.
[keys.normal]
m.i.z = "..."(similarly for just keys.normal.m.i) and it seems to work fine, are you saying specifically that we should have a way to remap all fallback characters, something like
[keys.normal]
m.i.fallback = "..."which would accept a command that listens for a character?
the-mikedavis
removed
the
S-waiting-on-author
thomasschafer
force-pushed
the
tschafer-add-go-to-next-inner-matching-chars-2
branch
3 times, most recently
from
a567fdc to
62519b4
Compare
thomasschafer
force-pushed
the
tschafer-add-go-to-next-inner-matching-chars-2
branch
from
4f8f49c to
196989f
Compare
|
@the-mikedavis sorry for another ping, I've responded to/made changes based on your comments if you can take a look 🙏 |
|
ngl this would straight up be revolutionary |
thomasschafer
force-pushed
the
tschafer-add-go-to-next-inner-matching-chars-2
branch
from
196989f to
67a4bae
Compare
thomasschafer
force-pushed
the
tschafer-add-go-to-next-inner-matching-chars-2
branch
from
67a4bae to
c39007e
Compare
Adds the ability to match inside/around the next or previous matching character pair: for instance, with the cursor as
█below:typing
]"would select the contents of the string (i.e.bar).Screen.Recording.2024-09-15.at.10.20.34.mov
Supersedes #11260 based on discussion there.
Partially resolves: