Add skeleton code for OSC 7 #8921

skyline75489 · 2021-01-28T10:25:57Z

Summary of the Pull Request

This adds the skeleton code for OSC 7 to the Windows Terminal. No actual functionality is implemented yet,

References

Related: #3158
Supersedes: #7668
Supports: #8214

PR Checklist

Closes #xxx
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

github-actions · 2021-01-28T10:43:44Z

New misspellings found, please review:

hostname

To accept these changes, run the following commands

perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/5757ec679b03a4240130c3c53766c91bbc5cd6a7.txt
.github/actions/spell-check/expect/655f007265b351e140d20b3976792523ad689241.txt
.github/actions/spell-check/expect/a129ff14ec985d6b7bf09e296a831c955d41040b.txt
.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('"AAAAA consecteturadipiscingelit dnceng Loremipsumdolorsitamet Namquiseratal Nullametrutrummetus Remoting rgdx Textbox yzx "');
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/c435f90b4666678e6151a5878c2aafa09d02b1cd.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"hostname remoting textbox "');
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/c435f90b4666678e6151a5878c2aafa09d02b1cd.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.

skyline75489 · 2021-01-29T07:27:13Z

src/terminal/parser/OutputStateMachineEngine.cpp

+    }
+
+    hostname = string.substr(current, nextSlash - current);
+    current = nextSlash + 1;


This is debatable. For a Unix path file://localhost/usr/local/bin, the correct interpretation would be hostname = localhost, path = /usr/local/bin . For a Windows path file://WIN-DESKTOP-1/D:, the correct interpretation would be hostname = WIN-DESKTOP-1, path = D:. The difference makes the detection of / indeterministic.

I found this during messing around fish shell, which have builtin OSC 7 support. The path fish emits is the form of file://localhost/usr/local/bin. Both iTerm2 and terminal.app handles the path perfectly.

CC @j4james @TBBle for awareness.

The FD standard specifies file://localhost/some/path or file:///some/path but allows file://some/path for bug-compatiblity, if I recall the research from the other OSC 7 PR. I think file://myhost/some/path was messy, because if I recall correctly, the FD spec would be looking for /myhost/some/path on the local system (file:// can only refer to local system file paths in the FD spec, so this falls into the back-compat processing) while on Windows, file://myhost/some/path means \\myhost\some\path, i.e. a UNC path using the hostname as the hostname.

Windows is happy with file://localhost/C:/games and file:///C:/games and takes them both as 'local' when asked via "Run".

So file://WIN-DESKTOP-1/D: would be very likely to be invalid, as I don't think a CIFS share can have : in its name. (I could be wrong. Either way, it's certainly not what the user intended).

I think my suggestion at the time was to receive a URL, and if the hostname was the local system name, replace it with localhost before letting any Windows APIs see it.

It all gets super-messy with WSL though, as file://wsl$/Ubuntu/home/users is the correct file URL for /home/users in the Ubuntu WSL instance, but there was definitely strong objection to using file:$(wslpath -m ${PWD}) to generate this, in favour of some magic at another level so that the WSL environment could ignore that it was in WSL when generating OSC 7.

All that said, why don't we have a URL parser around already? Do we need to recreate URL parsing by hand?

Ideally this parsing needs to be specific to the target profile I think. If it's a Windows path, then you need to check for paths starting with /C: or /C| , drop the leading / and potentially replace | wth : (although I wouldn't be too concerned about the latter case).

And of course if the target profile is something like WSL or Cygwin, then we've got that additional translation that's needed to convert the path into a format that the profile will accept. So maybe the Windows-specific quirks could be handled at that level too. And in that case, it's probably OK to just return /C:\path at this point.

I really wouldn't worry too much about UNC paths though. If there is a shell or application that's sending us a UNC path (and I think that in itself is unlikely), it's almost certainly going to be be in the form file://hostname/\\UNC\path, which could be handled in much the same way as the /C:\path format.

Originally I just want to add the skeleton code and NOT touching things that are profile-related. That is blocked by the future implementation of profile-aware tabs. Also it will complicate the PR too much. But now after reading you guys comments I realize my intention seems unrealistic.

@j4james I agree with you. But I’m not comfortable doing profile-related thing in the low level VT code. The VT parser should not care about the environment, right? Also I honestly do not think we can detect the profiles here technically.

Yeah, I'm totally in agreement with you about the leaving the profile-related stuff out of this. That's why I was saying you could probably ignore the Windows quirks at this level too, and just return the path component as is. Then whoever it handling that path at the higher levels can worry about how to interpret it.

Oh OK, now I understand what you mean. Thanks for the inspiration!

@TBBle I tried to implement the backward combability thing regarding file:/<path> . But I'm lost at how can I tell if file://usr/local/bin means hostname ="", path = "/usr/local/bin" or hostname = "usr", path = "/local/bin. I found this when I was trying to come up with the test cases, and I failed to find a solution.

I think that exact point is where the last OSC 7 attempt got stuck as well: Is the file:// URL we're receiving here per the FD spec, or per the Windows spec?

In the end, we can't support both, because as you've noted, they have different semantics for overlapping cases.

We could perhaps support the common subset by ignoring the FD spec's back-compat parse (file://usr/local/bin =>/usr/local/bin), and ignoring the Windows remote-host parse (file://server/share/file => \\server\share\file), and hence having unambiguous parsing for those URLs we don't reject as "wrong host" (FD) or "remote host" (Windows), although that does block using OSC 7 when my shell's CWD is a remote (UNC) path, which includes WSL mounts accessed from the host.

Whichever format you choose, I can't see a way to avoid making some or all shells aware that they're on Windows, without already-rejected hacking at process information to determine details about the virtualised filesystem they are reporting paths against. The proposal is to punt that to a higher layer, but I don't think a higher layer can do anything differently on this point.

skyline75489 · 2021-01-29T10:47:51Z

There is an URL parser in winrt. Down here the code need to be Win7 clean, which means i cannot use the winrt URL class. I do not know othet classes that can be used here. Perhaps one of the core devs will have a idea. 获取 Outlook for Android<https://aka.ms/ghei36>

WSLUser · 2021-01-29T17:45:25Z

There is an URL parser in winrt. Down here the code need to be Win7 clean,

Do we need to support OSC7 on WIn7 though? I'm not so sure that any of the VT sequences are being used through Visual Studio on Win7 unless they were implemented in Terminal itself (not openconsole/conhost). @javierdlg might know if and which VT sequences are utilized by Visual Studio and if this particular sequence should be supported (for Win7 specifically).

zadjii-msft

Okay this might be annoying, but I think we should add the tests for OutputStateMachineEngine::_ParseFileUri now. There's some weird logic that goes on in that function, and I think it'd be helpful to have automated tests for those cases to show what we do and do not accept.

src/cascadia/TerminalCore/TerminalDispatch.cpp

src/terminal/parser/OutputStateMachineEngine.cpp

skyline75489 · 2021-02-09T03:07:46Z

I think we should add the tests for OutputStateMachineEngine::_ParseFileUri now.

This is a reasonable request. I'll try to add some later.

I'm happy that I can use UrlUnescapeInPlace in onecore. Still, do we have a better option? Something that can handle the complicated URL parsing logic? If not I'll have to continue rolling this very initial implementation of a parser.

skyline75489 · 2021-02-09T03:35:44Z

@WSLUser It isn't really about the usability of OSC 7 on Windows 7. It's about the code can be successfully compiled for Windows 7 & for more Windows editions inside Microsoft. I do not know all the details. That's why I consulted the experts on this.

skyline75489 · 2021-02-11T11:18:54Z

src/terminal/parser/OutputStateMachineEngine.cpp

+    hostname = string.substr(current, nextSlash - current);
+    current = nextSlash;
+    std::wstring _path = std::wstring(string.substr(current, std::wstring::npos));
+    if (SUCCEEDED(UrlUnescapeInPlace(_path.data(), URL_ESCAPE_AS_UTF8 | URL_DONT_UNESCAPE_EXTRA_INFO)))


The URL_UNESCAPE_AS_UTF8 is only available on Win8+, where as URL_ESCAPE_AS_UTF8 is available on Win7+. And we have:

#define URL_UNESCAPE_AS_UTF8 URL_ESCAPE_AS_UTF8

But I'm not sure the flag will do anything on Win7, though.

I think the flag URL_UNESCAPE_AS_UTF8 is very important since URLs escaped by Linux will require this to be correctly decoded. See test cases below.

skyline75489 · 2021-02-11T11:24:55Z

@zadjii-msft Mind taking a look at my test cases?

javierdlg · 2021-02-11T18:39:46Z

There is an URL parser in winrt. Down here the code need to be Win7 clean,

Do we need to support OSC7 on WIn7 though? I'm not so sure that any of the VT sequences are being used through Visual Studio on Win7 unless they were implemented in Terminal itself (not openconsole/conhost). @javierdlg might know if and which VT sequences are utilized by Visual Studio and if this particular sequence should be supported (for Win7 specifically).

We do currently support Win7 for VS and won't be dropping support anytime soon, so I'd prefer to keep that support :)

DHowett

I'm gonna block this so I can get my concerns written out. It's not about any specific thing in the code -- just the overall approach. Need a little bit.

skyline75489 · 2021-02-23T02:48:55Z

Sure. Take the time. This is just a prototype kinda work. Didn't really expect this to be shipped or used any time soon.

skyline75489 · 2021-07-20T02:11:40Z

Stale PR. Closed.

Add skeleton code for OSC 7

c435f90

Spell check

54ec9bb

skyline75489 commented Jan 29, 2021

View reviewed changes

zadjii-msft requested changes Feb 8, 2021

View reviewed changes

src/cascadia/TerminalCore/TerminalDispatch.cpp Show resolved Hide resolved

src/terminal/parser/OutputStateMachineEngine.cpp Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 8, 2021

zadjii-msft assigned skyline75489 Feb 8, 2021

zadjii-msft added the Area-VT Virtual Terminal sequence support label Feb 8, 2021

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 8, 2021

Merge branch 'main' into chesterliu/dev/osc-7-2021-edition

398e6cb

skyline75489 added 2 commits February 9, 2021 13:04

Feedback

4b9b417

Fix unescape

44d14b7

skyline75489 commented Feb 11, 2021

View reviewed changes

WSLUser mentioned this pull request Feb 19, 2021

Output OSC 7 "File URI" sequence where appropriate microsoft/winget-cli#763

Closed

DHowett requested changes Feb 22, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 22, 2021

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 23, 2021

skyline75489 added 2 commits March 15, 2021 08:05

Merge branch 'main' into chesterliu/dev/osc-7-2021-edition

a90da70

Merge branch 'main' into chesterliu/dev/osc-7-2021-edition

5ff9e35

skyline75489 closed this Jul 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add skeleton code for OSC 7 #8921

Add skeleton code for OSC 7 #8921

skyline75489 commented Jan 28, 2021 •

edited

Loading

github-actions bot commented Jan 28, 2021

skyline75489 Jan 29, 2021

skyline75489 Jan 29, 2021

TBBle Jan 29, 2021 •

edited

Loading

TBBle Jan 29, 2021

j4james Feb 8, 2021

skyline75489 Feb 8, 2021

j4james Feb 8, 2021

skyline75489 Feb 8, 2021

skyline75489 Feb 9, 2021

TBBle Feb 9, 2021

skyline75489 commented Jan 29, 2021 via email •

edited

Loading

WSLUser commented Jan 29, 2021 •

edited

Loading

zadjii-msft left a comment

skyline75489 commented Feb 9, 2021

skyline75489 commented Feb 9, 2021

skyline75489 Feb 11, 2021

skyline75489 Feb 11, 2021

skyline75489 commented Feb 11, 2021

javierdlg commented Feb 11, 2021

DHowett left a comment

skyline75489 commented Feb 23, 2021

skyline75489 commented Jul 20, 2021

Add skeleton code for OSC 7 #8921

Add skeleton code for OSC 7 #8921

Conversation

skyline75489 commented Jan 28, 2021 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

github-actions bot commented Jan 28, 2021

New misspellings found, please review:

⚠️ Reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TBBle Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyline75489 commented Jan 29, 2021 via email • edited Loading

WSLUser commented Jan 29, 2021 • edited Loading

zadjii-msft left a comment

Choose a reason for hiding this comment

skyline75489 commented Feb 9, 2021

skyline75489 commented Feb 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyline75489 commented Feb 11, 2021

javierdlg commented Feb 11, 2021

DHowett left a comment

Choose a reason for hiding this comment

skyline75489 commented Feb 23, 2021

skyline75489 commented Jul 20, 2021

skyline75489 commented Jan 28, 2021 •

edited

Loading

TBBle Jan 29, 2021 •

edited

Loading

skyline75489 commented Jan 29, 2021 via email •

edited

Loading

WSLUser commented Jan 29, 2021 •

edited

Loading