Should the bold/bright attribute apply to SGR 38/48 indexed colors? #5384
Comments
ghost
added
Needs-Triage
|
The WT behavior is a follow-on from #2661, actually. Here's what's coming out of the debug tap:
We successfully preserve "the user only wants bright to be enabled for the second run", but that brightness applies to If #2661 is fixed, I think we'll get...
and WT won't attempt to brighten it. |
DHowett-MSFT
added
the
Needs-Author-Feedback
Hmmm... I could have sworn I was getting The former calls and the latter calls the same method from here: So either way you're going to end up with an indexed color, and any indexed color less than 8 gets brightened. |
ghost
added
Needs-Attention
|
oh, you actually used |
DHowett-MSFT
added
Area-TerminalControl
DHowett-MSFT
removed
Needs-Triage
|
Just another bug for the pile of bugs that we're growing because we have two AdaptDispatch implementations. Augh. |
Xterm 331 changed this. See https://gitlab.gnome.org/GNOME/vte/-/issues/149. |
|
Just wondering: Before cleaning this up, don't you guys wish to
|
I think that change must have been reverted, because the version I tested was definitely more recent than that. And I've now just rebuilt from the latest source (version 354), and that still does not brighten SGR 38 colors.
Not something I'm interested in doing.
I can put up with a lot of things, but the day we break the legacy brightness functionality is the day I stop contributing to this project and manage my own fork. Hopefully that'll never happen. That said, I was thinking a nice compromise would be to treat SGR 1 as bright (but not bold) for "legacy" colors, and as a bold font (but not bright) for the extended ITU colors. That remains backwards compatible with the legacy behaviour, while still allowing for a bold font for those that do want it. And as far as I can tell, this is the default setting for Mintty, so we wouldn't be alone if we went that route. |
You're right, version 348 reverts the change. I didn't know this, thanks for the update! It's not mentioned in the changelog, though. Anyway, I think we can conclude that the other behavior was accidental.
I personally don't care if it's you or someone else, I fully respect if you don't care about it; but if WT wishes to provide a decent, modern Unix-y terminal emulation experience then it really needs to have bold typeface (and while at it, italic too). I'd personally set it as 1.0-blocker. IMO it doesn't make too much sense to talk about how the SGR codes exactly map to colors, without having bold (as in heavier font being displayed) support for now; and then at one point re-evaluate all of this with bold in the game. I'd find it much cleaner to have bold first, and then discuss how to resolve the SGR bold vs. colors nightmare.
I hope that too, first, because it would be a bad product decision, and second, because the team and the users would miss your truly valuable contributions! I didn't repeat it here, but I think I made it clear in another thread that I do not want this behavior the only one; I do not even ask for it to be the default. However, modern Unix-y behavior with color schemes like Solarized require a mode where SGR 1 doesn't temper with the colors. It needs to be one of the modes supported by WT – along with, of course, other modes that you personally care about more, and are imporant for Windows backwards compatibility.
I disagree with this. This doesn't clean up the current mess at all, just pushes it slightly elsewhere. Also note that terminfo-based apps (included ncurses-based ones, too) only have 256 indices to pick a color from, and they have no control over whether indices 0..15 map to the legacy or to the extended escape sequences. Some terminfo versions/entries do the legacy one, some do the 256-color one. Terminfo and ncurses expose the concept of 256 palette colors altogether, not 16+256 ones. So such apps would have no control whatsoever about which actual look they can achieve. The only reasonable way forward, as Kitty, VTE and Alacritty (in this chronological order) already did as their default, and is available as an option in xterm, urxvt and probably many others too, is to fully separate bold typeface from the colors. Again, as an option, with other backwards compatibility options provided too, as the given project finds appropriate. |
I don't think this is an issue for us. The decision to render bold with a heavier font would happen in the renderer. It's going nothing to do with the SGR dispatching code that I'm working on now. The dispatcher simply records the bold state as a flag for the renderer to process. It's probably a bit messier than I'm making out, but it's still not relevant to this area of the code.
"Reasonable" is not the word I'd use to describe what you're doing. But lets just agree to disagree. |
) This PR introduces a new `ColorType` to allow us to distinguish between `SGR` indexed colors from the 16 color table, the lower half of which can be brightened, and the ISO/ITU indexed colors from the 256 color table, which have a fixed brightness. Retaining the distinction between these two types will enable us to forward the correct `SGR` sequences to conpty when addressing issue #2661. The other benefit of retaining the color index (which we didn't previously do for ISO/ITU colors) is that it ensures that the colors are updated correctly when the color scheme is changed. ## References * This is another step towards fixing the conpty narrowing bugs in issue #2661. * This is technically a fix for issue #5384, but that won't be apparent until #2661 is complete. ## PR Checklist * [x] Closes #1223 * [x] CLA signed. * [x] Tests added/passed * [ ] Requires documentation to be updated * [x] I've discussed this with core contributors already. ## Detailed Description of the Pull Request / Additional comments The first part of this PR was the introduction of a new `ColorType` in the `TextColor` class. Instead of just the one `IsIndex` type, there is now an `IsIndex16` and an `IsIndex256`. `IsIndex16` covers the eight original ANSI colors set with `SGR 3x` and `SGR 4x`, as well as the brighter aixterm variants set with `SGR 9x` and `SGR 10x`. `IsIndex256` covers the 256 ISO/ITU indexed colors set with `SGR 38;5` and `SGR 48;5`. There are two reasons for this distinction. The first is that the ANSI colors have the potential to be brightened by the `SGR 1` bold attribute, while the ISO/ITO color do not. The second reason is that when forwarding an attributes through conpty, we want to try and preserve the original SGR sequence that generated each color (to the extent that that is possible). By having the two separate types, we can map the `IsIndex16` colors back to ANSI/aixterm values, and `IsIndex256` to the ISO/ITU sequences. In addition to the VT colors, we also have to deal with the legacy colors set by the Windows console APIs, but we don't really need a separate type for those. It seemed most appropriate to me to store them as `IsIndex256` colors, since it doesn't make sense to have them brightened by the `SGR 1` attribute (which is what would happen if they were stored as `IsIndex16`). If a console app wanted a bright color it would have selected one, so we shouldn't be messing with that choice. The second part of the PR was the unification of the two color tables. Originally we had a 16 color table for the legacy colors, and a separate table for the 256 ISO/ITU colors. These have now been merged into one, so color table lookups no longer need to decide which of the two tables they should be referencing. I've also updated all the methods that took a color table as a parameter to use a `basic_string_view` instead of separate pointer and length variables, which I think makes them a lot easier and safer to work with. With this new architecture in place, I could now update the `AdaptDispatch` SGR implementation to store the ISO/ITU indexed colors as `IsIndex256` values, where before they were mapped to RGB values (which prevented them reflecting any color scheme changes). I could also update the `TerminalDispatch` implementation to differentiate between the two index types, so that the `SGR 1` brightening would only be applied to the ANSI colors. I've also done a bit of code refactoring to try and minimise any direct access to the color tables, getting rid of a lot of places that were copying tables with `memmove` operations. I'm hoping this will make it easier for us to update the code in the future if we want to reorder the table entries (which is likely a requirement for unifying the `AdaptDispatch` and `TerminalDispatch` implementations). ## Validation Steps Performed For testing, I've just updated the existing unit tests to account for the API changes. The `TextColorTests` required an extra parameter specifying the index type when setting an index. And the `AdapterTest` and `ScreenBufferTests` required the use of the new `SetIndexedXXX` methods in order to be explicit about the index type, instead of relying on the `TextAttribute` constructor and the old `SetForeground` and `SetBackground` methods which didn't have a way to differentiate index types. I've manually tested the various console APIs (`SetConsoleTextAttribute`, `ReadConsoleOutputAttribute`, and `ReadConsoleOutput`), to make sure they are still setting and reading the attributes as well as they used to. And I've tested the `SetConsoleScreenBufferInfoEx` and `GetConsoleScreenBufferInfoEx` APIs to make sure they can read and write the color table correctly. I've also tested the color table in the properties dialog, made sure it was saved and restored from the registry correctly, and similarly saved and restored from a shortcut link. Note that there are still a bunch of issues with the color table APIs, but no new problems have been introduced by the changes in this PR, as far as I could tell. I've also done a bunch of manual tests of `OSC 4` to make sure it's updating all the colors correctly (at least in conhost), and confirmed that the test case in issue #1223 now works as expected.
…crosoft#5834) This PR introduces a new `ColorType` to allow us to distinguish between `SGR` indexed colors from the 16 color table, the lower half of which can be brightened, and the ISO/ITU indexed colors from the 256 color table, which have a fixed brightness. Retaining the distinction between these two types will enable us to forward the correct `SGR` sequences to conpty when addressing issue microsoft#2661. The other benefit of retaining the color index (which we didn't previously do for ISO/ITU colors) is that it ensures that the colors are updated correctly when the color scheme is changed. ## References * This is another step towards fixing the conpty narrowing bugs in issue microsoft#2661. * This is technically a fix for issue microsoft#5384, but that won't be apparent until microsoft#2661 is complete. ## PR Checklist * [x] Closes microsoft#1223 * [x] CLA signed. * [x] Tests added/passed * [ ] Requires documentation to be updated * [x] I've discussed this with core contributors already. ## Detailed Description of the Pull Request / Additional comments The first part of this PR was the introduction of a new `ColorType` in the `TextColor` class. Instead of just the one `IsIndex` type, there is now an `IsIndex16` and an `IsIndex256`. `IsIndex16` covers the eight original ANSI colors set with `SGR 3x` and `SGR 4x`, as well as the brighter aixterm variants set with `SGR 9x` and `SGR 10x`. `IsIndex256` covers the 256 ISO/ITU indexed colors set with `SGR 38;5` and `SGR 48;5`. There are two reasons for this distinction. The first is that the ANSI colors have the potential to be brightened by the `SGR 1` bold attribute, while the ISO/ITO color do not. The second reason is that when forwarding an attributes through conpty, we want to try and preserve the original SGR sequence that generated each color (to the extent that that is possible). By having the two separate types, we can map the `IsIndex16` colors back to ANSI/aixterm values, and `IsIndex256` to the ISO/ITU sequences. In addition to the VT colors, we also have to deal with the legacy colors set by the Windows console APIs, but we don't really need a separate type for those. It seemed most appropriate to me to store them as `IsIndex256` colors, since it doesn't make sense to have them brightened by the `SGR 1` attribute (which is what would happen if they were stored as `IsIndex16`). If a console app wanted a bright color it would have selected one, so we shouldn't be messing with that choice. The second part of the PR was the unification of the two color tables. Originally we had a 16 color table for the legacy colors, and a separate table for the 256 ISO/ITU colors. These have now been merged into one, so color table lookups no longer need to decide which of the two tables they should be referencing. I've also updated all the methods that took a color table as a parameter to use a `basic_string_view` instead of separate pointer and length variables, which I think makes them a lot easier and safer to work with. With this new architecture in place, I could now update the `AdaptDispatch` SGR implementation to store the ISO/ITU indexed colors as `IsIndex256` values, where before they were mapped to RGB values (which prevented them reflecting any color scheme changes). I could also update the `TerminalDispatch` implementation to differentiate between the two index types, so that the `SGR 1` brightening would only be applied to the ANSI colors. I've also done a bit of code refactoring to try and minimise any direct access to the color tables, getting rid of a lot of places that were copying tables with `memmove` operations. I'm hoping this will make it easier for us to update the code in the future if we want to reorder the table entries (which is likely a requirement for unifying the `AdaptDispatch` and `TerminalDispatch` implementations). ## Validation Steps Performed For testing, I've just updated the existing unit tests to account for the API changes. The `TextColorTests` required an extra parameter specifying the index type when setting an index. And the `AdapterTest` and `ScreenBufferTests` required the use of the new `SetIndexedXXX` methods in order to be explicit about the index type, instead of relying on the `TextAttribute` constructor and the old `SetForeground` and `SetBackground` methods which didn't have a way to differentiate index types. I've manually tested the various console APIs (`SetConsoleTextAttribute`, `ReadConsoleOutputAttribute`, and `ReadConsoleOutput`), to make sure they are still setting and reading the attributes as well as they used to. And I've tested the `SetConsoleScreenBufferInfoEx` and `GetConsoleScreenBufferInfoEx` APIs to make sure they can read and write the color table correctly. I've also tested the color table in the properties dialog, made sure it was saved and restored from the registry correctly, and similarly saved and restored from a shortcut link. Note that there are still a bunch of issues with the color table APIs, but no new problems have been introduced by the changes in this PR, as far as I could tell. I've also done a bunch of manual tests of `OSC 4` to make sure it's updating all the colors correctly (at least in conhost), and confirmed that the test case in issue microsoft#1223 now works as expected.
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
added
Needs-Tag-Fix
|
🎉This issue was addressed in #6506, which has now been successfully released as Handy links: |
Environment
Windows build number: Version 10.0.18362.657
Windows Terminal version (if applicable): 0.10.781.0
Steps to reproduce
printf "\e[0;38;5;3mDARK \e[1mBRIGHT?\e[m\n"printf "\e[0;38;5;3mDARK \e[1mBRIGHT?\e[m\n"Expected behavior
Both terminals should produce the same output showing the same colors.
Actual behavior
Conhost displays the "BRIGHT?" text in the same dark shade as the "DARK" text. Windows Terminal displays it brighter.
In some ways this could be considered a narrowing bug (issue #2661), because one reason for the difference is that conhost converts the indexed color into an RGB value before storing it, while the Windows Terminal doesn't. And the bold/bright attribute has no effect on RGB colors, but does affect indexed colors.
That said, I don't think the WT implementation is correct, so fixing the "narrowing" issue in conhost is just going to make both of them wrong. There is some room for interpretation here, but I've been looking at how other terminals handle this, and AFAICT most (including XTerm and VTE) will only apply the brightness attribute to the 8 basic colors, and not the 256 indexed colors from SGR 38/48.
So this is really a question as much as it is a bug report. Which behavior do we want to follow here? Because the answer to that is going to affect how we address issue #2661.
The text was updated successfully, but these errors were encountered: