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

Saving/restoring character attributes (and DECSCNM)? #8913

Open
vefatica opened this issue Jan 27, 2021 · 7 comments
Open

Saving/restoring character attributes (and DECSCNM)? #8913

vefatica opened this issue Jan 27, 2021 · 7 comments
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) 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
Milestone

Comments

@vefatica
Copy link

Microsoft Windows 10 Pro for Workstations
10.0.19042.746 (2009, 20H2)
WindowsTerminalPreview_1.5.3242.0_x64

I have a VBEEP command (visual beep, plugin for TCC.EXE from JPSoft). It saves the character attributes in the viewport like this.

dwSize = con.csbi.dwSize.X * con.csbi.dwSize.Y;	// terminal viewport
ReadConsoleOutputAttribute(con.h, attrs_original, dwSize, {0,0}, &dwRead); // get current attributes

Then it momentarily changes all attributes to the reverse of the default, like this.

attrs_reversed = ((con.csbi.wAttributes & 0x00F0) >> 4) | ((con.csbi.wAttributes & 0x000F) << 4);
FillConsoleOutputAttribute(con.h, attrs_reversed, dwSize, {0,0}, &dwWritten);

Then it restores the original attributes, like this.

WriteConsoleOutputAttribute(con.h, attrs_original, dwSize, {0,0}, &dwWritten); // back to original attributes

But, as I hope can be seen below, After executing VBEEP, the attributes of previously displayed text seem to have changed. Check out the display of the previous "echos ..." commands before/after the "vbeep" command; lines that were half yellow before the vbeep, are all yellow afterward.

27.00.18.4588.v__.2021-01-27.12-39-39.mp4
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jan 27, 2021
@zadjii-msft
Copy link
Member

Huh. Could be two things:

  1. if you write to the attributes of the rightmost char in a row with FillConsoleOutputAttribute, then we extend that attribute to the end of the row, rather than leave it ending where it currently is.
  2. (perhaps more likely) when someone asks to ReadConsoleOutputAttribute for the entire row, we just fill in all the cells after the rightmost char with the attributes of the last cell. That ends up roundtripping, and filling the whole end of the row with those attrs.

I bet we could write a test for this fairly easily...

@zadjii-msft zadjii-msft added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) 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 Jan 27, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 27, 2021
@zadjii-msft zadjii-msft added this to the Windows vNext milestone Jan 27, 2021
@vefatica
Copy link
Author

I'm using reading/filling attributes for the whole viewport, all at once (specifically reading/filling every cell, or so I thought). Are you saying that, even so, what you described is happening in each row?

@j4james
Copy link
Collaborator

j4james commented Jan 27, 2021

I think I know what this is. When I added the DECSCNM functionality to Windows Terminal I noticed that it exposed a flaw in the way conpty optimized runs of spaces, and I think this is an example of that flaw. You can read more in the thread here: #6809 (comment)

At the time, @DHowett and I decided it was a fairly obscure edge case, so we thought we could leave it broken and probably nobody would notice. It seems we were wrong. 😄

@DHowett
Copy link
Member

DHowett commented Jan 28, 2021

Oh gosh, you're totally right. I'm also willing to call this P3 and say "DECSCNM's most common use is visual belling, so if it happens to look funny for a small number of milliseconds it's okay." 😄

@DHowett DHowett added Impact-Visual It look bad. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jan 28, 2021
@vefatica
Copy link
Author

"DECSCNM's most common use is visual belling, so if it happens to look funny for a small number of milliseconds it's okay."

Do you really think so?

I see there's a PR about flashing the pane on BEL. Will it use the same inversion as DECSCNM? That can get pretty hideous looking, especially if there's colored text (and those colors may be auto-extended to EOL on the inverted screen). Negative feedback wouldn't surprise me.

@DHowett
Copy link
Member

DHowett commented May 12, 2021

We've explicitly designed the visual bell implementation to do something only the Terminal UI can do-- nothing that looks like an application-controlled attribute :)

@vefatica
Copy link
Author

Are you saying that pane-flashing will not be the same as DECSCNM and that it's really nifty? If so, I can't wait to see it!

@zadjii-msft zadjii-msft modified the milestones: Windows vNext, 22H2 Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H2, Backlog Jul 5, 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.) 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
Projects
None yet
Development

No branches or pull requests

4 participants