WIP: Stabilize cursor

[ces42]

I tried to get vim-surround to always keep the cursor on the same character when adding/changing/deleting surroundings, if the move_cursor option is false. And if the cursor is inside a region that gets changed, it will be moved to the first character in that region. I prefer this to the current behavior of move_cursor = false because it means I don't need to "re-orient" myself after making a change with nvim-surround. Would you be interested in merging this?

Some people might be used to the current, so it might be good to keep an option for the old behaviour. I could add that.

Some examples: The uppercase letter marks the cursor position

 Old text           Command       current behavior      this PR
-------------------------------------------------------------------------
 one mAp pan        ysaw)         one (Map) pan         one (mAp) pan
 one (pAn) man      cs)[          one [ Pan ] man       one [ pAn ] pan
 gg(arg) + X + one  dsf           arg + x + One         arg + X + one
 sOme               ySawfgg<CR>   gG(                   gg(
                                      some                  sOme
                                  )                     )

gg(arg) + X + one dsf arg + X + one

[ces42]

lmao I'm an absolute idiot I just noticed nvim_buf_set_text already keeps track of the cursor position correctly, so most of the code here is unnecessary.

[ces42]

Ok now it's slightly different from before. If the cursor is inside one of the regions that gets changed, it will stay at the same position, or end up at the end of the changed region if staying put would mean it would end up outside the changing region (b/c that's how nvim_buf_set_text behaves). E.g. in <oNe>abc</one> doing catmy<CR> gives you <mY>abc</my> instead of <My><abc</my>

I think that moving the cursor to the beginning of the changing region would maybe be better (more consistent) but this version has the advantage of requiring very little change to the code.

[kylechui]

I might be misunderstanding here; are you trying to add a "sticky" option to move_cursor so the cursor always "sticks" to the same char? I am firmly against modifying any existing behavior, since IMO move_cursor = false should keep the cursor in the same row, col.

[ces42]

Yes that's the idea. I like that name. Keeping the current behaviour as default could be achieved by undoing the commenting of

    elseif not config.get_opts().move_cursor then
        -- M.set_curpos(pos.old_pos)
    end

in buffer.restore_curpos. If you did that then this PR would only change the behavior when move_cursor is neither nil nor "begin", so technically speaking a "sticky" option is unnecessary.

[kylechui]

I see, I think I'm not opposed to adding this functionality then. However, I would like to see a few things:

• Preserve_cursor -> preserve_cursor (also add type annotations, if possible)
• Add a new "sticky" option for move_cursor
• Test cases + docs!

[kylechui]

I would also like the options to be as explicit as possible (i.e. explicitly set move_cursor = "sticky"), to avoid ambiguity.

[ces42]

So what should the behaviour be when move_cursor is neither "begin", "sticky" of false/nil? Currently (current main branch) this would result in an undocumented behaviour which is essentially "sticky" except for the re-indenting. Do you think it's fine if the PR changed that?

[ces42]

I see, I think I'm not opposed to adding this functionality then. However, I would like to see a few things:

Cool!

* Test cases + docs!

OK I can create a copy of https://github.com/kylechui/nvim-surround/blob/633a0ab03159569a66b65671b0ffb1a6aed6cf18/tests/configuration_spec.lua#L125-L170 for the "sticky" option.

One question: How do I run the tests?

[kylechui]

So what should the behaviour be when move_cursor is neither "begin", "sticky" of false/nil? Currently (current main branch) this would result in an undocumented behaviour which is essentially "sticky" except for the re-indenting. Do you think it's fine if the PR changed that?

If you look at annotations.lua, you'll find:

---@field move_cursor false|"begin"|"end"

You can just extend this to false|"begin"|"end"|"sticky". We won't concern ourselves with undefined behavior, since people should be getting autocomplete/lint warnings for their configs.

As for tests, you can run a file using <Plug>PlenaryTestFile after you install plenary.

[ces42]

Running the tests isn't working. I have plenary in my lazy config. I open basics_spec.lua, do nnoremap <leader>t <Plug>PlenaryTestFile and the press <leader>t. But all the tests fail, also on your main branch. The plenary popup looks like this

Testing:        /home/ca/.local/share/nvim/lazy/nvim-surround/tests/basics_spec.lua                                                                         
Failit("||n channvim-surroundpcanssurroundltext-objectsi(ys, yss)                                                                                           
┆   ┆   set_...ocal/share/nvim/lazy/nvim-surround/tests/basics_spec.lua:11: Expected objects to be the same.                                                
┆   ┆   ┆   Passedrin:vim-surround'.setup(args)",                                                                                                           
┆   ┆   })  (table: 0x7f043e387478) {                                                                                                                       
┆   ┆   set_c*[1]s={'local str = test' }                                                                                                                    
┆   ┆   vim.Expected:al cS))")                                                                                                                              
┆   ┆   chec(table:(0x7f043e387298) {                                                                                                                       
┆   ┆   ┆   "*[1]i=e'localsstro=n"test"'p}",                                                                                                                
┆   ┆   ┆   "args",                                                                                                                                         
┆   ┆   ┆   stack traceback:                                                                                                                                
┆   ┆   })      ...ocal/share/nvim/lazy/nvim-surround/tests/basics_spec.lua:11: in function 'check_lines'                                                   
┆   ┆   set_curp...ocal/share/nvim/lazy/nvim-surround/tests/basics_spec.lua:24: in function <...ocal/share/nvim/lazy/nvim-surround/tests/basics_spec.lua:20>

[kylechui]

Apologies for the long delay; I think it might have something to do with nvim-surround not being properly added to packpath by lazy, and the plugin not being found properly? This is currently an issue for myself as well, but wasn't back when I was using packer.

[miguelbarao]

Any progress on this feature? It should be really helpful with dot repeat to add multiple surroundings.

[kylechui]

My bad, completely forgot this PR existed. I fixed the unit tests a while ago, but there's been many(?) commits since then so this PR is probably out of date. If there's no response here soon I might try implementing the feature myself to save some time.

[kylechui]

Moving development to #334