Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Hidden cursors sometimes reappear unexpectedly #15449

Closed
j4james opened this issue May 26, 2023 · 5 comments · Fixed by #17148
Closed

Hidden cursors sometimes reappear unexpectedly #15449

j4james opened this issue May 26, 2023 · 5 comments · Fixed by #17148
Labels
Area-VT Virtual Terminal sequence support In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Conpty For console issues specifically related to conpty
Milestone
Backlog

Comments

@j4james
Copy link
Collaborator

j4james commented May 26, 2023

Windows Terminal version

1.18.1421.0

Windows build number

10.0.19045.2913

Other Software

No response

Steps to reproduce

I don't have a simple test case to reproduce this yet, but what I'm doing is playing a terminal game that hides the cursor on startup.

Expected Behavior

I shouldn't be able to see the cursor while the game is in progress.

Actual Behavior

9 times out of 10, the cursor is visible.

Having added some logging in AdaptDispatch, I can see that conhost is receiving a single DECTCEM sequence to hide the cursor, but what Windows Terminal ends up receiving is two requests to hide the cursor, followed by another request to show it again. I'm guessing this is the result of some conpty reordering that has gone wrong.

A git bisect suggests that this might be a regression caused by PR #14677. I'm not absolutely certain of that though, because the issue doesn't always reproduce 100% of the time.

I'll try and come up with a simple test case when I have more time, but I just wanted to get the issue filed before I forget about it.
@j4james j4james added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 26, 2023
@carlos-zamora carlos-zamora added Product-Conpty For console issues specifically related to conpty Area-VT Virtual Terminal sequence support and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 31, 2023
@carlos-zamora carlos-zamora added this to the Backlog milestone May 31, 2023
@j4james
Copy link
Collaborator Author

j4james commented Oct 1, 2023

I think this was fixed by PR #15991. Prior to commit 41f7ed7 I could reproduce the problem quite easily, but now it seems to be fine. I'm happy for this issue to be closed.

@lhecker
Copy link
Member

lhecker commented Nov 16, 2023

I'll be closing this issue then. 🙂

@lhecker lhecker closed this as completed Nov 16, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Nov 16, 2023
@j4james
Copy link
Collaborator Author

j4james commented Apr 23, 2024

@lhecker I've noticed that this bug is back again. I can reproduce in 1.20.10822.0 and the latest commit to main. A git bisect suggests it regressed in bdf2f6f, but I suspect now that it may be timing related, and the "fix" in 41f7ed7 and "regression" in bdf2f6f are just because the flushing affects the timing. So it's possible the bug was still there the whole time, and that commit isn't really to blame. I still don't have a simple test case, but I'm going to reopen the issue in the meantime so I don't forget about it.

@j4james j4james reopened this Apr 23, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Tag-Fix Doesn't match tag requirements label Apr 23, 2024
@lhecker
Copy link
Member

lhecker commented Apr 23, 2024 •
edited

Oww, that's unfortunate to hear. 😕 My series of PRs around the flushing of VtEngine really isn't particularly great, and I'm fairly dissatisfied with them. I couldn't think of an alternative approach to make it robust with any unknown escape sequence though and I kind of still can't. Maybe I'm missing something obvious.

I'm still eager to get rid of VtEngine entirely however 1, and I'm hoping to get it all done at some point within a year from now.

Footnotes

  1. I mentioned this before elsewhere, but what I mean is my idea of an algorithmic console API -> VT translation, and by using the ConPTY TextBuffers only for serving the ReadConsole* APIs. That'll certainly result in bugs (when it goes out of sync with the terminal), but I suspect that the total number of bugs then would still be less than the total number of bugs we have now, especially since it goes out of sync right now as well. ↩

@j4james
Copy link
Collaborator Author

j4james commented Apr 27, 2024

I finally figured out the source of the problem. It's this code:

terminal/src/renderer/vt/XtermEngine.cpp

Lines 111 to 117 in 378b659

// If the cursor was previously visible, let's hide it for this frame,
// by prepending a cursor off.
if (_lastCursorIsVisible != Tribool::False)
{
_buffer.insert(0, "\x1b[?25l");
_lastCursorIsVisible = Tribool::False;
}

If _noFlushOnEnd is set, the buffer won't necessarily be flushed at the end of every frame, so when the insert happens, we could have multiple frames pending in the buffer. This means the HideCursor sequence is inserted at the start of the wrong frame, and in my case it was being inserted before the ShowCursor sequence that it should have been cancelling.

It can easily be fixed just by tracking the buffer size at the start of the frame, and using that saved offset for the insert position here.

@j4james j4james mentioned this issue Apr 27, 2024
Merged
1 task
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Apr 27, 2024
github-merge-queue bot pushed a commit that referenced this issue Apr 29, 2024
@j4james
Fix the DECTCEM reset position in the conpty stream (#17148)
172229d 
## Summary of the Pull Request

When the conpty renderer determines that it needs to hide the cursor,
it does so by inserting a `DECTCEM` reset sequence at the start of the
output buffer, assuming that is the start of the frame. But when the
`_noFlushOnEnd` flag is set, you can have multiple frames pending in the
buffer, and the `DECTCEM` sequence will then end up in the wrong place.

This PR fixes the issue by saving the buffer size at the start of the
frame, and using that saved offset as the insert position for the
`DECTCEM` sequence.

## Validation Steps Performed

I have a game that was frequently affected by this issue (the cursor
would be visible when it was meant to be hidden). With this PR applied,
it now works perfectly.

## PR Checklist
- [x] Closes #15449
github-merge-queue bot pushed a commit that referenced this issue Apr 29, 2024
@j4james
Fix the DECTCEM reset position in the conpty stream (#17148)
ef318a1 
## Summary of the Pull Request

When the conpty renderer determines that it needs to hide the cursor,
it does so by inserting a `DECTCEM` reset sequence at the start of the
output buffer, assuming that is the start of the frame. But when the
`_noFlushOnEnd` flag is set, you can have multiple frames pending in the
buffer, and the `DECTCEM` sequence will then end up in the wrong place.

This PR fixes the issue by saving the buffer size at the start of the
frame, and using that saved offset as the insert position for the
`DECTCEM` sequence.

## Validation Steps Performed

I have a game that was frequently affected by this issue (the cursor
would be visible when it was meant to be hidden). With this PR applied,
it now works perfectly.

## PR Checklist
- [x] Closes #15449
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Apr 29, 2024
DHowett pushed a commit that referenced this issue Apr 30, 2024
@j4james @DHowett
Fix the DECTCEM reset position in the conpty stream (#17148)
244549a 
## Summary of the Pull Request

When the conpty renderer determines that it needs to hide the cursor,
it does so by inserting a `DECTCEM` reset sequence at the start of the
output buffer, assuming that is the start of the frame. But when the
`_noFlushOnEnd` flag is set, you can have multiple frames pending in the
buffer, and the `DECTCEM` sequence will then end up in the wrong place.

This PR fixes the issue by saving the buffer size at the start of the
frame, and using that saved offset as the insert position for the
`DECTCEM` sequence.

## Validation Steps Performed

I have a game that was frequently affected by this issue (the cursor
would be visible when it was meant to be hidden). With this PR applied,
it now works perfectly.

## PR Checklist
- [x] Closes #15449

(cherry picked from commit ef318a1)
Service-Card-Id: 92457089
Service-Version: 1.19
DHowett pushed a commit that referenced this issue Apr 30, 2024
@j4james @DHowett
Fix the DECTCEM reset position in the conpty stream (#17148)
12a5c95 
## Summary of the Pull Request

When the conpty renderer determines that it needs to hide the cursor,
it does so by inserting a `DECTCEM` reset sequence at the start of the
output buffer, assuming that is the start of the frame. But when the
`_noFlushOnEnd` flag is set, you can have multiple frames pending in the
buffer, and the `DECTCEM` sequence will then end up in the wrong place.

This PR fixes the issue by saving the buffer size at the start of the
frame, and using that saved offset as the insert position for the
`DECTCEM` sequence.

## Validation Steps Performed

I have a game that was frequently affected by this issue (the cursor
would be visible when it was meant to be hidden). With this PR applied,
it now works perfectly.

## PR Checklist
- [x] Closes #15449

(cherry picked from commit ef318a1)
Service-Card-Id: 92457090
Service-Version: 1.20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-VT Virtual Terminal sequence support In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Conpty For console issues specifically related to conpty
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Fix the DECTCEM reset position in the conpty stream j4james/terminal
3 participants
@lhecker @j4james @carlos-zamora