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 querying DECAC with DECRQSS #13091

Closed
j4james opened this issue May 12, 2022 · 7 comments · Fixed by #14990
Closed

Add support for querying DECAC with DECRQSS #13091

j4james opened this issue May 12, 2022 · 7 comments · Fixed by #14990
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase
Milestone

Comments

@j4james
Copy link
Collaborator

j4james commented May 12, 2022

Description of the new feature/enhancement

This is a follow-up to PR #13058. Ideally we should be able to query the state of the DECAC color assignments using a DECRQSS query, so if an application changes those values, it can also restore them to what they were initially.

Proposed technical implementation details (optional)

The problem here is that DECAC works differently from most other DECRQSS queries. Typically there is only one potential response for a query, but in the case of DECAC we need to know whether to return the Normal Text colors, or the Window Frame colors.

This is what the VT525 manual has to say on the subject:

DECAC can be reported using DECRQSS and DECRPSS. Use Ps1 in DECRQSS to select the item to be reported.

But it's not clear to me what that means. Is the Ps1 meant to be included as a parameter value following the DCS introducer, or within the actual body of the string data? For example, if you're querying the window frame (color item 2), do you use this:

\eP2$q,|\e\\

or this:

\eP$q2,|\e\\

Of the modern terminal emulators I've tested, only three support DECAC in any capacity, and only two of those respond to a DECRQSS query for DECAC, but not in the same way. If I was forced to make a choice, I'd likely go with the second option, but I'm hoping we can get confirmation of the correct behavior from someone with a VT525.

@j4james j4james added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label May 12, 2022
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels May 12, 2022
@j4james
Copy link
Collaborator Author

j4james commented May 12, 2022

/cc @jerch If you get back to doing some testing on your VT525, this is one the escape sequences that I'd love to get more info on. And I'd be happy to put together some test scripts if that would help.

@jerch
Copy link

jerch commented May 13, 2022

/cc @jerch If you get back to doing some testing on your VT525, this is one the escape sequences that I'd love to get more info on. And I'd be happy to put together some test scripts if that would help.

Yes, if you have ready-to-go test scripts, that would certainly speed up the process, as I could simply run it and return you the output (without actually going into sequence semantics on my own, which is more time consuming).

@j4james
Copy link
Collaborator Author

j4james commented May 13, 2022

Thanks @jerch - that would be much appreciated. I've created a test script here:
https://gist.github.com/j4james/eca9bd29189f2abe851f206f1cbda0cd

When it runs, it'll log the responses to a file name decac.log, so it should hopefully be easy to report back. But note that it just appends to the end of the file each time, so you may need to delete it between runs if you rerun the test.

If possible, it would be good if you could assign the default text colors for the terminal to blue on white (more specifically index values 4 on 7) before running the test. You should be able to do this from the Setup menu: select Color, then Assign colors..., then configure the values for the Normal text option. This is not essential though - I'm just curious to see if that has any effect on the default parameters.

@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Help Wanted We encourage anyone to jump in on these. Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels May 16, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 16, 2022
@zadjii-msft zadjii-msft added this to the Backlog milestone May 16, 2022
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jun 13, 2022
@j4james
Copy link
Collaborator Author

j4james commented Mar 12, 2023

@al20878 I've just remembered this is another test I was hoping to get someone to run on a VT525, so while I'm bombarding you with requests, I'd be grateful if you could add this to the list. The comment above has some notes on how the test should ideally be run (see #13091 (comment)).

@al20878
Copy link

al20878 commented Mar 12, 2023

@j4james Downloaded the script, read the instructions, will try to follow them, and report back the results. Stay tuned ;-)

@al20878
Copy link

al20878 commented Mar 12, 2023

So I set the color as 4 on 7 and ran the script. Screen shot:

decac

Log:

decac.log

@j4james
Copy link
Collaborator Author

j4james commented Mar 12, 2023

@al20878 Thank you again. I think this has given me all the info I need.

And I'm grateful for the screenshot, because that's made me realise that our DECAC implementation is not quite correct. I had thought the color changes would apply to existing text on the screen, but it only seems to effect new output. Will need to rethink our architecture somewhat.

@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Mar 14, 2023
DHowett pushed a commit that referenced this issue 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
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Mar 17, 2023
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 Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants