Fix handling of cursor position reports in passthrough mode #13109
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
Issue-Bug
lhecker
approved these changes
May 16, 2022
| cursor.SetPosition(coordCursorShort); | ||
| cursor.SetHasMoved(true); | ||
| return true; | ||
| const auto api = gsl::not_null{ ServiceLocator::LocateGlobals().api }; |
There was a problem hiding this comment.
Is the gsl::not_null there because of the linter?
There was a problem hiding this comment.
Yeah. I was just trying to get the audit build to pass. But I was thinking that it might have been preferable to put this on the api field in the Globals class, because once we start auditing more of the code base, we'll assumedly get the C26429 error everywhere the api is used.
|
I had thought I might be able to fix some of the other problems with the CPR handling as part of this PR, but it turns out that's more complicated than I thought, so I'm just sticking with this basic patch for now. |
carlos-zamora
approved these changes
May 17, 2022
carlos-zamora
added
the
AutoMerge
|
Hello @carlos-zamora! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit
carlos-zamora
pushed a commit
that referenced
this pull request
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
|
🎉 Handy links: |
ghost
mentioned this pull request
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
When the conpty passthrough mode is enabled, it often needs to send
DSR-CPRqueries (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 theConGetSetAPI. 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
Detailed Description of the Pull Request / Additional comments
Prior to the
ConGetSetrefactoring, the code that handledDSR-CPRresponses (InteractDispatch::MoveCursor) would pass the cursor position toConGetSet::SetCursorPosition, which in turn would forward it to theSetConsoleCursorPositionImplAPI, and from there to theVtIoclass.After the refactor, all of those intermediate steps were removed - the cursor was simply updated directly in
InteractDispatch::MoveCursor, and theVtIocall was moved fromSetConsoleCursorPositionImpltoInteractDispatch(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
SetConsoleCursorPositionImplAPI being called fromInteractDispatchin order to handle its ownDSR-CPRresponses, and that's why things stopped working when the two PRs merged.So what I've done now is made
InteractDispatch::MoveCursormethod callSetConsoleCursorPositionImplagain (although without the intermediateConGetSetoverhead), and moved theVtIo::SetCursorPositioncall back intoSetConsoleCursorPositionImpl.This is not ideal, and there are still a bunch of problems with the
DSR-CPRhandling 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.