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

OSC 8 support for conhost and terminal #7251

Merged
merged 35 commits into from
Sep 3, 2020
Merged

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Aug 11, 2020

Summary of the Pull Request

Conhost can now support OSC8 sequences (as specified here). Terminal also supports those sequences and additionally hyperlinks can be opened by Ctrl+LeftClicking on them.

References

#204

PR Checklist

  • Closes Add support for OSC 8, Hyperlink, to the console infrastructure #204
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo 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

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

Validation Steps Performed

Open up a PowerShell tab and type

${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

@ghost ghost added Area-VT Virtual Terminal sequence support Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase labels Aug 11, 2020
@oising
Copy link
Collaborator

oising commented Aug 11, 2020

Just a wee note for the interested - in powershell core (pwsh) you can use the escape code `e in lieu of needing [char]27 for ESC - thank @rkeithhill for that one.

src/buffer/out/TextAttribute.hpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Outdated Show resolved Hide resolved
src/terminal/parser/OutputStateMachineEngine.cpp Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 11, 2020
src/host/screenInfo.hpp Outdated Show resolved Hide resolved
src/renderer/vt/VtSequences.cpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/host/screenInfo.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalSelection.cpp Outdated Show resolved Hide resolved
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 12, 2020
@DHowett
Copy link
Member

DHowett commented Aug 12, 2020

Chatted with Pankaj over Teams about a design change:

[11:45 AM] Dustin Howett
So, right now we have the link clicking behavior down in the core, which makes it so that anybody who consumes the core (visual studio is a good example!) automatically gets the behavior of opening links with shellexecute when you click them

[11:45 AM] Dustin Howett
If we move the behavior up into TerminalControl (which consumes core), we would need to implement the same behavior in the WPF Control (the one VS uses)

[11:45 AM] Dustin Howett
but that might not be flexible enough

[11:46 AM] Dustin Howett
I think it might be an even better design if we make TerminalControl emit an event (like ConnectionStateChanged or one of its other events) when a hyperlink is clicked

[11:46 AM] Dustin Howett
so TerminalApp -- the thing everyone knows as "Windows Terminal" gets final say in it. It can present a UI, it can use ShellExecute, or whatever

[11:47 AM] Dustin Howett
this way, visual studio would even be able to customize the behavior! if they want the link to open in their own browser, they would intercept this message

[11:47 AM] Dustin Howett
if they wanted to add links to files that open up in the source code editor, they would intercept this message

[11:48 AM] Dustin Howett
The design principles behind TerminalControl generally include "some other app could just drop one on the screen and do whatever they want with it" -- so every bit of customizable behavior we give it is better for that principle

[11:49 AM] Dustin Howett
and if an app wants "final say" in how links work inside it, we can't get that by making the innermost component call ShellExecute (smile)

[11:49 AM] Dustin Howett
that's my take. what do you think?

@DHowett
Copy link
Member

DHowett commented Aug 12, 2020

Updates notes from plan chat:

We're going to not render hyperlinks right now. We don't want to damage their attributes, and we can render them better when we do it right.

We had a discussion about how to support IDs provided by applications as well.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Overall I'm very happy with this. I have a couple bigger concerns, but I think this is pretty darn close.

  • The more times I saw AddHyperlink being used for both starting and stopping hyperlink attributes, the more I thought it needed to be two separate methods.
  • Should we convert the std::wstring params we're parsing from the VT sequence to a std::unordered_map<std::wstring, std::wstring> params, and pass that map of parameter k/v pairs around instead of just a raw string of params?
    • this might make it easier for us to get the id out of the params when we need it (even if the rest of the params would be ignored).
  • Does this work in the terminal if you emit "^[]8;;https://github.com/microsoft/terminal^[\This is a link!", sleep for a second, then emit "^[]8;;^[\" AND NOT a trailing newline? I'm guessing that conpty won't trigger a frame to complete the hyperlink sequence, but maybe it does just work?
    • I've very okay with this being a follow-up issue, if it does repro.
  • Should we file a follow-up for highlighting/underlining URI's as we mouse over them?

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/host/outputStream.cpp Outdated Show resolved Hide resolved
src/renderer/vt/VtSequences.cpp Outdated Show resolved Hide resolved
src/renderer/vt/Xterm256Engine.cpp Outdated Show resolved Hide resolved
src/terminal/parser/OutputStateMachineEngine.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalSelection.cpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 18, 2020
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I'm going to clear my review because I'll be gone Friday/Monday.
@miniksa -- I'm going to defer to your opinion on ATTR_ROW gaining a GetHyperlinks

@DHowett
Copy link
Member

DHowett commented Aug 28, 2020

It looks like the bot merging pull requests breaks dependent branches. If you merge this manually, please copy the PR body into the actual commit message -- that's the only way to record it in immmutable history! 😄

@DHowett
Copy link
Member

DHowett commented Aug 28, 2020

We could write a roundtrip test for that case by:

Might be worth doing this. :)

@skyline75489
Copy link
Collaborator

If it’s possible to have some kind of metadata attached to rows/chars, that would also help my potential sixel implementation, which, just like HyperLinks, also needs a way to store and render things on a character level.

Thanks for the work @PankajBhojwani. Can’t wait to see this available. 😁

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

I think we're good. I'm sure something will come up that we'll have to fix, but looks pretty solid to me right now.

@PankajBhojwani PankajBhojwani merged commit 614507b into master Sep 3, 2020
@PankajBhojwani PankajBhojwani deleted the dev/pabhoj/osc8_support branch September 3, 2020 17:58
@ofek
Copy link
Contributor

ofek commented Sep 3, 2020

Awesome! When will this be released?

@DHowett
Copy link
Member

DHowett commented Sep 3, 2020

Any time between an hour ago and an eternity from now 😄

Astute observers who have been following our monthly release cadence may consult their star charts to determine how far from now a month after our last release will be!

@daniel-liuzzi
Copy link

${ESC}=[char]27

Curious as to why are curly braces needed here?

$ESC = [char]27 seems to work just the same.

@DHowett
Copy link
Member

DHowett commented Sep 15, 2020

They’re not. It’s throwaway test code to validate this PR, so it’s not trying to make a recommendation :)

If it were to make a recommendation, that recommendation would be “move to powershell core where you can use `e”.

@ghost
Copy link

ghost commented Sep 22, 2020

🎉Windows Terminal Preview v1.4.2652.0 has been released which incorporates this pull request.:tada:

Handy links:

@insekticid
Copy link

And what about support editor:// protocol handler?
https://github.com/aik099/PhpStormProtocol

image

@skyline75489
Copy link
Collaborator

@insekticid Please open separate issue instead of commenting on this thread.

@PankajBhojwani
Copy link
Contributor Author

@insekticid Thank you for the suggestion! Since this is regarding adding another URI scheme, please feel free to contribute to the discussion happening over at #7562

@joshuabiddle
Copy link

joshuabiddle commented Mar 3, 2023

I'd like to use this ability to open an html file stored on a remote computer using the UNC path. I haven't been able to get it to open. Any ideas?

Example: \\COMPUTER\UNC\Path\To\File.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for OSC 8, Hyperlink, to the console infrastructure