kylechui / nvim-surround

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

Visual block mode bug #275

Closed 9mm closed 1 month ago

9mm commented 8 months ago

Checklist

Neovim Version

NVIM v0.9.4
Build type: Release
LuaJIT 2.1.1699180677

Plugin Version

Tagged (Stable)

Minimal Configuration

return {
  'kylechui/nvim-surround',
  version = '*',
  event = 'InsertEnter',
  config = function()
    require('nvim-surround').setup()
  end,
}

Sample Buffer

I can't tell if I've never noticed this or its new, but there appears to be a bug with visual block mode where the length of the LAST line is used to compute something, rather than the longest line.

Take this text:

aaaaaaaaaaa
bbbb
cccccccccccccccc
dddddddd
eeee

Keystroke Sequence

start on line 1 col 1, and press <C-v> to enter visual block mode. Now press G and $

This will highlight the entire text. Now press S" and the output is broken

Expected behavior

"aaaaaaaaaaa"
"bbbb"
"cccccccccccccccc"
"dddddddd:
"eeee"

Actual behavior

"aaaaa"aaaaaa
"bbbb"
"ccccc"ccccccccccc
"ddddd"ddd
"eeee"

Additional context

No response

kylechui commented 8 months ago

This is intended behavior; it should add the surround around the "block" of text that you have. To implement what you want, you might want to first visually select the text, then hit :normal yss".

9mm commented 8 months ago

I see that does it, but I also disagree. IMO if I wanted to have it select an actual "block" then I would do this:

image

I'm talking about $ though, which is no longer selecting a "block", its selecting to the end of the line:

image

Having the later image result in this seems very weird to me:

image

On top of that, if the idea is to TRULY do a "block"... it doesnt appear to do that job consistently, for example here is what happens when I select the actual block from image 1. Now compare it with the image directly above.

image

So this idea of selecting a "block" when using $ is very unexpected to me, in almost every way, while the actual block behavior (which involves just selecting the literal square of text) works perfectly and expected.

Given you already have a working actual block behavior, it seems $ should operate as expected by wrapping what is visually selected, that is what tpope does in vim-surround, etc.

kylechui commented 8 months ago

Oh I misunderstood; that is quite strange. I'll look into it when I get the chance.

kylechui commented 7 months ago

I see the issue; I didn't take into account the fact that if you hit $ then it'll assume end of line for all lines in the region. I'm not actually sure how to handle this case, since in normal visual block mode you can press l enough and it'll put the cursor "one past the end". Might have to read the docs to see how to differentiate between the two cases. Any pointers appreciated!

Without $

image

Using $

image

9mm commented 7 months ago

Yes I definitely did notice that tiny nuanced detail, how when you press $ it actually allows you to extend PAST the part where you can move normally. I thought that was interesting when I made this issue (all the edge cases one must consider when building a text editor!)

my only advice might be to check tpope vim-surround as he implements this feature

kylechui commented 7 months ago

Ah yeah the main issue with taking inspiration from vim-surround is that (to my knowledge), Vim doesn't provide API functions for modifying the buffer, so the implementation involves a lot of just selecting the buffer and yanking/deleting/pasting. I'll look through the Neovim API (at some point, sorry I'm busy due to school :sob:) to see if there's a function in there that can help disambiguate these cases.

gegoune commented 7 months ago

Without diving any further (sorry!); doesn't vitrualedit influence behaviour here?

kylechui commented 7 months ago

You would be correct (also it seems that setting virtualedit causes a different bug :skull:)

esn89 commented 6 months ago

Hi @kylechui

I have a similar issue in Visual mode as well, with this block of text:

image

I select it by:

vEE then

S), but it doesn't surround.

NVIM v0.9.4
Build type: Release
LuaJIT 2.1.1700008891

   system vimrc file: "$VIM/sysinit.vim"
  fall-back for $VIM: "/opt/local/share/nvim"

Run :checkhealth for more info
    {
        'kylechui/nvim-surround',
        version = '*',
        event = 'VeryLazy',
        config = function()
            require('nvim-surround').setup({})
        end,
    },
kylechui commented 6 months ago

@esn89 I don't think your issue is related to the existing issue; can you open up a new issue to avoid polluting this one?

esn89 commented 6 months ago

Hey there, sure thing.

aarondill commented 3 months ago

this can be detected sometimes by using the cursorwant return from getcurpos. if the user selects a region then presses o to move the cursor, it seems there’s no current way to get this information .

9mm commented 1 month ago

Do you know if you plan to fix this one? This seems like a pretty major bug for a surround plugin, since I made this issue I have encountered it dozens of times, it's quite painful.

I suspect needing to wrap multiple arbitrary length lines of text with a surrounding is a really common use case

kylechui commented 1 month ago

I would gladly fix it if I knew of a way to actually differentiate between these two cases (block selecting the entire line for every line vs. up to current line length for every line), but haven't found anything myself. The best I can do at the moment is offer a workaround which is doing yss" in normal mode, then selecting your lines and doing :norm . to dot-repeat the action.

9mm commented 1 month ago

Hmm ok. At least the dot repeat isnt the end of the world but damn, I do really miss this from tpope surround. Thanks for thinking about it!

9mm commented 1 month ago

Does this relevant nvim-sandwich code help? https://github.com/machakann/vim-sandwich/blob/74cf93d58ccc567d8e2310a69860f1b93af19403/autoload/operator/sandwich.vim#L83-L107

I asked in mini.surround and he mentioned he wants to keep his code simple but thats the relevant code of vim sandwich

I know this code is in vim-surround but he mentioned that code doesn't make any sense

kylechui commented 1 month ago

Oh wow I think that actually exactly solves this issue! I'll try it out sometime this week; thanks for finding it!

kylechui commented 1 month ago

Hey @9mm (or anyone else that is willing to help out), please try the linked PR and see if that fixes things; I did some local testing and it seems to work just fine. I also added a new unit test

9mm commented 1 month ago

dude it works!!! Nice!!!!

kylechui commented 1 month ago

Feel free to update to 2.1.9 :) Thanks again for finding the fix!

9mm commented 1 month ago

you are awesome. that was basically the last remaining headache on converting from 10 year vim config to neovim. you da man!