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

Unable to set colors above index 15 with OSC 4, take 2 #3717

Closed
egmontkob opened this issue Nov 26, 2019 · 5 comments · Fixed by #6506
Closed

Unable to set colors above index 15 with OSC 4, take 2 #3717

egmontkob opened this issue Nov 26, 2019 · 5 comments · Fixed by #6506
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements 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.
Milestone

Comments

@egmontkob
Copy link

Environment

Windows build number: 10.0.18362.0
Windows Terminal version (if applicable): 0.7.3291.0

Steps to reproduce

Allegedly already fixed in #313, but doesn't work in v0.7:

echo -ne '\e]4;15;rgb:ab/cd/ef\e\\'
echo -ne '\e]4;16;rgb:ab/cd/ef\e\\'

Expected behavior

Set palette index 15 and 16 to the given color.

Actual behavior

Color index 15 is set to the given value (works for 0..15), index 16 remains unchanged (doesn't work for 16..255).

@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 Nov 26, 2019
@zadjii-msft zadjii-msft added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. labels Nov 26, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 26, 2019
@zadjii-msft zadjii-msft removed the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Nov 26, 2019
@zadjii-msft
Copy link
Member

My guess is that setting the colors >15 probably works in conhost, but doesn't roundtrip through conpty.

@zadjii-msft zadjii-msft added this to the 21H1 milestone Nov 26, 2019
@zadjii-msft zadjii-msft added the Help Wanted We encourage anyone to jump in on these. label Nov 26, 2019
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 30, 2019
@j4james
Copy link
Collaborator

j4james commented Nov 30, 2019

This doesn't really work on conhost either. You can technically set all the colors, and any future use of those colors will be displayed correctly, but your existing output won't update. In fact even the first 16 colors won't update if you've set them using the 256-color index syntax. I think that is what issue #1223 is all about.

Fixing #1223 is a bit of a nightmare, but I put together a hack solution just to see what would happen, and it also partially resolves the issue in the Windows Terminal. Sometimes, though, the colors just don't refresh, and I don't know enough about that side of the code to figure out why.

@DHowett-MSFT
Copy link
Contributor

I wonder: does fixing #2661 and then triggering a full invalidate on all screenful renderers help #1223? It seems like it may..

@j4james
Copy link
Collaborator

j4james commented Nov 30, 2019

Actually I was going to say fixing #1223 is probably a prerequisite for fixing #2661. If you want to preserve the color index when passing the attributes through conpty, that assumedly requires that the index was already preserved on the conhost side when writing to the output buffer - and that's essentially the fix for #1223.

@ghost ghost added the In-PR This issue has a related PR label Jun 15, 2020
DHowett pushed a commit that referenced this issue Jul 1, 2020
This PR reimplements the VT rendering engines to do a better job of
preserving the original color types when propagating attributes over
ConPTY. For the 16-color renderers it provides better support for
default colors and improves the efficiency of the color narrowing
conversions. It also fixes problems with the ordering of character
renditions that could result in attributes being dropped.

Originally the base renderer would calculate the RGB color values and
legacy/extended attributes up front, passing that data on to the active
engine's `UpdateDrawingBrushes` method. With this new implementation,
the renderer now just passes through the original `TextAttribute` along
with an `IRenderData` interface, and leaves it to the engines to extract
the information they need.

The GDI and DirectX engines now have to lookup the RGB colors themselves
(via simple `IRenderData` calls), but have no need for the other
attributes. The VT engines extract the information that they need from
the `TextAttribute`, instead of having to reverse engineer it from
`COLORREF`s.

The process for the 256-color Xterm engine starts with a check for
default colors. If both foreground and background are default, it
outputs a SGR 0 reset, and clears the `_lastTextAttribute` completely to
make sure any reset state is reapplied. With that out the way, the
foreground and background are updated (if changed) in one of 4 ways.
They can either be a default value (SGR 39 and 49), a 16-color index
(using ANSI or AIX sequences), a 256-color index, or a 24-bit RGB value
(both using SGR 38 and 48 sequences).

Then once the colors are accounted for, there is a separate step that
handles the character rendition attributes (bold, italics, underline,
etc.) This step must come _after_ the color sequences, in case a SGR
reset is required, which would otherwise have cleared any character
rendition attributes if it came last (which is what happened in the
original implementation).

The process for the 16-color engines is a little different. The target
client in this case (Windows telnet) is incapable of setting default
colors individually, so we need to output an SGR 0 reset if _either_
color has changed to default. With that out the way, we use the
`TextColor::GetLegacyIndex` method to obtain an approximate 16-color
index for each color, and apply the bold attribute by brightening the
foreground index (setting bit 8) if the color type permits that.

However, since Windows telnet only supports the 8 basic ANSI colors, the
best we can do for bright colors is to output an SGR 1 attribute to get
a bright foreground. There is nothing we can do about a bright
background, so after that we just have to drop the high bit from the
colors. If the resulting index values have changed from what they were
before, we then output ANSI 8-color SGR sequences to update them.

As with the 256-color engine, there is also a final step to handle the
character rendition attributes. But in this case, the only supported
attributes are underline and reversed video.

Since the VT engines no longer depend on the active color table and
default color values, there was quite a lot of code that could now be
removed. This included the `IDefaultColorProvider` interface and
implementations, the `Find(Nearest)TableIndex` functions, and also the
associated HLS conversion and difference calculations.

VALIDATION

Other than simple API parameter changes, the majority of updates
required in the unit tests were to correct assumptions about the way the
colors should be rendered, which were the source of the narrowing bugs
this PR was trying to fix. Like passing white on black to the
`UpdateDrawingBrushes` API, and expecting it to output the default `SGR
0` sequence, or passing an RGB color and expecting an indexed SGR
sequence.

In addition to that, I've added some VT renderer tests to make sure the
rendition attributes (bold, underline, etc) are correctly retained when
a default color update causes an `SGR 0` sequence to be generated (the
source of bug #3076). And I've extended the VT renderer color tests
(both 256-color and 16-color) to make sure we're covering all of the
different color types (default, RGB, and both forms of indexed colors).

I've also tried to manually verify that all of the test cases in the
linked bug reports (and their associated duplicates) are now fixed when
this PR is applied.

Closes #2661
Closes #3076
Closes #3717
Closes #5384
Closes #5864

This is only a partial fix for #293, but I suspect the remaining cases
are unfixable.
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements 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 Jul 1, 2020
@ghost
Copy link

ghost commented Jul 22, 2020

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

Handy links:

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 Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements 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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants