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

DEC double-height lines do not properly reset #15575

Open
ClaireCJS opened this issue Jun 19, 2023 · 17 comments
Open

DEC double-height lines do not properly reset #15575

ClaireCJS opened this issue Jun 19, 2023 · 17 comments
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Milestone

Comments

@ClaireCJS
Copy link

ClaireCJS commented Jun 19, 2023

Windows Terminal version

1.17.11461.0

Windows build number

10.0.19045.3086

Other Software

No response

Steps to reproduce

Create DEC double-height text via an echo command. (It can be tricky to echo the escape character.)

Clear screen, do a directory.

Watch as double-height line artifacts that shouldn't be there are there.
Not just once. Multiple double-height lines can keep reoccurring.

  • Even issuing the ANSI reset ("0m") code fails to remove the artifacting behavior.
  • Even clearing the screen multiple times fails to remove the artifacting behavior.

image

image
image

Expected Behavior

The expected behavior is one the line is finished rendering, no double-height letters would ever appear again except if another line is issued with the escape code to turn that double-height letters on.

Actual Behavior

The actual behavior is that after the line is finished rendering, double-height letters still keep appearing -- often many many many times -- even without another line issued with the escape code to turn double-height letters on.

@ClaireCJS ClaireCJS added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 19, 2023
@lhecker lhecker added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-VT Virtual Terminal sequence support Product-Terminal The new Windows Terminal. Priority-3 A description (P3) labels Jun 19, 2023
@ClaireCJS
Copy link
Author

Observation - if you make the 1st line of the screen big, no matter hw many times you clear the screen, that first line is STUCK as big.

At least in my case, where i'm only echo'ing a few lines prior to clearing the screen repeatedly (and thus never causing the screen to scroll)

@DHowett
Copy link
Member

DHowett commented Jun 19, 2023

@j4james I'm having trouble reproducing this one; have you seen anything like this before?

@237dmitry
Copy link

237dmitry commented Jun 19, 2023

If escape symbol entered with Ctrl+[ there are no artifacts in Command Prompt
In pwsh this escape sequence resets with cls after Ctrl+L.

Command.Prompt.2023-06-19.19-47-02.mp4

@ClaireCJS
Copy link
Author

I can consistently reproduce this infinitely. Any line that is set to double-height remains set as double-height until it is scrolled away.

KEEP.-.windows.terminal.double-height.bug.mp4

@ClaireCJS
Copy link
Author

And here i am reproducing it infinitely:

KEEP.-.windows.terminal.double-height.bug.2.mp4

@DHowett
Copy link
Member

DHowett commented Jun 19, 2023

Amazing! Okay, I have a hunch.

I bet your shell is clearing the screen in one of the ways we cannot automatically detect1 as a clear.
Do you happen to know how it does that? We would love to support this type of clear.


Footnotes

  1. PowerShell uses FillConsoleOutputCharacterW and SetConsoleCursorPosition. CMD uses ScrollConsoleScreenBuffer. Both of these result in the line rendition attribute being reset.

@j4james
Copy link
Collaborator

j4james commented Jun 19, 2023

I bet your shell is clearing the screen in one of the ways we cannot automatically detect

I haven't had a chance to look at the code yet, but this would be my guess too. Frankly I'd be surprised if the line attributes were cleared by anything other than the VT erase sequences - I wasn't really expecting these operations to interoperate with the legacy console APIs. It would be nice if we could make them work at least some of the time though.

@ClaireCJS
Copy link
Author

Amazing! Okay, I have a hunch.

I bet your shell is clearing the screen in one of the ways we cannot automatically detect1 as a clear. Do you happen to know how it does that? We would love to support this type of clear.

Hi! Great hunch! The TCC command-line is very heavily supported, even if it's userbase is small. They often answer support forum posts within a day or two.

I left a message asking if someone with inside knowledge would roll in here and leave a comment answering your question! I think we can figure this out!! :)

@ClaireCJS
Copy link
Author

ClaireCJS commented Jun 19, 2023

Wow, that was fast!

I was using a plain CLS. This is directly from the TCC author:

"If you use CLS /S, TCC scrolls the screen using the Windows ScrollConsoleScreenBuffer API.

If you use CLS /C, TCC uses the Windows FillConsoleOutputCharacterW and FillConsoleOutputAttribute APIs to write the entire console buffer to blanks with the desired attribute.

A plain CLS uses the Windows FillConsoleOutputCharacterW and FillConsoleOutputAttribute APIs to write the current screen window to blanks with the desired attribute."

I checked, and regardless of /C or /S options, I still get the same behavior.

@vefatica
Copy link

You can see it in powershell also. After this (in TCC)

image

CLEAR ... DIR in Powershell gives this

image

@j4james
Copy link
Collaborator

j4james commented Jun 19, 2023

I've had a chance to look at the code now, and I can see that the cmd CLS works because it's using ScrollConsoleScreenBuffer to scroll the entire buffer out of existence, and the new rows that are revealed in a scroll operation will always have a single-width line rendition. If TCC is doing the same thing for CLS /S, then I would expect that to work too, but maybe there some other complication there.

Then I've also had a look at the two PowerShell versions, and they both use FillConsoleOutputCharacter for their implementation of CLS, and that doesn't reset the line renditions. As a quick hack, I added a check to see if the entire buffer was being filed in that operation, and if so I would make it clear all the line rendition as well. That fixes the issue for PowerShell, but I'm not sure that's necessarily the approach we want to take.

I think what we need to do now is see exactly what TCC is doing, figure out why ScrollConsoleScreenBuffer doesn't work for CLS /S, whether my hack for FillConsoleOutputCharacter will work CLS /C, and then we can decide exactly how we want to address this.

@ClaireCJS
Copy link
Author

Amazing! Okay, I have a hunch.

I bet your shell is clearing the screen in one of the ways we cannot automatically detect1 as a clear. Do you happen to know how it does that? We would love to support this type of clear.

It looks like you are absolutely correct!

Someone over on the TCC forum side of things mentioned that you can clear just the window with an ansi code: "\x1b[H\x1b[0J"

When I cleared it this way, the bug is consistently gone.
When I cleared it the standard TCC internal ways, the big is consistently present.

So I think your hunch seems dead on.

The TCC thread, by the way, is over here:

https://jpsoft.com/forums/threads/could-i-get-a-tcc-dev-rexx-to-comment-on-this-github-thread-for-windows-terminal-about-which-method-tcc-uses-to-clear-the-screen.11575/#post-65992

@j4james
Copy link
Collaborator

j4james commented Jun 20, 2023

I've tested my hack on TCC now and it doesn't help. The way TCC clears the screen with CLS /C is by filling the buffer 50 lines at a time, so we can't detect that as a "whole buffer" fill. And the way CLS /S works is to by moving everything up to the active row into the scrollback (with SetConsoleWindowInfo), and then filling the new viewport range one line at a time, so that doesn't work either.

So to make TCC work as expected, we'd probably have to reset the line renditions for any FillConsoleOutputCharacter call that covered the full width of the page. That doesn't seem like a terrible idea, though, since there's no real expectation for how these sequences should interact with legacy APIs. But I'll leave that call for someone else to make.

@DHowett
Copy link
Member

DHowett commented Jun 20, 2023

So to make TCC work as expected, we'd probably have to reset the line renditions for any FillConsoleOutputCharacter call that covered the full width of the page.

I'm cool with that. @lhecker, does that make sense?

@lhecker
Copy link
Member

lhecker commented Jun 20, 2023

Yeah absolutely.

I don't know what the release cycle for TCC is, but I would personally greatly prefer if they could scroll off the text buffer in a single call instead, just like cmd.exe does. With #15524 in place, I want to start to properly decommit memory in the future whenever cls or its equivalents are executed. But if cls is implemented iteratively, like with FillConsoleOutputCharacter, then this won't be realistically possible.

So yeah, I'd be happy if we were to fix the bug this way, but I feel like it'd be more optimal if TCC changed its approach to clearing the buffer. And it would benefit TCC too by reducing the total working set to a minimum every time cls is run.

@vefatica
Copy link

And it would benefit TCC too by reducing the total working set to a minimum every time cls is run.

How does that happen?

What about clearing via VT control sequences?

@lhecker
Copy link
Member

lhecker commented Jun 20, 2023

How does that happen?

You mean how it would work if someone were to scroll off the entire contents? We could detect such API calls and short-circuit them as a call to TextBuffer::Reset() which effectively ends up calling VirtualFree() on the terminal's backing buffer with the MEM_DECOMMIT flag. The next time the backing buffer is accessed, we'd (in simplified terms) MEM_COMMIT the backing buffer back in, which will result in zeroed out memory and newly constructed, completely reset rows in the buffer.

Thinking about it and what James said, I feel like a combination of both approaches would be optimal. The interaction between VT sequences and console APIs is unclear at the moment and we should consider resetting these VT related attributes like with the row-wide FillConsoleOutputCharacter (are the other areas as well?). But TCC should probably also use a more effective and faster backing buffer reset technique like scrolling off the contents in a single call, as described above.

@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jun 21, 2023
@carlos-zamora carlos-zamora added this to the Backlog milestone Jun 21, 2023
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-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

7 participants