-
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
Initial implementation of OSC 7 #7668
Conversation
I'll add the explanation here to make the PR description clean. So I've went over basically every comments in #3158 . As a daily terminal users and former/current contributor of this repo, I understand most of the struggles from both the terminal users' side and WT developers' side. I decided to come up with a minimal, reliable and working approach, by first adding the following constraints:
With these constraints in mind, I managed to make this PR. It's extremely simple. See for yourself. The diff contains merely 100 lines. To be honest I don't think it will be a very large PR even after polished, assuming that this approach is approved by the Console team and the community in the end. I really hope that this particular PR itself can fully fixes #3158. However, with the terminal doing only the very basic stuff, the burden moves to the shells. I've searched many things before I could made a working script. Due to possible licensing issue, I'll link my scripts below and assume that it will not be included in this repo:
There's several issues remaining to be discussed/solved:
Some of the issues are more script-related. Some of them are terminal-related. Aside from the known issues and other possible caveats, I'm having a blast with this. Thanks everyone for your contribution in #3158 . Now let's see where this will take us. |
New misspellings found, please review:
To accept these changes, run the following commands
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉
|
{ | ||
if (_pConApi->IsConsolePty()) | ||
{ | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well conhost does not have tabs. So this should be OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this isn't going to be implemented in conhost, then the method shouldn't be added to AdaptDispatch
at all. Just let it fall back to the base implementation that returns false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ITermDispatch
is a virtual class. I can not fall back to the base implementation for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. My mistake. For some reason I assumed it derived from TermDispatch
the same way TerminalDispatch
does. It probably should, but that's not your problem.
if (profileGuid == focuesdProfileGuid) | ||
{ | ||
auto workingDirectory = tabImpl->GetActiveTerminalControl().WorkingDirectory(); | ||
auto validWorkingDirectory = !workingDirectory.empty() && ::PathFileExists(workingDirectory.data()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation is very important. Without this, PowerShell and WSL will refuse to spawn when there's invalid path, leaving a very ugly error message in WT window.
For example, someone opens PowerShell and ssh into a Linux server. The server sends OSC 7 with pure Linux path. This path is illegal and will be caught by PathFileExists
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprisingly enough, if you open a PowerShell tab and run wsl.exe
in it, the path \\wsl$\Ubuntu\path
sent by WSL can also be used by new PowerShell tabs. It will correctly enter the WSL working directory. I remember seeing this on some article about WSL and Windows filesystem interoperablity. I just didn't not except that it works so seamlessly.
And I think this only works on Windows 2004 and WSL 2. Correct me if I'm wrong.
I think it's kinda important to get this one working. If I have a tab open where I have used ssh to connect to a host and I want to duplicate that session on another tab, we need to get this working. It should allow for a password prompt (because of ssh.exe) if you don't have an ssh key or you configured a password on your ssh key but if you do have a password-less ssh key (the most common scenario), this should be a seamless experience. |
Yeah I’d also love that seamless experience. I think it has something todo with the fact that OpenSSH-Win32 is not a native Win32 program. It’s been ported to Win32 only a few years. I’m guessing there’s a way to make it work and I feel that it may require some modification in the OpenSSH-Win32 itself. |
does this work for cmd? |
If that is the case, I hope you could provide them with the needed info to get it working and/or provide them a PR to address the issue. |
Okay for obvious reasons I'm not reviewing this in its entirety. However, thoughts off the top of the dome: When we're creating the new terminal control, don't iterate over all the tabs to see if any of them have the same profile as the requested one. There should be a way to directly ask the page for the currently focused control, and just use that control's working directory. That should make this work for both tabs and panes. We might want to discuss whether or not we want to scope this only to the |
I'm not exaggerating when I say this. Do not interpret this as a joke or as hyperbole. This is impossible in the general case. Unless you wish for Windows Terminal to take a dependency on the exact remote shell in use (you do not), you cannot propagate the working directory over ssh. There is no protocol feature in ssh--specifically, the part that handles remoting ptys and launching shells--that supports transmitting a working directory. |
I think @zadjii-msft's approach of using the current control's working directory would allow removing the strict profile isolation. This would be nice, because as well as "duplicate tab", the other use-case I have for this functionality would be start a "git bash" session in the same directory my current PowerShell session is in. I'd be fine if this was a non-default behaviour for 'New tab for Profile X' (Ctrl-Shift-Alt-3 would still be faster than Ctrl-Shift-3, beat, |
Git bash does not recognize VT control sequences in WT the same way as it does in its own mintty. I don’t have a clue why. Theoretically it should work just fine if the control sequences can be generated. |
Cygwin versions < 3.1 (and therefore msys versions based on them) did not support They implemented their own VT parser. |
After my experiments so far, there’s only one thing stopping me from lifting the same profile constraint - CMD can not recognize UNC path (\wsl$) generated by WSL CMD will fall back to c:\somewhere I don’t remember. Aside from this, I’m OK with cross profile OSC 7 handling. As i mentioned in other comment, as long as the URI is strictly Windows-specific, all shells that respond to CWD setting should work (again not sure if Git bash works). |
Git for Windows 2.27 upgraded to a Cygwin 3.1-based MSYS2.
It's off by default, but unless running the installer in silent mode, it should prompt to enable it during install/upgrade. I thought I had told Git for Windows to notify me of updates, but I was still using 2.24.0, and hadn't thought to go check for an update manually until now. |
CMD wouldn't work with any UNC patch, without using pushd to get a temporary drive letter for it. So I don't think that's a problem for Windows Terminal here, and more than it would be a WT problem if one of the user's shells is configured to ignore the working directory and immediately I assume we something like the below in CMD:
which would be a reasonable fallback. |
Now that I've had a chance to see how this is working, I'm afraid I wouldn't consider it an acceptable solution. It may solve the problem of opening a new tab in the current directory, but it's not a standard If a Linux shell has built-in support for Similarly, if I have manually setup my shell to use a uri of the form Bottom line: if we can't support |
I thought about the difference between my current implementation and the standard. Here's my two cents. First when OSC 7 was first seen on Terminal.app, it's more like a proprietary escape code that for macOS exclusively. When terminal-wg decided to finalize a standard, they chose to use the Freedesktop file URI spec But here with WIndows Terminal that's not the case. Windows and *nix are fundamentally and historically different in so many ways. I don't think people would actually expect things to work exactly the same way between a local Windows machine and remote Linux machines. So I thinks it's perfectly OK to "have manually setup my shell to use a uri of the form \wsl$\Ubuntu... to get it to work in Windows Terminal, then that likely means it will no longer work in other terminals that actually support the standard path syntax". Because WSL is not and never will be a "real" Linux environment. Same for PowerShell/Git Bash/CMD/etc. The Windows Terminal & related shell environment are born different from the outside *nix world. I don't like it, but it's the world we live in. Point is, the OSC 7 in WT and related shell environments are destined to be exclusively working on Windows OS only. And I don't think people would be frustrated about it. The beauty of OSC 7 is, I think, that the methodology itself is platform-independent. That does not necessarily mean the actual implementation should be platform-independent. At least on Windows I don't see that's possible. |
I don't think that's an accurate statement. Support for GUI Linux apps running on WSL is definitely on the roadmap. Once that's the case, you could be running Gnome Terminal and Windows Terminal side by side accessing the same WSL subsystem. But even ignoring that, there are already third party terminals (like Mintty) that can connect to WSL, and that support the standard If you don't want to follow the standard, that's fine - just invent your own sequence instead. There is absolutely no reason to use |
Honestly I can’t say that being compatible with current OSC 7 standard is technically not possible. Move the path handling logic from the zshrc to WT, find a way to detect WSL profile, put them together and it should work the same. It’s just I don’t really want this kind of “dirty” logic inside WT, just from my point of view. I don’t make the call here. And I’m willing to hear from everyone. So yeah. Not following the standard outside is not ideal, I admit. |
I feel that "detect that it was WSL, identify which instance, and turn that WSL instance-internal I agree that we should support a standard OSC 7, not introduce a new variant. That said, I'd be curious to know how mintty running on Windows would handle OSC 7 from WSL, whether it expects If it's the latter, we already need OSC 7 to report Windows-resolvable In the way OSC 7 has been described and implemented in Terminal.app and terminal-wg, at least, it assumes the Which means that OSC 7 for WSL is only slightly more rational than OSC 7 for ssh: Useful for showing in a titlebar, for example, but you can't carry it between sessions without much more intrusive logic to transform the 'remote' URL into a 'local' URL, and apply it. The only difference is that WSL has a feasible 'apply it', but the 'transform it' step is still going to be a sticking point. |
I updated the PR and added hostname validation. I've tested more cases and it surprised me how well most shells behaves when accepting a Windows directory as starting directory. For example I can cd into I think I'm close to the proper solution for this. Even if we are going to do the transformation for WSL, that would be a special case for WSL and for WSL only. Knowing the current behaviour of CMD, PowerShell and Git Bash makes me more convinced that all the other shells should be sending OSC 7 with a valid Windows path.
I second this. I think this is the correct thing to do. |
I did write "the correct option", but I meant "it is a strictly correct option". I don't think it's "incorrect" to have support for specific known sources of non-Windows-filesystem paths, but I would consider any such support separately to getting the OSC 7 feature working with Edit: Awesome. I did not know you could |
(Still working my way through the backlog...)
It does! WSL patches in a
|
This is, unfortunately, a feature request that comes up from time to time. People want the following things to "just work":
But they can't just work in the most general case. I'd hate to go down the rabbit hole of attempting to determine if WSL happened to be running when a path was generated or a file was dropped or something. I just.. I don't have a good solution to this, in any form. Heuristics is a fancy word for "guessing your ass off" |
I’m more than satisfied to know we have cygpath and wslpath at our disposal. This means we won’t need to “go down the rabbit hole of attempting to determine WSL is running”. We can still have a very smooth experience with the help of various shell confs in my gists above. |
I agree, That doesn't help, e.g., vim plugin which is generating OSC 7 commands, and doesn't know it needs to apply That would also work for OSC 8 Edit: Updated, now I'm actually on a machine with WSL installed, and |
Another interesting observation from me: This is a great example showing that we actually can not use the path that a"normal" Linux would provide. If we want everything to work without copying large amount of code from |
I was really hoping the substitute for It's a shame Windows Explorer doesn't appear to know how to render that character. I'm not sure how it could but even so. (Edit: What happens if you use the character Edit: I tested this, because I was curious. It's ending up at |
} | ||
|
||
const auto prefix = uri.substr(0, 7); | ||
if (!prefix.compare(L"file://") == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Uri class might be useful here - you can use it to easily obtain the scheme/hostname/path etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually can not find a way to use WIndows::Founcation::Uri
class. I have little experience with WinRT C++.
By the way do we need to worry about using this on Win7 or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. Windows Terminal needs Windows 10 1903 or later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WinRT introduction has an example of specifically using Windows.Foundation.Uri
from Win32 C++. It looks like the examples come with the WinRT VSIX extension.
The C++ code itself looks reasonable, but if this is the first use of WinRT in the project, you might need to do other things to set up the include paths and linking. I've not done any development on Windows Terminal itself, so I don't know if that'll be simple like the README implies, or complex if that README is hiding more complicated steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shared components like DX renderer actually require Win7 support, because it will be integrated into Visual Studio in the future. I'm not sure TerminalApi
falls into that category.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Despite the fact those WinRT goodies are very handy, I can not use it to parse the OSC 7 URI. The ::winrt::Windows::Foundation::Uri
class expect the URI format to be:
scheme://username:password @host:port/path.extension ?query #fragment
The FreeDesktop File URI spec is:
file://<hostname>/<path>
The behaviour of winrt Uri class is not very satisfying. For example for an Uri file:///D:/
, the winrt Uri gives you this:
Scheme: file
Host:
Path: /D:/
Even weird, for an Uri in PowerShell file://COMPUTERNAME/C:\\
, the winrt Uri class gives you this:
Scheme: file
Host: computername:
Path: /
I suspect that it can not handle \
in a URI. And it prefers lowercase hostname? I don't really get it. We are, after all, parsing a FreeDesktop file URI so I can expect it to work. So that isn't a good choice to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those URI parses examples are basically correct. I don't know what you're expecting differently in the first case? Hostnames in URIs are not case-sensitive (leads back to the DNS spec, which is not case-sensitive).
For the second case, \
is not a valid path separator in a URI, so I somewhat expected a path /C:\\/
but I'm not overly surprised that it just fails weirdly, since that's not a valid Windows file:// URI (I don't believe :
is valid in a UNC path name) and I assume Windows::Foundation::Uri
is noticing that. I know system.uri
in PowerShell does checking for file://
URIs and rejects "invalid" ones (like file://wsl$/...
, which is invalid-per-the-URI-spec, even though it's valid (surprisingly) in the Windows file:// URI spec, because Windows supports non-DNS-valid hostnames).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, we should never see \
in an OSC 7 URI, they need to be FreeDesktop File URI spec URIs (or as close as we can do) and the path separator is /
, not \
. So if we get file:///D:/
, then the correct parse is as you gave above, which if we feed it back into urlmon, will be D:\
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though I get the correct path, the Uri class is not happy with the host name. For a URI file://COMPUTER-NAME/D:/Projects
, the Uri class gives you this:
Scheme: file
Host: computername:
Path: /D:/Projects
Sounds like if we are using the Uri class we'll have to give up on hostname, because it is in fact not a valid file URI on WIndows. The hostname MUST be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Seems like a bug? system.uri
handles that fine.
PS C:\Users\paulh> [system.uri]("file://COMPUTER-NAME/D:/Projects")
AbsolutePath : /D:/Projects
AbsoluteUri : file://computer-name/D:/Projects
LocalPath : \\computer-name\D:\Projects
Authority : computer-name
HostNameType : Dns
IsDefaultPort : True
IsFile : True
IsLoopback : False
PathAndQuery : /D:/Projects
Segments : {/, D:/, Projects}
IsUnc : True
Host : computer-name
Port : -1
Query :
Fragment :
Scheme : file
OriginalString : file://COMPUTER-NAME/D:/Projects
DnsSafeHost : computer-name
IdnHost : computer-name
IsAbsoluteUri : True
UserEscaped : False
UserInfo :
You can even see here that computer-name
is a "DNS-safe" hostname, so it's not the same problem as the wsl$
hostname.
Unfortunately, this component must work on Windows 7. Anything “TerminalCore” and below (in the folders named conhost and shared in the SLN file) is Windows 7-clean, and anything in the rest of src/cascadia is Windows 10 1903. TerminalCore builds into a component for Visual Studio. :) |
The best thing to do would be to defer URI handling to TerminalControl or even higher. |
@DHowett on the scale of 1-10, how much do you feel that I'm heading to the "correct" direction in this PR? I mean it may not be technically vital in the WT infrastructure but it's a popular request from the community. I think we all want it to happen. |
I unsubscribed from this PR because it just makes me angry, but for the last time now, I'm begging you, if you can't implement this in a way that's compatible with the format used by existing shells and terminals, then please just invent another sequence for it. It's not going to make any difference to us if it's |
Although @j4james has unsubscribed and won't see this, I want to say that as I have understood the discussion, the intention is to implement OSC 7 as specified and used by existing terminals and shells: Shells (or others) send a It would not be compatible with other implementations for a WSL session to send I specifically call out a container here because, like WSL and Cygwin, applications inside a container are running inside a mount namespace that is not visible or reasonably resolvable to things outside it. In theory, VTE could poke at the running processes, identify a container by interrogating the container runtime, parsing its volumes, and determining what the volume mapping was, if the CWD turns out to be a local filesystem mount. WSL and Cygwin have the advantage that they provide a utility that can produce a system-resolvable, If the thing generating the OSC 7 sequence cannot produce a |
Alright, I've re-read this thread and others, and I think I have an opinion on the matter. Obviously, this is a super contentious sequence, which is why I hadn't already implemented it 😜. It's also an issue that's now intertwined with a couple other scenarios, so for the completeness of discussion, let's also refer to
Basically, we've got two simultaneously incompatible issues. Either we:
I think at the end of the day, I agree with @j4james. If we implement
|
Closed in favor of #8330 |
Summary of the Pull Request
This is a very early implementation of OSC 7 in WIndows Terminal
References
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed