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

Allow trailing semicolon when parsing OSC 9;4 #10024

Merged
3 commits merged into from
May 5, 2021
Merged

Allow trailing semicolon when parsing OSC 9;4 #10024

3 commits merged into from
May 5, 2021

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented May 3, 2021 •
edited by carlos-zamora
Loading

Summary of the Pull Request

When we parse OSC 9;4, allow a trailing semicolon (i.e. allow 9;4; or something like 9;4;3;).

PR Checklist

Validation Steps Performed

OSC 9;4 sequences with or without trailing semicolons work

@ghost ghost added Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels May 3, 2021
zadjii-msft
zadjii-msft requested changes May 3, 2021
View reviewed changes
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.

Can we get the cases from #9960 (comment) added as tests? Those are some great edge cases. Either in TerminalBufferTests or ControlCoreTests

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 3, 2021
@PankajBhojwani
Copy link
Contributor Author

PankajBhojwani commented May 3, 2021 •
edited
Loading

Can we get the cases from #9960 (comment) added as tests? Those are some great edge cases. Either in TerminalBufferTests or ControlCoreTests

I actually planned to handle those in a follow up, since implementing those requires our dispatcher to ask the core for information regarding the current taskbar state, and so is a little more involved than this trailing semicolon issue Oops no I was being silly, will add them in

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 3, 2021
@PankajBhojwani
Copy link
Contributor Author

Tests added! I put them in TerminalApitTests because we already had some taskbar tests there, and I'd rather all of them be in one place

zadjii-msft
zadjii-msft approved these changes May 5, 2021
View reviewed changes
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.

Thanks for adding tests!

@carlos-zamora
Copy link
Member

@msftbot merge this in 1 hour

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 5, 2021
@ghost
Copy link

ghost commented May 5, 2021

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Wed, 05 May 2021 18:12:27 GMT, which is in 1 hour

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit f518235 into main May 5, 2021
@ghost ghost deleted the dev/pabhoj/parse_osc_9 branch May 5, 2021 18:13
DHowett pushed a commit that referenced this pull request May 24, 2021
@PankajBhojwani @DHowett
Allow trailing semicolon when parsing OSC 9;4 (#10024)
3c64fcd 
## Summary of the Pull Request
When we parse OSC 9;4, allow a trailing semicolon (i.e. allow `9;4;` or something like `9;4;3;`).

## PR Checklist
* [x] Closes #9960
* [X] Tests added/passed

## Validation Steps Performed
OSC 9;4 sequences with or without trailing semicolons work

(cherry picked from commit f518235)
@DHowett
Copy link
Member

DHowett commented May 24, 2021

YES i forgot to merge this for the 1.9 cut

YES i broke the seal and re-spun the build

@DHowett
Copy link
Member

DHowett commented May 24, 2021

oops, this was not even the right PR. What is even wrong with me

@ghost
Copy link

ghost commented May 25, 2021

🎉Windows Terminal Preview v1.9.1445.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request May 25, 2021
Closed
@DHowett DHowett mentioned this pull request May 26, 2021
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett DHowett DHowett approved these changes

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees
No one assigned
Labels
Area-VT Virtual Terminal sequence support AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incomplete support of ConEmu's OSC 9;4 sequences
4 participants
@PankajBhojwani @carlos-zamora @DHowett @zadjii-msft