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

Initial implementation of OSC 7 #7668

Closed

Conversation

skyline75489
Copy link
Collaborator

Summary of the Pull Request

This is a very early implementation of OSC 7 in WIndows Terminal

References

PR Checklist

  • Closes Open new terminal tab in same directory as existing tab (OSC 7?) #3158
  • 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

Validation Steps Performed

@ghost ghost added Area-Settings Issues related to settings and customizability, for console or terminal Area-VT Virtual Terminal sequence support Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conpty For console issues specifically related to conpty Product-Powershell Issues in the modern command interpreter, Powershell.exe. Product-Terminal The new Windows Terminal. labels Sep 18, 2020
@skyline75489
Copy link
Collaborator Author

skyline75489 commented Sep 18, 2020

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:

  • OSC 7 handling is strictly isolated between different profiles
  • Terminal does not probe the shell process running inside, like ever. No one wants this.
  • Terminal does not probe the type of the profile being used. I know that "source" being Windows.Terminal.Wsl is a strong sign that indicates the profile is a WSL one. But no. Don't do this.
  • Terminal does not probe the type of the URIs passed in by OSC 7. I know there's way to determine whether a URI is likely to be a Windows one or Linux one. But no. Don't do this.

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:

  • Shells other than Bash/Zsh not tested.
  • Different Bash/Zsh version not tested.
  • Different Windows/WSL version not tested. I've only tested with Windows 2004 + WSL 2 Ubuntu.
  • Different locales and complicated paths not testsed.
  • Pure SSH profile does not work. It straight up ignores the current directory setting.
  • Tab with multi-pane is not handled. I'm not a multi-pane user so I'm not sure what the expected behaviour is.

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. New Tab and Duplicate Tab work in the way exactly what you'd imagine. The working directory is being successfully transferred between old tabs and new tabs. When using WSL in WT, it just reminds you of a normal Linux terminal. No hassle. No pain. It just works.

Thanks everyone for your contribution in #3158 . Now let's see where this will take us.

@github-actions
Copy link

New misspellings found, please review:

  • focuesd
To accept these changes, run the following commands
perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"akb Autogenerated debugbreak DECLL DECSMBV Inplace notypeopt restrictederrorinfo Scs Switchto Wlk "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/3f9ad1f71103c6ebb306e8f0646d490cbf6f801c.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"autogenerated focuesd inplace "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/3f9ad1f71103c6ebb306e8f0646d490cbf6f801c.txt is added to your repository...'
✏️ Contributor please read this

By 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.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 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. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

{
if (_pConApi->IsConsolePty())
{
return false;
Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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());
Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

@WSLUser
Copy link
Contributor

WSLUser commented Sep 18, 2020

Pure SSH profile does not work. It straight up ignores the current directory setting.

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.

@skyline75489
Copy link
Collaborator Author

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.

@LuanVSO
Copy link
Contributor

LuanVSO commented Sep 18, 2020

does this work for cmd?
cmd OSC7

@WSLUser
Copy link
Contributor

WSLUser commented Sep 18, 2020

I feel that it may require some modification in the OpenSSH-Win32 itself

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.

@zadjii-msft
Copy link
Member

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 duplicateTab action, or if we want to add a flag to openTab/splitPane to enable (or disable) this behavior.

@DHowett
Copy link
Member

DHowett commented Sep 18, 2020

Pure SSH profile does not work. It straight up ignores the current directory setting. (@skyline75489)

I think it's kinda important to get this one working. (@WSLUser)

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.

@TBBle
Copy link

TBBle commented Sep 18, 2020

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, cd /d/project/repos/whateverIhappenedToBeWorkingOn as I do often enough now).

@skyline75489
Copy link
Collaborator Author

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.

@DHowett
Copy link
Member

DHowett commented Sep 18, 2020

Git bash does not recognize VT control sequences

Cygwin versions < 3.1 (and therefore msys versions based on them) did not support ENABLE_VIRTUAL_TERMINAL_PROCESSING on Windows 10+, even though it's been out for 5 years.

They implemented their own VT parser.

@skyline75489
Copy link
Collaborator Author

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).

@TBBle
Copy link

TBBle commented Sep 19, 2020

Git for Windows 2.27 upgraded to a Cygwin 3.1-based MSYS2.

This release comes with a Git Bash that optionally uses Windows-native pseudo consoles.

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.

@TBBle
Copy link

TBBle commented Sep 19, 2020

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 cd somewhere else. Or... was a remote shell that couldn't rationally honour the working directory.

I assume we something like the below in CMD:

PS Microsoft.PowerShell.Core\FileSystem::\\keitaro\C> cmd
'\\keitaro\C'
CMD.EXE was started with the above path as the current directory.
UNC paths are not supported.  Defaulting to Windows directory.
Microsoft Windows [Version 10.0.19041.508]
(c) 2020 Microsoft Corporation. All rights reserved.

C:\Windows>

which would be a reasonable fallback.

@j4james
Copy link
Collaborator

j4james commented Sep 19, 2020

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 OSC 7 implementation (at least as I understand it).

If a Linux shell has built-in support for OSC 7, and my current directory is /home/james, then the uri it's going to set will be something like file://localhost/home/james, and definitely not file://\\\\wsl$\Ubuntu/home/james. If we only understand the latter, and not the former, then we don't support OSC 7.

Similarly, if I 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. We've essentially taken over an existing sequence and repurposed it in a way that breaks other software - that's not good.

Bottom line: if we can't support OSC 7 in a standard way, then I think we shouldn't be using it, and should just come up with our own proprietary sequence.

@skyline75489
Copy link
Collaborator Author

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
. This is a deviation from the Terminal.app one, even though in practice the difference should not really matter. The thing is, people would naturally expect things to work exactly the same between local macOS/Linux machine and remote Linux machines. That's of course because they're all *nix after all. So everything is good.

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.

@j4james
Copy link
Collaborator

j4james commented Sep 19, 2020

Because WSL is not and never will be a "real" Linux environment.

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 OSC 7 sequence. If we encourage people to ignore that standard and use a proprietary WT syntax instead, that will end up breaking the functionality in other terminals. This is exactly what Microsoft did with Internet Explorer back when it was the dominant browser, and that did not go down well.

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 OSC 7 in an incompatible way unless you're deliberately trying to break other software.

@skyline75489
Copy link
Collaborator Author

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.

@TBBle
Copy link

TBBle commented Sep 19, 2020

I feel that "detect that it was WSL, identify which instance, and turn that WSL instance-internal /path into //wsl$/<instanceName>/path" is going to be only marginally less-awful than "Just scan the various possible CWD internal objects in the process control block", and hence not a reasonable result.

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 OSC 7 file://hostname/home/myaccount/, or if it would expect OSC 7 file://wsl$/Ubuntu/home/myaccount.

If it's the latter, we already need OSC 7 to report Windows-resolvable file:// URLs. If it's the former (seems more likely), it can't possibly use that URL, since it isn't itself running in the WSL environment, so that URL wouldn't resolve.

In the way OSC 7 has been described and implemented in Terminal.app and terminal-wg, at least, it assumes the file:// URL is sensible for where the shell is running, and the shell doesn't know where the terminal is running, so it can't be expected to adjust the file:// URL to suit it, e.g. it won't contain wsl$ as the hostname or have the WSL distribution name prepended as the first path segment, which is what would be needed for a 'windows-local' file:// URL.

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.

@skyline75489
Copy link
Collaborator Author

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 /etc in WSL and Git Bash will happily accept it and change its current directory to //wsl$/Ubuntu/etc. It also works the other way around.

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.

If the OSC 7-generating process in question cannot produce a Windows filesystem path, the correct option is to ignore it.

I second this. I think this is the correct thing to do.

@TBBle
Copy link

TBBle commented Sep 21, 2020

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 file:// URLs that do reference a Windows-resolvable filesystem path, as that carries significant extra complexity and risk, and I would not want to have the OSC 7 feature bog down (further) on that.

Edit: Awesome. I did not know you could cd to a UNC path in git-bash (also tested in MSYS 2), that's cool! It means my list of URLs earlier needed to be updated for that case, as it's now closer to PowerShell than git-bash. That said, file://$(hostname)/$(cygpath --mixed $PWD) works everywhere for Cygwin-based environments, although you have to change $(hostname) to localhost for Windows explorer to accept it.

@DHowett
Copy link
Member

DHowett commented Sep 22, 2020

(Still working my way through the backlog...)

@TBBle

Edit: I kind-of wish WSL has something like cygpath now, to hide all the above

It does! WSL patches in a wslpath binary that does bidirectional path translation and takes into account mtab.

duhowett@dhowett-sl:~$ wslpath 'C:\Users'
/mnt/c/Users
duhowett@dhowett-sl:~$ wslpath '\\wsl$\Debian\home'
/home
duhowett@dhowett-sl:~$ wslpath -w /etc
\\wsl$\Debian\etc
duhowett@dhowett-sl:~$ wslpath -w /mnt/c
C:\

@DHowett
Copy link
Member

DHowett commented Sep 22, 2020

we must have special-case code in WT to detect a WSL session and mangle its URLs into the Windows path layout.

This is, unfortunately, a feature request that comes up from time to time. People want the following things to "just work":

  • Setting your startingDirectory to a WSL path
  • Pasting a path off the clipboard (assuming we can even tell it's a path; do people think that this happens for free? :P)
  • Dropping a file on the terminal window
  • OSC 8 hyperlinks emitted by e.g. coreutils (see similar discussion in Support the "file" URI scheme #7526)

But they can't just work in the most general case. wslpath runs inside a specific distribution and looks into the mount table before responding.

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"

@skyline75489
Copy link
Collaborator Author

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.

@TBBle
Copy link

TBBle commented Sep 22, 2020

I agree, cygpath -m and wslpath -m remove the need to do any guessing, on the basis that the code inside the environment can use those to get Windows-environment paths, so WSL WT can just consume the 'path' component of the file:// URL without further mangling to account for the source environment.

That doesn't help, e.g., vim plugin which is generating OSC 7 commands, and doesn't know it needs to apply cygpath -m or wslpath -m, though. But that's a different problem to solve, and I think the vim plugin is the more-correct place to solve it.

That would also work for OSC 8 file:// URLs, if that was the blocker on #7526. (The security concern there is a different matter, and it's more likely that such a URL is generated by an application than a shell, so less immediate utility there, but it does answer one part of the issue).

Edit: Updated, now I'm actually on a machine with WSL installed, and wslpath -m works, and so doesn't need slash-replacement. So file://$(hostname)/$(wslpath -m ${PWD}) is trivially-generatable correct OSC 7 URL value in WSL.

@skyline75489
Copy link
Collaborator Author

skyline75489 commented Sep 22, 2020

Another interesting observation from me: wslpath is doing a fantastic job converting UNC paths. For a valid Linux path my \folder, the default tool vte-urlencode-cwd will convert it to my%5Cfolder, using the standard URL escape. However, wslpath will convert it to myfolder. The reason is obvious. The character \ is not valid character for Windows paths. There must be some kind of internal mapping inside WSL that handles this. And wslpath is fully aware of that.

image

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 wslpath, the path conversion for WSL has to be handled inside the distro environment.

@TBBle
Copy link

TBBle commented Sep 22, 2020

I was really hoping the substitute for \ (0x5c) was going to be or , but it turns out it's (0xf05c), a private-use character. I like the alignment of the low-byte being the same.

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: perhaps I'm being dumb, and the middle-dot is a rendering for this character. I had assumed it was a 'unknown character' marker... Other random PUA characters render the same.)

What happens if you use the character in a filename inside WSL? ^_^

Edit: I tested this, because I was curious. It's ending up at \ on disk, i.e. the characters \ and in WSL2 both end up as \ on-disk under /mnt/c/ (i.e. via the mount from Windows), and in Windows all are shown as as . On the Windows side, only can be used, and it appears back as \ on the WSL side. On the Linux-local filesystem, \ and are separate files, but both appear as when accessed through the \\wsl$ UNC path in Windows Explorer. At that point, things go a little weird (renaming a file or folder with reports "file is too big for the disk"), so probably avoid using literal inside WSL.

}

const auto prefix = uri.substr(0, 7);
if (!prefix.compare(L"file://") == 0)
Copy link
Contributor

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

Copy link
Collaborator Author

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?

Copy link

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.

Copy link

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link

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).

Copy link

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:\.

Copy link
Collaborator Author

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.

Copy link

@TBBle TBBle Sep 25, 2020

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.

@DHowett
Copy link
Member

DHowett commented Sep 25, 2020

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. :)

@DHowett
Copy link
Member

DHowett commented Sep 25, 2020

The best thing to do would be to defer URI handling to TerminalControl or even higher.

@skyline75489
Copy link
Collaborator Author

@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.

@j4james
Copy link
Collaborator

j4james commented Oct 4, 2020

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 OSC 7 or OSC 4321 (or whatever), but we're just making problems for everyone else if we repurpose the existing sequence in an incompatible way.

@TBBle
Copy link

TBBle commented Oct 4, 2020

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 file:// URL, and if the Terminal can resolve it as local, then it is consider the CWD of that session, for whatever purposes the Terminal wants a CWD; in the WT case, for opening new tabs or sessions in the same CWD.

It would not be compatible with other implementations for a WSL session to send file://myhostname/usr/bin and have Windows Terminal resolve that magically into the equivalent of file://wsl$/MyWSLSession/usr/bin or file://localhost/wsl$/MyWSLSession/usr/bin, in the same way that it would unexpected for a Linux desktop stack to resolve file://myhostname/workspace from inside a container into file://myhosthome/var/lib/docker/volumes/SomeUUID/workspace.

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, file:// path within their environment. That's up to the thing generating the OSC 7 sequence to use, not the Terminal to try and parse it.

If the thing generating the OSC 7 sequence cannot produce a file:// URL that works for the Terminal, it's not the Terminal's OSC 7 implementation that's faulty.

@zadjii-msft
Copy link
Member

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:

  • Support this sequence with no "smart" path translation. We'll need to literally use whatever directory they emit (assuming the hostname matches)
    • Users who are currently using OSC 7 in their *nix scripts and emitting file://$hostname/$PWD in WSL, cygwin, will get unexpected behavior. We'll try and treat $PWD as a path to a Windows directory - which might be a path that exists. When they try to open a new tab with that same directory, it'll open not in the distro the sequence originated in, but in the Windows filesystem. Not great!
  • Find some way to magically translate paths provided by OSC7 into the correct Windows path. WSL clients won't need to change any of the scripts they were already using for this to "just work".
    • There's no heuristic way of doing this, without the client application providing some extra information to us. We can't inspect the process tree to figure out if the process that's emitting this sequence is a WSL process or a docker process or an ssh, and even if we could, then we wouldn't be able to correlate wsl.exe to the distro within WSL that's being used. Similarly, we can't use the profile to do this, because the user might be using WSL w/in their "Command Prompt" profile (for example).
    • We could introduce some other sequence for additional metadata to be provided. Something like "I know I'm a WSL, and I'm <distroName>, when I emit paths, handle them specially". If we did introduce such a new custom sequence, then we're not really faithfully implementing OSC7 now are we? For clients to work as they did before, they'd still need to add emitting this custom sequence

I think at the end of the day, I agree with @j4james. If we implement OSC7 as requiring a Windows-style path, then that'll necessitate that client applications will need to be modified to work right on Windows, and I hate that idea.

OSC9;9 - the ConEmu solution

This one doesn't seem that bad. Since it's a windows-first sequence, it doesn't need to know anything else about who's emitting the path. We can always assume it has been emitted as a Windows path. On the surface, this seems to work great, save for one scenario - ssh. This sequence doesn't accept any sort of hostname, it only takes the path. So if you were ssh'd into another machine, and that one emitted a OSC9;9, then there's no way of determining that sequence wasn't intended for the current machine.

I think I'm okay with us implementing this as-is for the time being though. It's not a perfect sequence, but it's good enough, and doesn't come with the compat baggage that OSC7 does. It can certainly be supported with the caveat "This won't work in an ssh session, or in any scenario where it's emitted by a remote connection. It will always be treated as a path on the local machine".

Other notes

I want this to just work. I want there to be one sequence that everyone can emit reliably, and have it just work, and I don't think that OSC9;9 is that either. I'm gonna be filing a thread later today with the body of this post and a proposal for more discussion. This is a great feature that I think we all want, but we do need to make it work for both Windows and Linux terminals. The only thing I'm confident about in this whole discussion is that we (the terminal emulator ecosystem) don't have a perfect solution yet.

@skyline75489
Copy link
Collaborator Author

Closed in favor of #8330

@skyline75489 skyline75489 deleted the chesterliu/dev/osc-7 branch January 25, 2021 02:57
@skyline75489 skyline75489 mentioned this pull request Jan 28, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Area-VT Virtual Terminal sequence support Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conpty For console issues specifically related to conpty Product-Powershell Issues in the modern command interpreter, Powershell.exe. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open new terminal tab in same directory as existing tab (OSC 7?)
8 participants