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

Provide a more detailed Device Attributes report #14906

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Feb 25, 2023

This is an update of our Primary Device Attributes report, which better
indicates the feature extensions that we now support.

Detailed Description of the Pull Request / Additional comments

This first parameter of the response is 61, representing a conformance
level of 1. The subsequent parameters identify the supported feature
extensions.

1 = 132 column mode
6 = Selective erase
7 = Soft fonts
22 = Color text
23 = Greek character sets
24 = Turkish character sets
28 = Rectangular area operations
32 = Text macros
42 = ISO Latin-2 character set

Most of these features are handled entirely within AdaptDispatch, so
they apply to all clients. However, 132 column mode is only supported by
ConHost, so we don't report that for conpty clients.

And note that soft fonts won't necessarily work in all conpty clients,
but we don't have an easy way of determining that, so we just report
soft font support for everyone.

Validation Steps Performed

I've manually verified that the DA1 report is returning the expected
response in Vttest, both from ConHost and Windows Terminal.

I've also updated the DeviceAttributesTests in the adapter tests to
account for the new expected response.

Closes #14491

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-VT Virtual Terminal sequence support Product-Conhost For issues in the Console codebase labels Feb 25, 2023
@j4james j4james force-pushed the update-da-report branch from a891fe9 to 484d331 Compare March 1, 2023 01:33
@j4james
Copy link
Collaborator Author

j4james commented Mar 2, 2023

I really don't know what to do with this. It seems that sending a DA request to the conpty client is what is causing the feature tests to fail, assumedly because there isn't a full terminal on the other end of the conpty connection. If that's a scenario we actually need to support, then this approach is never going to work.

And I don't think adding a timeout is a realistic solution, because then we've just introduced a startup delay for this use case, which I don't think is acceptable.

We could pass a parameter telling conhost it's safe to make a DA request (like we do with inheritCursor) but then we might as well have passed the DA report as the actual parameter, and avoid the whole rigmarole of conhost calling back to the client to get that info.

For that matter, I'm not sure why we didn't do that with the inheritCursor option. Why don't we just pass the cursor coordinates on the command line, instead of telling conhost it needs to request the coordinates with a DSR-CPR query?

At this point I'm starting to think I should forget about trying to make this work over conpty and at least get the DA report working correctly in conhost. It'll still be an improvement for conpty clients, but we just won't be able to identify soft font support.

I should also mention that I'd hoped this process could serve as a model for querying other information from the conpty client, like the color table and cursor style. That would allow to support additional querying operations which aren't currently possible. The proposed screen reader DSR report in #14342 would be another use case.

But maybe the long term solution is that we need to get the passthrough mode working, and just live with current conpty limitations in the mean time.

@j4james
Copy link
Collaborator Author

j4james commented Apr 13, 2023

I've now dropped the feature testing on the conpty client and simply hardcoded the DA response to one of two values (ConHost has support for 132 column mode, and conpty clients don't).

This means that we could we reporting soft font support for some terminals that don't have that, but that seems like the least worst option at this point.

@j4james j4james marked this pull request as ready for review April 13, 2023 22:44
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.

SGTM! Sorry we let this one sit for so long!

@DHowett DHowett merged commit a49b5a6 into microsoft:main Apr 26, 2023
@j4james j4james deleted the update-da-report branch May 30, 2023 01:06
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 Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the Device Attributes report
3 participants