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 basic support for the DECRQSS settings query #11152

Merged
5 commits merged into from Sep 8, 2021

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Sep 5, 2021

This PR adds support for the DECRQSS (Request Selection or Setting)
escape sequence, which is a standard VT query for reporting the state of
various control functions. This initial implementation only supports
queries for the DECSTBM margins, and the SGR graphic rendition
attributes.

This can be useful for certain forms of capability detection (#1040). As
one example in particular, it can serve as an alternative to the
COLORTERM environment variable for detecting truecolor support
(#11057).

Of the settings that can be queried by DECRQSS, the only other one
that we could be supporting at the moment is DECSCUSR (Cursor Style).
However, that would require passing the query through to the conpty
client, which is a lot more complicated, so I thought it best to leave
for a future PR.

For now this gets the basic framework in place, so we are at least
responding to queries, and even just supporting the SGR attributes
query is useful in itself.

Validation

I've added a unit test verifying the reports for the DECSTBM and SGR
settings with a range of different parameters. I've also tested the
DECSTBM and SGR reports manually in Vttest, under menu 11.2.5.3.6
(Status-String Reports).

@@ -141,6 +141,8 @@ class Microsoft::Console::VirtualTerminal::ITermDispatch
const DispatchTypes::DrcsFontUsage fontUsage,
const VTParameter cellHeight,
const DispatchTypes::DrcsCharsetSize charsetSize) = 0; // DECDLD

virtual StringHandler RequestSetting() = 0; // DECRQSS
Copy link
Member

Choose a reason for hiding this comment

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

/cc @oising we were chatting about how to indicate what "sub-things" we support for things that have multiple .. well, sub-things.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I'm cool with this, just the one question

const auto iterator = std::back_insert_iterator(response);
if (color.IsIndex16())
{
const auto index = XtermToWindowsIndex(color.GetIndex());
Copy link
Member

Choose a reason for hiding this comment

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

Is this conversion backwards? Won't GetIndex() return the windows index, which we'll want to use as a VT index?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Semantically yes, but in practice it's a bidirectional operation (if that's the right term). It's just swapping bits 0 and 2. I suppose we could create a couple of aliases for these functions to makes things more readable, but I really would like to just get rid of them one day.

Copy link
Member

Choose a reason for hiding this comment

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

well I'll be danged, that does happen to work the other way, doesn't it

const auto id = idBuilder->Finalize(ch);
switch (id)
{
case VTID('m'):
Copy link
Member

Choose a reason for hiding this comment

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

Why'd we use the raw constants here instead of the predefined ones like SGR_SetGraphicsRendition? Is it just because they live in an annoying-to-get-to place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I would have liked to use them. But maybe we can move them somewhere more accessible at some point in the future.

@DHowett
Copy link
Member

DHowett commented Sep 8, 2021

Thanks for doing this!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 8, 2021
@ghost
Copy link

ghost commented Sep 8, 2021

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 6140fd9 into microsoft:main Sep 8, 2021
@j4james j4james deleted the feature-decrqss branch September 26, 2021 15:54
@ghost
Copy link

ghost commented Oct 20, 2021

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

Handy links:

DHowett pushed a commit that referenced this pull request Mar 17, 2023
This PR adds support for querying the color indices set by the `DECAC`
control, using the existing `DECRQSS` implementation.

## References and Relevant Issues

The initial `DECRQSS` support was added in PR #11152.
The `DECAC` functionality was added in PR #13058, but at the time we
didn't know how to format the associated `DECRQSS` query.

## Detailed Description of the Pull Request / Additional comments

For most `DECRQSS` queries, the setting being requested is identified by
the final characters of its escape sequence. However, for the `DECAC`
settings, you also need to include a parameter value, to indicate which
color item you're querying.

This meant we needed to extend the `DECRQSS` parser, so I also took this
opportunity to ensure we correctly parsed any parameter prefix chars. We
don't yet support any setting requiring a prefix, but this makes sure we
don't respond incorrectly if an app does query such a setting.

## Validation Steps Performed

Thanks to @al20878, we've been able to test how these queries are parsed
on a real VT525 terminal, and I've manually verified our implementation
matches that behavior.

I've also extended the existing `DECRQSS` unit test to confirm that we
are responding to the `DECAC` queries as expected.

Closes #13091
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants