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

Improve correctness of Sixel background fill #18260

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Nov 28, 2024

Summary of the Pull Request

This is an attempt to improve the correctness of the background fill
that the Sixel parser performs when an image is scrolled because it
doesn't fit on the screen.

This new behavior may not be exactly correct, but does at least match
the VT340 implementation more closely than it did before.

References and Relevant Issues

The initial Sixel parser implementation was in PR #17421.

Detailed Description of the Pull Request / Additional comments

When a Sixel image has the background select parameter set to 0 or 2, it
fills the background up to the active raster attribute dimensions prior
to writing out any actual pixel data. But this fill operation is clamped
at the boundaries of the screen, so if the image doesn't fit, and needs
to scroll, we have to perform an additional fill at that point to cover
the background of the newly revealed rows (this is something we weren't
doing before).

This later fill uses the width of the most recent raster attributes
command, which is not necessarily the same as the initial background
fill, and the height that it covers doesn't seem to take the raster
attributes into account at all. I'm not sure yet how the height is
supposed to be determined, but for now we're filling enough of the
screen to cover the sixel rows that have been output.

Validation Steps Performed

We've run a couple of tests on a real VT340, and I've confirmed that the
new implementation matches the behavior that we were seeing there.

I've also confirmed that the test case in issue #17946 now matches what
the OP was expecting, and the test case in #17887 at least works more
consistently now when scrolling (this is not what the OP was expecting
though).

PR Checklist

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-VT Virtual Terminal sequence support Product-Terminal The new Windows Terminal. labels Nov 28, 2024
@j4james
Copy link
Collaborator Author

j4james commented Nov 28, 2024

Since I'm not certain this is actually correct, I don't know if it's a good idea to merge at this point in time, but you're welcome to do so if you think it worthwhile.

@lhecker lhecker added the Needs-Discussion Something that requires a team discussion before we can proceed label Dec 4, 2024
@lhecker
Copy link
Member

lhecker commented Dec 9, 2024

[...] I don't know if it's a good idea to merge at this point in time, but you're welcome to do so if you think it worthwhile.

We discussed this today: Given that >1 person stumbled over this and that this brings us closer to the VT340 behavior, we felt like this is worth merging. 🙂

@j4james j4james marked this pull request as ready for review December 9, 2024 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Discussion Something that requires a team discussion before we can proceed Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sixel band with solid background color get transparent in some cases sixel unexpected background fill
3 participants