kylechui / nvim-surround

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

[Bug] When used with which-key.nvim the insert surround operator outputs g@ #15

Closed akinsho closed 2 years ago

akinsho commented 2 years ago

Hi,

Was quite interested to try this plugin out, but it seems to conflict with using which-key.nvim when pressing ysiw' it stops at the ys and inserts g@ into the buffer instead of completing the mapping. Some other mappings such as cs also seem to encounter this issue.

This though isn't a problem for vim-surround which completes the mapping without issue.

Details: nvim version: nightly os: macOS 12.3

This is not a huge amount of detail, apologies. I assume that if you try this plugin with which key, you will be able to easily reproduce this behaviour. If not, please let me know, and I'll try to add more information.

kylechui commented 2 years ago

Hi there, do you mind testing out my implement-dot-repeat branch instead of main? It seems like main is a bit buggier than the other branch; I might merge earlier than I had intended

akinsho commented 2 years ago

@kylechui just gave that a try, and it still seems to be broken, unfortunately pressing ysi or a just enters g@ into the buffer

kylechui commented 2 years ago

Hmm that seems quite strange, I tried this minimal config:

return require("packer").startup(function()
    use("wbthomason/packer.nvim")
    use({
        "kylechui/nvim-surround",
    branch = "implement-dot-repeat",
        config = function()
            require("nvim-surround").setup({})
        end,
    })
    use({
        "folke/which-key.nvim",
        config = function()
            require("which-key").setup({})
        end,
    })
end)

and wasn't able to reproduce it. I wonder if it's something to do with MacOS or the latest nightly, as another user (#11) also had a similar issue with conflicting keymaps that I can't replicate. I'll try installing the nightly right now and see if that's the case. Edit: @akinsho Just downloaded the latest nightly (NVIM v0.8.0-dev+547-ge837f29ce) and still couldn't reproduce; maybe try with either the latest nightly or 0.7 stable? If neither of those work, then this might be a MacOS issue (but that also seems strange/unlikely)

akinsho commented 2 years ago

@kylechui did a bit more debugging, and it seems it was actually https://github.com/Matt-A-Bennett/vim-surround-funk interacting with the ys mappings for some reason, although vim-surround doesn't seem to mind. With that plugin disabled it works fine, but one issue I'm still seeing which does seem related to which key is using the cs mapping. This seems to block input when I try and change surrounding quotes. Till I press <c-c> which then prints

image

EDIT: the same behaviour happens with ds

kylechui commented 2 years ago

Hmm I don't actually use which-key, so I'm not entirely sure what mappings it has setup by default that could be conflicting with nvim-surround. As for the <C-c> issue, it looks like it might be builtin behavior(?), as I'm using vim.fn.getchar() to get the character inputs from the user. I will look into whether there's a way to suppress the error message or redirect it some other way

kylechui commented 2 years ago

image @akinsho I learned that :checkhealth which_key exists, and it doesn't look like which-key should have any keymap conflicts with nvim-surround by default, so I think vim-surround-funk is most likely the culprit. I don't know how vim-surround-funk is implemented, but I suspect the author wrote it with vim-surround in mind to avoid any potential conflicts.

akinsho commented 2 years ago

@kylechui thanks for having a go with which key, so just to clarify I have fully disabled surround funk, so I don't believe it is related to the issue I'm seeing with nvim-surround. I think it caused the ys issue, but I don't know what is causing the cs issue. I had a brief look at its code and I don't believe it does any special case handling to escape issue with vim-surround.

I'm not a 100% sure, but I think something about how the surrounds are being applied is "leaking" since using either vim-surround-funk or vim-surround which both use the operator function don't cause these interactions.

The issue with <c-c> and getchar is fairly trivial to fix, since if you wrap the function with pcall that will swallow any error it throws.

EDIT: I don't believe it is an issue of a straightforward mappings clash, since one would just overwrite the other. I think instead the mappings are interacting with something else

kylechui commented 2 years ago

Thanks for the tip about pcall, just pushed a change to implement-dot-repeat that fixes it. I see that in the screenshot you have a set opfunc=v, which looks like something I'm using to find selections in nvim-surround, but only partially filled. I think somehow they are both getting triggered at the same time, but I'm not sure how/why; I'll try to read more about how to use which-key

kylechui commented 2 years ago

I'm not a 100% sure, but I think something about how the surrounds are being applied is "leaking" since using either vim-surround-funk or vim-surround which both use the operator function don't cause these interactions.

I think it might have to do with the fact that some of the mappings in vim-surround-funk are prefixes of nvim-surround's (e.g. ysf from vim-surround-funk and ys from nvim-surround). I'll install it and see if I run into similar issues or not. Edit: Issue has successfully been replicated!

kylechui commented 2 years ago

@akinsho Running :verbose nmap ys yields the following screenshot: image If you try typing ys and waiting ~1-2s for it to timeout, then you can finish the rest of the nvim-surround mapping and it works normally, e.g. ys wait iw)

kylechui commented 2 years ago

Hi @akinsho, please update to the latest commit on branch implement-dot-repeat to see if the issue persists; I changed all the keymaps to expr = true and it seems to have fixed the issue on my end.

akinsho commented 2 years ago

~That seems to have fixed the issues @kylechui 🙌🏿 thanks~

Actually I spoke too soon, surround-funk does not appear to be the issue any more for me but the keys ys and cs still hang for me, but I don't know why the only errors I see relate to which key but could be a red herring

kylechui commented 2 years ago

Do you mind running a minimal config for me just to see if it might be another plugin or how you setup the plugins that might be the issue?

    use({
        "kylechui/nvim-surround",
        config = function()
            require("nvim-surround").setup()
        end,
    })
    use({
        "folke/which-key.nvim",
        config = function()
            require("which-key").setup({})
        end,
    })

Also please attach screenshots of the error messages, thanks!

akinsho commented 2 years ago

@kylechui I've manage to boil it down to a mapping actually

vim.cmd("packadd nvim-surround")

vim.keymap.set("c", "::", "<C-r>=fnameescape(expand('%:p:h'))<cr>/") -- THIS TRIGGERS IT
require("nvim-surround").setup()

It seems it's actually not related at all to which-key and is actually because somehow that mapping gets triggered, and it blocks nvim waiting for command line input.

I don't know why the commandline is being triggered there, but one other side effect of this is that it cause the comamndline to flash up if using cmdheight=0 on nightly. I think maybe these mappings should be being executed silently or using <Cmd> to stop actually going into the commandline.

kylechui commented 2 years ago

@akinsho Sorry for the late reply, but I wasn't able to replicate your issue on a the most recent nightly (NVIM v0.8.0-dev+550-gf075feee3) with the following minimal_config.lua:

vim.cmd([[set cmdheight=0]])
vim.keymap.set("c", "::", "<C-r>=fnameescape(expand('%:p:h'))<cr>/")
return require("packer").startup(function()
    use("wbthomason/packer.nvim")
    use({
        "kylechui/nvim-surround",
        branch = "fix-keymaps",
        config = function()
            require("nvim-surround").setup()
        end,
    })
end)

Can you try using the above as well via nvim -u minimal_config.lua? Also perhaps the reason for the flickering command line is that in my function to find the selection that the cursor is in, I set an operatorfunction using :set opfunc=...; I will try your suggestion of changing it to <Cmd> and silent to see if that helps

Edit: It appears that neither silent nor <Cmd> will do anything for the cmdheight I think, as both still leave the echoing :set opfunc=... line in my command line. However, I changed the code to instead set the operatorfuncs via a Lua variable instead, which cleared up the issue for me (and hopefully for you too!). I've decided to put work concerning this on a new branch fix-keymaps, so please test it out and let me know what happens

akinsho commented 2 years ago

@kylechui apologies for the run around, it turns out this issue does need which key to reproduce. Please try

vim.cmd("packadd nvim-surround")
vim.cmd("packadd which-key.nvim")

vim.keymap.set("c", "::", "<C-r>=fnameescape(expand('%:p:h'))<cr>/")
require("nvim-surround").setup()
require("which-key").setup()

then nvim -u <config-above.lua>

and then if you enter some text and then surround it, then try and change or remove the surrounds you'll see the cursor block.

Sorry I was initially trying it with which key but when I saw it was due to a mapping I just assumed that which key wasn't related but it still is.

kylechui commented 2 years ago

I've finally been able to reproduce the issue (somewhat); the command line flashes for me and says Press ENTER or type command to continue, and then doesn't delete nor change the surrounding pair. It appears that adding surrounds still works, so I think I know where to start looking through the code. P.S. No worries for the run around, glad we were just able to finally narrow it down to something!

Edit: My only concern now is that I wonder if this is an upstream issue, as (to my knowledge) I never call anything from the command line in the latest commit on fix-keymaps. I've changed everything to directly modify the operatorfunc via Lua variables, instead of vim.api.nvim_feedkeys(":set opfunc=...",...)

Edit 2: I removed the keymap that you added (the :: one) and I still am having the issue, so I think it probably still has to do with which-key only. I saw in the help menu that setting cmdheight=0 causes all outputs to need the hit-enter prompt, so I'm thinking that maybe which-key nvim-surround is causing some output that's triggering the hit-enter prompt, which is then consuming the deletion character, hence "disabling" the deletion

Edit 3: I just re-tested the minimal config with vim-surround and it works without any errors, so it's definitely nvim-surround that's causing the issues

Edit 4: I'm 99% sure it has to do with the fact that I'm calling the operatorfunc "manually" in utils.lua when I'm getting the surrounding selection (via feedkeys("g@a" .. char)), but I haven't been able to think of a way to refactor that code to return "g@a" .. char to test that out yet. Also as a quick aside apologies in advance for being unable to sort things out in the next few days, I'll probably be recovering from getting my wisdom teeth removed

kylechui commented 2 years ago

@akinsho The strangest thing is happening now; it doesn't work for me with a minimal config, but does when I use my default config that I use for everything else (with which-key included). I still see the flickering Press ENTER or type command to continue, but it seems to just skip past it and move on, modifying the buffer as intended. At the very least the fix-keymaps branch shouldn't hang anymore, but might still have some issues with actually performing the action.

kylechui commented 2 years ago

Sorry for the repeat ping @akinsho, but please retry the fix-keymaps branch. I've put the portion of the code that sets the operator marks [ and ] into their own function, and am silently calling that function. It seems to have fixed the issue on my end

akinsho commented 2 years ago

@kylechui just gave it a try and that definitely improves/fixes things. One thing that I noticed though... 😅 was that if I tweaked the mapping slightly the ds surrounding mapping started to break

image

Basically the mapping is supposed to insert the current buffer path into the commandline when you type ::: i.e. : opens the commandline then :: will expand the file path.

The mapping has a / suffix at the end just for ease of use but I've found that if I remove it and try and use ds' or something that shows the error above e.g.

cnoremap('::', "<C-r>=fnameescape(expand('%:p:h'))<cr>"), is the mapping that triggers the ds bug

kylechui commented 2 years ago

@akinsho Whoops forgot to properly escape the ' character when passing it into a function as an argument. Please try the latest commit

akinsho commented 2 years ago

@kylechui thanks, that works now 👍🏿. Think that's all the issues I had resolved. Great work 🚀