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

fix(core): match inside/around closest pair inconsitency #10611

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

woojiq
Copy link
Contributor

@woojiq woojiq commented Apr 27, 2024

Fixes one inconsistency missed in #8294

Make tree-sitter version of matching inside/around to replicate current behavior of plaintext files.
That is:

  • match around - always tries to extend a selection to the next pair,
    even when the selection is already around a pair.
  • match inside - do not extend a selection when it already completely
    matches the pairs.

Not related directly to this PR

Also, I noticed that if we have a selection with len=1, then "head" is always larger than "anchor" (even if we just moved one character to the left), and as a result mim|mam does not preserve direction in such cases. @pascalkuthe, Is this the intended behavior? With a selection of len>1 "anchor" and "head" are correct.
Though in my new tests everything is ok, probably because the #[| ]#creates the correct "Range".
Answer: #10611 (comment)

@woojiq woojiq force-pushed the fix_inconsistency_match_inside branch from 209c6ca to 2810946 Compare April 27, 2024 06:52
@woojiq woojiq marked this pull request as draft April 27, 2024 07:02
@woojiq woojiq marked this pull request as ready for review April 27, 2024 07:12
@woojiq
Copy link
Contributor Author

woojiq commented Apr 27, 2024

@thomasaarholt I fixed your remark, I didn't notice anything strange anymore. Feel free to test it out if you wish.
P.S. What would you really expect from "mim" in your cases? Should it match inside the quotes or not because you are already on top?
To be honest, I don't know what the "correct" behavior is, if any of you can tell me what you expect from matching inside/around when you're already on top of a pair, I can change it to be more correct.

@thomasaarholt
Copy link
Contributor

Sweet! I'll try to test this this evening and quickly get back to you - out for the day!

@pascalkuthe
Copy link
Member

The direction of a single width selection doesn't matter/is essentially ab implementation detail hidden from the user. Even if its sometimes backwards internally it should always be forward for single width selections sk thay behavior is intentional

@David-Else
Copy link
Contributor

I was wondering about the pairs in Markdown, for example

within `GameState` so that

does not mim when the cursor is inside the back ticks, but it does alt-o select the entire word with backticks, as I imagine mam would if it worked. It seems it is a tree-sitter object, but not being treated as a pair?

Also, in markdown, select a word and ms" to add quotes, but mim inside it does not work. md" deletes the quotes fine with the cursor anywhere inside the word surrounded by the ". Surely mim and mam should be working too?

@pascalkuthe
Copy link
Member

the markdown case is caused by the fact that markdown handles these pairs as injections and while there is some basic support for injections in match bracket its very very basic and doesn't handle these cases yet.

I am working on larger changes to TS that would cover this

@pascalkuthe
Copy link
Member

For example (rust):
fn func() {func("string");}
When the cursor was on the opening quote and you pressed "mim", new selection was inside
quotes, but it must be inside parentheses (around the quotes). Previously, this only worked for closed pairs.

actually I wanted to go in the opposite direction here and make mim and mam on the quote actually select the quote rather than the parent pair. I think this would not have been possible in the past (since it was ambiguous) but its possible now thanks to TS. This matches the behaviour of mi(

@the-mikedavis the-mikedavis changed the title fix(core): match inside closest pair inconsitency fix(core): match inside closest pair inconsistency Apr 27, 2024
@pascalkuthe
Copy link
Member

@woojiq I think this was mostly a miscommunication on my part/skipping past that comment by accident. I think you just need to turn .to() at the start of that function into .to() - 1 you even left a comment about it I somehow just missed that

@woojiq
Copy link
Contributor Author

woojiq commented Apr 27, 2024

Thanks for the clarification. The problem is that if I add "-1" than if we are already "around" some pair (hit "mam") then one more "mam" will do nothing and you have to type "2mam" or move the cursor. This seems logically, but from personal experience I think that each subsequent "mam" should expand to the next pair, while "mim" should not (this is how it worked before my commits)
Correct me if the expected behavior is different:
fn main() {func("strin#[|g]#")} -> 100mim -> fn main() {func("#[string|]#")} -> mam ->
fn main() {func(#["string"|]#)} -> mam -> fn main() {func#[("string")|]#} -> mam ->
fn main() #[{func("string")}|]# -> mam -> selection is unchanged.

@pascalkuthe
Copy link
Member

pascalkuthe commented Apr 27, 2024

This could theoretically be useful. I think this may have been an oversight in the PR that changed mi( and ma( to respect selection (instead of just the cursor) in that mi and ma should ignore a pair that is already fully selected.

However, for consistency if we are going to change that we should change it for all textobjects consistently. I think for now we should just make mim/mam work like other textobjects do currently. Then we can have a separate PR for changing the behaviour of all textobjects and have the discussion wether that is the right thing to do there

@pascalkuthe
Copy link
Member

I managed to fix all the bugs that were caused by mm behaving incorrectly in #10613 (both the python and markdown cases)

@woojiq woojiq force-pushed the fix_inconsistency_match_inside branch 3 times, most recently from 9bf10fa to 0890141 Compare April 27, 2024 18:23
@woojiq
Copy link
Contributor Author

woojiq commented Apr 27, 2024

I think I achieved the desired behavior. I'd ask y'all to give it a try (especially on edge-cases) and tell me if you think this behavior is acceptable. It feels good for me. <description and commit message was updated>

@woojiq woojiq changed the title fix(core): match inside closest pair inconsistency fix(core): match inside/around closest pair using tree-sitter Apr 27, 2024
@woojiq woojiq changed the title fix(core): match inside/around closest pair using tree-sitter fix(core): match inside/around closest pair inconsitency Apr 27, 2024
Copy link
Contributor

@thomasaarholt thomasaarholt left a comment

Choose a reason for hiding this comment

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

Great! This is now acting the way I expect it to! Thank you very much for fixing!

@woojiq
Copy link
Contributor Author

woojiq commented Jun 28, 2024

Any chance we can include it in the next release? Because the matching around/inside on master (on 24.03 it's ok) is slightly broken and everyone will have it with the next release.
UPD: rebased on the latest master.

Make tree-sitter version of matching inside/around to replicate current behavior of plaintext files.
That is:
* match around - always tries to extend a selection to the next pair,
even when the selection is already around a pair.
* match inside - do not extend a selection when it already completely
matches the pairs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants