Passthrough mode's input handling appears to have regressed #13106
Labels
Issue-Bug
It either shouldn't be doing this or needs an investigation.
Needs-Tag-Fix
Doesn't match tag requirements
Needs-Triage
It's a new issue that the core contributor team needs to triage at the next triage meeting
Resolution-Fix-Committed
Fix is checked in, but it might be 3-4 weeks until a release.
Comments
j4james
added
the
Issue-Bug
ghost
added
Needs-Tag-Fix
6 tasks
ghost
added
Resolution-Fix-Committed
ghost
pushed a commit
that referenced
this issue
May 17, 2022
## Summary of the Pull Request When the conpty passthrough mode is enabled, it often needs to send `DSR-CPR` queries (cursor position reports) to the client terminal to obtain the current cursor position. However, the code that originally handled the responses to these queries got broken by the refactoring of the `ConGetSet` API. This PR is an attempt to correct that regression. ## References The conpty passthrough mode was introduced in PR #11264. The refactoring that broke the cursor position handling was in PR #12703. ## PR Checklist * [x] Closes #13106 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] 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 Prior to the `ConGetSet` refactoring, the code that handled `DSR-CPR` responses (`InteractDispatch::MoveCursor`) would pass the cursor position to `ConGetSet::SetCursorPosition`, which in turn would forward it to the `SetConsoleCursorPositionImpl` API, and from there to the `VtIo` class. After the refactor, all of those intermediate steps were removed - the cursor was simply updated directly in `InteractDispatch::MoveCursor`, and the `VtIo` call was moved from `SetConsoleCursorPositionImpl` to `InteractDispatch` (since that was the only place it was actually required). However, when the conpty passthrough mode was introduced - which happened in parallel - it relied on the `SetConsoleCursorPositionImpl` API being called from `InteractDispatch` in order to handle its own `DSR-CPR` responses, and that's why things stopped working when the two PRs merged. So what I've done now is made `InteractDispatch::MoveCursor` method call `SetConsoleCursorPositionImpl` again (although without the intermediate `ConGetSet` overhead), and moved the `VtIo::SetCursorPosition` call back into `SetConsoleCursorPositionImpl`. This is not ideal, and there are still a bunch of problems with the `DSR-CPR` handling in passthrough mode, but it's at least as good as it was before. ## Validation Steps Performed I've just manually tested various shells with passthrough mode enabled, and confirmed that they're working better now. There are still issues, but nothing that wasn't already a problem in the initial implementation, at least as far as I can tell.
carlos-zamora
pushed a commit
that referenced
this issue
May 17, 2022
## Summary of the Pull Request When the conpty passthrough mode is enabled, it often needs to send `DSR-CPR` queries (cursor position reports) to the client terminal to obtain the current cursor position. However, the code that originally handled the responses to these queries got broken by the refactoring of the `ConGetSet` API. This PR is an attempt to correct that regression. ## References The conpty passthrough mode was introduced in PR #11264. The refactoring that broke the cursor position handling was in PR #12703. ## PR Checklist * [x] Closes #13106 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] 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 Prior to the `ConGetSet` refactoring, the code that handled `DSR-CPR` responses (`InteractDispatch::MoveCursor`) would pass the cursor position to `ConGetSet::SetCursorPosition`, which in turn would forward it to the `SetConsoleCursorPositionImpl` API, and from there to the `VtIo` class. After the refactor, all of those intermediate steps were removed - the cursor was simply updated directly in `InteractDispatch::MoveCursor`, and the `VtIo` call was moved from `SetConsoleCursorPositionImpl` to `InteractDispatch` (since that was the only place it was actually required). However, when the conpty passthrough mode was introduced - which happened in parallel - it relied on the `SetConsoleCursorPositionImpl` API being called from `InteractDispatch` in order to handle its own `DSR-CPR` responses, and that's why things stopped working when the two PRs merged. So what I've done now is made `InteractDispatch::MoveCursor` method call `SetConsoleCursorPositionImpl` again (although without the intermediate `ConGetSet` overhead), and moved the `VtIo::SetCursorPosition` call back into `SetConsoleCursorPositionImpl`. This is not ideal, and there are still a bunch of problems with the `DSR-CPR` handling in passthrough mode, but it's at least as good as it was before. ## Validation Steps Performed I've just manually tested various shells with passthrough mode enabled, and confirmed that they're working better now. There are still issues, but nothing that wasn't already a problem in the initial implementation, at least as far as I can tell. (cherry picked from commit e1086de) Service-Card-Id: 82005205 Service-Version: 1.14
|
🎉This issue was addressed in #13109, which has now been successfully released as Handy links: |
This issue was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Windows Terminal version
Commit 6ffc3dc
Windows build number
10.0.19041.1415
Other Software
No response
Steps to reproduce
passthroughModeopton.Expected Behavior
Something reasonably normal.
Actual Behavior
After every keypress the cursor position jumps forward several characters.
I thought at first that this was because passthrough mode was still a work in progress, since the first time I got to test it it was already broken, but I've since gone back to the revision where it was first added, and it was working fine at that point. It looks to me like I broke it in PR #12703, although I'm not yet sure how.
The text was updated successfully, but these errors were encountered: