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

preview information shown in hover at the cursor #395

Merged
merged 11 commits into from
Jun 25, 2019

Conversation

jerdna-regeiz
Copy link
Contributor

Uses floating window support of nvim 0.4.0 to show hover information in an unobtrusive way.

Implemented it and then found #347 . While it is good work I did create a pull request anyway as I saw some flaws in other request.

  • It contains some code duplication ("rendering text")
  • It does remap globally and then unmaps it
  • It is quite obstusive as it grabs focus
  • It uses quite some script globals

Thought about commenting on #347, but the last activity has been some time.

My approach does not aim at only hover, but at preview (yeah I know only hover uses it).
It is Implemented such that most of the code for preview is used, including "text rendering",
filetype/syntax and the option for keeping focus.

If the hover is not focused, it is closed on cursor move. If it is focused, it may be closed with
, which is only mapped in the scratch buffer.
This makes it a lot less obstusive and does not intrude the workflow.

As #347 seems to stand still currently, I offer to create any fusion of the two approached which
works best and fits the most with vim-lsp. So any comment is welcome.

@prabirshrestha
Copy link
Owner

Definitely seems to be better. Here are some of the things we need to think of.

  1. Would be good if the width was bigger since I do have quite lot of space to render. For some it does take proper width.
    image
  2. Add support for custom mapping so that we can close and focus on the float window. There are times when I would want to copy from docs.
  3. When in focus on the float window and move mouse it shouldn't be closed as I would like to select it.

Not required for this PR but something that we should think of if possible. These are big items so ok to not have it now.

  1. vim8 should behave in similar fashion
  2. use floatingpreview for more than just hover such as autocomplete, diagnostics.

@jerdna-regeiz
Copy link
Contributor Author

1. Would be good if the width was bigger since I do have quite lot of space to render. For some it does take proper width.

Strage, I'll have to look into it as width and height are determined by first using all available space and then shrinking it to the actually required space.

2. Add support for custom mapping so that we can close and focus on the float window. There are times when I would want to copy from docs.

Already thought about that myself! Something like (lsp-focus-hover) or (lsp-hover-focus)?
I'll try to implement it such that if the preview/hover is already closed it will open up again as well as focus.
Alternatively one could reuse the hover binding to focus, if the preview is still open.

3. When in focus on the float window and move mouse it shouldn't be closed as I would like to select it.

It does stay open when using the mouse if in focus. At least for me =(

Not required for this PR but something that we should think of if possible. These are big items so ok to not have it now.

1. vim8 should behave in similar fashion

This is currently not possible as far as I can see, as floating windows is a neovim unique feature.

2. use floatingpreview for more than just hover such as autocomplete, diagnostics.

Diagnostics is a nice idea! As soon as I worked out the quirks with this PR, I'll work on that.
Depending on the used completion engine this is already possible to have floating autocomplete preview. I use ncm2 with https://github.com/ncm2/float-preview.nvim

@jerdna-regeiz
Copy link
Contributor Author

Integrated vim8.1 popup feature (thanks to @mattn).

vim8.1 popups are very much crippled in comparison to neovim floats, as there is no focus and/or keyinput available for popup-windows. Therefore keeping focus and copy using the keyboard and scrolling won't be possible in the current implementation of popup-windows.
Further one has to keep a keen eye on this feature, as the documentation states "THIS IS UNDER DESIGN - ANYTHING MAY STILL CHANGE".
But(!) they already support borders! =)

Features like keeping it open and having dedicated closing mappings will be possible for both vim8 and nvim.

@jerdna-regeiz
Copy link
Contributor Author

Implemented focus and close for floating nvim windows. Vim8.1 still gives me quirks for close (telling me the window is no popup window...), which have to be investigated further.

Added an option to disable automatic closing of floating windows (nvim only currently, when I fix above mentioned bugs, I'll do that for vim8.1 as well)

Copy link
Owner

@prabirshrestha prabirshrestha left a comment

Choose a reason for hiding this comment

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

Looking great and seems to work both in vim8 and neovim. @mattn Can you also have a look at this PR.

  1. Can we request vim to add support for focusing on popup so I can navigate on large hovers? Otherwise it doesn't become so useful for these scenarios. My primary reason for this is because I do work on lot of other projects and it is very useful scroll through the docs inline in the editor. One workaround is to have a binding for floating window to force preview window.
    image
  2. I don't use neovim as the primary editor but would be good to have request on neovim to add support for borders.

As for autocomplete I don't think we need to worry about it yet. Hopefully completeopt+=popup will solve this (vim/vim#4544) as I definitely don't want to be having two codepath to support both vim-lsp omnifunc and asyncomplete.vim

autoload/lsp/ui/vim/output.vim Outdated Show resolved Hide resolved
doc/vim-lsp.txt Show resolved Hide resolved
@prabirshrestha
Copy link
Owner

I personally would like to map esc and ctrl-c to close preview window. Would it be possible to have an autocmd for popup open and close so I can bind and unbind it? We should also document this. Find it a bit weird to move cursor to close popup.

@jerdna-regeiz
Copy link
Contributor Author

I personally would like to map esc and ctrl-c to close preview window. Would it be possible to have an autocmd for popup open and close so I can bind and unbind it? We should also document this. Find it a bit weird to move cursor to close popup.

autocmd sounds great. Implemented.

@jerdna-regeiz
Copy link
Contributor Author

1. Can we request vim to add support for focusing on popup so I can navigate on large hovers? Otherwise it doesn't become so useful for these scenarios. My primary reason for this is because I do work on lot of other projects and it is very useful scroll through the docs inline in the editor.  One workaround is to have a binding for floating window to force preview window.

Guess we should open a request. Could not find something similar in a quick scan.

2. I don't use neovim as the primary editor but would be good to have request on neovim to add support for borders.

There is already a feature request for neovim to implement it (and when I remember correctly somebody is working on it). Have to search for it later.

@prabirshrestha
Copy link
Owner

@jerdna-regeiz could you resolve the merge conflicts?

@jerdna-regeiz
Copy link
Contributor Author

jerdna-regeiz commented Jun 24, 2019

@jerdna-regeiz could you resolve the merge conflicts?

sure. Had to first figure out why closing a popup was a problem in vim 8.1...
Edit: rebased

@jerdna-regeiz
Copy link
Contributor Author

Added a imo cool feature to have a doubetap on the preview.
In case the preview function is called twice with the same data, it will execute a configured function.
Default is to focus the preview window.

@jerdna-regeiz
Copy link
Contributor Author

Is there anything that I should try to include into this pull request?

Following features are still in my mind, but I guess they should be handled in a separate PR:

  • Borders for nvim: there is no official support for it yet, but it can be done "hackish" by using two overlapping windows with one slightly bigger.
  • Floating info for diagnostics: by trigger, or automatic. Should be straight forward.
  • Finegrained autoclose: configure to close on x and/or y cursor movement if autoclosing
  • Reopen preview: reopen a preview with the same information (like when checking parameters again)
  • smart hover: reopen previous preview if triggering hover and no information for current item (again like when writing parameters)

@prabirshrestha prabirshrestha merged commit 1790680 into prabirshrestha:master Jun 25, 2019
@prabirshrestha
Copy link
Owner

This is great. Thanks for the PR. It is not merged in master.

@edganiukov
Copy link

Floating window for neovim always located top left corner, is it by design or a bug?

@clason
Copy link
Contributor

clason commented Jun 26, 2019

Great feature, thanks! One question: Is it meant/possible to have multiline boxes with automatic linewrapping? One language server returns a single, long, line on a specific hover, and most of that is truncated now.

@thomasfaingnaert
Copy link
Collaborator

@clason If you're using Vim, something like this should work:

diff --git a/autoload/lsp/ui/vim/output.vim b/autoload/lsp/ui/vim/output.vim
index 7b52f4c..ebcdfeb 100644
--- a/autoload/lsp/ui/vim/output.vim
+++ b/autoload/lsp/ui/vim/output.vim
@@ -116,6 +116,7 @@ function! lsp#ui#vim#output#floatingpreview(data) abort
     let s:winid = popup_atcursor('...', {
         \  'moved': 'any',
 		    \  'border': [1, 1, 1, 1],
+                    \  'maxwidth': 80
 		\})
   endif
   return s:winid

I'll see if I can create a PR for this as well.

@clason
Copy link
Contributor

clason commented Jun 26, 2019

I'm using neovim (latest master), sorry, should have mentioned this.

@thomasfaingnaert
Copy link
Collaborator

@clason I think Neovim uses a different API, so you may want to check the help. I'm actually compiling Neovim at the moment, so I'll be able to check in couple minutes.

@thomasfaingnaert
Copy link
Collaborator

@clason
For Neovim, this works:

diff --git a/autoload/lsp/ui/vim/output.vim b/autoload/lsp/ui/vim/output.vim
index 7b52f4c..7e32491 100644
--- a/autoload/lsp/ui/vim/output.vim
+++ b/autoload/lsp/ui/vim/output.vim
@@ -99,6 +99,7 @@ function! lsp#ui#vim#output#floatingpreview(data) abort
     " Try to get as much pace right-bolow the cursor, but at least 10x10
     let l:width = max([s:bufwidth(), 10])
     let l:height = max([&lines - winline() + 1, 10])
+    let l:width = min([l:width, 80])
 
     let l:opts = s:get_float_positioning(l:height, l:width)

@jerdna-regeiz
Copy link
Contributor Author

Floating window for neovim always located top left corner, is it by design or a bug?

Bug. Should be at the cursor. Can you provide a minimal config where it happens, as I can't reproduce it.

@jerdna-regeiz
Copy link
Contributor Author

Great feature, thanks! One question: Is it meant/possible to have multiline boxes with automatic linewrapping? One language server returns a single, long, line on a specific hover, and most of that is truncated now.

Neovim uses real buffers, thus we can enable soft line wrap in the buffer/window.
Habe a look at the lsp_float_open event and lsp#ui#vim#output#getpreviewwinid().
With that you can set what ever option you want in the buffer.

@clason
Copy link
Contributor

clason commented Jun 26, 2019

No, sorry, that just shifts the floating window to the right?

From the documentation (:h api-floatwin):

let buf = nvim_create_buf(v:false, v:true)                                             
    call nvim_buf_set_lines(buf, 0, -1, v:true, ["test", "text"])
    let opts = {'relative': 'cursor', 'width': 10, 'height': 2, 'col': 0,
        \ 'row': 1, 'anchor': 'NW'}
    let win = nvim_open_win(buf, 0, opts)

@edganiukov
Copy link

edganiukov commented Jun 26, 2019

Floating window for neovim always located top left corner, is it by design or a bug?

Bug. Should be at the cursor. Can you provide a minimal config where it happens, as I can't reproduce it.

Found that let g:lsp_preview_keep_focus = 0 cause the issue.
With let g:lsp_preview_keep_focus = 1 all good.

@jerdna-regeiz
Copy link
Contributor Author

Found that let g:lsp_preview_keep_focus = 0 cause the issue.
With let g:lsp_preview_keep_focus = 0 all good.

Ahh have not checked that for quite some time. I'll try it later

@thomasfaingnaert
Copy link
Collaborator

@clason Hmm, I don't have a language server with sufficiently long hovers, so I tested with max_width = 10 in clangd and that appeared to work. I'll try again with longer lines. Might be better to create a new issue for this and continue the discussion there.

@clason
Copy link
Contributor

clason commented Jun 26, 2019

I tested maxwidth=10 as well; this truncates correctly, but doesn't wrap (or maybe it does, the point is that it's still a single line high). But I'll open a new issue, no problem.

(The vim fix works, by the way.)

@thomasfaingnaert
Copy link
Collaborator

@clason Can you try #416?

@clason
Copy link
Contributor

clason commented Jun 26, 2019

Yes, that works nicely, thank you very much!

@jerdna-regeiz
Copy link
Contributor Author

Found that let g:lsp_preview_keep_focus = 0 cause the issue.
With let g:lsp_preview_keep_focus = 1 all good.

Fixed in #419

@prabirshrestha
Copy link
Owner

#419 is merged.

@clason
Copy link
Contributor

clason commented Sep 13, 2019

@jerdna-regeiz I'm trying to refactor your code to avoid entering the buffer on float creation, and I'm running into a few issues. Do you have time for talking about this a bit?

@jerdna-regeiz
Copy link
Contributor Author

Sure. What questions do you have?
Thougt about it as well, but decided against it to have the same behaviour for preview as well as hover. But then one could change that as well :)
I had some issues as well, but can't remember which.

@clason
Copy link
Contributor

clason commented Sep 13, 2019

That's pretty much it; my changes break hover (but only on the first line of the buffer) and preview (if the preview contains too many lines). Maybe I'll create a WIP preview and ask for your suggestions?

@clason
Copy link
Contributor

clason commented Sep 13, 2019

@jerdna-regeiz my WIP PR is at #509, would appreciate any comments.

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.

5 participants