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

Documentation / Open in browser button #295

Closed
wants to merge 4 commits into from

Conversation

DunetsNM
Copy link
Contributor

Publishing draft PoC / seeking feedback.

Documentation is opened in webview panel which is OK but a bit constraining: there's no context menu, go back/forward buttons, Ctrl+F etc. I was unable to find any ways to make the webview more rich (as I understood this is by design).

However there's a simple way to open the doc file externally in a browser (atm chrome is hardcoded).

image

On button click it closes the tab in VS code and reopens the doc file URL in Chrome:

image

Copy link
Collaborator

@lukel97 lukel97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@@ -33,16 +34,40 @@ export namespace DocsBrowser {
panel = window.createWebviewPanel('haskell.showDocumentationPanel', ttl, ViewColumn.Beside, {
localResourceRoots: [Uri.parse(documentationDirectory)],
enableFindWidget: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we had this enabled but I just tried this out and can't seem to get it to work either. Is this a vs-code bug?

// TODO : move chrome to the settings

// open file in browser and close the webview in VS code
await openPath(path, { app: 'chrome' });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the built-in vscode open functionality instead so we can avoid the extra dependency on open?

import { env } from 'vscode';
// ...
env.openExternal(vscode.Uri.parse(path));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't know about it, will try, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK , env.openExternal suffers from the same issue as open called with default program, it strips anchor/tag in file url which is annoying

e.g. URL passed to openExternal:

file:///C:/sr/snapshots/b84f27c3/doc/haskell-lsp-types-0.22.0.0/src/Language.Haskell.LSP.Types.Location.html#Range

URL opened in browser (opens at top):

file:///C:/sr/snapshots/b84f27c3/doc/haskell-lsp-types-0.22.0.0/src/Language.Haskell.LSP.Types.Location.html

will check later if it can be done without pulling the 'open' dependency

@DunetsNM
Copy link
Contributor Author

seems impossible to preserve anchor in URL without resorting to explicit browser name (via 'open' or smth similar). When a URL run from command line the anchor gets stripped in the browser.

It is possible to look up the executable via x-default-browser (pushed an experimental commit) which means even more extra dependencies ('open' alone is not too bad, needs only a couple). Alternative is to provide a user setting with default e.g. "chrome" but that is not obvious and may leave some users unhappy.

What do you think? @bubba

@DunetsNM DunetsNM marked this pull request as ready for review September 30, 2020 04:04
@lukel97
Copy link
Collaborator

lukel97 commented Oct 2, 2020

Hi @DunetsNM, sorry for the late reply. I've gone down a bit of a rabbit hole with the fragment part, cloned + built vscode locally to see what was going on with env.openExternal, only to find it was an issue with electron. Then I tried recreating the code that electron uses to open things on Mac, i.e. [NSWorkspace openURL:(NSURL*)url], and it also doesn't work with the fragment. Then I tried out your PR and found that the Mac command open doesn't seem to work with fragments at all, and this includes the open npm package:
https://github.com/sindresorhus/open/blob/f3748e4f5a04a41e27034aff1eb08e8b86d01c1c/index.js#L57-L70
What operating system are you using this from? I've accepted that macOS is probably a lost cause but I'm happy to use open from npm if it fixes the issue on windows/linux

@lukel97
Copy link
Collaborator

lukel97 commented Oct 2, 2020

So it turns out the reason that cmd-f for finding in the view is because we are forced to use an iframe: clicking on the top bar first means that it works: microsoft/vscode#70339

@DunetsNM
Copy link
Contributor Author

DunetsNM commented Oct 3, 2020

Hi @bubba , my OS is Windows . I haven't tried it on linux. apparently macOS is indeed lost here.

tbh I'm not too happy with this solution myself now: besides extra dependencies and broken macOS it is also rather slow (there's a visible delay when link opened explicitly via chrome, while env.openExternal is instant).

Am I fixing a symptom rather than the cause here? None of that would be a problem if it wasn't a file:/// url in the first place.
Instead I can just translate file URL to the https:// on hackage and rename link to "Open on Hackage" smth like this:

image

I don't know if there's any guarantee that hackage will have equivalent online page for every file but a few that I tried worked just fine. Even if some don't exist it's still tolerable.

I don't think I can or should replace file:/// with https:/// hackage link upstream in hls/ghcide:

  • seems like we have to invoke ghc with -haddock flag anyway (i.e. install the docs locally) to make documentation link appear in the tooltip: Hover docs including links for dependencies haskell-language-server#208 (comment)
  • code that deduces right html file works only with local files atm (strictly speaking that code is incorrect, a more reliable way would be to parse haddock index file or doc-index.json and find symbol location there)

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

Successfully merging this pull request may close these issues.

2 participants