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

Allow "" to act as a pair for mim #4475

Closed
lukepighetti opened this issue Oct 26, 2022 · 10 comments · Fixed by #8294
Closed

Allow "" to act as a pair for mim #4475

lukepighetti opened this issue Oct 26, 2022 · 10 comments · Fixed by #8294

Comments

@lukepighetti
Copy link

If you have the text ("yoo█oo") with your cursor placed in the middle and use mim, it will select inside the parens instead of inside the quotes. Expected result is to act inside the quotes

helix 22.08.1 (66276ce)

@matoous
Copy link
Contributor

matoous commented Oct 26, 2022

Would require " to be added as a pair here: https://github.com/helix-editor/helix/blob/master/helix-core/src/surround.rs#L6, nothing more is needed afaik. While at it, ` and ' might also be good candidates.

@dgmulf
Copy link

dgmulf commented Oct 27, 2022

Perhaps mim should automatically work with all pairs defined in the [editor.auto-pairs] config key?

@lukepighetti
Copy link
Author

Would require " to be added as a pair here: https://github.com/helix-editor/helix/blob/master/helix-core/src/surround.rs#L6, nothing more is needed afaik. While at it, and'` might also be good candidates.

Interestingly, I did try that, but after compile it wasn't working. I noticed that we have no cases where open and close characters are identical, and I wonder if the existing code is not setup to handle this case.

@matoous
Copy link
Contributor

matoous commented Oct 28, 2022

@lukepighetti your are right. I will try to take a look over the weekend 👀
Edit: tried, struggled with it, went to work on other PRs where I felt more confident. Free for grab or I will revisit this once done with other stuff.

@lukepighetti
Copy link
Author

Looks like @sudormrfbin was the last to touch this part of the codebase. Maybe he has some tips for us.

@asvln
Copy link

asvln commented Nov 25, 2022

Interestingly, I did try that, but after compile it wasn't working. I noticed that we have no cases where open and close characters are identical, and I wonder if the existing code is not setup to handle this case.

Note for future implementors, adding any character pair (same or not) to the PAIRS const in surround.rs does not make them work with mim.

@thomasaarholt
Copy link
Contributor

thomasaarholt commented Apr 26, 2024

This doesn't seem completely fixed yet. Here I show examples in python and rust on the latest master.

asciicast

I have working LSPs for both languages.

❯ hx --health python
Configured language servers:
  ✓ pyright-langserver: /opt/homebrew/bin/pyright-langserver
  ✓ ruff-lsp: /opt/homebrew/bin/ruff-lsp
Configured debug adapter: None
Configured formatter: None
Highlight queries: ✓
Textobject queries: ✓
Indent queries: ✓
❯ hx --health rust
Configured language servers:
  ✓ rust-analyzer: /opt/homebrew/bin/rust-analyzer
Configured debug adapter: lldb-dap
Binary for debug adapter: 'lldb-dap' not found in $PATH
Configured formatter: None
Highlight queries: ✓
Textobject queries: ✓
Indent queries: ✓

For testing:

foo_python = "mim does not work on this string"
let foo_rust = "mim only works when the cursor is positioned on the first \""

I observe that mm doesn't work on strings in Python, and that mim only works for the first quotation mark in Rust, and not at all in Python.

@pascalkuthe
Copy link
Member

Those two things are unrelated. The python case has an odd grammar for the string literal which is not being handeled due to an existing bug kn mm (I am aware of). The case of mim not working on the second quote is likely a bug in the new mim implementation.

U don't think it's useful to necormamce old issues that are mostly solved except some edgecase burried kn the comments.

@woojiq
Copy link
Contributor

woojiq commented Apr 26, 2024

@thomasaarholt hey, thanks for the reproduction steps.
I agree with @pascalkuthe about mm bug. But anyway thanks for detailed asciinema, I see some inconsistency in the new behavior as well. I will look at the weekend.

@thomasaarholt
Copy link
Contributor

thomasaarholt commented Apr 26, 2024

@pascalkuthe, the main reason I didn't create a new issue was that this one was closed 48 hours ago. I was honestly worried about being told off for making a new issue, which is why I commented here. 🤷
Thank you for identifying for me that mm is a problem with the Python side.

@woojiq, thank you! I really appreciate your contribution, it's one that makes helix feel even more intuitive to me!

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 a pull request may close this issue.

7 participants