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

Restore the DECCTR color table report over conpty #13227

Merged
4 commits merged into from Jun 10, 2022

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Jun 4, 2022

Summary of the Pull Request

Up to now we haven't supported passing DCS sequences over conpty, so
any DCS operations would just be ignored in Windows Terminal. This PR
introduces a mechanism whereby we can selectively pass through
operations that could reasonably be handled by the connected terminal
without interfering with the regular conpty renderer. For now this is
just used to support restoring the DECCTR color table report.

References

Support for DECCTR was originally added to conhost in PR #13139.

PR Checklist

  • Closes Add support for passing DCS sequences over conpty #13223
  • CLA signed.
  • Tests added/passed
  • Documentation updated.
  • Schema updated.
  • I've discussed this with core contributors already. If not
    checked, I'm ready to accept this work might be rejected in favor of a
    different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

The way this works is we have a helper method in AdaptDispatch that
DCS operations can use to create a passthrough StringHandler for the
incoming data instead of their usual handler. To make this passthrough
process more efficient, the handler buffers the data before forwarding
it to conpty.

However, it's important that we aren't holding back data if output is
paused before the string terminator, so we need to flush the buffer
whenever we reach the end of the current write operation. This is
achieved by querying a new method in the StateMachine that lets us
know if we're currently dealing with the last character.

Another issue that came up was with the way the StateMachine caches
sequences that it might later need to forward to conpty. In the case of
string sequences like DCS, we don't want the actual string data cached
here, because that will just waste memory, and can also result in the
data being mistakenly output. So I've now disabled that caching when
we're in any of the string processing states.

Validation Steps Performed

I've manually confirmed that the DECCTR sequence can now update the
color table in Windows Terminal. I've also added a new unit test in
ConptyRoundtripTests to verify the sequence is being passed through
successfully.

@ghost ghost added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jun 4, 2022
@j4james j4james marked this pull request as ready for review June 4, 2022 20:32
@lhecker
Copy link
Member

lhecker commented Jun 5, 2022

I hope we can replace the character-wise parsing with wmemchr or std::find and the use of std::functions with static dispatch in the future. It would probably work great for DCS sequences.

@j4james
Copy link
Collaborator Author

j4james commented Jun 5, 2022

@lhecker I'm not sure I understand exactly what you're proposing, but I definitely agree that the current architecture isn't optimal, so I'd be happy to see improvements there. I suspect it might require a major rewrite of the StateMachine class though.

@lhecker
Copy link
Member

lhecker commented Jun 5, 2022

@lhecker I'm not sure I understand exactly what you're proposing, but I definitely agree that the current architecture isn't optimal, so I'd be happy to see improvements there. I suspect it might require a major rewrite of the StateMachine class though.

It's just that I saw this line:

return [&, buffer = std::wstring{}](const auto ch) mutable {

and it reminded me how we're currently limited to character-wise parsing.
We could search for the terminating ST using SIMD-searchers like std::find_n in the future (SIMD variants of that function ship with VS 2022 17.3 Preview 2). This would allow batch-wise processing which I imagine might make some things simpler or at least more performant. I haven't checked whether it's possible to do this, or how one would do this, but I'm definitely interested in making the VT parser more "formal" and adding optimization of that kind in if feasible.

@j4james
Copy link
Collaborator Author

j4james commented Jun 5, 2022

We could search for the terminating ST using SIMD-searchers like std::find_n in the future

Unfortunately it's a little more complicated than that. There are a bunch of characters we need to check for, including SUB, CAN, ESC, and potentially all of the C1 controls. But I think something along those lines could still be more efficient than our current approach. And if the DCS StringHandler could just be passed a substring of the output stream instead of one character at a time, we could avoid hacks like that IsProcessingLastCharacter method I just added.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I trust the combined brains of you and Leonard on the state transitions being correct here for SosPmApc, DcsPassThrough and DcsIgnore; thanks for doing this!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 10, 2022
@ghost
Copy link

ghost commented Jun 10, 2022

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit c754f4d into microsoft:main Jun 10, 2022
@ghost
Copy link

ghost commented Jul 6, 2022

🎉Windows Terminal Preview v1.15.186 has been released which incorporates this pull request.:tada:

Handy links:

@j4james j4james deleted the enhance-passthrough-decctr branch August 31, 2022 00:32
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for passing DCS sequences over conpty
3 participants