-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add support for OSC 8, Hyperlink, to the console infrastructure #204
Comments
If it helps prioritising this feature, I'd like to use it to implement better error messages in Yarn: the package names would be clickable and point to the package websites. |
Could MSFT please comment on if this is possible for us to work on? As in, is this something that can be done only from the OSS side or will it also require changes to ConPTY in Windows itself? I find the architecture quite difficult to grok. |
DISREGARD THAT. I thought I was commenting in #574. This issue's a little different. For conhost to support this VT sequence:
Then for the Terminal, after 4 (but not needing 3/5/6): |
@zadjii-msft My current specific interest could reduce this list even further. I'd like to add support for the OSC hyperlink extension to the wezterm (https://github.com/wez/wezterm) Windows port. Reading over the changes from https://github.com/microsoft/terminal/pull/891/commits to add support for OSC 10/11 sounds like it has direct parallels in the changes I'll need for OSC 8. I understand from the maintainer of wezterm that the VT sequences themselves are not directly passed on to wezterm. Is this due to the lack of "parse and re-emit" support in this repository? Am I correct in saying that OutputStateMachineEngine would be the correct place to add such support to? Is this source file part of "conhost"? I've read the sentence "pass them through to the terminal" here from you and also several times in the docs but don't understand exactly how this can relate to 3rd party users of ConPTY. Final question... how can wezterm know about my changes to conhost? Do I have to somehow embed the open source fork? (My assumption is that wezterm right now is using a conhost provided by Windows 10). Please correct any horrible assumptions / failures in understanding I might have. Thank you so much! |
Experiences from implementing it in gnome-terminal: It was freaking easy to implement a demo, which was feature complete and everything, except the "tiny" issue that each character cell contained the entire target URL (in addition to the Unicode character and the SGR properties such as colors, boldness etc.), so the memory consumption went up from ~12 bytes per charcell to ~2k per charcell. Needless to say, this was a proof of concept, not meant to ever hit the codebase. And then it was freaking hard to come up with a solution that consumes reasonable amount of memory, assuming reasonable use of the feature. The details probably don't really matter because gnome-terminal stores the onscreen data and the scrollback buffer in totally different formats, so whatever we did there probably wouldn't apply to WT. But basically for the onscreen bits we have 2 bytes per cell storing an index to a per-terminal pool of hyperlinks (with garbage collecting and stuff), and for the scrollback we store the URLs in runlength-encoding (i.e. once per continuous run of the same URL) in the corresponding disk file (just as we do the SGR attributes). Converting back and forth between the two, handling if the user scrolls back across the boundary etc. was a true PITA. I hope WT's architecture allows for something cleaner for you guys, e.g. 4 bytes per cell indexing to the pool which is common for the writable part as well as the scrollback (still need to figure out refcounting and/or GC, etc.). Would be lovely to hear how much trouble it was to other terminals that implemented it :) |
I've been wondering about these things myself @egmontkob. The implementation I'm looking at is written in rust which I can imagine simplifies the GCing requirements you had. Based on a very quick perusal it looks like Hyperlinks are not instantiated per cell but shared and referenced via pointers. Not sure it uses a pool / arena as, well, I have no Rust skills. |
To complete the picture, there are two more reasons which made it quite cumbersome in vte (gnome-terminal). One is that we're paranoid about long-time memory fragmentation, so we try to avoid malloc/free (new/delete, etc.) as much as possible. E.g. even in the hyperlink pool, allocated memory for each hyperlink is grown on demand but isn't shrunk or freed until the terminal is closed. The other one is: due to the way our scrollback is stored on disk, we don't really know what data (incl. hyperlinks) scrolls out for good at the top of the scrollback. I mean, knowing it would require an extra read which we don't want to do for performance reasons. If a terminal doesn't suffer from these constraints, some simple refcounted pointer (e.g. std::shared_ptr) could be a good solution. One reference by the parser as long as it's the active URL, and one per character cell that got populated with it. |
Okay yea lets do this. Right now, all VT output goes through When the parser encounters a string it doesn't understand, it's supposed to flush the string through to the connected terminal, if there is one. So, since conhost doesn't yet understand this sequence, and it's an OSC sequence, it should be hitting this block of code: terminal/src/terminal/parser/OutputStateMachineEngine.cpp Lines 752 to 757 in cc9d2ca
That's the code that should tell the state machine to "pass the string through" to the terminal, whatever application that might be. (You might be hitting #2011/#2665 while debugging this, but any subsequent printable character should cause the frame to flush, so I doubt you're hitting this too badly.) I'd start debugging there. It's definitely possible to use the OSS-built conhost to host conptys instead of the OS one - that's what the Windows Terminal does today. However, I don't think we've really had time to polish that story for external consumers right now. We're currently just building a .lib & .dll for the conpty APIs, and making sure to link that first, and that's a little janky. @DHowett-MSFT can go into greater details on that topic. @egmontkob thanks for the great tips. That's pretty much the way I envisioned it - using a single ID in a `TextAttributeRun` to host the URL. We don't really have any difference between scrollback and viewport in the TextBuffer, so we don't need to worry about that too much. @miniksa is also interested in this, so I'm cc'ing him. |
This sounds reasonable. I think we'd also want to consider some sort of heuristic-based detection of hyperlinks (in addition to being able to surface ones declared with this VT code) to meet our goal of the hyperlink feature we dreamed of for our announce video. I also agree that the concept of having a table of links and hanging some sort of attribute off the relevant area of the buffer would be sufficient. I don't know if we'd do this in |
Sure thing :) But that's quite a different story. |
FWIW, iTerm2 does this on the mac. When you hold down command, any hyperlinks it recognizes get underlined and blue. |
This feature is demonstrated in the blessed python library documentation, as @edburns mentions,
|
…lse (#148) This test failure indicates to me that windows reads `does_styling` as False in travis, so no URL is emitted, and that's ok for now, since Windows 10 doesn't yet support hyperlinks, microsoft/terminal#204 When it does, I think there is nothing for us to do, I think it works as-is before and after.
@edburns Not only does iTerm2 do it, but according to the Symfony blog, iTerm2 collaborated with GNOME Terminal on a hyperlinking standard. Might be worth asking them where that standard is published.
|
Fortunately, @egmontkob's already weighed in on this issue with his authority and some implementation details 😄 |
😆 I just realized the first link in this post is about what I just posted about. Sorry. |
So. this happening at some point? Probably last thing holding me back from switching. |
@egmontkob, for what it's worth, I know your comments were from a while ago... would probably be best to use another 12 bits (3-bytes per character total) for a number that is a reference to a list of links experienced in the terminal... that just adds to the list up to the 12-bit unsigned value max (4095)... then loop if the end is hit... could do an arbitrary limit of a lower number as well. This way you're only growing a little bit per character in addition to the list for the links themselves. -- edit: Only real risk would be for going back into a really long scroll buffer would potentially be a wrong link... but shouldn't be a huge issue in that. and would only really be a per-session issue as well. |
So, just to level-set for when this issue is closed by PR #7251: This issue only tracks support for application-generated hyperlinks. We ended up going with something similar to #204 (comment), except using a |
@DHowett Now's a good time to go to AirBnb. book availability at the last Blockbuster Video, and watch a VHS copy of Lawnmower Man. Because we just entered virtual reality! Thank you very much. |
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request Conhost can now support OSC8 sequences (as specified [here](https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda)). Terminal also supports those sequences and additionally hyperlinks can be opened by Ctrl+LeftClicking on them. <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> ## References #204 <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [X] Closes #204 * [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx <!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments Added support to: - parse OSC8 sequences and extract URIs from them (conhost and terminal) - add hyperlink uri data to textbuffer/screeninformation, associated with a hyperlink id (conhost and terminal) - attach hyperlink ids to text to allow for uri extraction from the textbuffer/screeninformation (conhost and terminal) - process ctrl+leftclick to open a hyperlink in the clicked region if present <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed Open up a PowerShell tab and type ```PowerShell ${ESC}=[char]27 Write-Host "${ESC}]8;;https://github.com/microsoft/terminal${ESC}\This is a link!${ESC}]8;;${ESC}\" ``` Ctrl+LeftClick on the link correctly brings you to the terminal page on github ![hyperlink](https://user-images.githubusercontent.com/26824113/89953536-45a6f580-dbfd-11ea-8e0d-8a3cd25c634a.gif)
Fantastic work @PankajBhojwani thanks :) |
Anybody who'd like to track followup work, including automatic detection of URIs, should move on over to 👉 👉 #5001, the scenario issue tracking all further hyperlink efforts. |
🎉This issue was addressed in #7251, which has now been successfully released as Handy links: |
There is a new ansi code for hyperlinks in the console.
https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda
It would be great if this was possible in the windows console.
The text was updated successfully, but these errors were encountered: