RishabhRD / nvim-cheat.sh

cheat.sh integration for neovim in elegant way
151 stars 8 forks source link

Closing window causes lsp error #4

Closed akinsho closed 3 years ago

akinsho commented 3 years ago

Hi 👋🏿 thanks for the cool plugin.

Just tried it out and I'm noticing some interaction with my native neovim lsp client.

E5108: Error executing lua .../akin/neovim/share/nvim/runtime/lua/vim/lsp/handlers.lua:343: RPC[Error] code_name = MethodNotFound, message = "method textDocument/documentHigh
light is not supported by any of the servers registered for the current buffer"

before I open the buffer this works fine, but on close when my cursor moves back to the buffer I came from this error seems to happen

My document highlight setup is

  if client and client.resolved_capabilities.document_highlight then
    extend(
      commands.LspCursorCommands,
      {
        {"CursorHold", "<buffer>", "lua vim.lsp.buf.document_highlight()"},
        {"CursorHoldI", "<buffer>", "lua vim.lsp.buf.document_highlight()"},
        {"CursorMoved", "<buffer>", "lua vim.lsp.buf.clear_references()"}
      }
    )
  end

I explicitly check the client can handle it before adding the command so I'm guessing your most recent commit which removes the client, happens too late otherwise the client would be nil at this point

RishabhRD commented 3 years ago

Hi @akinsho thanks for reporting! Yeah! I was looking for some mechanism with which specific buffer never gets attached to LSP server. However I didn't get any such thing. I would just read nvim-lspconfog source and LSP client documentation one more time. Do you have any idea how can we get this? For this reason I thought to disable LSP on publish diagnostic event. That is surely too late. That's why this error occurred. I got a little careless while writing this part and didn't think about this case :sweat_smile:

One more thing I am considering is I think I should not stop LSP client in plugin itself. This should be user decision based on buffer name maybe using autocommand. So, instead of handling LSP, provide splits and tab buffers some predictable name and users can respond on it. What you think on this opinion?

akinsho commented 3 years ago

Hmm that's slightly tricky 🤔 I think even if you used a predictable name, a user of the native lsp would have to check every autocommand and every part of their config to make sure this filename was excluded, which is a lot of extra boilerplate that is error prone to get this to work correctly. I'm wondering if there is any type of buffer that the native lsp ignores by default, like if the buftype was nofile or the buffer was nobuflisted does that still trigger the lsp since it shouldn't (in theory the lsp shouldn't activate if there is no underlying file).

I noticed recently that the telescope contributors started using vim buffers for showing previews a little while ago and those don't trigger the lsp as far as I know so there might be a clue there

RishabhRD commented 3 years ago

Hi @akinsho One way of not loading nvim-lsp is to use noautocmd for filetype event. However, it would also stop the highlighting of buffer as it is also based on filetype events. So, it is clearly not the solution.

However, surprisingly I also noticed that lsp doesn't load for floating windows. Even in this plugin, lsp server doesn't attach to the buffer, if the buffer is in a floating window. However, it attaches if the buffer is in split or tabs.

I guess this is why telescope didn't have the LSP problem.

RishabhRD commented 3 years ago

But as you pointed out, it seems I need to check the buftype of floating windows. I guess this would really hint at something useful.

Edit: I just checked the filetype of floating window buffer. It was nofile. Maybe we want the same behavior. I don't think someone would really want to use the result buffer as a file. What you think @akinsho

akinsho commented 3 years ago

@RishabhRD I think the way around that is that a user can do :w to-new-file.js and that will work if they wanted to save it from help 👇🏿

    "nofile" and "nowrite" buffers are similar:
    both:       The buffer is not to be written to disk, ":w" doesn't
            work (":w filename" does work though).

alternatively it also says

"acwrite" implies that the buffer name is not related to a file, like
    "nofile", but it will be written.  Thus, in contrast to "nofile" and
    "nowrite", ":w" does work and a modified buffer can't be abandoned
    without saving.

so maybe you could also use acwrite

akinsho commented 3 years ago

also another thing you could try is set them to nofile by default and then in a <buffer> autocommand you could do BufWritePre <buffer> set buftype='' I haven't tried this autocommand version since it might need some sort of different hook like BufWriteCmd etc

RishabhRD commented 3 years ago

Actually nofile works. As acwrite doesn't stop lsp from attaching. I didn't try BufWritePre till now, I would try it in my free time. But I guess, nofile should work fine. Let's leave the issue open, for a day or two. Let's see if we can find any issue with it.

akinsho commented 3 years ago

That change worked by the way 🙌🏿 no more error

This probably worth another issue but I hate to open a bunch of issues 😅 but I think it's weird that floating window opens immediately and is empty, so you wait another 2-3sec for the request to complete. I think it would be better to only open the floating window when the data has come back, or add a loading window.

EDIT: in your GIF the request is quite fast but I've tried a few more obscure requests and sometimes they can take a while.

RishabhRD commented 3 years ago

Yeah! This can include many factors like network issues, etc. that can cause the delay and we see the empty buffer at starting. I would just open the separate issue for this then :sweat_smile: and I would mention you. There we can discuss the possible solutions. :smile: