[WIP] Refactor neovim floats

[clason]

The purpose of this PR is to avoid entering the buffer opened with nvim_open_win, which triggers -- possibly very expensive -- CursorMoved autocommands on the original buffer, see #507.

This PR also contains commits from #500 and #507 in order to easily test the Documentation behavior; it should be removed before merging.

This is not quite working yet:

1. Documentation preview contents are right-aligned, so that the beginning rather than the end of long lines are truncated. (That's an issue with documentation.vim trying to put the float on the left side of the pum but failing for some reason.)
2. Hover doesn't show if triggered on the first line of the buffer.
3. PeekDefinition doesn't show for if the target range is too large, and if the buffer contains unsaved changes, these are not picked up.

@jerdna-regeiz @thomasfaingnaert I would appreciate feedback, since the interaction of the functions in output.vim isn't quite clear to me.

[thomasfaingnaert]

Now that we're not focusing the float, this would apply to the main buffer, I believe.

[clason]

Ah, you're right. Didn't notice that. How weird...

But even without that line, I get strange styling of the main buffer whenever a float is open (color changes in the status bar).

[thomasfaingnaert]

Hover doesn't show if triggered on the first line of the buffer.

What do you mean by this? It seems to work fine on my end:

[thomasfaingnaert]

Maybe we should also consider getting rid of the "adjust size" functions, and only have a function "set content" that sets the content, and sets the size automatically (just like Vim floats behave).

[clason]

Weird, doesn't work for me (with texlab), even with

\begin{equation}\label{eq:1}
    1+2=2                                                               
\end{equation}

Maybe we should postpone this until v0.4.0 is released and there is a (semi-)stable API to target (and compare).

[clason]

Yeah, there's too many intersecting code paths currently depending on the LSP function. I don't know the difference between half of these functions... Might be good to modularize it.

Maybe also separate vim, neovim and preview and hide the different implementations behind a higher-level API: have output.vim basically just contain "open float here with this buffer", "update buffer contents", "close float" (anything else?) and then an output directory with utils.vim (common functions), popup.vim, float.vim, and preview.vim, containing the different implementations (maintained by different people).

@hrsh7th You're also currently working on float-related stuff

[thomasfaingnaert]

Yeah, might be a good idea to cleanup output.vim a bit before we rely on it for documentation and other features.

[thomasfaingnaert]

Would you be OK with turning this PR is a general "refactor output.vim" PR?
We can have a discussion on how to API should look like here.

[clason]

I would be OK with that; unless there is someone better suited for actually implementing it? (Is there a way to allow others to push to this branch?)

[thomasfaingnaert]

Not that I know of, but I've added your fork as remote, so I can branch off from your branch and git fetch --all && git merge clason/nvim-floats, and you can do something similar.

[thomasfaingnaert]

You can also add them as collaborators to your fork, but then they'll be able to push to all branches, which may or may not be what you want.

[clason]

Sounds good. This PR probably ends up being junked anyway and replaced with a clean proposal for merging.

[thomasfaingnaert]

Things would be a lot simpler if nvim floats mirrored Vim's API somewhat (or vice versa) ;)

[clason]

I trust you ;) Maybe that's more convenient.

[thomasfaingnaert]

Probably good though to precisely document which functions will be available before starting, and what their parameters should be.

For the creation of the buffer+float, does it make sense to have that in the common API, because all features will have a different way of doing that: hover/peek def will want to create one located at the cursor, documentation on Vim will want to use the Vim float (and not open one itself), documentation on Neovim will create one manually placed next to the completion menu. (At least until completeopt+=popup is merged).

[clason]

One possibility for the API would be to first define a lower-level API that mirrors what you would like to have, and then for vim just pass it through while for neovim we'd try to implement the same behavior with their (even lower-level) API -- if they end up including higher-level APIs, too, we then just replace them.

Probably good though to precisely document which functions will be available before starting, and what their parameters should be.

Yes, we should decide on an LSP-centric high-level API that actually gets called from outside (and if we find a new situation, a new high-level function should be created instead of trying to directly use low-level calls).

[thomasfaingnaert]

As a start (basically what we need from Vim's API):

• popup_create({what}, {options}): Open a popup showing {what} (buffer number, string, string[])
  options is a dict, can cointain options from :h popup_create-arguments (though only the ones we need)
  Returns the window ID of the float/popup.

For now, I think we need line, col, pos, maxwidth, firstline, moved.

• popup_close({winid}): Closes the popup with the given ID. (Do we actually need this? Popups should close when the cursor moves, though I guess we need it for Neovim floats and pressing ESC inside them).

• poup_set_content({widid}, {data}): Set the content of the popup with given ID to {data}. {data} can be anything returned from the language server: String, String[], MarkedString, MarkedString[], MarkupContent

Something like this?

[clason]

Probably also style (plaintext vs. markdown)?

What about wrapping?

popup_close probably counts as intermediate-level that need not be exposed.

(Neovim plans to add higher-level APIs in the 0.5.0 cycle. One plan is to add functions for "ballooneval-as-floats", which seems to be half of what we need here, the other half being completeopt.)

[thomasfaingnaert]

Well, for Vim wrap is true by default, so I haven't included it. (The idea is that for Neovim, we would implement wrap ourselves if needed)

[thomasfaingnaert]

For the style: this would be obtained from the {data} to popup_set_content.

[thomasfaingnaert]

Is there an approximate release date for 0.5.0 actually?

[clason]

So wrap would be default -- is there a situation for which we don't want this?

(No release date for 0.5.0, they're still working on 0.4.0)

[thomasfaingnaert]

I can't really think of a situation where you wouldn't want to wrap.

[thomasfaingnaert]

Let's maybe also discuss the opening and setting content: would it make sense to have the open function not take a {what} argument, but just do the creation of a buffer, floating window, and positioning (but not sizing) based on its {options}?
Then, the sizing would happen in set_content.

[thomasfaingnaert]

Pushed small basic vimdoc, feel free to expand.

[clason]

Looks good! Comments, for discussion:

1. API contract should only cover top-level API, others might change as needed. (Meaning vim-lsp can rely on the API stability only for the top level, and should avoid calling intermediate level or below).
2. With this in view, I'd promote parse_lsp_response to top-level (and possibly move it to `vim/utils)?
3. If just want syntax highlighting (or none) according to filetype is wanted, passing an empty [] for syn_ranges is probably easiest? Should be mentioned, then.
4. options should be options for the tooltip, not the content, presumably. This would be one place to specify {'close':{'on_move',...}} -- or other way around {'on_move':'close'/'follow'}? Also, noborder for vim popup (or border if noborder is made default)?

[thomasfaingnaert]

1. API contract should only cover top-level API, others might change as needed. (Meaning vim-lsp can rely on the API stability only for the top level, and should avoid calling intermediate level or below).

Yes, the API will be strictly hierarchical, I just added it in the documentation to have a quick overview, as a sort of summary of our discussion, I guess. The other layers should be removed later.

2. With this in view, I'd promote `parse_lsp_response` to top-level (and possibly move it to `vim/utils)?

Yeah, I basically regard it as "top-level, but LSP specific", i.e. we would have a function that calls parse_lsp_response, and uses that return value for the top level, non-lsp specific API. (Separated it just if we decide to extract this later).

3. If just want syntax highlighting (or none) according to filetype is wanted, passing an empty `[]` for `syn_ranges` is probably easiest? Should be mentioned, then.

Sure, I'll add that.

4. The `data` fields are `content` and `syn_ranges`, right -- unless there are better names? I'd mention that explicitly, then.

For hover and documentation, you would call parse_lsp_response with the data you get from the language server, and pass the return value as {lines} and {syn-ranges} to the top-level API. For peek definition, you would just read the correct range and not call parse_lsp_response. Feel free to suggest better names, though.

[jerdna-regeiz]

Nice work =)
When working out a more high level api for the popup, keep in mind: there are features in both vim and neovim that the other just does not have at all. Like focus the popup or borders.

positioning at the cursor (as anchor) was already possible when I wrote it. It did not make the code any easier, as the size and position (above/below and left/right) calculation requires the same information (in fact it was some how uglier).

I would not recommend leaving the old preview behind as by far not everybody has a recent vim or neovim at hand.

Still have to look at the code itself, I liked what I saw at a first glance.

[thomasfaingnaert]

@jerdna-regeiz
I think the code itself is put on hold for a bit :)
I definitely would not want to abandon preview, but it's really different from float, so we decided to ignore it a little bit.
Probably worth revisiting that now, though, before expanding on the API too much.
Feel free to suggest improvements as well, we really need feedback.

@clason Maybe adding @jerdna-regeiz as a collaborator would be good as well?

[thomasfaingnaert]

When working out a more high level api for the popup, keep in mind: there are features in both vim and neovim that the other just does not have at all. Like focus the popup or borders.

That's also something that needs discussing. I assume we still want to keep the doubletap functionality, which is only possible in Neovim at the moment.
For borders, theoretically we can create a second float behind the first to emulate a border, but I think that's overkill; we better wait until neovim upstream handles borders.

[clason]

Yes, the API will be strictly hierarchical, I just added it in the documentation to have a quick overview, as a sort of summary of our discussion, I guess. The other layers should be removed later.

Can be kept, because more documentation = more better, just with a caveat.

Yeah, I basically regard it as "top-level, but LSP specific", i.e. we would have a function that calls parse_lsp_response, and uses that return value for the top level, non-lsp specific API. (Separated it just if we decide to extract this later).

Makes sense 👍

For hover and documentation, you would call parse_lsp_response with the data you get from the language server, and pass the return value as {lines} and {syn-ranges} to the top-level API. For peek definition, you would just read the correct range and not call parse_lsp_response. Feel free to suggest better names, though.

That comment was fired from the hip, ignore that. It's fine as is.

@jerdna-regeiz

When working out a more high level api for the popup, keep in mind: there are features in both vim and neovim that the other just does not have at all. Like focus the popup or borders.

Good point; these things will go in options, and the backend just ignore options it can't handle (yet). I actually prefer no borders to vim's massive Great Walls, but YMMV. (Nested floats look neat, but are not worth the effort with resizing/positioning).

positioning at the cursor (as anchor) was already possible when I wrote it. It did not make the code any easier, as the size and position (above/below and left/right) calculation requires the same information (in fact it was some how uglier).

True; the row/col need to be fudged a bit. Hopefully the high-level API of vim and neovim will converge a bit, and thus it makes sense to try to use their features as much as possible (within reason, of course).

I would not recommend leaving the old preview behind as by far not everybody has a recent vim or neovim at hand.

I had assumed that people who work with language servers are not upgrade averse, but may be wrong, of course. The problem is that preview only makes sense for signature help and documentation, while hover and peek don't. In particular, signature is a tooltip feature for popup and float, but a documentation-like feature for preview. That makes everything hard (but not impossible) to separate cleanly. But the current proposal should make it easy to add new backends, including preview -- that was one of the points! (I'd just prefer not to have to worry about that at first.)

I think the code itself is put on hold for a bit :)

Yes, the current code is garbage. :)

@clason Maybe adding @jerdna-regeiz as a collaborator would be good as well?

Done!

[thomasfaingnaert]

And yes, the current code is garbage.

Keep in mind you're speaking to the person who wrote it :p

[clason]

No, I meant the current code in this PR! The current code on master is working fine, after all, in contrast to the one I knocked out :)

(Edited the comment to make this a bit clearer.)

[thomasfaingnaert]

4. `options` should be options for the _tooltip_, not the content, presumably. This would be one place to specify `{'close':{'on_move',...}}` -- or other way around `{'on_move':'close'/'follow'}`? Also, `noborder` for vim popup (or `border` if `noborder` is made default)?

That's the plan. Can you add the options to the doc (and any additional ones, if you think of them).

(As an aside; once the academic year resumes next week, I'll probably not have much time anymore. I really hope this gains some traction so I don't have to miss the progress :) )

[clason]

That's the plan. Can you add the options to the doc (and any additional ones, if you think of them).

Will do -- once I come to a conclusion on which way around ;) @jerdna-regeiz Can you think of any?

(As an aside; once the academic year resumes next week, I'll probably not have much time anymore. I really hope this gains some traction so I don't have to miss the progress :) )

We still have four more weeks, but these will be very busy weeks for me -- moreso as I've been procrastinating here a bit ;) So the same goes for me, although I'll try to not let it go completely dormant.

[thomasfaingnaert]

We still have four more weeks, but these will be very busy weeks for me -- moreso as I've been procrastinating here a bit ;)

I noticed. Now I'm actually wondering if my professors work on their Vim config during the holidays ;)

[clason]

If their definition of a holiday is "a slow week at the office, and a grant proposal to write", possibly -- unless they use Emacs. (Or, more likely, TeXShop.)

[clason]

closed in favor of #510 to focus on the API design.

@thomasfaingnaert @jerdna-regeiz @hrsh7th

(To avoid littering that issue with too many back-and-forth comments, some discussion could be moved to gitter -- a vim-lsp chatroom might be useful.)

[thomasfaingnaert]

@clason Haven't used Gitter before. Can I create the room or must the repo owner do that?

[clason]

Good question! You could simply try?

[thomasfaingnaert]

Seems like I can create a community, but if I want to link it to the GitHub repo, I need to own it.
Let's hear @prabirshrestha's thoughts first

[clason]

Makes sense, although that would only get the fancy feed and linking to issues/PRs -- for discussion, it doesn't have to be linked. (I'm worried of having 100+ comments on the new PR in the first day already -- I tend to think by writing ;))

[thomasfaingnaert]

Also seems that I can't create a community with the name vim-lsp (I guess because a GitHub organisation with the same name already exists: https://github.com/vim-lsp)

[clason]

Ah. In this case, it would indeed be better for @prabirshrestha to do this (and maybe move the vim-lsp(-related) repo(s) to this organization)?

(If this is in fact his organization and not someone else's...)

[thomasfaingnaert]

I think it is someone else's, but still gitter.im/prabirshrestha/vim-lsp is probably better than gitter.im/vim-lsp-and-some-random-suffix-i-came-up-with

[thomasfaingnaert]

Let's just create a gitter for your fork for now, might be easier?

[clason]

Then it might be possible (and reasonable) to appropriate it; I think github has procedures for that.
https://help.github.com/en/articles/github-username-policy

Otherwise I agree; but I can definitely create a temporary one for this PR. I don't have an organization, but I just named it clason-vim-lsp. It'll be deleted later anyway.

[prabirshrestha]

Even I find Gitter easier to use. Let me know what name you want. It would be good to have a permanent one.