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.

Sign up for GitHub

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

Jump to bottom

Improve OSC 9;9 parsing logic & add tests #8934

Merged
2 commits merged into from
Feb 4, 2021
Merged

Improve OSC 9;9 parsing logic & add tests #8934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c60dfbd
Improve OSC 9;9 parsing logic & add tests
skyline75489 Jan 29, 2021
66ce9e2
typo
skyline75489 Feb 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/cascadia/TerminalCore/TerminalDispatch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,19 @@ bool TerminalDispatch::DoConEmuAction(const std::wstring_view string) noexcept
{
if (parts.size() >= 2)
{
return _terminalApi.SetWorkingDirectory(til::at(parts, 1));
const auto path = til::at(parts, 1);
// The path should be surrounded with '"' according to the documentation of ConEmu.
// An example: 9;"D:/"
if (path.at(0) == L'"' && path.at(path.size() - 1) == L'"' && path.size() >= 3)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that this is actually making OSC 9;9 even more Win32-specific. Since " is an invalid dir name on WIndows, but is a valid dir name on Linux.

Copy link
Collaborator Author

@skyline75489 skyline75489 Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

To be specific, when we see a path provided by OSC 9;9 , for example, "D:" on Windows, the actual path is guaranteed to be D: because neither " nor : is valid character in dir names on Windows. But on Linux, you can create a dir that's literally named "D:" by mkdir "\"D:\"".

I can't blame ConEmu for designing the sequence this way since it's a Win32 native application at the very beginning. Just to point it out.

Also CC @Maximus5 for awareness.

Update: on second thought, maybe I'm a bit paranoid about this. The CWD should always be an absolute path, right? There's almost no chance an absolute path would both start and end with " to cause the confusion. I sincerely hope the confusion I described only exist in my imagination.

{
return _terminalApi.SetWorkingDirectory(path.substr(1, path.size() - 2));
}
else
{
// If we fail to find the surrounding quotation marks, we'll give the path a try anyway.
// ConEmu also does this.
Copy link
Collaborator Author

@skyline75489 skyline75489 Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is why I thought I was doing it right. ConEmu won't blame users for forgetting “. I think we should follow,

return _terminalApi.SetWorkingDirectory(path);
}
}
}

Expand Down
57 changes: 57 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ namespace TerminalCoreUnitTests
TEST_METHOD(AddHyperlinkCustomIdDifferentUri);

TEST_METHOD(SetTaskbarProgress);
TEST_METHOD(SetWorkingDirectory);
};
};

Expand Down Expand Up @@ -388,3 +389,59 @@ void TerminalCoreUnitTests::TerminalApiTest::SetTaskbarProgress()
VERIFY_ARE_EQUAL(term.GetTaskbarState(), gsl::narrow<size_t>(1));
VERIFY_ARE_EQUAL(term.GetTaskbarProgress(), gsl::narrow<size_t>(80));
}

void TerminalCoreUnitTests::TerminalApiTest::SetWorkingDirectory()
{
Terminal term;
DummyRenderTarget emptyRT;
term.Create({ 100, 100 }, 0, emptyRT);

auto& stateMachine = *(term._stateMachine);

// Test setting working directory using OSC 9;9
// Initial CWD should be empty
VERIFY_IS_TRUE(term.GetWorkingDirectory().empty());

// Invalid sequences should not change CWD
stateMachine.ProcessString(L"\x1b]9;9\x9c");
VERIFY_IS_TRUE(term.GetWorkingDirectory().empty());

stateMachine.ProcessString(L"\x1b]9;9\"\x9c");
VERIFY_IS_TRUE(term.GetWorkingDirectory().empty());

stateMachine.ProcessString(L"\x1b]9;9\"C:\\\"\x9c");
VERIFY_IS_TRUE(term.GetWorkingDirectory().empty());

// Valid sequences should change CWD
stateMachine.ProcessString(L"\x1b]9;9;\"C:\\\"\x9c");
VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"C:\\");

stateMachine.ProcessString(L"\x1b]9;9;\"C:\\Program Files\"\x9c");
VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"C:\\Program Files");

stateMachine.ProcessString(L"\x1b]9;9;\"D:\\中文\"\x9c");
VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"D:\\中文");

// Test OSC 9;9 sequences without quotation marks
stateMachine.ProcessString(L"\x1b]9;9;C:\\\x9c");
VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"C:\\");

stateMachine.ProcessString(L"\x1b]9;9;C:\\Program Files\x9c");
VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"C:\\Program Files");

stateMachine.ProcessString(L"\x1b]9;9;D:\\中文\x9c");
VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"D:\\中文");

// These OSC 9;9 sequences will result in invalid CWD. We shouldn't crash on these.
stateMachine.ProcessString(L"\x1b]9;9;\"\x9c");
VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"\"");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the future I think we should consider this an invalid sequence, especially after we figure out the way to identify WSL profiles and make OSC 7 work. Right now I'm not sure how to do with it.


stateMachine.ProcessString(L"\x1b]9;9;\"\"\x9c");
VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"\"\"");

stateMachine.ProcessString(L"\x1b]9;9;\"\"\"\x9c");
VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"\"");

stateMachine.ProcessString(L"\x1b]9;9;\"\"\"\"\x9c");
VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"\"\"");
}