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

Add support for the VSCode mark sequences* #15727

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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 src/terminal/adapter/ITermDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ class Microsoft::Console::VirtualTerminal::ITermDispatch
virtual bool DoITerm2Action(const std::wstring_view string) = 0;

virtual bool DoFinalTermAction(const std::wstring_view string) = 0;
virtual bool DoXtermJsAction(const std::wstring_view string) = 0;

virtual StringHandler DownloadDRCS(const VTInt fontNumber,
const VTParameter startChar,
Expand Down
7 changes: 7 additions & 0 deletions src/terminal/adapter/adaptDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3729,6 +3729,13 @@ bool AdaptDispatch::DoFinalTermAction(const std::wstring_view string)
return false;
}

// We literally only implement the xterm.js sequences that are aliases for the
// final term ones. Just implement exactly the same.
bool AdaptDispatch::DoXtermJsAction(const std::wstring_view string)
{
return DoFinalTermAction(string);
}
Comment on lines +3732 to +3737
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding another interface method, we can also do this:

case OscActionCodes::FinalTermAction:
case OscActionCodes::XtermJsAction:
    success = _dispatch->DoFinalTermAction(string);
    break;

While I understand that they aren't ideologically the same thing, they're semantically identical at the moment. As such I believe that the split has no "tangible" benefit right now and I think it would be better without it (in other similar places as well for the same reason).

Copy link
Member

Choose a reason for hiding this comment

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

That would make this review so so much shorter...!


// Method Description:
// - DECDLD - Downloads one or more characters of a dynamically redefinable
// character set (DRCS) with a specified pixel pattern. The pixel array is
Expand Down
1 change: 1 addition & 0 deletions src/terminal/adapter/adaptDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ namespace Microsoft::Console::VirtualTerminal
bool DoITerm2Action(const std::wstring_view string) override;

bool DoFinalTermAction(const std::wstring_view string) override;
bool DoXtermJsAction(const std::wstring_view string) override;

StringHandler DownloadDRCS(const VTInt fontNumber,
const VTParameter startChar,
Expand Down
1 change: 1 addition & 0 deletions src/terminal/adapter/termDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ class Microsoft::Console::VirtualTerminal::TermDispatch : public Microsoft::Cons
bool DoITerm2Action(const std::wstring_view /*string*/) override { return false; }

bool DoFinalTermAction(const std::wstring_view /*string*/) override { return false; }
bool DoXtermJsAction(const std::wstring_view /*string*/) override { return false; }
Copy link
Member

Choose a reason for hiding this comment

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

I'd call them VS Code sequences as they're not built into xterm.js or mentioned anywhere in the repo

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good note! I assumed that everything terminal-parsing related was in xterm.js itself.


StringHandler DownloadDRCS(const VTInt /*fontNumber*/,
const VTParameter /*startChar*/,
Expand Down
8 changes: 7 additions & 1 deletion src/terminal/parser/OutputStateMachineEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,8 @@ bool OutputStateMachineEngine::ActionVt52EscDispatch(const VTID id, const VTPara
bool OutputStateMachineEngine::ActionCsiDispatch(const VTID id, const VTParameters parameters)
{
// Bail out if we receive subparameters, but we don't accept them in the sequence.
if (parameters.hasSubParams() && !_CanSeqAcceptSubParam(id, parameters)) [[unlikely]]
if (parameters.hasSubParams() && !_CanSeqAcceptSubParam(id, parameters))
[[unlikely]]
Comment on lines +420 to +421
Copy link
Member

Choose a reason for hiding this comment

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

curious, did we merge this without code format somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/microsoft/terminal/blame/6873c85d2d8c818c2c5ef99971f370c447ebd2de/src/terminal/parser/OutputStateMachineEngine.cpp#L419 seems to show that this came from #15648, and that format passed.

I honestly have no idea why it thinks both are right?

{
return false;
}
Expand Down Expand Up @@ -868,6 +869,11 @@ bool OutputStateMachineEngine::ActionOscDispatch(const wchar_t /*wch*/,
success = _dispatch->DoFinalTermAction(string);
break;
}
case OscActionCodes::XtermJsAction:
{
success = _dispatch->DoXtermJsAction(string);
break;
}
default:
// If no functions to call, overall dispatch was a failure.
success = false;
Expand Down
1 change: 1 addition & 0 deletions src/terminal/parser/OutputStateMachineEngine.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ namespace Microsoft::Console::VirtualTerminal
ResetBackgroundColor = 111, // Not implemented
ResetCursorColor = 112,
FinalTermAction = 133,
XtermJsAction = 633,
ITerm2Action = 1337,
};

Expand Down