Skip to content

Commit

Permalink
Fix handling of cursor position reports in passthrough mode (#13109)
Browse files Browse the repository at this point in the history
## 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
  • Loading branch information
j4james authored and carlos-zamora committed May 17, 2022
1 parent 858359e commit bc9e43a
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 8 deletions.
4 changes: 4 additions & 0 deletions src/host/getset.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,10 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
position.Y < 0));
// clang-format on

// MSFT: 15813316 - Try to use this SetCursorPosition call to inherit the cursor position.
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
RETURN_IF_FAILED(gci.GetVtIo()->SetCursorPosition(position));

RETURN_IF_NTSTATUS_FAILED(buffer.SetCursorPosition(position, true));

LOG_IF_FAILED(ConsoleImeResizeCompStrView());
Expand Down
11 changes: 3 additions & 8 deletions src/terminal/adapter/InteractDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,10 @@ bool InteractDispatch::MoveCursor(const VTInt row, const VTInt col)

const auto coordCursorShort = til::unwrap_coord(coordCursor);

// MSFT: 15813316 - Try to use this MoveCursor call to inherit the cursor position.
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
RETURN_IF_FAILED(gci.GetVtIo()->SetCursorPosition(coordCursorShort));

// Finally, attempt to set the adjusted cursor position back into the console.
auto& cursor = _api.GetTextBuffer().GetCursor();
cursor.SetPosition(coordCursorShort);
cursor.SetHasMoved(true);
return true;
const auto api = gsl::not_null{ ServiceLocator::LocateGlobals().api };
auto& info = ServiceLocator::LocateGlobals().getConsoleInformation().GetActiveOutputBuffer();
return SUCCEEDED(api->SetConsoleCursorPositionImpl(info, coordCursorShort));
}

// Routine Description:
Expand Down

0 comments on commit bc9e43a

Please sign in to comment.