[host] Changing a color table value above 16 does not update the color of other cells with that index #1223
Labels
Area-VT
Virtual Terminal sequence support
Issue-Bug
It either shouldn't be doing this or needs an investigation.
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
zadjii-msft
added
Product-Conhost
ghost
added
Needs-Triage
zadjii-msft
removed
the
Needs-Triage
zadjii-msft
added a commit
that referenced
this issue
Aug 27, 2019
I'm going to just remove the 16 color table entirely, and treat it as a subset of the 256 table. Hopefully this works? It doesn't break any unittests yet...
zadjii-msft
added a commit
that referenced
this issue
Aug 27, 2019
Fixes #1223 We'll lookup a color (for rendering) using the _entire_ 256 color table. When setting a 256 color, we'll set the TextColor to that index, instead of the current color at that index. Everything else is unchanged. **TODO**: This probably affects which TextAttributes will pass IsLegacy. 256 color ones will now be "legacy". This is bad, we should make sure to not leave it that way.
|
@j4james just for the record - I think you have a prototype branch floating around for this bug, from discussion in #3717. I also had a prototype branch for this that didn't end up making the cut of the last Windows release. I'm not really planning on investigating anytime soon, but feel free to poke through and compare. |
|
I was just experimenting, and nuked the code afterwards, but it was very close to your prototype actually. One slight difference was that I didn't want to touch the That felt horrible, though, which is one of the reasons I didn't want to keep it. But I also strongly suspect that memcpy comment was bogus and could really be ignored. As far as I can see, the shortcut properties should be initialized via the Anyway, the bottom line is I'm not planning on looking at this anytime soon either. I've got a few other things I'm playing with at the moment that I'd like to try and finish first. But I'll keep it on my todo list for the future, assuming you don't get back to it first. |
|
FYI, I'm looking into this again now. I've been working on a couple of PRs that are steps towards fixing #2661, and I think this conhost color table issue is the last major roadblock. |
5 tasks
DHowett-MSFT
pushed a commit
that referenced
this issue
May 8, 2020
This is an attempt to simplify the SGR (Select Graphic Rendition) implementation in conhost, to cut down on the number of methods required in the `ConGetSet` interface, and pave the way for future improvements and bug fixes. It already fixes one bug that prevented SGR 0 from being correctly applied when combined with meta attributes. * This a first step towards fixing the conpty narrowing bugs in issue #2661 * I'm hoping the simplification of `ConGetSet` will also help with #3849. * Some of the `TextAttribute` refactoring in this PR overlaps with similar work in PR #1978. ## Detailed Description of the Pull Request / Additional comments The main point of this PR was to simplify the `AdaptDispatch::SetGraphicsRendition` implementation. So instead of having it call a half a dozen methods in the `ConGetSet` API, depending on what kinds of attributes needed to be set, there is now just one call to get current attributes, and another call to set the new value. All adjustments to the attributes are made in the `AdaptDispatch` class, in a simple switch statement. To help with this refactoring, I also made some change to the `TextAttribute` class to make it easier to work with. This included adding a set of methods for setting (and getting) the individual attribute flags, instead of having the calling code being exposed to the internal attribute structures and messing with bit manipulation. I've tried to get rid of any methods that were directly setting legacy, meta, and extended attributes. Other than the fix to the `SGR 0` bug, the `AdaptDispatch` refactoring mostly follows the behaviour of the original code. In particular, it still maps the `SGR 38/48` indexed colors to RGB instead of retaining the index, which is what we ultimately need it to do. Fixing that will first require the color tables to be unified (issue #1223), which I'm hoping to address in a followup PR. But for now, mapping the indexed colors to RGB values required adding an an additional `ConGetSet` API to lookup the color table entries. In the future that won't be necessary, but the API will still be useful for other color reporting operations that we may want to support. I've made this API, and the existing setter, standardise on index values being in the "Xterm" order, since that'll be essential for unifying the code with the terminal adapter one day. I should also point out one minor change to the `SGR 38/48` behavior, which is that out-of-range RGB colors are now ignored rather than being clamped, since that matches the way Xterm works. ## Validation Steps Performed This refactoring has obviously required corresponding changes to the unit tests, but most were just minor updates to use the new `TextAttribute` methods without any real change in behavior. However, the adapter tests did require significant changes to accommodate the new `ConGetSet` API. The basic structure of the tests remain the same, but the simpler API has meant fewer values needed to be checked in each test case. I think they are all still covering the areas there were intended to, though, and they are all still passing. Other than getting the unit tests to work, I've also done a bunch of manual testing of my own. I've made sure the color tests in Vttest all still work as well as they used to. And I've confirmed that the test case from issue #5341 is now working correctly. Closes #5341
5 tasks
ghost
pushed a commit
that referenced
this issue
May 27, 2020
) 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.
ghost
added
Resolution-Fix-Committed
jelster
pushed a commit
to jelster/terminal
that referenced
this issue
May 28, 2020
This is an attempt to simplify the SGR (Select Graphic Rendition) implementation in conhost, to cut down on the number of methods required in the `ConGetSet` interface, and pave the way for future improvements and bug fixes. It already fixes one bug that prevented SGR 0 from being correctly applied when combined with meta attributes. * This a first step towards fixing the conpty narrowing bugs in issue microsoft#2661 * I'm hoping the simplification of `ConGetSet` will also help with microsoft#3849. * Some of the `TextAttribute` refactoring in this PR overlaps with similar work in PR microsoft#1978. ## Detailed Description of the Pull Request / Additional comments The main point of this PR was to simplify the `AdaptDispatch::SetGraphicsRendition` implementation. So instead of having it call a half a dozen methods in the `ConGetSet` API, depending on what kinds of attributes needed to be set, there is now just one call to get current attributes, and another call to set the new value. All adjustments to the attributes are made in the `AdaptDispatch` class, in a simple switch statement. To help with this refactoring, I also made some change to the `TextAttribute` class to make it easier to work with. This included adding a set of methods for setting (and getting) the individual attribute flags, instead of having the calling code being exposed to the internal attribute structures and messing with bit manipulation. I've tried to get rid of any methods that were directly setting legacy, meta, and extended attributes. Other than the fix to the `SGR 0` bug, the `AdaptDispatch` refactoring mostly follows the behaviour of the original code. In particular, it still maps the `SGR 38/48` indexed colors to RGB instead of retaining the index, which is what we ultimately need it to do. Fixing that will first require the color tables to be unified (issue microsoft#1223), which I'm hoping to address in a followup PR. But for now, mapping the indexed colors to RGB values required adding an an additional `ConGetSet` API to lookup the color table entries. In the future that won't be necessary, but the API will still be useful for other color reporting operations that we may want to support. I've made this API, and the existing setter, standardise on index values being in the "Xterm" order, since that'll be essential for unifying the code with the terminal adapter one day. I should also point out one minor change to the `SGR 38/48` behavior, which is that out-of-range RGB colors are now ignored rather than being clamped, since that matches the way Xterm works. ## Validation Steps Performed This refactoring has obviously required corresponding changes to the unit tests, but most were just minor updates to use the new `TextAttribute` methods without any real change in behavior. However, the adapter tests did require significant changes to accommodate the new `ConGetSet` API. The basic structure of the tests remain the same, but the simpler API has meant fewer values needed to be checked in each test case. I think they are all still covering the areas there were intended to, though, and they are all still passing. Other than getting the unit tests to work, I've also done a bunch of manual testing of my own. I've made sure the color tests in Vttest all still work as well as they used to. And I've confirmed that the test case from issue microsoft#5341 is now working correctly. Closes microsoft#5341
jelster
pushed a commit
to jelster/terminal
that referenced
this issue
May 28, 2020
…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 issue was addressed in #5834, which has now been successfully released as Handy links: |
This issue was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
From MSFT:20105972
in bash:
printf "\033[38;5;130mHello World"
printf "\033]4;130;rgb:ff/ff/00\07"
The first "Hello World" should appear in a brownish color. Then, after executing the second command, the value of the [130] entry should be updated to yellow. The hello world text should also be yellow now.
We could achieve this by allowing for TextColor's to have up to 256 stored in their _index member. I'm not sure we'd need more than that - though LookupColor might be confused by that, so we'd need to maybe add a separate is256Index value to the type enum.
The text was updated successfully, but these errors were encountered: