Allow user to send textDocument/hover with range if needed

[ayoub-benali]

This a better alternative to scalameta/metals-sublime#45 and this implementation is just a workaround until microsoft/language-server-protocol#377 is part of the LSP spec

On the LSP-* packages the users just have to set {"keys": ["ctrl+h"], "command": "lsp_hover", "args": {"use_selection": true}} a key binding

[ayoub-benali]

There could be already a pop due a hover which is in a different location from the selection. So we should close it to show the new popup at the right place

[rchl]

I would much prefer the solution I've mentioned in the metals repo.

With my suggestion everything would work without any extra code in the metals package (apart for the override to enable a flag) and without accessing (semi) internal stuff.

With changes here, things still won't work automatically on mouse hover.

[rchl]

And since that feature has a high chance of getting into the spec (it seems), it would be very simple to just remove the flag later and have the feature enabled by default if it's done properly from the start.

[ayoub-benali]

I think from the LSP-* side it doesn't really matter if there is a need to send an explicit argument with the command or setting some flag. What ever works best for you and @rwols

With changes here, things still won't work automatically on mouse hover.
I think changes are needed here

LSP/plugin/documents.py

Line 415 in c1e25f0

if hover_zone != sublime.HOVER_TEXT or self.view.is_popup_visible():

no matter the approach, no ?

[rchl]

What do you think has to be changed here? I would think we can always send a point to lsp_hover and we could figure out later if that's over a selection or not.

[ayoub-benali]

I thought the check would be done there before calling lsp_hover. I guess you want check to be done after the fact.

[rwols]

Why can't we send the range as well as position in all cases?

{
  "textDocument": "...",
  "position": {"line": 0, "character": 0},
  "range": {"begin": {"line": 0, "character": 0}, "end": {"line": 1, "character": 0}}
}

or would this break some servers?

[ayoub-benali]

or would this break some servers? I think so yeah

[rwols]

This client puts the range in the "position" field for rust-analyzer :/ fannheyward/coc-rust-analyzer#839

[ayoub-benali]

That is somehow worse 😅

[predragnikolic]

There is no TextDocumentRangeParams in the lsp spec,
so I would move the TextDocumentRangeParams type from protocol.py to plugin/core/views.py

[predragnikolic]

I went through @rchl suggestion, and I think it is ok.

    def request_symbol_hover_async(self, listener: AbstractViewListener, point: int) -> None:
        hover_promises = []  # type: List[Promise[ResolvedHover]]
        document_position = text_document_position_params(self.view, point)
        for session in listener.sessions_async('hoverProvider'):
+            hover_supports_range = session._plugin.get_experimental_flags().get('hover_supports_range', False)
+            if hover_supports_range:
+                hover_range = None
+                for region in self.view.sel():
+                    if region.contains(point):
+                        hover_range = region_to_range(self.view, region).to_lsp()
+                if hover_range:
+                    document_position['position'] = hover_range
            hover_promises.append(session.send_request_task(
                Request("textDocument/hover", document_position, self.view)
            ))

@rchl is this what you imagined?

[rchl]

Yep. Only with a getter for get_experimental_flags so that we don't access private member but yes.

[ayoub-benali]

So you all prefer adding a def get_experimental_flags(self): dict: to AbstractPlugin ?

[rwols]

Why can’t this be a server capability? Then we don’t need any new methods on AbstractPlugin

[ayoub-benali]

That would be better indeed. Let's ask @ckipp01 and @tgodzik.
Could Metals add a flag under experimental that explicitly convey to the client that Metals accepts ranges for textDocument/hover. This flag could be removed once it is part of the official Spec.

[ckipp01]

I would much prefer the solution I've mentioned in the metals repo.

@rchl what was your suggestion on the Metals repo? I looked and didn't find it.

Why can't we send the range as well as position in all cases?

Yea, this would break for Metals. In. Metals we do something very simple in this case and just check if there is a position if so, take it, or else grab the range https://github.com/scalameta/metals/blob/main/metals/src/main/scala/scala/meta/internal/metals/HoverExtension.scala, so having both would break this logic.

Why can’t this be a server capability? Then we don’t need any new methods on AbstractPlugin

The approach we ended up taking on this was to keep it all as simple as possible. We didn't foresee how this would be problematic at all for the clients, since nothing changes except you now can send in a range or a position given a hover. It seemed to be quite easy to do in VS Code and even easier for Neovim. For example to support this in nvim-metals it's literally 3 lines.

https://github.com/scalameta/nvim-metals/pull/232/files

I know that doesn't help here, but just mentioning it since because it was so easy in the other editors, we didn't really think about it being problematic. If a client doesn't know about it or can't support it, then nothing changes at all for them. If they can support it, great. Then once this becomes part of the spec, then we can add that and clients can then use it via that, and then after a while we'll probably sunset the hover back to normal.

Why can’t this be a server capability?

I don't know Sublime internals at all, but can you elaborate on how this would help here?

[ayoub-benali]

The main problem is that this client LSP package is used a lot of users against other LSP servers besides Metals.
This package cannot send each time a range (when available) as the user hovers a range because It is most likely unhanded by the server.

So the best option is to send a range only when the server explicitly says during initialization that it accepts a range as an argument.

I can make a PR into Metals adding that flag, just want to confirm the idea first.

[ckipp01]

The main problem is that this client LSP package is used a lot of users against other LSP servers besides Metals. This package cannot send each time a range (when available) as the user hovers a range because It is most likely unhanded by the server.

Ahhhh sorry! I didn't realize this conversation was on the sublime LSP repo and not the Sublime Metals repo!!

So the best option is to send a range only when the server explicitly says during initialization that it accepts a range as an argument.

Well I'm still not sure this is best right? Since they are both valid and used differently. If the user just wants a hover, then do the normal hover and send the single position. The selection range is a whole different feature, so I'd recommend keeping the hover as is, but then making the range a different binding to be used only when the user explicitly wants it.

I can make a PR into Metals adding that flag, just want to confirm the idea first.

Yea, I mean if it's just setting a flag then sure, I don't see an issue with that. However, it will be removed in the future when it's in the spec, which then makes me worry that adding it into Sublime LSP only for Metals is a bit extreme. Is there any way to do this only contained in the Metals package without touching core?

[ayoub-benali]

so I'd recommend keeping the hover as is, but then making the range a different binding to be used only when the user explicitly wants it.

This is what I tried in: scalameta/metals-sublime#45 but as a user you end up with two different commands, the default lsp_hover and the new lsp_metals_hover_range which are very similar in behavior and it is confusing.

However, it will be removed in the future when it's in the spec, which then makes me worry that adding it into Sublime LSP only for Metals is a bit extreme.

That is fine, once it is in the spec, LSP and hopefully metals would handle the hover the official way. This just a temporary solution.

Is there any way to do this only contained in the Metals package without touching core?
There isn't a way to override the core lsp_hover in https://github.com/scalameta/metals-sublime

[ckipp01]

which are very similar in behavior and it is confusing.

It's been a while since I looked, but I believe in IntelliJ these are two separate commands as well? I think users may actually be used to this since again, the behavior is different, it's two different features in reality. But with that being said, having the same command is useful, but I'd still want to be able to do both.

[tgodzik]

which are very similar in behavior and it is confusing.

It's been a while since I looked, but I believe in IntelliJ these are two separate commands as well? I think users may actually be used to this since again, the behavior is different, it's two different features in reality. But with that being said, having the same command is useful, but I'd still want to be able to do both.

The approach with textDocument/hover sending the range is a workaround for VS Code mostly (done also for Haskell and Rust if I remember correctly?), since there is no API that we can use for showing custom modals. I think a lot of users might be used to having it as a separate command, which is nice that it's possible in Sublime. On the other hand we would need to add a custom command each time to each sublime plugin and I don't think anything like textDocument/selectionExpression will get into the LSP spec soon, so maybe doing it via textDocument/hover is a more maintainable way?

I am ok with both approaches and Metals for sure can set a custom capability, that might also be useful for neovim or emacs.

[ayoub-benali]

Redone here: #1900