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

🎁 Support other terminals CWD escape sequence #157783

Merged
merged 17 commits into from Aug 19, 2022

Conversation

babakks
Copy link
Contributor

@babakks babakks commented Aug 10, 2022

Fixes #157567
Fixes #157568
Fixes #157569

Unfortunately, I couldn't test the terminals/emulators myself. So, take it as an initial implementation. There are some questions, but I decided to submit the PR first and then update it based on the answers.

The questions are regarding the sequence OSC 7 ; scheme://cwd ST:

  1. Is the scheme:// a literal string or is a placeholder for actual scheme identifier? Naturally it should be the latter.
  2. Since the URI format, naturally the CWD value should be URI-encoded. Is it true here?

I've put // TODO markers to update the code based on the answers:

// Checking for: `OSC 7 ; scheme://cwd ST`
if (command.startsWith('scheme://')) {
    // TODO: I'm not sure `scheme` here is literal or can be `file` or something else.
    // TODO: Possibly the path is URL-encoded, but I have no means to test it.
    const cwd = command.substring(9);
    this._updateCwd(cwd);
    return true;
}

I also added a new helper method _parseKeyValueAssignment for parsing key-value assignments. In the existing implementation, we hadn't noticed multiple equal signs (=) in the command arguments so just splitting and taking the first value would result in a truncated string. This is handled in the _parseKeyValueAssignment method.

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks babakks changed the title Support other terminals CWD escape sequence 🎁 Support other terminals CWD escape sequence Aug 10, 2022
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

This is great, FYI #157570 is a good follow up if you're looking for more

@Tyriar Tyriar added this to the August 2022 milestone Aug 10, 2022
@Tyriar Tyriar self-assigned this Aug 10, 2022
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
…lass

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
…r=<Cwd> ST`

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks
Copy link
Contributor Author

babakks commented Aug 12, 2022

@Tyriar I applied the reviews. Also, updated the tests (which I hadn't found them earlier because they were under workbench directory) to verify the correct function.

Sequence OSC 7; scheme://cwd ST

To make sure how this sequence should be handled, I've tested different cases on my workstation (details in the next comment), and found out these should hold for the terminal host to apply the sequence data:

  • URL scheme must exist and be file://
  • The hostname (as in file://hostname/...) is skipped and can be any value (even empty: file:///)
  • A mere file:// is not valid (should at least be file:///)
  • URL encoded characters (i.e, %xx sequences) are decoded to their unicode equivalent. Note that this means if you have both directories local" and local%22 (where %22 is the URI-encoded equivalent of the " character), the local" would be the interpretted CWD.

@babakks
Copy link
Contributor Author

babakks commented Aug 12, 2022

Seeing how OSC 7; scheme://cwd ST is handled

On my workstation (xterm-256color on Ubuntu 20.04 and gnome-terminal), I tested different scenarios to see how my terminal host (gnome-terminal) supports the OSC 7; scheme://cwd ST sequence. You can repeat these tests to verify the conclusions I've made in the previous comment.

To initiate the test, run:

sudo mkdir /test-root
sudo mkdir /test-root/local
sudo mkdir '/test-root/local"'
sudo mkdir '/test-root/local%22'

Now, to perform each test case:

  • Open a fresh terminal instance
  • Enter the test case code: echo ...
  • Press Ctrl+Shift+T to open a new terminal tab (you can use your mouse too).
  • Check if the newly opened terminal's working directory matches the test case's.

Test cases (one per line):

# Different hostname values:
echo -ne $'\e]7;file://\a'                                         # No hostname
echo -ne $'\e]7;file:///\a'                                        # Empty
echo -ne $'\e]7;file://test-root/local\a'                          # No hostname
echo -ne $'\e]7;file:///test-root/local\a'                         # Empty
echo -ne $'\e]7;file://local/test-root/local\a'                    # 'local'
echo -ne $'\e]7;file://localhost/test-root/local\a'                # 'localhost'
echo -ne $'\e]7;file://babak-laptop/test-root/local\a'             # Actual value
echo -ne $'\e]7;file://some-unknown-hostname/test-root/local\a'    # Unreal value

# URL encoded chars (`%xx`):
echo -ne $'\e]7;file:///test-root/local%22\a'
echo -ne $'\e]7;file:///test-root/local"\a'
echo -ne $'\e]7;file:///test-root/%6c%6f%63%61%6c\a'

# Different scheme values:
echo -ne $'\e]7;/test-root\a'
echo -ne $'\e]7;//test-root\a'
echo -ne $'\e]7;///test-root\a'
echo -ne $'\e]7;:///test-root\a'
echo -ne $'\e]7;http:///test-root\a'
echo -ne $'\e]7;ftp:///test-root\a'
echo -ne $'\e]7;ssh:///test-root\a'

@babakks babakks requested a review from Tyriar August 12, 2022 21:02
Tyriar
Tyriar previously approved these changes Aug 12, 2022
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

I haven't tested this yet but it looks solid! Love the tests 👏

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks @babakks 👍

@Tyriar Tyriar merged commit 3cb5824 into microsoft:main Aug 19, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 3, 2022
@babakks babakks deleted the support-other-terminals-cwd-sequence branch November 10, 2022 13:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants