search

scalameta / metals-feature-requests

Issue tracker for Metals feature requests
37 stars 4 forks source link

migrate to metals-goto-location to window/showDocument #164

Open ayoub-benali opened 4 years ago

ayoub-benali commented 4 years ago

LSP Spec 3.16 defines Show Document Request

It would be nice for Metals to migrate the custom metals-goto-location to this new standard request to avoid custom client implementations.

Search terms LSP, spec, show document

kpbochenek commented 4 years ago

Custom command contains additional field other-window that is not present in Show Document Request. It makes particular functionality 'Analyze-stacktrace' much more pleasant to use. Maybe setting external=true in Show Document Request could be interpreted that way, but this is a generic message and we should not hijack this field for specific metals use.

Interface is almost 1:1 same (uri == uri, range == selection) so without this 'other-window' flag migration could be instant.

ayoub-benali commented 4 years ago

I didn't know that otherWindow was used for Analyze-stacktrace. Since the draft was merged yesterday, it might makes sense to propose an extra flag for other window as it isn't finalized yet.

kpbochenek commented 4 years ago

Thanks for a link to this specific commit :+1: probably adding otherWindow is a little too specific, but maybe guys will have some suggestions how to modify it to allow servers to customize this message for their needs a little

tgodzik commented 4 years ago

I am not sure otherWindow is needed in LSP. We create either WebView with links to commands or code lenses with commands. So in reality it's not the server invoking the command, but the client itself. Having it in LSP spec doesn't really help us, we still need to invoke commands on the client side.

laughedelic commented 4 years ago

So in reality it's not the server invoking the command, but the client itself. Having it in LSP spec doesn't really help us, we still need to invoke commands on the client side.

I think it's the other way around:

The show document request is sent from a server to a client to ask the client to display a particular document in the user interface.

so clients are expected to implement a UI for that or invoke external program if the request has external flag. So as @ayoub-benali said, using this request could help to simplify client plugins implementation.

tgodzik commented 4 years ago

@laughedelic It will help us in some scenarios, but I haven't clarified that I was talking about the analyze stacktrace command. For that command we create a new document or webview, which users can use to navigate over the code. And that document already contains either links to commands or commands in code lenses, which clients need to know how to invoke.

And that is the only part of Metals that uses otherWindow, that's why I mean that having it in LSP doesn't help. We could possibly modify it to try and have the client send a server command that would invoke the new LSP method, but not yet sure if it will work in all scenarios.

tgodzik commented 4 years ago

Anyway I added an issue in the repo to discuss it: https://github.com/microsoft/language-server-protocol/issues/1140

tgodzik commented 3 years ago

It seems that actually the idea was included in the spec:

https://microsoft.github.io/language-server-protocol/specifications/specification-3-16/#window_showDocument

There is an option to use takeFocus, I will experiment with it one the new lsp4j release comes in.