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

Add skeleton code for OSC 7 #8921

Closed
wants to merge 7 commits into from
Closed

Add skeleton code for OSC 7 #8921

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c435f90
Add skeleton code for OSC 7
skyline75489 Jan 28, 2021
54ec9bb
Spell check
skyline75489 Jan 29, 2021
398e6cb
Merge branch 'main' into chesterliu/dev/osc-7-2021-edition
skyline75489 Feb 9, 2021
4b9b417
Feedback
skyline75489 Feb 9, 2021
44d14b7
Fix unescape
skyline75489 Feb 11, 2021
a90da70
Merge branch 'main' into chesterliu/dev/osc-7-2021-edition
skyline75489 Mar 15, 2021
5ff9e35
Merge branch 'main' into chesterliu/dev/osc-7-2021-edition
skyline75489 Apr 15, 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
1 change: 1 addition & 0 deletions .github/actions/spell-check/expect/expect.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -990,6 +990,7 @@ HMONITOR
HORZ
hostable
hostlib
hostname
HPA
HPAINTBUFFER
HPCON
Expand Down
6 changes: 6 additions & 0 deletions src/cascadia/TerminalCore/TerminalApi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,12 @@ bool Terminal::SetTaskbarProgress(const size_t state, const size_t progress) noe
return true;
}

// Method Description:
// - Updates the current working directory.
// Arguments:
// - uri: The new working directory. Note that the uri is expected to be valid on Windows.
// Return Value:
// - true
bool Terminal::SetWorkingDirectory(std::wstring_view uri) noexcept
{
_workingDirectory = uri;
Expand Down
12 changes: 12 additions & 0 deletions src/cascadia/TerminalCore/TerminalDispatch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,18 @@ bool TerminalDispatch::ResetMode(const DispatchTypes::ModeParams param) noexcept
return _ModeParamsHelper(param, false);
}

// Method Description:
// - Updates the current working directory.
// Arguments:
// - hostname: The hostname
// - path: The path. Note that the path may not be valid on Windows.
// Return Value:
// - true
bool TerminalDispatch::SetWorkingDirectory(const std::wstring_view /*hostname*/, const std::wstring_view /*path*/) noexcept
{
return true;
skyline75489 marked this conversation as resolved.
Show resolved Hide resolved
}

// Method Description:
// - Start a hyperlink
// Arguments:
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalCore/TerminalDispatch.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ class TerminalDispatch : public Microsoft::Console::VirtualTerminal::TermDispatc
bool SetMode(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::ModeParams /*param*/) noexcept override; // DECSET
bool ResetMode(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::ModeParams /*param*/) noexcept override; // DECRST

bool SetWorkingDirectory(const std::wstring_view hostname, const std::wstring_view path) noexcept override;

bool AddHyperlink(const std::wstring_view uri, const std::wstring_view params) noexcept override;
bool EndHyperlink() noexcept override;

Expand Down
2 changes: 2 additions & 0 deletions src/terminal/adapter/ITermDispatch.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ class Microsoft::Console::VirtualTerminal::ITermDispatch
const VTParameter parameter1,
const VTParameter parameter2) = 0;

virtual bool SetWorkingDirectory(const std::wstring_view hostname, const std::wstring_view path) = 0;

virtual bool AddHyperlink(const std::wstring_view uri, const std::wstring_view params) = 0;
virtual bool EndHyperlink() = 0;

Expand Down
10 changes: 10 additions & 0 deletions src/terminal/adapter/adaptDispatch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2374,6 +2374,16 @@ bool AdaptDispatch::WindowManipulation(const DispatchTypes::WindowManipulationTy
return success;
}

// Method Description:
// - Ascribes to the ITermDispatch interface
// - Not actually used in conhost
// Return Value:
// - false (so that the command gets flushed to terminal)
bool AdaptDispatch::SetWorkingDirectory(const std::wstring_view /*hostname*/, const std::wstring_view /*path*/) noexcept
{
return false;
}

// Method Description:
// - Starts a hyperlink
// Arguments:
Expand Down
2 changes: 2 additions & 0 deletions src/terminal/adapter/adaptDispatch.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ namespace Microsoft::Console::VirtualTerminal
const VTParameter parameter1,
const VTParameter parameter2) override; // DTTERM_WindowManipulation

bool SetWorkingDirectory(const std::wstring_view hostname, const std::wstring_view path) noexcept override;

bool AddHyperlink(const std::wstring_view uri, const std::wstring_view params) override;
bool EndHyperlink() override;

Expand Down
2 changes: 2 additions & 0 deletions src/terminal/adapter/termDispatch.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ class Microsoft::Console::VirtualTerminal::TermDispatch : public Microsoft::Cons
const VTParameter /*parameter1*/,
const VTParameter /*parameter2*/) noexcept override { return false; }

bool SetWorkingDirectory(const std::wstring_view /*hostname*/, const std::wstring_view /*path*/) noexcept override { return false; }

bool AddHyperlink(const std::wstring_view /*uri*/, const std::wstring_view /*params*/) noexcept override { return false; }
bool EndHyperlink() noexcept override { return false; }

Expand Down
55 changes: 55 additions & 0 deletions src/terminal/parser/OutputStateMachineEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
#include "ascii.hpp"
#include "../../types/inc/utils.hpp"

#include <Shlwapi.h>
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved

using namespace Microsoft::Console;
using namespace Microsoft::Console::VirtualTerminal;

Expand Down Expand Up @@ -722,6 +724,17 @@ bool OutputStateMachineEngine::ActionOscDispatch(const wchar_t /*wch*/,
TermTelemetry::Instance().Log(TermTelemetry::Codes::OSCRCC);
break;
}
case OscActionCodes::SetWorkingDirectory:
{
std::wstring hostname;
std::wstring path;
success = _ParseFileUri(string, hostname, path);
if (success && !path.empty())
{
success = _dispatch->SetWorkingDirectory(hostname, path);
}
break;
}
case OscActionCodes::Hyperlink:
{
std::wstring params;
Expand Down Expand Up @@ -888,6 +901,48 @@ try
}
CATCH_LOG_RETURN_FALSE()

#pragma warning(push)
#pragma warning(disable : 26477) // Suppress USE_NULLPTR_NOT_CONSTANT

// Routine Description:
// - Given a file URI string, attempts to parse the URI encoded.
// The file URIs are of the form:
// file://<hostname>/<path>
bool OutputStateMachineEngine::_ParseFileUri(const std::wstring_view string,
std::wstring& hostname,
std::wstring& path) const
{
if (string.size() < 8)
{
return false;
}

const auto prefix = string.substr(0, 7);
if (!prefix.compare(L"file://") == 0)
{
// We don't support URI format other than "file://".
return false;
}

size_t current = 7;
const auto nextSlash = string.find(L"/", current);
if (nextSlash == std::wstring::npos)
{
// Invalid URI. Ignore it.
return false;
}

hostname = string.substr(current, nextSlash - current);
current = nextSlash + 1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

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

Copy link

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

std::wstring _path = std::wstring(string.substr(current, std::wstring::npos));
UrlUnescapeInPlace(path.data(), 0);
path = _path;

return true;
}

#pragma warning(pop)

#pragma warning(push)
#pragma warning(disable : 26445) // Suppress lifetime check for a reference to gsl::span or std::string_view

Expand Down
5 changes: 5 additions & 0 deletions src/terminal/parser/OutputStateMachineEngine.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ namespace Microsoft::Console::VirtualTerminal
SetWindowTitle = 2,
SetWindowProperty = 3, // Not implemented
SetColor = 4,
SetWorkingDirectory = 7, // No functionality is implemented
Hyperlink = 8,
ConEmuAction = 9,
SetForegroundColor = 10,
Expand All @@ -185,6 +186,10 @@ namespace Microsoft::Console::VirtualTerminal
std::wstring& content,
bool& queryClipboard) const noexcept;

bool _ParseFileUri(const std::wstring_view string,
std::wstring& hostname,
std::wstring& path) const;

static constexpr std::wstring_view hyperlinkIDParameter{ L"id=" };
bool _ParseHyperlink(const std::wstring_view string,
std::wstring& params,
Expand Down