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

Add LspPeekDefinition #412

Merged
merged 34 commits into from
Jul 21, 2019

Conversation

thomasfaingnaert
Copy link
Collaborator

Adds :LspPeekDefinition, which displays the definition in the preview window, similar to VSCode's peek definition.

Fixes #411

@prabirshrestha
Copy link
Owner

Can you make use of the changes that came in this PR which just got merged. #395

@thomasfaingnaert
Copy link
Collaborator Author

thomasfaingnaert commented Jun 26, 2019

I pushed a basic version that works using:

  • preview:
    preview

  • Vim popups:
    vim popup

  • Neovim floats:
    neovim float

I just need to implement alignment in Vim popups and clean the code up a bit as well.

@prabirshrestha
Copy link
Owner

@jerdna-regeiz could you have a look at this PR first.

Copy link
Contributor

@jerdna-regeiz jerdna-regeiz left a comment

Choose a reason for hiding this comment

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

Nice!
I'll love that feature.
Still have some remarks (other than the already TODO-comments from yourself).

autoload/lsp/ui/vim/output.vim Outdated Show resolved Hide resolved
autoload/lsp/ui/vim/output.vim Outdated Show resolved Hide resolved
autoload/lsp/ui/vim/output.vim Outdated Show resolved Hide resolved
ftplugin/lsp-hover.vim Outdated Show resolved Hide resolved
plugin/lsp.vim Show resolved Hide resolved
@jerdna-regeiz
Copy link
Contributor

When win_execute is implemented in nvim we should use that instead of switching focus multiple times (maybe we could even manage without switching at all). But for now I have found no other solution as well :( Variables could be set with nvim_win_set_{var,option}, but I have no alternative for calling zz and the like for adjustment.

@thomasfaingnaert
Copy link
Collaborator Author

@jerdna-regeiz Thanks for reviewing my PR! Regarding my TODO, I tried win_execute with normal! zz and friends, and that doesn't seem to work. Do you know a way to do the alignment in Vim? At the moment, only setting the firstline seems to work, but that's kind of annoying to use because you would have to know the window height in advance, and calculate the position based on that. (At least for center and bottom)

@jerdna-regeiz
Copy link
Contributor

@thomasfaingnaert Have you tried to set the option afterwards with popup_setoptions like when the resize is done for nvim, because the size/content is known then?
We should up the version check to patch 8.1.1561 for popup_setoptions to work.

@thomasfaingnaert
Copy link
Collaborator Author

There's also quite a bit of complex conditionals, which I'm not a big fan of. Perhaps we should set 3 boolean script variables s:use_preview, s:use_vim_popup, s:use_neovim_float instead of always having to type things like

if s:supports_floating && g:lsp_preview_float && !has('nvim')

@jerdna-regeiz
Copy link
Contributor

There's also quite a bit of complex conditionals, which I'm not a big fan of. Perhaps we should set 3 boolean script variables s:use_preview, s:use_vim_popup, s:use_neovim_float instead of always having to type things like

I agree, but it could be done in a separate PR to not mix a new feature with refactoring.

@thomasfaingnaert
Copy link
Collaborator Author

Tried fixing Vim alignment of the popup window in ccaf0f9, but I'm facing one more issue: l:height is 1, as if the popup still contains ..., even though the content should already be set by then, right?

@thomasfaingnaert
Copy link
Collaborator Author

thomasfaingnaert commented Jun 27, 2019

Seems like using a timer and specifying the width and height explicitly works.

Top:
top

Center:
center

Bottom:
bottom

Remaining problems:

  • There are artifacts of a bigger popup window Fixed using redraw!
  • The logic breaks down when the popup window is above the cursor Fixed

Add LspPeekImplementation, LspPeekTypeDefinition and LspPeekDeclaration.
@thomasfaingnaert
Copy link
Collaborator Author

Added Peek commands for implementation, type definition and declaration. I have not been able to test these, because I don't have a language server that supports these, but they should work the same as LspPeekDefinition.

As for LspReferences, I'm not sure if it would make sense to use the popup window for those, because you have to interact with the quickfix list to go through the list of references?

@clason
Copy link
Contributor

clason commented Jul 15, 2019

I think it's fine as is -- if somebody comes up with a use case for PeekReferences (I don't see one either), that could be added in a future PR. LGTM!

@prabirshrestha
Copy link
Owner

@thomasfaingnaert This has merge conflicts. Can you fix it?

@thomasfaingnaert
Copy link
Collaborator Author

@prabirshrestha Fixed merge conflicts.
I'm not sure what changed in the meantime, but Vim's floating window isn't positioned well anymore: it seems to place the preview above the cursor even when there are only 2 lines available above:
image
I fixed this in 18fcb43 by forcing the popup above/below the cursor depending on if the cursor is in the first or second half of the screen.
Should I change this, or do we leave it as is?

@prabirshrestha
Copy link
Owner

@thomasfaingnaert Current behavior is good.

I had also sent you an email but haven't got any reply. Can you send me an email. You can find my email at https://github.com/prabirshrestha

@prabirshrestha prabirshrestha merged commit 813ea77 into prabirshrestha:master Jul 21, 2019
@thomasfaingnaert thomasfaingnaert deleted the peek-definition branch July 21, 2019 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "peek definition"
4 participants