[jwortmann]

I though about how LSP handles textDocument/documentLink request, and I think we can make it more efficient than the current implementation. Currently, this request is made after each change while typing. This is necessary if links should be underlined in the editor.

But the main interaction with links is via the hover popup which shows a "Open Link" (or "Open in Browser") link at the bottom of the popup, if applicable. But the mouse hover probably happens significantly less often than the (debounced) changes from typing, and that means if the user has disabled the underline for links in the LSP settings, we do a lot of unnecessary textDocument/documentLink requests. In that case it would be sufficient to only do that request on demand when the hover popup is requested. I have implemented that in this PR, including result caching if there are multiple hover actions without a buffer change in between. Also the DocumentLinks are only resolved (if necessary) when the link in the popup is clicked. If the underline style is used, requests are still made after each change, like before.

Currently there are 3 different options for the "link_highlight_style" setting, one of it is "disabled" which disabled the DocumentLink functionality completely. I think with this refactoring the "disabled" option isn't needed anymore, because if the underline style isn't used, the requests are only done on demand, so it should have any performance impact. In case someone really wants to disable any DocumentLink functionality, they of course still can do that via "disabled_capabilities" in the server config.

I think the on-demand-request strategy for DocumentLink is also a good reason to not use the underline style by default, so I changed that default value (probably worth to mention that in the change log, if we want to do this change).

There is only small drawback, but I think it's only an extremely niche use case; there is a lsp.link_available context for key bindings that would check whether there is a link on the given point (caret position). This was implemented for a special user request to use the same key binding for opening a DocumentLink and for the "Goto Definition" feature. So you could use both of these commands with the same key trigger, but prioritize lsp_open_link while using the mentioned "context" to fall back to "Goto Definition" if there is no link at the given point. But if we don't trigger the DocumentLink request after each change, we can't tell that anymore. So this only works like before if the "link_highlight_style" setting is set to "underline" now.

---

I've also rewritten the lsp_open_link command to always fetch the latest links from the server via textDocument/documentLink request, which is needed now in case of the underline style being disabled.

And another change from this PR is that the hover popup only uses the result from one documentLinkProvider session now (in case of multi-session setup), which means that if you click on "Open Link" and there are multiple session with that capability, we don't show the quick panel anymore to select one of the results. I assume that if there are multiple sessions, the link targets would be the same anyway, so I think we can save that additional step and the quick panel wasn't really useful.

[rwols]

Are comments actually allowed in this file? I guess so.

[jwortmann]

There already are a few comments in this file, like

LSP/sublime-package.json

Line 289 in b5d3208

// \u00A0 is used as a workaround for https://github.com/sublimehq/sublime_text/issues/3373

So it seems to be allowed.

[rchl]

I've noticed that we have this menu item that says "Follow link" while we've changed the text to "Open link" in 271724d

I've also noticed that it doesn't work the same as the "Open link" in the popup. At least for custom json-schema scheme. The menu item tries to open that link in the browser on main while on this branch it forwards to the plugin.

The link in the popup has some additional logic that tries to parse line:column which actually is expected in this case.

That is not really a new issue though (even if this branch changes behavior). But at least one thing that could be done is rename the menu item text to "Open link"

[rchl]

Though one thing that is not great is that on this branch the "Follow link" menu item is always active (at least with default setting) and usually does nothing.

EDIT: Even with underline enabled it still is always active.

[jwortmann]

I've updated the menu label and added back is_enabled which uses the cached result if the underline style is enabled. But we still run a request when the command is run, to be sure that we always have the latest result. If underline isn't used, the menu item is always enabled (because there aren't any cached results).

I've also noticed that it doesn't work the same as the "Open link" in the popup. At least for custom json-schema scheme. The menu item tries to open that link in the browser on main while on this branch it forwards to the plugin.

Hm, that is strange, I think I used more or less the same logic from the hover popup.

    def _open_uri_async(self, session: Session, uri: URI) -> None:
        scheme = parse_uri(uri)[0]
        if scheme == 'file':
            if window := self.view.window():
                open_file_uri(window, uri)
        elif scheme.lower() in {'http', 'https'} or (not scheme and uri.startswith('www.')):
            open_in_browser(uri)
        else:
            session.open_uri_async(uri)

For unknown URI scheme it should fall back to session.open_uri_async but it seems that only evaluates the line:col from fragment for file URIs. I think we should move this part

LSP/plugin/hover.py

Lines 347 to 354 in b5d3208

	def try_open_custom_uri_async(self, uri: str) -> None:
	uri_parts = urlsplit(uri)
	r = lsp_range_from_uri_fragment(uri_parts.fragment)
	if r:
	uri = urlunsplit(uri_parts._replace(fragment=''))
	for session in self.sessions():
	if session.try_open_uri_async(uri, r) is not None:
	return

also into session.open_uri_async if we want to do that for custom schemes too and not only for file URIs. But I think it should actually go through the plugin handler anyway?

Edit: Oh, I see the problem is that we don't know from the hover popup which session to use to resolve the custom URI scheme (currently it seems to just try all sessions).

[rchl]

also into session.open_uri_async if we want to do that for custom schemes too and not only for file URIs. But I think it should actually go through the plugin handler anyway?

It seems like a nice thing that LSP can handle links like

to jump to a different location and only fallback to the plugin otherwise.
Otherwise we would have to replicate that logic in plugin command handlers.
So I think we should move that logic to where it can be used in more cases.

[jwortmann]

So I think we should move that logic to where it can be used in more cases.

I agree, the #line:col fragment convention seems to be useful to work for all URIs, and not only file URIs.
But I think we can do that refactoring separately, because that behavior wasn't changed in this PR.

[rchl]

I will probably keep this on by default so not sure I agree with changing the default but I don't have strong enough opinion to fight for it.