starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-03 21:25:34 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Parameter values of more than 5 digits are not supported #534

New Issue
Closed
opened 2026-01-30 21:54:32 +00:00 by claunia · 10 comments
claunia commented 2026-01-30 21:54:32 +00:00
Owner
Copy Link

Originally created by @j4james on GitHub (Jan 21, 2019).

Originally assigned to: @j4james, @zadjii-msft on GitHub.

  • Your Windows build number:

Microsoft Windows [Version 10.0.17763.195]

  • What you're doing and what's happening:

I was trying to see how well the October update of the console dealt with Vttest, and also determine the cause of any failures. In most cases it was just a matter of missing functionality (which I can understand might not be a priority), but the "leading zeros" test (the last screen of the "Test of cursor movements") feels like it could be a bug.

The point of the test was to determine whether leading zeros were supported in parameter values. And while the Windows console does handle them some of the time, it fails when the parameter value is more than 5 digits long. For example, consider these two sequences for setting the text color to green:

printf "\033[00032mGREEN\033[m\n"
printf "\033[000032mGREEN?\033[m\n"

The first one works (3 leading zeros, total length 5), but the second one fails (4 leading zeros, total length 6). This is a screenshot of those sequences in the WSL bash shell:

image

And for comparison, this is what they look like in XTerm:

image

Most of the time I'd expect this kind of edge case to be handled differently by everyone, and not worth worrying about, but this is one case that actually seems to be quite consistently implemented - every terminal emulator I've tried has supported an unlimited number of leading zeros.

  • What's wrong / what should be happening instead:

I'd expect the console to handle any number of leading zeros in an escape sequence parameter value. 11 digits ought to be enough to pass the current version of Vttest, but I think it would be preferable if there were no limit to the length, since that's what everyone else seems to support.

Originally created by @j4james on GitHub (Jan 21, 2019). Originally assigned to: @j4james, @zadjii-msft on GitHub. * Your Windows build number: Microsoft Windows [Version 10.0.17763.195] * What you're doing and what's happening: I was trying to see how well the October update of the console dealt with [Vttest](https://invisible-island.net/vttest/vttest.html), and also determine the cause of any failures. In most cases it was just a matter of missing functionality (which I can understand might not be a priority), but the "leading zeros" test (the last screen of the "Test of cursor movements") feels like it could be a bug. The point of the test was to determine whether leading zeros were supported in parameter values. And while the Windows console does handle them some of the time, it fails when the parameter value is more than 5 digits long. For example, consider these two sequences for setting the text color to green: printf "\033[00032mGREEN\033[m\n" printf "\033[000032mGREEN?\033[m\n" The first one works (3 leading zeros, total length 5), but the second one fails (4 leading zeros, total length 6). This is a screenshot of those sequences in the WSL bash shell: ![image](https://user-images.githubusercontent.com/4181424/51447598-b46ce700-1d17-11e9-8f7f-3cc52cd2c7a5.png) And for comparison, this is what they look like in XTerm: ![image](https://user-images.githubusercontent.com/4181424/51447613-d2d2e280-1d17-11e9-8766-df966fb33554.png) Most of the time I'd expect this kind of edge case to be handled differently by everyone, and not worth worrying about, but this is one case that actually seems to be quite consistently implemented - every terminal emulator I've tried has supported an unlimited number of leading zeros. * What's wrong / what should be happening instead: I'd expect the console to handle any number of leading zeros in an escape sequence parameter value. 11 digits ought to be enough to pass the current version of Vttest, but I think it would be preferable if there were no limit to the length, since that's what everyone else seems to support.
claunia commented 2026-01-30 21:54:35 +00:00
Author
Owner
Copy Link

@miniksa commented on GitHub (Jan 22, 2019):

I've filed MSFT:20265729 to handle this. You're right, we stop parsing after a certain number of numbers and consider it an error.

Our overall goal is to be as consistent with everyone else as possible or failing that, as consistent as possible with Xterm.

@miniksa commented on GitHub (Jan 22, 2019): I've filed MSFT:20265729 to handle this. You're right, we stop parsing after a certain number of numbers and consider it an error. Our overall goal is to be as consistent with everyone else as possible or failing that, as consistent as possible with Xterm.
claunia commented 2026-01-30 21:54:36 +00:00
Author
Owner
Copy Link

@FarPointer commented on GitHub (May 9, 2019):

@j4james How'd you get vttest built on Windows? Did you need to use Cygwin?

@FarPointer commented on GitHub (May 9, 2019): @j4james How'd you get vttest built on Windows? Did you need to use Cygwin?
claunia commented 2026-01-30 21:54:36 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (May 9, 2019):

If I had to guess, he was just running it under WSL - that's how I do it.

@zadjii-msft commented on GitHub (May 9, 2019): If I had to guess, he was just running it under WSL - that's how I do it.
claunia commented 2026-01-30 21:54:37 +00:00
Author
Owner
Copy Link

@FarPointer commented on GitHub (May 9, 2019):

Ah, yes, I could have discerned that from the screenshots and "WSL" callout above

@FarPointer commented on GitHub (May 9, 2019): Ah, yes, I could have discerned that from the screenshots and "WSL" callout above
claunia commented 2026-01-30 21:54:37 +00:00
Author
Owner
Copy Link

@DHowett-MSFT commented on GitHub (May 18, 2019):

For the interested:

b726a3d05d/src/terminal/parser/stateMachine.cpp (L444-L448)

There's a comment here in the VT parsing state machine about storing things in a short. If you want to work on this, be careful!

@DHowett-MSFT commented on GitHub (May 18, 2019): For the interested: https://github.com/microsoft/terminal/blob/b726a3d05d35928becc8313f6ac5536c9f503645/src/terminal/parser/stateMachine.cpp#L444-L448 There's a comment here in the VT parsing state machine about storing things in a `short`. If you want to work on this, be careful!
claunia commented 2026-01-30 21:54:38 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Jun 9, 2019):

I've just had a chance to play with the code this weekend, and this looks to me like it could be a trivial fix. If the total value accumulated is zero, then you know you've only received a zero prefix, so there is no need to increment the iParamAccumulatePos. This means you can have as many leading zeros as you want without impacting the 5 digit limit.

It just requires a slight reordering of the code and a condition on the increment. So the digit storage and accumulation would look something like this:

// store the digit in the 1s place.
*_pusActiveParam += usDigit;

// if the total is zero, it must be a leading zero digit, so we don't count it.
if (*_pusActiveParam != 0)
{
    // otherwise mark that we've now stored another digit.
    _iParamAccumulatePos++;
}

And then similar changes for the OSC parameter processing.

Does this seem like a reasonable approach? And if so, is it worthwhile me creating a PR for it?

@j4james commented on GitHub (Jun 9, 2019): I've just had a chance to play with the code this weekend, and this looks to me like it could be a trivial fix. If the total value accumulated is zero, then you know you've only received a zero prefix, so there is no need to increment the iParamAccumulatePos. This means you can have as many leading zeros as you want without impacting the 5 digit limit. It just requires a slight reordering of the code and a condition on the increment. So the digit storage and accumulation would look something like this: // store the digit in the 1s place. *_pusActiveParam += usDigit; // if the total is zero, it must be a leading zero digit, so we don't count it. if (*_pusActiveParam != 0) { // otherwise mark that we've now stored another digit. _iParamAccumulatePos++; } And then similar changes for the OSC parameter processing. Does this seem like a reasonable approach? And if so, is it worthwhile me creating a PR for it?
claunia commented 2026-01-30 21:54:39 +00:00
Author
Owner
Copy Link

@DHowett-MSFT commented on GitHub (Jun 9, 2019):

That sounds good to me! Especially so long as you add tests 😄

@zadjii-msft is the code owner here, so he should be able to help.

@j4james: would you like me to assign this issue to you?

@DHowett-MSFT commented on GitHub (Jun 9, 2019): That sounds good to me! Especially so long as you add tests :smile: @zadjii-msft is the code owner here, so he should be able to help. @j4james: would you like me to assign this issue to you?
claunia commented 2026-01-30 21:54:39 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Jun 9, 2019):

That seems reasonable to me!

@zadjii-msft commented on GitHub (Jun 9, 2019): That seems reasonable to me!
claunia commented 2026-01-30 21:54:39 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Jun 9, 2019):

@DHowett-MSFT I haven't looked at the testing side of things yet, but I think I can probably figure it out. So yes please do assign to me.

@j4james commented on GitHub (Jun 9, 2019): @DHowett-MSFT I haven't looked at the testing side of things yet, but I think I can probably figure it out. So yes please do assign to me.
claunia commented 2026-01-30 21:54:39 +00:00
Author
Owner
Copy Link

@DHowett-MSFT commented on GitHub (Jun 9, 2019):

There you go! Thanks for looking into this.

@DHowett-MSFT commented on GitHub (Jun 9, 2019): There you go! Thanks for looking into this.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-VT Help Wanted Issue-Bug Needs-Tag-Fix Product-Conhost Work-Item
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#534
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?