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.

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

More cursor droppings #8213

Closed
j4james opened this issue Nov 10, 2020 · 2 comments · Fixed by #8434
Closed

More cursor droppings #8213

j4james opened this issue Nov 10, 2020 · 2 comments · Fixed by #8434
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@j4james
Copy link
Collaborator

j4james commented Nov 10, 2020

Environment

Windows build number: Version 10.0.18362.1082
Windows Terminal version (if applicable): commit 64aa911

Steps to reproduce

  1. Build a recent version of OpenConsole.
  2. Using that build, open a bash shell in conhost.
  3. Open the console properties and go to the Layout section.
  4. Uncheck the Wrap text output on resize option.
  5. Set the Screen Buffer Width to 100 and the Window Width to 80.
  6. Save these settings and then execute the following command in the bash shell:
printf "\e[2 q\e[2J\e[1;82H"; sleep 1; printf "\e[5B \b"; sleep 5

What this sequence is doing:

  1. Set the cursor to a non-blinking block so it's easier to see.
  2. Clear the screen.
  3. Move the cursor position to column 82 which forces the viewport to scroll right.
  4. Sleep for a second so the cursor becomes visible at that position.
  5. Move the cursor down a few lines.
  6. Output a space and backspace to force the cursor to redraw.
  7. Sleep a few seconds so you can see the results.

Expected behavior

When the cursor moves down, it should be erased from line 1, and then redrawn on line 5.

Actual behavior

When the cursor moves down, it isn't erased from line 1, so you end up with two copies of the cursor.

image

What's happening is that when the cursor is moved, the SetConsoleCursorPositionImpl method adjusts the viewport to make sure the new position will be visible onscreen. Before that happens, though, there's a call to SCREEN_INFORMATION::MoveToBottom which forces the viewport X coordinate to zero. So at the time of the actual move calculation, it briefly appears as if the cursor is offscreen, and thus not visible, so the code doesn't trigger a repaint.

So in step 5 of the test case, when the cursor moves down, there's no repaint to hide the old position, and also actually no repaint to render the new position. It's only the output in step 6 that forces the cursor to repaint, but by that point it's too late to erase the old cursor position.

I think the fix for this is just to make MoveToBottom preserve the current X coordinate. This would actually improve the behaviour of the unwrapped viewport in general, so either way I think it's a good thing. However, the catch is that it's then more likely for the console to trigger the crash from issue #1976, so that needs to be fixed first (I think I have a solution for that too).

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Nov 10, 2020
@zadjii-msft
Copy link
Member

Oh gosh, welcome to the cursor turds rabbit hole 😄. Thanks for the investigation and the write up - best of luck to any who dare venture forth into madness

@zadjii-msft zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Conhost For issues in the Console codebase labels Nov 12, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 12, 2020
@zadjii-msft zadjii-msft added this to the Console Backlog milestone Nov 12, 2020
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 12, 2020
@ghost ghost added the In-PR This issue has a related PR label Nov 28, 2020
@ghost ghost closed this as completed in #8434 Nov 30, 2020
ghost pushed a commit that referenced this issue Nov 30, 2020
When the viewport is moved to the "virtual bottom" of the buffer (via
the `MoveToBottom` method), it is important that the horizontal viewport
offset be left as it is, otherwise that can result in some undesirable
side effects.

Since the VT coordinate system is relative to the top of the viewport,
many VT operations call the `MoveToBottom` method to make sure the
viewport is correctly positioned before proceeding. There is no need for
the horizontal position to be adjusted, though, since the X coordinates
are not constrained by the viewport, but are instead relative to the
underlying buffer.

Setting the viewport X coordinate to 0 in `MoveToBottom` (as we were
previously doing) could result in the cursor being pushed off screen.
And if the operation itself was moving the cursor, that would then
trigger another viewport move to bring the cursor back into view. These
conflicting movements meant the viewport was always forced as far left
as possible, and could also result in cursor "droppings" as the cursor
lost track of where it had been.

I've now fixed this by updating the `GetVirtualViewport` method to match
the horizontal offset of the active viewport, instead of having the X
coordinate hardcoded to 0. 

## Validation Steps Performed

I've manually confirmed that this fixes the cursor "droppings" test case
reported in issue #8213.

I've also added a screen buffer test that makes sure the `MoveToBottom`
method is working as expected, and not changing the horizontal viewport
offset when it moves down.

Closes #8213
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Nov 30, 2020
@ghost
Copy link

ghost commented Jan 28, 2021

🎉This issue was addressed in #8434, which has now been successfully released as Windows Terminal Preview v1.6.10272.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants