kylechui / nvim-surround

Add/change/delete surrounding delimiter pairs with ease. Written with :heart: in Lua.
MIT License
3.18k stars 62 forks source link

With cursor in between 2 groups of quotes it should focus the right group #39

Closed williamgoulois closed 2 years ago

williamgoulois commented 2 years ago

Thank you for this plugin ❤️

As discussed here https://github.com/kylechui/nvim-surround/issues/12#issuecomment-1179650216

There is an issue if a cursor is between two surroundings that use the same character for opening and closing (i think that ``,"and'` are concerned so all quotes)

Reproduction video

https://user-images.githubusercontent.com/37271970/178130489-bffca46a-5946-413b-bca7-b6aed8559dd7.mov

Possible solutions

I think that we can get around that by using Treesitter to determine the most inner and closest surrounding but it would maybe need a whole new implementation and is not trivial.

Otherwise we could add special handling if the modification is concerning quotes but it would be less elegant.

kylechui commented 2 years ago

Playing around with :TSPlaygroundToggle, it seems like some of the code that I'm using :h opfunc for could be replaced by TreeSitter nodes, e.g. string. This would definitely help with the edge case presented, but I wonder if other delimiters can/should be rewritten to use TS to better modify them (insertion seems to be relatively easy/straightforwards). The only issue I could maybe see is that different TS implementations for different languages could call "strings" different things, which would complicate things quite a bit.

Edit: I actually think having a separate edge-case for quotes is probably easier than trying to refactor things to "sometimes use TS, sometimes not", but I haven't really tried yet so I don't know. Additionally, this would be better for reduced dependencies for the plugin, as it would be quite strange (IMO) to have TS be a requirement but only really be useful for this small edge-case.

williamgoulois commented 2 years ago

@kylechui I got an idea to determine if we are in this situation :

If the number of " to the left of the cursor is the same as to the right we are in between.

Example (numbers represent the number of " with cursor at position to the left/right) local a = "this is" .. "a string" -- comment 000000011111112223333334444444444 -- to the left 444444433333222111111110000000000 -- to the right

The left should equals the right only if we are in the middle of the two groups of "

Edit: Removed the code part as it fucked up the alignment of the numbers 🙃

kylechui commented 2 years ago

@williamgoulois I think your example fails if we consider three pairs of quotes, e.g.

--  "str1", "str2", "str3" --

However while playing around with this, I made the "discovery" that it seems that Vim can "smartly" detect what quote pair is "correct/valid" if your cursor is on one of the quotes. I think there can be a reasonably short modification to the current codebase to take this case into account.

kylechui commented 2 years ago

@williamgoulois I added ~5 new lines of code to branch add-smart-quotes, and it seems to be working quite well with the new tests that I've added. Please try it out for yourself and let me know if this is the desired behavior!

Edit: I've also added some documentation about "jumping" overall at :h nvim-surround.jump

Edit 2: This still seems to be broken for inserts, not only this specific case but also reverse jumping as a whole

Edit 3: I might not be able to actually fix the case for inserting, since I don't know of a way to retrieve the text object after inserting it

Edit 4: I've documented the inconsistent behavior, hopefully nobody questions it too much haha

williamgoulois commented 2 years ago

@kylechui Tested it and looked at the new docs. This seems to behave perfectly ! 🚀

I tested for inserting and it works only forwards (with all). I can confirm that inserting backwards does not work as a whole. And i also encountered some bugs while trying to delete or change backwards. See this recording for example :

https://user-images.githubusercontent.com/37271970/178154344-bcec763f-a04e-4907-8606-0d351111be6c.mov

For the docs i found the first paragraph not very clear. I'm talking about this one :

Under certain circumstances, |nvim-surround| can "jump" to the "nearest" delimiter. It decides how to jump based on the position of the cursor relative to the pair of delimiters:

  • If there exist pairs surrounding the cursor, jump to the right-most (startwise) pair
  • Else if there exist pairs to the right of the cursor, jump to the left-most (startwise) pair
  • Else if there exist pairs to the left of the cursor, jump to the right-most (endwise) pair
kylechui commented 2 years ago

@williamgoulois Yeah I think I'll delete that paragraph of text since it's not very clear. As for the "bug" that you found, remember that nvim-surround always prioritizes pairs that occur after the cursor before pairs that occur before. Because of how implementation works, it is forward-searching to the end of the file before it even considers reverse-searching. Note that this is different for quote-types since for quotes they only forward/reverse search on the same line.

Edit: This behavior should be consistent with vim-surround, which does not have reverse search, and so always jumps to the "next" surround pair in the file, no matter how far down that may be (if not quotes).

Edit 2: Documentation has been updated to (hopefully) be better than it was before

kylechui commented 2 years ago

Merging this into main as it seems to be working as intended, and passes automated testing.