-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
Fix handling of cursor position reports in passthrough mode #13109
Conversation
cursor.SetPosition(coordCursorShort); | ||
cursor.SetHasMoved(true); | ||
return true; | ||
const auto api = gsl::not_null{ ServiceLocator::LocateGlobals().api }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the gsl::not_null
there because of the linter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks!
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 (
|
## 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: |
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 theConGetSet
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
Detailed Description of the Pull Request / Additional comments
Prior to the
ConGetSet
refactoring, the code that handledDSR-CPR
responses (InteractDispatch::MoveCursor
) would pass the cursor position toConGetSet::SetCursorPosition
, which in turn would forward it to theSetConsoleCursorPositionImpl
API, and from there to theVtIo
class.After the refactor, all of those intermediate steps were removed - the cursor was simply updated directly in
InteractDispatch::MoveCursor
, and theVtIo
call was moved fromSetConsoleCursorPositionImpl
toInteractDispatch
(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 fromInteractDispatch
in order to handle its ownDSR-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 callSetConsoleCursorPositionImpl
again (although without the intermediateConGetSet
overhead), and moved theVtIo::SetCursorPosition
call back intoSetConsoleCursorPositionImpl
.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.