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

Go to definition support for external files #2128

Open
nikku opened this issue Dec 4, 2022 · 12 comments
Open

Go to definition support for external files #2128

nikku opened this issue Dec 4, 2022 · 12 comments
Labels

Comments

@nikku
Copy link
Contributor

nikku commented Dec 4, 2022

Is your feature request related to a problem? Please describe.

I'm implementing markmark, a language server for markdown at the moment.

That language server shall allow me to Go to definition on any link, including external resources, i.e. images, but also HTTP(S) URLs. What I'm doing to accomplish this LSP server wise is returning the external (image or remote resource) URI with the expectation that it is being opened by sublime text. That works partially:

  • Opening external image works (opens + focuses)
  • Opening remote HTTP(S) URL does not work
  • In both cases a Unable to open URI error messasge pops

Describe the solution you'd like

I'm able to open remote URIs and local (non text resources).

  • Remote URIs are being opened using sublime texts standard Open XYZ feature
  • Local (non-text) URIs are supported through sublime text's standard open file / preview image feature (if supported by the editor, with a fallback to Open XYZ feature

Describe alternatives you've considered

From the practical point of view I could limit my LSP to only return matches for text files; I could alternatively send explicit "open file" requests (not sure if that is possible?).

However that defeats the purpose of Go to definition: The ideas is that I can use it everywhere, without special context.

Additional context

Screen capture of current usage within sublime LSP:

capture OHfd0q_optimized

@rchl
Copy link
Member

rchl commented Dec 4, 2022

As far as opening images, this seems like something that could be treated as a bug if it works like in the screenshot where we open the image but still show an error.

As far as other URI schemes than file:, LSP defers that to a server-specific plugin since there is no universal way of treating other schemes.

For example LSP-Deno implements support for deno: scheme like this: https://github.com/sublimelsp/LSP-Deno/blob/0a902bf7013dedb6d70e36b8604810e6675e09b3/plugin.py#L16-L30

@nikku
Copy link
Contributor Author

nikku commented Dec 4, 2022

As far as other URI schemes than file:, LSP defers that to a server-specific plugin since there is no universal way of treating other schemes.

Understood. But wouldn't https? be something to support natively in the client? Feels odd if such protocol would need to be implemented by various language servers.

@rchl
Copy link
Member

rchl commented Dec 4, 2022

I think that opening https links on "go to definition" would be abuse of that functionality. The intention is to go to the definition of the symbol within the editor rather than opening an external link.

Apart from that, can it really be argued that an URI in a markdown link is the definition of that link? I have some doubts.

If you want you could discuss this idea at https://github.com/microsoft/language-server-protocol/issues but personally I don't think that's a correct way to go.

For highlighting and opening external links there is a separate LSP functionality textDocument/documentLink.

@nikku
Copy link
Contributor Author

nikku commented Dec 4, 2022

Apart from that, can it really be argued that an URI in a markdown link is the definition of that link? I have some doubts.

The URI is not the definition, but the element that contains the definition. A definition is a pair of { uri, position } and in the markdown world the definition of a link can be:

  • A document that represents it
  • A heading that represents it (inside of a document)
  • A definition (inside of a document)
  • A tag defined (inside of a document)
  • ...

For highlighting and opening external links there is a separate LSP functionality textDocument/documentLink.

Thanks for that, I'll consider it. I'm just worried that this is too complicated for users (UX wise). I just want to jump to the definition of a link (the document, header, or remote URL). I don't want to invoke a separate keystroke to jump to a remote URL.

@nikku
Copy link
Contributor Author

nikku commented Dec 4, 2022

Follow up to #2128 (comment): Document links at work:

image

Unfortunately these are not the desired thing for an LSP: I'm used to resolution of file-based "definitions" via the respective shortcut, i.e. in JS LSPs. I want the same for markdown files.

capture QcB5BQ_optimized

@rchl
Copy link
Member

rchl commented Dec 5, 2022

I'm not sure, maybe it makes sense to support https: natively... It doesn't seem fully right though because:
a) the uri would not open in the editor but externally
b) the range is required in the response but couldn't really be applied in this case.

@nikku
Copy link
Contributor Author

nikku commented Dec 5, 2022

I'm not sure, maybe it makes sense to support https: natively... It doesn't seem fully right though [...]

On a thing the editor can clearly open (the context menu says "Open XYZ"), why shouldn't the language server reference it (and request to open) if it regards it as a "definition" to jump to? For me as a user this makes total sense.

Your call as a maintainer if you want to support it. If you'd accept a contribution to add this functionality, please give me a heads-up. Otherwise please close this issue as won't fix.

@rchl
Copy link
Member

rchl commented Dec 5, 2022

Ideally I would like to know an opinion of the LSP spec maintainer and it would also nice to know how VSCode behaves here. Did you by any chance test your server in it?

@nikku
Copy link
Contributor Author

nikku commented Dec 6, 2022

Ideally I would like to know an opinion of the LSP spec maintainer and it would also nice to know how VSCode behaves here. Did you by any chance test your server in it?

Will check in VSCode 👍

@jwortmann
Copy link
Member

I would say it's pretty clear that opening the links from your Markdown example should be realized via textDocument/documentLink. This is exactly the purpose of that LSP method, and this client will even indicate whether a particular link will open directly in ST or in a webbrowser.

If you want to use F12 / the same keybinding as for "Goto Definition" for the lsp_open_link command, we could think about adding a new context for the availability of links at the cursor position, which could then be used for the keybinding. This shouldn't be too difficult to implement by using

def get_document_link_at_point(self, view: sublime.View, point: int) -> Optional[DocumentLink]:

@nikku
Copy link
Contributor Author

nikku commented Dec 13, 2022

I would say it's pretty clear that opening the links from your Markdown example should be realized via textDocument/documentLink.

@jwortmann Yes these are links, but speaking from the users perspective in my LSP these are also URIs to documents that define a resource. I don't want to click it, but I want to jump to the definition of that resource, may it be internal (can be opened by editor) or external. Conversely I can ask for references to these definitions, too, as I would expect in a markdown LSP.

Links also add additional markup, which I do not intend to do.

Cf. also #2128 (comment).

@jwortmann
Copy link
Member

@jwortmann Yes these are links, but speaking from the users perspective in my LSP these are also URIs to documents that define a resource. I don't want to click it, but I want to jump to the definition of that resource, may it be internal (can be opened by editor) or external.

textDocument/definition is meant

to resolve the definition location of a symbol

and afaik the term "location" in the LSP specs usually means some place within a document, but not a document, other file, or remote URL itself. I would not say that a file or remote URL can be considered as the "definition" of a link. Maybe we could argue about something like path/file.md#some-section, where "some-section" is a reference to a heading, i.e. at least that would be a specific location within the file. But for example the LSP-html language server also uses textDocument/documentLink for such links with ids in HTML files.

Another example I can give from another server (Julia language server) is

include("path/to/file.jl")

which uses textDocument/documentLink for the string with the filepath as well.

Anyways, the result of the textDocument/definition is of type Location or LocationLink, and both of those have a DocumentUri, i.e. the ressource must be an internal file/document. And therefore responding with a remote HTTP(S) URL would be a violation of the LSP specs.

Links also add additional markup, which I do not intend to do.

I was advocating to hide the link underlining by default in #1974 (comment), but it was overruled by the other contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants