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 DA2 and DA3 device attributes reports #6850

Merged
4 commits merged into from
Jul 10, 2020

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Jul 9, 2020

This PR adds support for the DA2 (Secondary Device Attributes) and
DA3 (Tertiary Device Attributes) escape sequences, which are standard
VT queries reporting basic information about the terminal.

The Secondary Device Attributes response is made up of a number of
parameters:

  1. An identification code, for which I've used 0 to indicate that we
    have the capabilities of a VT100 (using code 0 for this is an XTerm
    convention, since technically DA2 would not have been supported by a
    VT100).
  2. A firmware revision level, which some terminal emulators use to
    report their actual version number, but I thought it best we just
    hardcode a value of 10 (the DEC convention for 1.0).
  3. Additional hardware options, which tend to be device specific, but
    I've followed the convention of the later DEC terminals using 1 to
    indicate the presence of a PC keyboard.

The Tertiary Device Attributes response was originally used to provide
a unique terminal identification code, and which some terminal emulators
use as a way to identify themselves. However, I think that's information
we'd probably prefer not to reveal, so I've followed the more common
practice of returning all zeros for the ID.

In terms of implementation, the only complication was the need to add an
additional code path in the OutputStateMachine to handle the > and
= intermediates (technically private parameter prefixes) that these
sequences require. I've done this as a single method - rather than one
for each prefix - since I think that makes the code easier to follow.

VALIDATION

I've added output engine tests to make sure the sequences are dispatched
correctly, and adapter tests to confirm that they are returning the
responses we expect. I've also manually confirmed that they pass the
Test of terminal reports in Vttest.

Closes #5836

@ghost ghost added Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase labels Jul 9, 2020
@j4james
Copy link
Collaborator Author

j4james commented Jul 9, 2020

For the record, I'm happy to consider returning a genuine version number in DA2 and some kind of ID in DA3 if that's preferable, but I thought we'd probably want to remain essentially anonymous. For more info on how these sequences are implemented and used, see issue #5836 (comment).

@j4james j4james marked this pull request as ready for review July 9, 2020 13:23
@DHowett
Copy link
Member

DHowett commented Jul 9, 2020

So, it looks like xterm (344) responds to DA2 as a "VT420", firmware version "344", 0 (^[[>41;344;0c) in the default configu shipped with Debian buster.

I know that Vim, as an example, uses the xterm version, but I don't know if it uses DA2 or \e[>0q to do so.

For what it's worth, I can't get that build of xterm to respond to \e[>0q, and vim fails to autodetect it ... 😄 so I'm guessing it doesn't use DA2.

Regardless; how much should we pretend to be xterm and how much should we not? Open question!

@DHowett
Copy link
Member

DHowett commented Jul 9, 2020

I do like the idea of remaining anonymous; I don't want people taking dependencies (like vim has done with xterm) on our implementation details especially when they change so rapidly in Terminal (and so slowly in Windows).
There's also no stable version number that we can use across Windows conhost and OpenConsole that would be monotonically increasing such that an application could rely on it.

@DHowett
Copy link
Member

DHowett commented Jul 9, 2020

I see now that you literally covered everything I just said, in the comment I opened to "read after I wrote my comments". Sorry. 😬

@j4james
Copy link
Collaborator Author

j4james commented Jul 9, 2020

Note that the \e[>0q sequence is a relative new thing. XTerm only added it in April (patch 354). I think it arose from discussions about standardising the TERM_PROGRAM and TERM_PROGRAM_VERSION environment variables (which is issue #1040 for us). But I'm in agreement with you that we're better off remaining anonymous and not supporting stuff like that until the app is more stable.

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 swear, you get fewer comments on your code reviews than the team does. I'm always impressed.

@DHowett DHowett requested a review from zadjii-msft July 9, 2020 20:14
@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Jul 10, 2020
@ghost ghost requested review from miniksa, carlos-zamora and leonMSFT July 10, 2020 19:37
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Yep. Wonderful. I agree with the "remaining anonymous" policy for now. We can always revisit in the future if need be, but once a version system is out there, it's almost impossible to retract.

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

ghost commented Jul 10, 2020

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 53b224b into microsoft:master Jul 10, 2020
@j4james j4james deleted the feature-device-attributes branch July 11, 2020 12:17
@ghost
Copy link

ghost commented Jul 22, 2020

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

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No support for xterm Send Device Attributes (Secondary DA) control sequence
3 participants