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

Feature Request: Allow URLs to be clicked #1388

Closed
noinkling opened this issue Jun 22, 2019 · 9 comments
Closed

Feature Request: Allow URLs to be clicked #1388

noinkling opened this issue Jun 22, 2019 · 9 comments
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.

Comments

@noinkling
Copy link

noinkling commented Jun 22, 2019

Summary of the new feature/enhancement

Detect URLs and make them clickable (open the URL in the default browser). This is a convenience feature present in many other terminals.

I believe the value is self-evident, but here's a concrete use case example regardless: when I run yarn outdated (Yarn is a package manager) it outputs a repository/homepage URL for every listed package. I want to be able to click one of those URLs to open the page quickly/easily and have a look at what has changed in the new version of the package.

Implementation details

  • Probably don't need to support anything more than text starting with http:// or https://.
  • URLs that span multiple lines (due to being truncated by the window width) should be handled correctly.
  • There should probably be an indication that the URL is clickable, e.g. cursor change + underline on mouse hover.
  • Most other terminals require an alt or ctrl click, I presume to guard against accidental clicks when copying and so forth.

You can look at something like VS Code's terminal for inspiration. Again this is all probably self-evident.

Stretch goal (covered in #204)

@noinkling noinkling added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jun 22, 2019
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jun 22, 2019
@zakius
Copy link

zakius commented Jun 22, 2019

IMO it should support all schemas for the sake of completion, avoiding further issues asking for addition of new ones and reusability of the code/library.

One thing that may get complex to get right is handling of parentheses: most of implementations I've met incorrectly skip the closing parenthesis character ()) if it's the last character in the link breaking multiple Wikipedia links. Probably there should be used some parentheses matching algorithm to decide if this last character is a part of link or not.

EDIT: typo

@noinkling
Copy link
Author

noinkling commented Jun 22, 2019

IMO it should support all schemas for the sake of completion, avoiding further issues asking for addition of new ones and reusability of the code/library.

I think the issue with that is that it's potentially a security issue since a registered protocol handler could be malicious. Maybe it just needs to follow the lead of browsers and popup a confirmation dialog when a URI has an unknown scheme/would be handled by an unknown program. There's a bit of discussion about this in the link I posted.

@zakius
Copy link

zakius commented Jun 22, 2019

The solution I'd like to use would be detecting all schemas and adding a sanity check to click handler with ability to whitelist and blacklist in settings

@TheModdersDen
Copy link

TheModdersDen commented Jun 22, 2019

Take a look at this: https://youtu.be/8gw0rXPMMPE?t=23. Looks like Microsoft already has this on their radar (I hope...) :)

EDIT: In case you miss it, it's the "wt install link-preview" part.

@egmontkob
Copy link

most of implementations I've met incorrectly skip the closing parenthesis character ()) if it's the last character in the link breaking multiple Wikipedia links

On the other hand, URLs are often enclosed within parentheses in text flow (see e.g. http://example.com/foobar) <- like here, and so are in Markdown files.

In gnome-terminal we addressed these two contradicting requirements by allowing parentheses () and square brackets [] as long as they occur in balanced pairs. This was implemented using recursive regular expression. See g-t 763980.

Another similar tricky case is the trailing apostrophe, see g-t 448044.

@zakius
Copy link

zakius commented Jun 24, 2019

In gnome-terminal we addressed these two contradicting requirements by allowing parentheses () and square brackets [] as long as they occur in balanced pairs.

I mentioned that a bit later in less technical terms, that's probably the exact reason why most of implementations opt for not catching these at all over being too eager

I'm a bit worried about these rare cases when there's closing paren without matching opening one, but we can't figure this out without polling the server unfortunately

@egmontkob
Copy link

we can't figure this out without polling the server

Haha, this is also a possibility – I personally wouldn't go for it, though. I'm sure you share my concerns about data leakage as well as slowness.

It's all guesswork, there's no perfect solution (well, there's #204 to address this gap). We found the matching parens thingy to work well enough, we haven't received a report since we implemented that. At least it definitely turned out to be better than either always matching the paren or never matching. It was a bit tricky to implement, digging into rarely used and hardly known regex features of certain regex libraries (so I'm not surprised that many terminals don't bother with it), but I think it was worth it. :)

@DHowett-MSFT
Copy link
Contributor

Thanks for the suggestions, all. This is actually a duplicate of #574.

@DHowett-MSFT DHowett-MSFT added the Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. label Jun 24, 2019
@DHowett-MSFT
Copy link
Contributor

I've migrated most of your comments over. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.
Projects
None yet
Development

No branches or pull requests

5 participants