Improved neovim floating windows, and misc. other improvements

[subnut]

Please see the commit messages for details

EDIT:
My main goal in this PR is to improve the (currently deplorable) state of the completion-documentation popup provided by vim-lsp. It just so happened that I began by improving the lsp#ui#vim#.....() functions and ended up with a re-write of documentation.vim, so I simply included both 😄

As a (good) side-effect of making the lsp#vim#ui functions relative to the global-grid for neovim, now all popup are accurately positioned, and their positioning does not depend on the current window, splits, tab-line, cmdheight (which is variable in neovim, BTW. If you want, you can make it occupy the whole screen 😬) etc.

[subnut]

@prabirshrestha Please check the failing macOS workflow. The error seems to be unrelated to this PR.

[thomasfaingnaert]

@subnut The failing workflow is just because the latest Neovim nightly build has no artifacts for Mac OS yet, so you can safely ignore it.

[subnut]

@thomasfaingnaert Thanks! 😄

So.... will you merge this? Or are we going to wait for other collaborators' opinions?

[thomasfaingnaert]

It's a bit busy at the moment, so I'd rather have @prabirshrestha, @hrsh7th, or @mattn look at this.

[thomasfaingnaert]

Oops, fixed 😄

[subnut]

New issue discovered.
The completion popup gets frozen. Fix available with me. Implementing soon.

Please see the GIF below

[subnut]

Fixed! See GIF -

[subnut]

@prabirshrestha, @hrsh7th, @mattn , @thomasfaingnaert
Please review and test. I tested all possible cases I could think of.

[prabirshrestha]

@subnut thanks for the PR I will have a look at it during the weekend. Popup has been one of the things i have wanted to fix for quite some time so good to see traction here.

I have also been working on and off on the popup api in vital.vim which you can find at vim-jp/vital.vim#748. there are still some missing feature but already contains majority of the feature. goal is to ship the popup.vim as part of vim-lsp and also other plugins can take advantage of it. There are also few other things I would like to add such as margin/border/padding/title and so on.

You can also find some of my thoughts at vim-jp/vital.vim#747.

as for <silent> free free to send another PR.

[subnut]

@prabirshrestha Yes, I'd seen that PR before. But I think that till that PR gets merged and is eventually integrated in vim-lsp in the future, we should improve the current state of the floating window, right?

As it stands right now (before this PR) the popup (especially the documentation popup, i.e. the on that is triggered on CompleteChanged) is unusable in neovim if you use splits. The vim popups are reasonably accurate/perfect. So, why not the same for neovim?

I think that the correct course of action would be to merge this PR for now, continue working on that API. When in the (near?) future that API is finalized, throw away all the current wrappers and use the new API. 🥳

I believe that there should be one single wrapper/API that is used by all other plugins, so that development can be concentrated in one place. But I think that till that future API arrives, we should improve what we currently have in our hands.

Also, I the <silent> mappings are relevant to this PR because the default algorithm doesn't use screenrow() or screencol(), so the <silent> is irrelevant

[subnut]

@prabirshrestha If you are worried about having to test my algorithm, then rest assured. I have tested it extensively myself. In fact I have spent 3 whole days on testing and perfecting before submitting this PR, and even after submitting, still continue to test and correct any errors in my part (see all those forced updates? 😁 )

EDIT: It may happen that I might have left out some feature/variable from the fresh algorithm implemented for neovim in documentation.vim. In the two force-pushes immediately after this comment, I have included g:lsp_preview_max_height, which I had previously forgotten to consider 😅. In such cases, please inform me by @ing me here or in any other issue.

[prabirshrestha]

we don't have to wait for complete rewrite with vital.vim. This PR should be able to be merged after review. I will spend sometime during the weekend to play with it a bit more. i dont expect the vital.vim Pr to be merged there anytime soon. We can use popup.vim from as part of vim-lsp even before it gets merged but this is a bigger refactor and can be done separately.

In the meantime here is a bug I saw. This exists even before this PR.

lastest vim on windows.

neovim 0.4 on windows.

neovim seems to be single line instead of multiple lines.

[subnut]

In the meantime here is a bug I saw. neovim seems to be single line instead of multiple lines.

Well I tried to implement a workaround for it, but it didn't work. I'll see what I can do over the next week....

EDIT:
If I am able to come up with anything, I shall submit a different PR, not in this PR.

[prabirshrestha]

Got another bug in neovim nightly on linux. NVIM v0.5.0-756-g288f7f855

You will need to use float2nr if you are doing any divisions.

[subnut]

Oops.. OK, will look into this tommorrow..
But could you please check if the documentation is correct? (Last commit)

[subnut]

@prabirshrestha Looking closer to your error message, it says that the error occurred at line 39 of s:show_documentation(), which starts at line 13, so that brings us to line 52, which is -

let l:height = max([&lines - &cmdheight - l:row, &previewheight])

Now, previously I didn't know that the max() and min() functions do not work with floats (:sweat_smile:)
But look carefully, the arguments of the max() are &lines - &cmdheight - l:row and &previewheight
We know that &lines,&cmdheight and &previewheight cannot be floats (try setting them to floats), so that leaves us with l:row

But l:row can only have either 0 or &lines, (see line 42 and 46), as those are the only place where it is defined before the max() at line 52. But neither of 0 and &lines can be floats. So, l:row also cannot be a float !

Also, integer - integer gives us integer.
If all arguments are integers, then why did the max() freak out?

Genuinely confused.

[prabirshrestha]

I debuged a bit further and seems like row and col for v:event in neovim is a float. Had to add manually convert it and it worked.

let a:event['row'] = float2nr(a:event['row'])
let a:event['col'] = float2nr(a:event['col'])

I'm running neovim nightly so not sure if it is a regression in neovim. Value was something like 12.0.

Are you running neovim 0.4?

[subnut]

Are you running neovim 0.4?

Yep!

[subnut]

@prabirshrestha Please check if the latest commit fixes that issue.

[prabirshrestha]

would be good if you could also file a bug in neovim. It would be good if float2nr was fixed in s:complete_done() so that s:show_documentation can use a:event. hopefully if neovim fixes it before their 0.5 release we could then remove this hack if 0.4 doesn't have this bug.

[subnut]

Hm... Ok. I will open an issue in neovim, but as I don't have first-hand experience regarding this issue, I will mention you there, and you have to provide the details there. Do you agree?

Or maybe, in my opinion, since you experienced this issue first-hand, you should file the issue, don't you think?

EDIT:
maybe this is a feature? So that GUIs can support external floating popups?

[subnut]

@hrsh7th I removed the <silent> from the mappings. I had originally added them because otherwise the neovim floating windows malfunctioned. But now, it seems they don't malfunction even if <silent> is not added.

Could you please test?

[hrsh7th]

@hrsh7th I removed the from the mappings. I had originally added them because otherwise the neovim floating windows malfunctioned. But now, it seems they don't malfunction even if is not added.

Could you please test?

OK. I will test it.

[hrsh7th]

I think it almost ready to merge.

I think the remaining consideration points are debouncing by default or not.

[subnut]

@prabirshrestha Is the documentation of g:lsp_documentation_debounce OK?

[subnut]

I think it almost ready to merge.

I think the remaining consideration points are debouncing by default or not.

@hrsh7th
The remaining consideration point has been resolved.
Now ready to merge?

[hrsh7th]

I think it ok to merge.

[subnut]

@prabirshrestha Who will merge this?

[hrsh7th]

I will if all requested changes are resolved.

[subnut]

@hrsh7th All requested changes have already been resolved, IIRC
If not, please show me which one hasn't been resolved. I will resolve it immediately.

[subnut]

I repeat, Are there any unresolved requested changes? If yes, please show me which one.

[hrsh7th]

I mean the PR still has request changes status.

But I think it OK to merge (I checked all inline comments already resolved)

[prabirshrestha]

Thanks everyone. Great change.

[mattn]

I don't make sure that it is related on this PR but current implementation show documentation of candidates in popup in broken location.

[subnut]

@mattn I am sure this is un-related, because I did not touch the completion at all
I only touched documentation popup.

Also, if you are on vim, then there should not have been any change at all. This PR only changed neovim behaviour.

[subnut]

@mattn Did you manage to fix the above issue? If yes, how,?

[mattn]

Sorry, it was already fixed. a56304f