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

Retain horizontal viewport offset when moving to bottom #8434

Merged
2 commits merged into from
Nov 30, 2020

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Nov 28, 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 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 28, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Seems pretty straightforward to me. Thanks!

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.

Yeah, this seems totally reasonable. Thanks!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 30, 2020
@ghost
Copy link

ghost commented Nov 30, 2020

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 90316be into microsoft:main Nov 30, 2020
@j4james j4james deleted the fix-movetobottom branch December 3, 2020 19:09
@ghost
Copy link

ghost commented Jan 28, 2021

🎉Windows Terminal Preview v1.6.10272.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Jan 28, 2021
This pull request 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 AutoMerge Marked for automatic merge by the bot when requirements are met 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More cursor droppings
3 participants