Skip to content

Conversation

@j4james
Copy link
Collaborator

@j4james j4james commented Jun 9, 2019

Summary of the Pull Request

Adds support for any number of leading zeros in the parameter values of VT escape sequences. Tested manually, with Vttest, and with unit tests.

PR Checklist

Detailed Description of the Pull Request / Additional comments

In the StateMachine::_ActionParam and StateMachine::_ActionOscParam methods, the parser would treat any parameter number with more than 5 digits as being the maximum value. This PR add checks which prevent leading zeros from contributing to that count, so the 5 digit limit now only applies to significant digits.

Validation Steps Performed

  • I've tested in the WSL bash shell with escape sequences containing many leading zeros, e.g.:
    printf "\033[0000000032mGREEN?\033[m\n".
  • I've confirmed that the leading zeros test in Vttest now works (the last screen in the Test of cursor movements).
  • I've added a couple of unit tests in Microsoft::Console::VirtualTerminal::OutputEngineTest for both the CSI and OSC sequences.

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.

Great work!

@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support Product-Conhost For issues in the Console codebase labels Jun 10, 2019
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you!

@zadjii-msft zadjii-msft merged commit 19dbec8 into microsoft:master Jun 10, 2019
@j4james j4james deleted the fix-leading-zeros branch June 10, 2019 19:15
@DHowett-MSFT
Copy link
Contributor

Hey, this just went out in the inbox windows console with insider build 18932! Congrats!

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 Product-Conhost For issues in the Console codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parameter values of more than 5 digits are not supported

3 participants