Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(peek): make sure popup_options are positive #2373

Merged
merged 2 commits into from Mar 21, 2022

Conversation

abzcoding
Copy link
Member

Description

@otto-gebb can you verify if this fixes your issue or not?

Fixes #2369

lua/lvim/lsp/peek.lua Outdated Show resolved Hide resolved
Co-authored-by: kylo252 <59826753+kylo252@users.noreply.github.com>
Copy link
Collaborator

@kylo252 kylo252 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

this entire function might have to be removed/refactored If #2369 is still broken, since it's relying on internal functions that aren't guaranteed to not change.

@abzcoding abzcoding merged commit 4affc21 into LunarVim:rolling Mar 21, 2022
@otto-gebb
Copy link

@abzcoding I edited the file
~/.local/share/lunarvim/lvim/lua/lvim/lsp/peek.lua, manually making the changes of this PR.

When I'm trying to "peek definition" of a symbol from a library I get the same error, except the line number in the call steck is now 44 (which means the code lines I added took effect).

Error executing vim.schedule lua callback: /home/gebb/.local/share/lunarvim/lvim/lua/lvim/lsp/peek.lua:44: 'width' key must be a positive Integer
stack traceback:
[C]: in function 'nvim_open_win'
/home/gebb/.local/share/lunarvim/lvim/lua/lvim/lsp/peek.lua:44: in function 'create_floating_file'
/home/gebb/.local/share/lunarvim/lvim/lua/lvim/lsp/peek.lua:73: in function 'handler'
/usr/share/nvim/runtime/lua/vim/lsp.lua:964: in function 'cb'
vim.lua:285: in function vim.lua:285

So I would say no, the changes don't fix the bug.

@abzcoding
Copy link
Member Author

abzcoding commented Mar 21, 2022

@kylo252 I think instead of if_nil, we should've used the math.max


@otto-gebb can you test this instead?

opts = vim.lsp.util.make_floating_popup_options(math.max(width, 30), math.max(height,10) , opts)

@otto-gebb
Copy link

@abzcoding with the latest proposed change (math.max) I get a different error:

Error executing vim.schedule lua callback: /home/gebb/.local/share/lunarvim/lvim/lua/lvim/lsp/peek.lua:46: Cursor position outside buffer
stack traceback:
[C]: in function 'nvim_win_set_cursor'
/home/gebb/.local/share/lunarvim/lvim/lua/lvim/lsp/peek.lua:46: in function 'create_floating_file'
/home/gebb/.local/share/lunarvim/lvim/lua/lvim/lsp/peek.lua:72: in function 'handler'
/usr/share/nvim/runtime/lua/vim/lsp.lua:964: in function 'cb'
vim.lua:285: in function vim.lua:285

@abzcoding
Copy link
Member Author

abzcoding commented Mar 21, 2022

@otto-gebb

can you add this after the opts line you added up there?
and get a screenshot or sth

print(vim.inspect(range))
opts = vim.lsp.util.make_floating_popup_options(math.max(width, 30), math.max(height, 10), opts)

@otto-gebb
Copy link

@abzcoding sure. Had to execute :messages to show the print-ed value.
image

@abzcoding
Copy link
Member Author

@otto-gebb sorry to bother you again, but could you add this one as well and get a screenshot

print(vim.inspect(contents)) -- this is the new one
print(vim.inspect(range))
opts = vim.lsp.util.make_floating_popup_options(math.max(width, 30), math.max(height, 10), opts)

@otto-gebb
Copy link

@abzcoding here you go:

image

BTW, using the debug output technique you showed I printed the values of width and height, which are both 0.

@abzcoding
Copy link
Member Author

yeah it seems like nvim_buf_get_line couldn't access the proper buffer content
while at it, can you do a

print(vim.inspect(location))
print(bufnr)

on top of them

@otto-gebb
Copy link

Sure.
image

It looks to me that the LSP cannot find the definition requested, so maybe the correct behavior of LunarVim should be to print an error instead of rendering a popup.

@abzcoding
Copy link
Member Author

Yeah, I agree. We should print an error if LSP cannot find the proper content. Thank you for helping through the debugging process

@abzcoding abzcoding deleted the fix/peek-width branch April 1, 2022 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Peek definition fails in C#
3 participants