Community
Participate
Working Groups
There is no way currently to provide links to source code or any kind of IDE client side action from MD documentation hovers. It'd be useful to add some kind of URL/URI extension point on the client side to register handlers for various URLs/URIs coming from the LS with hover content. Thus eclipse client (lsp4e) extension for LS X can handle any URL/URI from hover doc content sent from LS X.
New Gerrit change created: https://git.eclipse.org/r/118225
Implementation includes IntroURL handling. We could switch to IntroURL handling only if URI handler extension seems like a hack for the moment...
I would like to discuss the exact schema for this a bit. At the moment the proposal looks like this, if I am not mistaken from looking at the code: language-server://spring-boot-ls/somepath where "language-server" is the scheme that got defined by LSP4E to identify language-server-specific URLs. Then people could register an extension for the "spring-boot-ls" language server (in this example) that deals with URLs from that language server. My question here would be: Can we skip the specific "language-server" scheme and instead let people contribute scheme handlers via extension points directly? spring-boot-ls://somepath That way we could unify the URI handling in LSP4E a bit, having pre-defined extensions for "file://" (doing the open-resource stuff that happens at the moment), a "http://" and "https://" one for opening a browser (something that works for symbols, but not yet for documentlinks, like requested in Bug #531452), and language servers can contribute their own specific handlers. Of course there is the risk of different language servers contributing extensions for the same scheme (that would be avoided by the original proposal mentioned above), but that would be fine with me. Opinions?
I might be confused, but are not the URIs defined by the servers? If yes, they are not client specific, so users need to register whatever scheme the srver uses. Or am I missing something?
I have the impression that the story of "opening specific protocol links to user" has a bigger scope than LSP4E. As Alex identified, the intro page already use similar mechanics, and I know several use-cases which have implemented some Eclipse-specific links to trigger specific eclipse actions from the internal browser. So I'd rather see a decent solution here designed and shared at the scope of the whole Platform more than a narrowly scoped solution only used (and maintained) in LSP4E. I imagine there is already stuff to reuse here and there and to make more generic. So you could define you "spring-boot://" protocol scheme, put IDE-wide listeners for it in dedicated plugins, use in in the internal browser or wherever it makes sense, and the LS would simply send this kinds of URL from time to time and delegate to this external "URLOpenService". It's basically a similar pattern as registering Java Protocol Handlers in the JVM, except that here, we'd like the handler to perform a user-oriented action instead of taking care of a stream.
Agree. But would also like to get something into LSP4E in the meantime, so that we don't have to wait for those changes to appear in the platform, but can refactor the implementation over time into the platform. WDYT?
(In reply to Martin Lippert from comment #6) > Agree. But would also like to get something into LSP4E in the meantime, so > that we don't have to wait for those changes to appear in the platform, but > can refactor the implementation over time into the platform. WDYT? I really don't want LSP4E to repeat the typical issue of our community with projects doing work that would be so much more relevant upstream. On the contrary, I'd rather have LSP4E an example project of how an IDE component can drive innovation in Platform that is profitable to many other components. Are you ready to promise that: 1. the extension and API will be documented as being "volatile", ie that they will disappear one day in favor of similar ones in Platform? 2. you'll have resources to drive this addition to be in Platform for the next release after Photon (better start now)? 3. We get rid of this API as soon as Platform has a decent support for this use-case? 4. If some other consumer rely on the introduced extension and API and heavily depend on this, you'll support them in migrating to newer one as best as possible? If yes to all, then I think it's fair to allow such things to get in for eager consumers ASAP. If not, then we need to keep discussing until we find something that make us all happy ;) In the meantime, I'll try to dig in other parts of the Platform to find out whether there isn't a similar extension-point or service we could already rely on, at least partly.
I believe we may be able to reuse IWebBrowser (see comment on Gerrit) and use the browserSupport extension to register strategies to open URLs. The issue becomes how to associate a BrowserSupport with a protocol; for this we may just implement a convention such as "id==protocol" so your spring-boot URL Handler would be a BrowserSupport with id == "spring-boot" and LSP4E would look in the registry to find the right browser support and create/invoke the right IWebBrowser (where you'd place your business code). No API introduced, no need to change Platform (as far as I could see), only a convention to adopt in "ids" (which later could be turned into a more specific field on the extension point for wider usage). WDYT?
Complication with IWebBrowser and URL handling is that I cannot create a URL with unknown protocol (not registered in Java)... I was under the impression we wouldn't want to register a URL handler for lsp protocol... Therefore, I've been operating with URIs instead to avoid the URL. The web browser/web browser support code forces me to use URL and hence not a great fit unfortunately unless we'd like to register a handler for the protocol. Am I missing something?
(In reply to Alex Boyko from comment #9) > Complication with IWebBrowser and URL handling is that I cannot create a URL > with unknown protocol (not registered in Java)... [...] O've been operating with URIs instead to avoid the URL. Ok, I forgot about this issue; and I agree that dealing with URI is usually better. But... > I was under the impression we wouldn't want to register a URL handler for lsp protocol... There is no such "lsp protocol" for uris/urls. So instead of avoiding URL Handler, it'd be better to find a way to implement some custom protocol handling which is not provided by LSP4E because this seems way beyond LSP itself. I basically beleve that anything that can allow to deal with custom uris and which doesn't force LSP4E to define API/extension-point for that is a wiser approach than adding this responsibility to LSP4E only. > Therefore, I The web browser/web > browser support code forces me to use URL and hence not a great fit > unfortunately unless we'd like to register a handler for the protocol. Not great, but IMO it allows a much better separation of concerns than the current proposal and avoids to add extra complexity in LSP4E itself for a problem that's not so LSP-related.
After exploring "browserSupport" extension point I'm leaning towards not using it. I can register a URL Handler in the downstream plugin (in out eclipse LS plugin) via eclipse OSGi URLStreamHandlerService by calling BundleContext#registerService(...) on bundle start and unregister ("unget") it on bundle stop. I can register bundle support class as well. I find that the problem is finding the right BrowserSupport. There is no access to the registry. There is only one active BrowserSupport in the workbench. It is obtained straight from extension point configuration elements under the hood, thus I cannot specify the browser support i'd like to use. I could use WorkbenchBrowserSupport#setDesiredBrowserSupportId(...) but it's for debug purposes only and is not part of IBrowserSupport API. I could hack and find a configuration element by id in the extension registry directly to get to the desired BrowserSupport class. However seems that registering browser support would replace the default browser support then which is undesired i think and would have to hack browser support class implementation such that it'd be close to the default except to handling URLs of specific kind... Still this would only work ok for replacing the default browser support and not any custom browser support that 3rd party plugins might be registering as well... Seems like too much hacking... Maybe just handle IntroURLs for now and remove the URI handler extension point. Can we just support IntroURL for now?
(In reply to Vlad Dumitrescu from comment #4) > I might be confused, but are not the URIs defined by the servers? If yes, > they are not client specific, so users need to register whatever scheme the > srver uses. Or am I missing something? URL is defined on the server. The same URL would be present in hovers in Eclipse, VSCode, Atom, IntelliJ, etc. However, it's unlikely that any of these clients have support to handle that URL. Perhaps some URLs such as "open resource given by URI in an editor" would have handlers by default on the client side, but others like open java type given by fully qualified name may not be available out of the box on the client. Thus extension point to handle these special URLs (i.e. any other URL different from a possible set of standard URLs) on the client would allow for this. As the result any URL sent from LS can be handled if client is equipped with URL handler extension point.
(In reply to Alex Boyko from comment #12) > URL is defined on the server. The same URL would be present in hovers in > Eclipse, VSCode, Atom, IntelliJ, etc. However, it's unlikely that any of > these clients have support to handle that URL. Perhaps some URLs such as > "open resource given by URI in an editor" would have handlers by default on > the client side, but others like open java type given by fully qualified > name may not be available out of the box on the client. > Thus extension point to handle these special URLs (i.e. any other URL > different from a possible set of standard URLs) on the client would allow > for this. As the result any URL sent from LS can be handled if client is > equipped with URL handler extension point. Yes, that's what I understand too. So LSP4E can't decide how the URIs will look like, but each language client implementor will have to provide support for the right kind. (The original question was in response to comment #3)
(In reply to Alex Boyko from comment #11) > After exploring "browserSupport" extension point I'm leaning towards not > using it. > I can register a URL Handler in the downstream plugin (in out eclipse LS > plugin) via eclipse OSGi URLStreamHandlerService by calling > BundleContext#registerService(...) on bundle start and unregister ("unget") > it on bundle stop. That's cool. You may even be able to register it using declarative services too (never tried). Or maybe you can even modify the system property java.protocol.handler.pkgs in your plugin activation. > I could hack and find > a configuration element by id in the extension registry directly to get to > the desired BrowserSupport class. IMO it could be valid and not too much of a hack. There is nothing in Platform that enforces nor recommends extension point to have a single consumer and I don't imagine a scenario that would be harmed by LSP4E parsing the browserSupport extension point and instantiating classes from it. All that code would be internal so it's not causing too much issue regarding maintenance. > However seems that registering browser > support would replace the default browser support It seems to me that this is trye only if the browserSupport you add is set to default in extension declaration. If not, then the current default (which is configured with default="true") would remain the default and the current behavior is basically not modified by addition of your browserSupport. > Maybe just handle IntroURLs for now and remove the URI handler extension > point. Can we just support IntroURL for now? Whatever internal-only change (ie no API, no extension-point) is way safer to include in project and cheaper to maintain. Using introUrl would be in internal code so I have no objection against that. However I believe that the approach above it more powerful and generic, and that it would make it easier for your LS to provide the exact same URIs to all clients (whereas I don't think VSCode can understand Eclipse IntroURL so you'd have to make the returned URL depending on the client). If I were facing your use-cases, I think I'd rather choose the solution above (relying on browserSupport) which will make things simpler from LS side.
There is a WorkbenchBrowserSupport class. It's returned from Workbench.getBrowserSupport() method. The WorkbenchBrowserSupport is the wrapper around various registered browser support classes. There's a method responsible for loading the current active browser support which would try make the first non-default browser support active or the first found default one in the absence of non-default browser support. If I register a non-default browser support then it effectively becomes the active browser support and all #createBrowser(...) calls are routed to that browser support - think this is unwanted. Furthermore, I was wrong about the simplicity of looking up browserSupport config elements by id... they don't have ids, only default and class attributes. I made my contributed browser support class instance of IAdaptable to adapt to String.class and return the id/protocol... Not a great workaround... I'm still leaning towards using IntroURL for now to avoid something rather hacky...
(In reply to Alex Boyko from comment #15) > There's a method > responsible for loading the current active browser support which would try > make the first non-default browser support active or the first found default > one in the absence of non-default browser support. > If I register a > non-default browser support then it effectively becomes the active browser > support and all #createBrowser(...) calls are routed to that browser support > - think this is unwanted. Ok, I did read it incorrectly earlier and was assuming it was working the other way round. This is indeed a curious behaviour that's not so practical. > Furthermore, I was wrong about the simplicity of looking up browserSupport > config elements by id... they don't have ids, only default and class > attributes. Seems like it's using element.getDeclaringExtension().getUniqueIdentifier() which is the id you find on any extension (the node that contains the browserSupport element). It's also a strange choice... > I made my contributed browser support class instance of > IAdaptable to adapt to String.class and return the id/protocol... Not a > great workaround... Not really indeed. Overall, I think you clearly identified a bunch of pitfalls to make the browser support more practical. Would you mind opening bugs for them and make me CC? I think it'd be worth a round of improvement. > I'm still leaning towards using IntroURL for now to avoid something rather > hacky... Ok.
New Gerrit change created: https://git.eclipse.org/r/118428
New Gerrit change created: https://git.eclipse.org/r/118430
I have updated the patch just be handle IntroURLs for now for us to proceed with our plugin until we have something better on the platform side
Gerrit change https://git.eclipse.org/r/118430 was merged to [master]. Commit: http://git.eclipse.org/c/lsp4e/lsp4e.git/commit/?id=42de9aff0195e29c1231e30371b25c032544caa4
I haven't forgotten about the test the IntroURL in the hover. Spent half a day on this test and couldn't get it to work :-( I have a similar SWT snippet with the browser performing identical procedure and it works. I'll give this test another shot next week and will post in a gerrit patch even if it doesn't work - perhaps you have an idea why. 1. Create LSBasedHover 2. Create hover contents similar to other tests. Hover is MD intro URL link. 3. Create empty shell to host the browser from the hover 4. Create browser information control using the shell above 5. Set the HTML hover content on the browser information control 6. Get access to the browser control and wait for the page to load (browser ProgressListener) 7. Execute: browser.execute("document.getElementsByTagName('a')[0].click()") - Simulates click on the hyper link (works great with my SWT snippet) At this point i should hit my location listener breakpoint - doesn't happen. Maybe i should write something much simpler, but not sure how else trigger SWT LocationEvent... it's fired from the result of processing some native callbacks... If you have a better idea to test it - please let me know :-)
The problem is that the web browser content seems to be empty... the initial content never loads...
@Alex: I forgot to mention that while I've been a bit reluctant about the initial changes and so on, I really like the idea of providing support for IDE-side actions in the LSP. I think it really opens the door to very cool use-cases so thanks for that current implementation with IntroURL which is a very good initial iteration. I let you decide what to do with this ticket. If the current state is good enough for you, feel free to mark it as resolved and we can later create a new ticket for more generic URLs (as mentioned in Platform bug 531817 ), otherwise then let's keep it open to track incoming work.
New Gerrit change created: https://git.eclipse.org/r/120318
Gerrit change https://git.eclipse.org/r/120318 was merged to [master]. Commit: http://git.eclipse.org/c/lsp4e/lsp4e.git/commit/?id=de7c6d0ef5540dfbd4ef1c45e4057d2319acb2c4
@Alex: what's missing to close this bug?
I believe it's fixed. Marking as fixed
