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 the DECRQCRA checksum report #14989

Merged
merged 5 commits into from
Mar 14, 2023

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Mar 14, 2023

This PR implements the DECRQCRA escape sequence, which lets you
request a checksum of a portion of the screen. This is most useful in
automated testing to verify that the generated screen content is what it
was expected to be.

For now this functionality is gated behind a feature flag which is only
enabled for dev builds.

Detailed Description of the Pull Request / Additional comments

I've done my best to match the DEC checksum algorithm as closely as
possible, which we've determined by testing on a real VT525 terminal
(many thanks to @al20878 for that).

The checksum is an unsigned 16-bit value that starts off at zero, and
from which you then subtract the ordinal value of every character in the
selected range. It's also affected by the rendition attributes in the
selected cells.

  • Bold/Intense - subtract 0x80
  • Blinking - subtract 0x40
  • Reverse video - subtract 0x20
  • Underlined - subtract 0x10
  • Invisible - subtract 0x08
  • Protected - subtract 0x04
  • Background color - subtract the color index
  • Foreground color - subtract the color index * 0x10

I should note that our ordinal calculation only matches DEC for the
characters in the ASCII and Latin-1 range, because the original
algorithm predates Unicode. If we want to support the other character
sets correctly we'll need custom mapping tables, but I didn't think that
was essential for now.

It's also worth mentioning that we don't handle "empty" cells correctly,
but that's not the fault of the checksum calculation - it's just that
our default fill character is a space rather than a NUL.

Validation Steps Performed

I've manually compared our implementation against the tests results that
@al20878 got from the VT525, and confirmed that we match as well as was
expected (i.e. taking into account the limitations mentioned above).

I've also added a few basic unit tests that verify we're generating the
expected checksums for the various renditions and color attributes.

Closes #14974

@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-Terminal The new Windows Terminal. labels Mar 14, 2023
@j4james j4james marked this pull request as ready for review March 14, 2023 02:45
@j4james
Copy link
Collaborator Author

j4james commented Mar 14, 2023

Not sure if the build is failing is because I've done something wrong with the feature flag configuration, or it's just one of the usual CI failures. But as far as I can see, it's complaining about a circular dependency in TerminalAzBridge.vcxproj, which I don't think is related to anything I've changed.

@zadjii-msft
Copy link
Member

Yea that looked like another #14581 hit to me. Weird that it hit there, but 🤷

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 think it's really impressive how this all came together. More than happy to check this in as a debugging tool ☺️

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.

This is a joy to behold!

@DHowett DHowett merged commit 931aa8c into microsoft:main Mar 14, 2023
@j4james j4james deleted the feature-decrqcra branch March 21, 2023 11:10
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-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for DECRQCRA
3 participants