kylechui / nvim-surround

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

Surrounds for special keys (<BS>, <C-...>) and/or Change+Backspace=>Delete #179

Closed nabellows closed 1 year ago

nabellows commented 1 year ago

Checklist

Is your feature request related to a problem? Please describe. If possible, provide a sample buffer/keymap sequence, along with the expected result.

    require'nvim-surround'.setup({
        surrounds = {
            ["<BS>"] = {
                add = { "" , "" },
            },
            ["<C-v>"] = {
                add = { "<C-v>" , "<C-v>" },
            },
            ["v"] = {
                add = { "<C-v>" , "<C-v>" },
            },
        },
    })

ysiw<BS> does nothing (sort of as expected) cs(<BS> does nothing (not expected) ysiw<C-v> does nothing (not expected) cs(<C-v> does nothing (not expected)

But: ysiwv works as I would like to (expected) cs(v works as I would like to (expected)

Describe the solution you'd like A clear and concise description of what you want to happen. Backspace and other special characters should be allowed for custom surrounds. vim-surround silently had a feature (maybe untentionally) which I found useful: cs(<BS> for example, would delete the parentheses. This is particularly neat if you map something besides cs to <Plug>Csurround, such as <M-[> in my case. Then, you can just think of that key as "grab the surrounding" and either hit what you want to change it to, or delete it. This was my original use case for trying to create the surround above, but I also noticed that any other special character was not working for me (such as the dummy example above)

Additional context Add any other context or screenshots about the feature request here.

kylechui commented 1 year ago

Hi there, have you perhaps tried using :h nvim_replace_termcodes() to input those special characters? Or alternatively going into insert mode and typing <C-v><BS>?

idanarye commented 1 year ago

@kylechui I looked into the code, and the problem is in get_char - specifically in this check:

-- Return nil if error (e.g. <C-c>) or for control characters
if not ret_val or type(char_num) ~= "number" or char_num < 32 then
    return nil
end

Is there a reason for disabling them? Specific inputs that can cause problems?

kylechui commented 1 year ago

I think somebody mentioned a while ago that they would sometimes make typos and do something like ysiw<C-v> (or something to that effect), and at the time I didn't think that anybody would want to use control characters as inputs. I can change it to be so just <C-c> returns nil (this commit has some relevant information, the case where the input key is <C-c> would have to be added). However, I would like to get confirmation from a few other people that this wouldn't break their workflows before adding it in. Feel free to open a PR yourself if you would like to contribute as well :eyes:

idanarye commented 1 year ago

Okay, I looked some more and I think the problem is with invalid_key_behavior, which uses any character as a delimiter. I guess we don't want control characters there? So what if we move the check to get_delimiters, before calling invalid_key_behavior.*? That way if you have set explicit entry in surrounds for a control key it'll be used, but the fallback will only work for non-control keys.

kylechui commented 1 year ago

I do think what you suggest could work, although I'm still wary of creating this sort of "inconsistency", where you're allowed to set it explicitly but it doesn't work under the invalid_key_behavior case.

idanarye commented 1 year ago

How about moving the check into invalid_key_behavior? This will be more consistent - everything can handle control characters, but the default invalid key behaviors will reject control characters because they make no sense in their specific context. User defined invalid key behaviors can still decide to do something with them.

Of course, <Esc> and <C-c> will be rejected at get_char.

The downside of this approach is that it's a breaking change - users will have to change their custom invalid key behaviors.

kylechui commented 1 year ago

Now that I think about it more, I feel like it does make a good amount of sense to just have <Esc> and <C-c> as two special cases for cancelling an input, and the rest are handled as an actual input... I really just can't find that one issue from a while ago.

idanarye commented 1 year ago

Wouldn't that insert control characters into the buffer when you use it with ys or cs?

kylechui commented 1 year ago

Sorrym, I meant that in conjunction with your idea of moving a control-character check into invalid_key_behavior. I think for the majority of people, it would be a transparent change as well, as long as they hadn't changed their invalid_key_behavior.

idanarye commented 1 year ago

Okay. I'll make a quick PR.

idanarye commented 1 year ago

209

kylechui commented 1 year ago

Hmm... it appears to not work for my Alacritty terminal when I try binding <BS>, since vim.fn.getchar() returns a string instead of a number for some reason...

I am considering reverting this commit because of the inconsistencies with vim.fn.getchar().

kylechui commented 1 year ago

This might be able to be done if instead we use vim.fn.getcharstr() instead of vim.fn.getchar(), but I'm unsure of how to check for control characters under that constraint. Furthermore, each terminal handles control characters slightly differently, so I'm not 100% sure how this would be done. If you are able to get your branch working with some minimal config, tag me and I'll test it out.

idanarye commented 1 year ago

Looks like multibyte inputs are returned as strings. This is not an Alacritty things - it's a Vim thing. Single byte inputs are fine, so it worked for me when I tested it with <C-something>, but I now tested on my terminal (Kitty) with <Bs>, with F keys, and with <M-something>, and they all return strings.

Furthermore, each terminal handles control characters slightly differently, so I'm not 100% sure how this would be done.

I think it would suffice to just use nvim_replace_termcodes and let Neovim handle that. Since that function is cumbersome and long, I think we can just add this to config.lua:

M.termcode = function(str)
    return vim.api.nvim_replace_termcodes(str, true, true, true)
end

And then you could just:

local config = require("nvim-surround.config").setup({
require("nvim-surround").setup({
    surrounds = {
        [config.termcode("<Bs>")] = { ... },
    },
})

Even if some terminal does handle <Bs> differently, we should assume that for that terminal nvim_replace_termcodes will return that different code when the user hits <Bs>. And if it doesn't - that probably means that Neovim itself cannot handle backspaces on that terminal, which is a bigger problem than some plugin being unable to handle it.

I'll make a new PR.

idanarye commented 1 year ago

Here: #211

kylechui commented 1 year ago

@nabellows Please try the latest commit on main and let me know what you think (in hindsight I probably should have branched but oh well).