-
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
Cursor droppings are back in conhost #12739
Comments
DROP EVERYTHING, THEY'RE BACK. Thanks for noticing! Definitely can't let these back into the world 😝 |
I think this was broken by the deferred cursor drawing introduced in PR #10394. I can fix it by removing the terminal/src/host/outputStream.cpp Lines 45 to 55 in f936c44
But then we lose the performance improvements we were getting from the PR. Hopefully there's a way to fix it without undoing that completely. |
I've got a possible solution that doesn't require undoing the deferred drawing. We save the current cursor position in Hopefully the overhead of that additional maintenance doesn't outweigh the benefits we're getting from the deferred drawing, but I'm not the best person to evaluate the performance issues here. I'm also not positive this is going to work in all scenarios - need to play around with it some more. |
Fixing #12917 doesn't magically fix this 😢 Note: only repros when you're above the bottom of the viewport, so the view doesn't scroll |
Might get fixed by snapshots |
The main purpose of this PR was to merge the `ITerminalApi::PrintString` implementations into a shared method in `AdaptDispatch`, and avoid the VT code path depending on `WriteCharsLegacy`. But this refactoring has also fixed some bugs that existed in the original implementations. This helps to close the gap between the Conhost and Terminal (#13408). I started by taking the `WriteCharsLegacy` implementation, and stripping out everything that didn't apply to the VT code path. What was left was a fairly simple loop with the following steps: 1. Check if _delayed wrap_ is set, and if so, move to the next line. 2. Write out as much of the string as will fit on the current line. 3. If we reach the end of the line, set the _delayed wrap_ flag again. 4. Repeat the loop until the entire string has been output. But step 2 was a little more complicated than necessary because of its legacy history. It was copying the string into a temporary buffer, manually estimated how much of it would fit, and then passing on that partial buffer to the `TextBuffer::Write` method. In the new implementation, we just pass the entire string directly to `TextBuffer::WriteLine`, and that handles the clipping itself. The returned `OutputCellIterator` tells us how much of the string is left. This approach fixes some issues with wide characters, and should also cope better with future buffer enhancements. Another improvement from the new implementation is that the Terminal now handles delayed EOL wrap correctly. However, the downside of this is that it introduced a cursor-dropping bug that previously only affected conhost. I hadn't originally intended to fix that, but it became more of an issue now. The root cause was the fact that we called `cursor.StartDeferDrawing()` before outputting the text, and this was something I had adopted in the new implementation as well. But I've now removed that, and instead just call `cursor.SetIsOn(false)`. This seems to avoid the cursor droppings, and hopefully still has similar performance benefits. The other thing worth mentioning is that I've eliminated some special casing handling for the `ENABLE_VIRTUAL_TERMINAL_PROCESSING` mode and the `WC_DELAY_EOL_WRAP` flag in the `WriteCharsLegacy` function. They were only used for VT output, so aren't needed here anymore. ## Validation Steps Performed I've just been testing manually, writing out sample text both in ASCII and with wide Unicode chars. I've made sure it wraps correctly when exceeding the available space, but doesn't wrap when stopped at the last column, and with `DECAWM` disabled, it doesn't wrap at all. I've also confirmed that the test case from issue #12739 is now working correctly, and the cursor no longer disappears in Windows Terminal when writing to the last column (i.e. the delayed EOL wrap is working). Closes #780 Closes #6162 Closes #6555 Closes #12440 Closes #12739
🎉This issue was addressed in #14640, which has now been successfully released as Handy links: |
Windows Terminal version
OpenConsole 8a34a0e
Windows build number
10.0.19041.1415
Other Software
No response
Steps to reproduce
printf "\e[2 q\e[9999C "; sleep 1
The
CSI 2 q
is to get a non-blinking cursor which makes the issue easier to produce. TheCSI 9999C
moves the cursor to the end of the line, and the space initiates a delayed wrap. The sleep then gives the cursor a chance to be rendered before the next output will trigger the actual wrap.Expected Behavior
The output should wrap to the next line without leaving a visible cursor block behind.
Actual Behavior
The cursor at the end of the line isn't cleared when the output wraps, so you end up with two cursors visible on the screen.
This doesn't occur in my inbox version of conhost, so I think it might be a regression.
The text was updated successfully, but these errors were encountered: