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.

Sign up for GitHub

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

Jump to bottom

VT parser's "should process character by character" flag is a singleton #3108

Closed
DHowett-MSFT opened this issue Oct 8, 2019 · 1 comment · Fixed by #3110
Closed

VT parser's "should process character by character" flag is a singleton #3108

DHowett-MSFT opened this issue Oct 8, 2019 · 1 comment · Fixed by #3110
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal-1910

Comments

@DHowett-MSFT
Copy link
Contributor

terminal/src/terminal/parser/stateMachine.cpp

Lines 1351 to 1355 in 0691c21

static bool s_fProcessIndividually = false;
for (size_t cchCharsRemaining = cch; cchCharsRemaining > 0; cchCharsRemaining--)
{
if (s_fProcessIndividually)

Because of this, in...

  • conhost: VT input and VT output can cause eachother to switch into and out of stateful processing
  • Windows Terminal: Two tabs emitting VT at the same time can cause VT tearing, where the parser gives up partway through one tab's sequence because another tab completed parsing one.
@DHowett-MSFT DHowett-MSFT added Product-Conhost For issues in the Console codebase Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Priority-2 A description (P2) zInbox Bug labels Oct 8, 2019
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 8, 2019
@DHowett-MSFT DHowett-MSFT changed the title VT parser's "should process character by character" is a singleton VT parser's "should process character by character" flag is a singleton Oct 8, 2019
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 8, 2019
@DHowett-MSFT DHowett-MSFT added this to the Terminal-1911 milestone Oct 8, 2019
DHowett-MSFT pushed a commit that referenced this issue Oct 8, 2019
@DHowett
Move the VT parser's parse state into instanced storage
441b49d 
Fixes #3108
@DHowett-MSFT DHowett-MSFT mentioned this issue Oct 8, 2019
Merged
5 tasks
@ghost ghost added the In-PR This issue has a related PR label Oct 8, 2019
@DHowett-MSFT DHowett-MSFT mentioned this issue Oct 8, 2019
Closed
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Oct 9, 2019
DHowett-MSFT pushed a commit that referenced this issue Oct 9, 2019
@DHowett
Move the VT parser's parse state into instanced storage (#3110)
dd2fbef 
The VT parser used to be keeping a boolean used to determine whether it
was in bulk or single-character parse mode in a function-level static.
That turned out to not be great.

Fixes #3108; fixes #3073.
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Oct 9, 2019
@DHowett-MSFT DHowett-MSFT mentioned this issue Oct 14, 2019
Closed
DHowett-MSFT pushed a commit that referenced this issue Oct 17, 2019
@DHowett
Move the VT parser's parse state into instanced storage (#3110)
361e773 
The VT parser used to be keeping a boolean used to determine whether it
was in bulk or single-character parse mode in a function-level static.
That turned out to not be great.

Fixes #3108; fixes #3073.

(cherry picked from commit dd2fbef)
@ghost
Copy link

ghost commented Oct 23, 2019

🎉This issue was addressed in #3110, which has now been successfully released as Windows Terminal Preview v0.6.2951.0.:tada:

Handy links:

@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal-1910
Development

Successfully merging a pull request may close this issue.

Move the VT parser's parse state into instanced storage microsoft/terminal
2 participants
@DHowett-MSFT @zadjii-msft