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

Correct the cursor invalidation region #14661

Merged
1 commit merged into from
Jan 11, 2023

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Jan 10, 2023

Depending on the line rendition, and whether the cursor is over a wide
character or not, it's possible for the width to take up anywhere from 1
to 4 cells. And when it's more than 1 cell wide, part of the cursor may
end up off screen. However, our bounds check requires the entire cursor
to be on screen, otherwise it doesn't render anything, and that can
result in cursor droppings being left behind. This PR fixes that.

The bounds check that is causing this issue was introduced in #13001 to
fix a debug assertion.

To fix this, I've removed the bounds checking, and instead clip the
cursor rect to the boundaries of the viewport. This is what the original
code was trying to do prior to the #13001 fix, but was mistakenly using
the Viewport:Clamp method, instead of TrimToViewport. Since this
implementation doesn't require a clamp, there should be no risk of the
debug assertion returning.

Validation Steps Performed

I've confirmed that the test case in #14657 is now working correctly,
and not leaving cursor droppings behind. I've also tested under conhost
with buffer sizes wider than the viewport, and confirmed it can handle a
wide cursor partially scrolled off screen.

Closes #14657

@ghost ghost added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Jan 10, 2023
@j4james j4james marked this pull request as ready for review January 11, 2023 01:05
@DHowett
Copy link
Member

DHowett commented Jan 11, 2023

Excellent fix, thanks!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 11, 2023
@ghost
Copy link

ghost commented Jan 11, 2023

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 f1090a0 into microsoft:main Jan 11, 2023
@j4james j4james deleted the fix-wide-cursor-droppings branch January 12, 2023 16:13
@ghost
Copy link

ghost commented Jan 24, 2023

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

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cursor droppings on wide chars in Windows Terminal
3 participants