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

Console doesn't handle colored regions when reflowed #32

Closed
Tracked by #8000
bitcrazed opened this issue Oct 6, 2017 · 16 comments · Fixed by #12637
Closed
Tracked by #8000

Console doesn't handle colored regions when reflowed #32

bitcrazed opened this issue Oct 6, 2017 · 16 comments · Fixed by #12637
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Impact-Visual It look bad. 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. Work-Item It's being tracked by an actual work item internally. (to be removed soon)

Comments

@bitcrazed
Copy link
Contributor

bitcrazed commented Oct 6, 2017

  • Your Windows build number: (Type ver at a Windows Command Prompt)
    FCU

  • What you're doing and what's happening: (Copy & paste specific commands and their output, or include screen shots)

  1. Open Ubuntu session in Console
  2. Execute the following:
$ cd ~/
$ mkdir temp
$ cd temp
$ mkdir hello
$ mkdir world
$ clear 
$ ls
  1. Grab & drag right-hand Console window edge to resize window width

Expected:

Folders' colored background should only extend to the end of the folder name:

image

Actual:

Lines with folders that are last item on line extend folder background coloring to right hand end of line:

image

See the following for an example of where it goes very wrong:
image

And it gets worse when lines wrap!
image

References

Thanks to @jmorrill for reporting here: https://twitter.com/jmorrill/status/915982968094580736

@bitcrazed bitcrazed added Work-Item It's being tracked by an actual work item internally. (to be removed soon) Product-Conhost For issues in the Console codebase labels Oct 6, 2017
@zadjii-msft
Copy link
Member

zadjii-msft commented Oct 6, 2017

So this is basically because we don't know the difference between a space character that was purposefully emitted and just an empty cell in the buffer.

When we resize, we optimize things a little bit by just filling the rest of the row with the last color we saw (as we resize). This causes the rest of the row to fill up with the colors of the last character.

I can make it a bit better, but it will clear out any colored spaces at the end of the row:
image

See how the last two spaces of the white column were reset.

Might be an acceptable hit considering that resizing is already an AWFUL perf path.

(opened msft:14115086 to track)

@jmorrill
Copy link

jmorrill commented Oct 6, 2017

Thanks for looking into this guys.

IMO your suggestion of "I can make it a bit better, but it will clear out any colored spaces at the end of the row" looks more acceptable to me than the current behavior.

Curious what others think.

Windows Version: Microsoft Windows [Version 10.0.16299.15]

@bitcrazed
Copy link
Contributor Author

FWIW, this isn't a problem that's unique to Console - Hyper also struggles to handle this scenario:

image

@jmorrill
Copy link

jmorrill commented Oct 6, 2017

I've also seen this issue in hyper (and VS Code terminal window), but assumed it was something common to all of them and "lower level" in Windows.

@zadjii-msft
Copy link
Member

That's correct, Hyper and others all query an underlying conhost, so if we've got it wrong then it will be wrong for everyone.

@parkovski
Copy link

I have a similar (same?) issue with Vim/WSL with the termguicolors option set.
Before resize:
vim-wsl
After resize:
vim-wsl-resize
Expected (iTerm):
vim-iterm

@dra27
Copy link

dra27 commented Jan 8, 2018

I'm sure it's all part of the same thing - this also affects underlined text.

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label May 17, 2019
@miniksa miniksa added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. labels May 29, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 29, 2019
@encladeus
Copy link

Bump: As of Terminal 0.4.2382.0 this is still an issue. Using WSL Ubuntu shell I can recreate this many ways, but one easy way is to execute the following command:

before

Then resize the window and the line is repainted incorrectly:

after-resize

t5mat added a commit to t5mat/turnbinds that referenced this issue Dec 21, 2021
t5mat added a commit to t5mat/turnbinds that referenced this issue Dec 21, 2021
@zadjii-msft zadjii-msft modified the milestones: Windows vNext, 22H2 Jan 4, 2022
zadjii-msft added a commit that referenced this issue Mar 2, 2022
…. I was worng.

  My theory was "fuck perf, let's do it right, and figure out perf later". It didn't work.
  This wraps lines super weird, probably because I'm copying the whole line to the right side of the row, which is then starting to mark it as wrapped.
  Also like the test doesn't even pass.
zadjii-msft added a commit that referenced this issue Mar 2, 2022
  bunch of todos. It also totally takes care of #12567 as well
LuanVSO added a commit to LuanVSO/pwsh-profile that referenced this issue Mar 5, 2022
LuanVSO added a commit to LuanVSO/pwsh-profile that referenced this issue Mar 5, 2022
@ghost ghost added the In-PR This issue has a related PR label Mar 7, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H2, Terminal v1.14 Mar 7, 2022
@ghost ghost closed this as completed in #12637 Mar 21, 2022
@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 Mar 21, 2022
ghost pushed a commit that referenced this issue Mar 21, 2022
## THE WHITE WHALE

This is a fairly naive fix for this bug. It's not terribly performant,
but neither is resize in the first place.

When the buffer gets resized, typically we only copy the text up to the
`MeasureRight` point, the last printable char in the row. Then we'd just
use the last char's attributes to fill the remainder of the row. 

Instead, this PR changes how reflow behaves when it gets to the end of
the row. After we finish copying text, then manually walk through the
attributes at the end of the row, and copy them over. This ensures that
cells that just have a colored space in them get copied into the new
buffer as well, and we don't just blat the last character's attributes
into the rest of the row. We'll do a similar thing once we get to the
last printable char in the buffer, copying the remaining attributes.

This could DEFINITELY be more performant. I think this current
implementation walks the attrs _on every cell_, then appends the new
attrs to the new ATTR_ROW. That could be optimized by just using the
actual iterator. The copy after the last printable char bit is also
especially bad in this regard. That could likely be a blind copy - I
just wanted to get this into the world.

Finally, we now copy the final attributes to the correct buffer: the new
one.  We used to copy them to the _old_ buffer, which we were about to
destroy.

## Validation

I'll add more gifs in the morning, not enough time to finish spinning a
release Terminal build with this tonight. 

Closes #32 🎉🎉🎉🎉🎉🎉🎉🎉🎉
Closes #12567
DHowett pushed a commit that referenced this issue Mar 28, 2022
## THE WHITE WHALE

This is a fairly naive fix for this bug. It's not terribly performant,
but neither is resize in the first place.

When the buffer gets resized, typically we only copy the text up to the
`MeasureRight` point, the last printable char in the row. Then we'd just
use the last char's attributes to fill the remainder of the row.

Instead, this PR changes how reflow behaves when it gets to the end of
the row. After we finish copying text, then manually walk through the
attributes at the end of the row, and copy them over. This ensures that
cells that just have a colored space in them get copied into the new
buffer as well, and we don't just blat the last character's attributes
into the rest of the row. We'll do a similar thing once we get to the
last printable char in the buffer, copying the remaining attributes.

This could DEFINITELY be more performant. I think this current
implementation walks the attrs _on every cell_, then appends the new
attrs to the new ATTR_ROW. That could be optimized by just using the
actual iterator. The copy after the last printable char bit is also
especially bad in this regard. That could likely be a blind copy - I
just wanted to get this into the world.

Finally, we now copy the final attributes to the correct buffer: the new
one.  We used to copy them to the _old_ buffer, which we were about to
destroy.

## Validation

I'll add more gifs in the morning, not enough time to finish spinning a
release Terminal build with this tonight.

Closes #32 🎉🎉🎉🎉🎉🎉🎉🎉🎉
Closes #12567

(cherry picked from commit 855e136)
DHowett pushed a commit that referenced this issue Mar 28, 2022
## THE WHITE WHALE

This is a fairly naive fix for this bug. It's not terribly performant,
but neither is resize in the first place.

When the buffer gets resized, typically we only copy the text up to the
`MeasureRight` point, the last printable char in the row. Then we'd just
use the last char's attributes to fill the remainder of the row.

Instead, this PR changes how reflow behaves when it gets to the end of
the row. After we finish copying text, then manually walk through the
attributes at the end of the row, and copy them over. This ensures that
cells that just have a colored space in them get copied into the new
buffer as well, and we don't just blat the last character's attributes
into the rest of the row. We'll do a similar thing once we get to the
last printable char in the buffer, copying the remaining attributes.

This could DEFINITELY be more performant. I think this current
implementation walks the attrs _on every cell_, then appends the new
attrs to the new ATTR_ROW. That could be optimized by just using the
actual iterator. The copy after the last printable char bit is also
especially bad in this regard. That could likely be a blind copy - I
just wanted to get this into the world.

Finally, we now copy the final attributes to the correct buffer: the new
one.  We used to copy them to the _old_ buffer, which we were about to
destroy.

## Validation

I'll add more gifs in the morning, not enough time to finish spinning a
release Terminal build with this tonight.

Closes #32 🎉🎉🎉🎉🎉🎉🎉🎉🎉
Closes #12567

(cherry picked from commit 855e136)
@ghost
Copy link

ghost commented Apr 19, 2022

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

Handy links:

@ghost
Copy link

ghost commented Apr 19, 2022

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

Handy links:

LuanVSO added a commit to LuanVSO/pwsh-profile that referenced this issue Apr 19, 2022
DHowett pushed a commit that referenced this issue Jun 24, 2022
## THE WHITE WHALE

This is a fairly naive fix for this bug. It's not terribly performant,
but neither is resize in the first place.

When the buffer gets resized, typically we only copy the text up to the
`MeasureRight` point, the last printable char in the row. Then we'd just
use the last char's attributes to fill the remainder of the row.

Instead, this PR changes how reflow behaves when it gets to the end of
the row. After we finish copying text, then manually walk through the
attributes at the end of the row, and copy them over. This ensures that
cells that just have a colored space in them get copied into the new
buffer as well, and we don't just blat the last character's attributes
into the rest of the row. We'll do a similar thing once we get to the
last printable char in the buffer, copying the remaining attributes.

This could DEFINITELY be more performant. I think this current
implementation walks the attrs _on every cell_, then appends the new
attrs to the new ATTR_ROW. That could be optimized by just using the
actual iterator. The copy after the last printable char bit is also
especially bad in this regard. That could likely be a blind copy - I
just wanted to get this into the world.

Finally, we now copy the final attributes to the correct buffer: the new
one.  We used to copy them to the _old_ buffer, which we were about to
destroy.

## Validation

I'll add more gifs in the morning, not enough time to finish spinning a
release Terminal build with this tonight.

Closes #32 🎉🎉🎉🎉🎉🎉🎉🎉🎉
Closes #12567

(cherry picked from commit 855e136)
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Impact-Visual It look bad. 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. Work-Item It's being tracked by an actual work item internally. (to be removed soon)
Projects
None yet
Development

Successfully merging a pull request may close this issue.