Skip to content

VT sequence support for EraseInLine, EraseInDisplay, DeleteCharacter and InsertCharacter #2144

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
Merged
PankajBhojwani merged 5 commits into from
Jul 30, 2019
Merged

VT sequence support for EraseInLine, EraseInDisplay, DeleteCharacter and InsertCharacter #2144

PankajBhojwani merged 5 commits into from
Jul 30, 2019

Conversation

@PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Jul 29, 2019 •
edited by DHowett-MSFT
Loading

Summary of the Pull Request

Implemented the following VT sequences:

  • EraseInLine
  • EraseInDisplay
  • DeleteCharacter
  • InsertCharacter

(Minor: implemented CursorUp and CursorBackward in TerminalDispatch with existing APIs)

References

Related to #1883

PR Checklist

  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Without these VT sequences, non-Conhost connections behave strangely (since they do not go through conpty for VT parsing).

Validation Steps Performed

Compared behaviour with putty/other terminals and imitated them

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Jul 30, 2019

nit: title. rephrase to be something imperative, as though you were completing the sentence:

if merged, this pull request will ...

zadjii-msft
zadjii-msft requested changes Jul 30, 2019
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.

I would love to see some tests added for these guys, but I understand that there's probably not time to add them now.
I've got a couple nit-level questions I want answered, but otherwise great work!

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 30, 2019
carlos-zamora
carlos-zamora reviewed Jul 30, 2019
View reviewed changes
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 30, 2019
@PankajBhojwani PankajBhojwani changed the title Vt sequences VT sequence support for EraseInLine, EraseInDisplay and DeleteCharacter Jul 30, 2019
miniksa
miniksa requested changes Jul 30, 2019
View reviewed changes
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 30, 2019
zadjii-msft
zadjii-msft approved these changes Jul 30, 2019
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.

Alright I'm happy with this

@PankajBhojwani PankajBhojwani changed the title VT sequence support for EraseInLine, EraseInDisplay and DeleteCharacter VT sequence support for EraseInLine, EraseInDisplay, DeleteCharacter and InsertCharacter Jul 30, 2019
miniksa
miniksa approved these changes Jul 30, 2019
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

🚀

DHowett-MSFT
DHowett-MSFT approved these changes Jul 30, 2019
View reviewed changes
src/buffer/out/textBuffer.cpp

coordEndOfText.X = static_cast<short>(pCurrRow->GetCharRow().MeasureRight()) - 1;
fDoBackUp = (coordEndOfText.X < 0 && coordEndOfText.Y > 0);
fDoBackUp = (coordEndOfText.X < 0 && coordEndOfText.Y > viewportTop);
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT Jul 30, 2019

Choose a reason for hiding this comment

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

Technically, should this make sure that X is within viewportRight? Or something equivalent?

Copy link
Contributor Author

@PankajBhojwani PankajBhojwani Jul 30, 2019

Choose a reason for hiding this comment

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

It seems like X will be -1 if there was no non-space text in the row (based on what MeasureRight) is doing, so I don't think its relative to where the viewport is

@PankajBhojwani PankajBhojwani merged commit 63df881 into microsoft:master Jul 30, 2019
@PankajBhojwani PankajBhojwani deleted the VTSeq branch July 30, 2019 23:28
@j4james j4james mentioned this pull request Jul 31, 2019
Closed
DHowett-MSFT pushed a commit that referenced this pull request Aug 6, 2019
@DHowett
VT sequence support for EraseInLine, EraseInDisplay, DeleteCharacter …
ddd413c 
…and InsertCharacter (#2144)

* We now support EraseInLine, EraseInDisplay, DeleteCharacter and InsertCharacter

(cherry picked from commit 63df881)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlos-zamora carlos-zamora carlos-zamora left review comments

@miniksa miniksa miniksa approved these changes

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

+1 more reviewer

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@PankajBhojwani @DHowett-MSFT @carlos-zamora @miniksa @zadjii-msft