-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah good note! I assumed that everything terminal-parsing related was in |
||
|
||
StringHandler DownloadDRCS(const VTInt /*fontNumber*/, | ||
const VTParameter /*startChar*/, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. curious, did we merge this without code format somehow? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
@@ -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; | ||
|
There was a problem hiding this comment.
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:
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).
There was a problem hiding this comment.
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...!