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

Known unresolved issues after removing VtEngine / adding VT passthrough #17643

Open
9 of 16 tasks
lhecker opened this issue Aug 1, 2024 · 0 comments · Fixed by #17668, #17666 or #17645
Open
9 of 16 tasks

Known unresolved issues after removing VtEngine / adding VT passthrough #17643

lhecker opened this issue Aug 1, 2024 · 0 comments · Fixed by #17668, #17666 or #17645
Assignees
@lhecker
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty
Milestone
Backlog

Comments

@lhecker
Copy link
Member

lhecker commented Aug 1, 2024 •
edited
Loading

Tasks
Preview Give feedback
  7. Original console buffer is not restored when closing a secondary one #17817
    Area-VT In-PR Issue-Bug Needs-Tag-Fix Product-Conpty
    lhecker
  13. Incorrect DECRQM for Win32InputMode #17737
    Area-Input Area-VT Issue-Bug Product-Conpty
  14. Crash when switching back from the alt buffer #17709
    Area-Output In-PR Issue-Bug Needs-Tag-Fix Product-Terminal Severity-Crash
    lhecker
  15. Some ScrollConsoleScreenBuffer directions have stopped working correctly #17801
    Area-VT In-PR Issue-Bug Needs-Tag-Fix Product-Conpty
    lhecker
  16. scroll-out in secondary console buffer pollutes main scrollback buffer #17874
    Issue-Bug Needs-Triage
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 1, 2024
@microsoft microsoft deleted a comment from similar-issues-ai bot Aug 1, 2024
@lhecker lhecker added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Aug 1, 2024
@lhecker lhecker added this to the Terminal v1.22 milestone Aug 1, 2024
@alokedesai alokedesai mentioned this issue Aug 7, 2024
Closed
@carlos-zamora carlos-zamora added Product-Conpty For console issues specifically related to conpty Area-VT Virtual Terminal sequence support labels Aug 7, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 7, 2024
@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 7, 2024
This was linked to pull requests Aug 7, 2024
Fix cooked read reflow under ConPTY #17668
Merged
Remove VT color quirk for PowerShell #17666
Merged
Address feedback from #17510 #17645
Merged
@lhecker lhecker self-assigned this Aug 8, 2024
@lhecker lhecker mentioned this issue Aug 20, 2024
Merged
@lhecker lhecker linked a pull request Aug 20, 2024 that will close this issue
Use DECCRA/DECFRA for ScrollConsoleScreenBuffer #17747
Merged
@lhecker lhecker removed a link to a pull request Aug 20, 2024
Use DECCRA/DECFRA for ScrollConsoleScreenBuffer #17747
Merged
lhecker added a commit that referenced this issue Aug 23, 2024
@lhecker
Use DECCRA/DECFRA for ScrollConsoleScreenBuffer (#17747)
040f261 
This adds logic to get the DA1 report from the hosting terminal on
startup. We then use the information to figure out if it supports
rectangular area operations. If so, we can use DECCRA/DECFRA to
implement ScrollConsoleScreenBuffer.

This additionally changes `ScrollConsoleScreenBuffer` to always
forbid control characters as the fill character, even in conhost
(via `VtIo::SanitizeUCS2`). My hope is that this makes the API
more consistent and robust as it avoids another source for
invisible control characters in the text buffer.

Part of #17643

## Validation Steps Performed
* New tests ✅
@j4james j4james mentioned this issue Sep 1, 2024
Merged
microsoft-github-policy-service bot pushed a commit that referenced this issue Sep 4, 2024
@j4james
Pass through DCS responses when VT input mode is disabled (#17845)
6e5827a 
## Summary of the Pull Request

When an app makes a VT request that returns a `DCS` response, and it
hasn't also enabled VT input mode, the new passthrough implementation
loses that response. All the app receives is an `Alt`+`\` key press
triggered by the `ST` terminator. This PR fixes that issue.

## References and Relevant Issues

This is one of the unresolved issues tracked in #17643.

## Detailed Description of the Pull Request / Additional comments

The way `DCS` sequences are handled in the input state machine engine is
by returning a nullptr from `ActionDcsDispatch`, which tells the state
machine to ignore that content. But the sequence is still buffered, and
when the `ST` terminator is eventually received, that buffer is flushed,
which passes the whole thing through to the app.

Originally this only worked when VT input mode was enabled, otherwise
the `ST` sequence is converted into a key press, and the buffered `DCS`
content is lost. The way it works now is we set a flag when the `DCS`
introducer is received, and if that flag is set when the `ST` arrives,
we know to trigger a flush rather a key press.

## Validation Steps Performed

I've tested a `DA3` request from the cmd shell (i.e. `echo ^[[=c`), and
confirmed that now works as expected. I've also hacked Windows Terminal
to disable win32-input mode, so I could check how it works with conpty
clients generating standard VT input, and confirmed that an `Alt`+`\`
keypress is still translated correctly.
DHowett pushed a commit that referenced this issue Sep 6, 2024
@j4james @DHowett
Pass through DCS responses when VT input mode is disabled (#17845)
385a9a6 
## Summary of the Pull Request

When an app makes a VT request that returns a `DCS` response, and it
hasn't also enabled VT input mode, the new passthrough implementation
loses that response. All the app receives is an `Alt`+`\` key press
triggered by the `ST` terminator. This PR fixes that issue.

## References and Relevant Issues

This is one of the unresolved issues tracked in #17643.

## Detailed Description of the Pull Request / Additional comments

The way `DCS` sequences are handled in the input state machine engine is
by returning a nullptr from `ActionDcsDispatch`, which tells the state
machine to ignore that content. But the sequence is still buffered, and
when the `ST` terminator is eventually received, that buffer is flushed,
which passes the whole thing through to the app.

Originally this only worked when VT input mode was enabled, otherwise
the `ST` sequence is converted into a key press, and the buffered `DCS`
content is lost. The way it works now is we set a flag when the `DCS`
introducer is received, and if that flag is set when the `ST` arrives,
we know to trigger a flush rather a key press.

## Validation Steps Performed

I've tested a `DA3` request from the cmd shell (i.e. `echo ^[[=c`), and
confirmed that now works as expected. I've also hacked Windows Terminal
to disable win32-input mode, so I could check how it works with conpty
clients generating standard VT input, and confirmed that an `Alt`+`\`
keypress is still translated correctly.

(cherry picked from commit 6e5827a)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgSwKBc
Service-Version: 1.22
@cpendery cpendery mentioned this issue Sep 13, 2024
Closed
@lhecker lhecker modified the milestones: Terminal v1.23, Backlog Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lhecker lhecker

Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty
Projects
None yet
Milestone
Backlog
2 participants
@lhecker @carlos-zamora