Skip to content

ConPTY: Emit DSR CPR on resize#19089

Merged
lhecker merged 6 commits intomainfrom
dev/lhecker/18725-resize-cpr
Aug 6, 2025
Merged

ConPTY: Emit DSR CPR on resize#19089
lhecker merged 6 commits intomainfrom
dev/lhecker/18725-resize-cpr

Conversation

@lhecker
Copy link
Member

@lhecker lhecker commented Jul 1, 2025

This will help terminals with a reflow behavior unlike
the one implemented in ConPTY, such as VS Code.

Closes #18725

Validation Steps Performed

  • Tested under a debugger ✅
  • Use PowerShell 5 and reflow lines in the ConPTY buffer until they're
    pushed outside the top viewport. Then type something in the prompt.
    Cursor is in a consistent position, even if slightly off. ✅

lhecker
lhecker commented Jul 1, 2025
View reviewed changes
src/host/VtInputThread.cpp
// - inheritCursor - a bool indicating if the state machine should expect a
// cursor positioning sequence. See MSFT:15681311.
VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor) :
VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, std::function<void()> capturedCPR) :
Copy link
Member Author

@lhecker lhecker Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK if this approach is ideal.

@lhecker lhecker requested a review from DHowett July 9, 2025 12:43
DHowett
DHowett requested changes Jul 14, 2025
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i shall like to Noodle about this

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 14, 2025
@lhecker
Copy link
Member Author

lhecker commented Jul 21, 2025

[...] my tool generated this comment:

  1. std::make_unique does not return null. = Your diagnosis is wrong.
  2. That's true, but not an important issue. More importantly: Which classes and where? = Not helpful.
  3. Also true, but again: Where? = Not that helpful.
  4. This one is correct.
  5. This one is correct.
  6. This one is correct.
  7. There are only 2 places in this PR that change the header includes. Both of them properly group them. = Your diagnosis is wrong.
  8. Same as above.

[...] I'd be super grateful if you could quickly reply to these two yes/no questions:

  1. I can't speak for the other two, but it'd be a "no" for me. I've been using C++ for >2 decades so I can spot mere stylistic issues like this very easily. Even finding straight up bugs is only slightly more difficult. The real problem is finding overall implementation faults that span the entire project. Example: Could your tool have spotted the bug in Move ConPTY handoff logic into WindowEmperor #19088 which was only later found and fixed in Fix a crash during commandline handoff #19096? I suspect not. And it was a super simple bug too. We have subtle flaws in our PRs that sometimes take a year or so to finally fix. So far, not a single AI tool has been effective for me in this regard.
  2. I'd expect it to be inline review comments on the PR, so I know which lines of code you are referring to. But if you had done that without our agreement, I would've disliked that. Also, we have access to GitHub Copilot which offers the same thing your comment offers.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 21, 2025
DHowett
DHowett reviewed Jul 29, 2025
View reviewed changes
src/host/VtIo.cpp Outdated

void VtIo::RequestCursorPositionFromTerminal()
{
if (!_lookingForCursorPosition)
Copy link
Member

@DHowett DHowett Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, but nobody sets this to true?

Copy link
Member Author

@lhecker lhecker Jul 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The debouncing logic was broken so it was always sent. (It does get set to true at the top of the file though.)

DHowett
DHowett requested changes Jul 29, 2025
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% clear on how this all works together

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 29, 2025
@lhecker lhecker merged commit c55aca5 into main Aug 6, 2025
19 checks passed
@lhecker lhecker deleted the dev/lhecker/18725-resize-cpr branch August 6, 2025 11:06
lhecker added a commit that referenced this pull request Aug 11, 2025
@lhecker
Revert "ConPTY: Emit DSR CPR on resize (#19089)"
703e191 
This reverts commit c55aca5
because it is unaware of the VT state and may inject the DSR CPR
while e.g. a DCS is going on.
DHowett pushed a commit that referenced this pull request Aug 11, 2025
@lhecker
Revert "ConPTY: Emit DSR CPR on resize (#19089)" (#19237)
0c06490 
This reverts commit c55aca5
because it is unaware of the VT state and may inject the DSR CPR
while e.g. a DCS is going on.

Reopens #18725
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DHowett DHowett DHowett approved these changes

@carlos-zamora carlos-zamora carlos-zamora approved these changes

Assignees

No one assigned

Labels

Needs-Attention The core contributors need to come back around and look at this ASAP.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ConPTY: Ask for the cursor position after each resize

3 participants

@lhecker @DHowett @carlos-zamora