Refactor the SGR implementation in AdaptDispatch #5758

j4james · 2020-05-05T22:40:58Z

Summary of the Pull Request

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.

References

This a first step towards fixing the conpty narrowing bugs in issue render: defer conversion of TextColor into COLORREF until actual rendering time (avoid ConPTY narrowing bugs) #2661
I'm hoping the simplification of ConGetSet will also help with De-duplicate code for TerminalDispatch and AdaptDispatch #3849.
Some of the TextAttribute refactoring in this PR overlaps with similar work in PR Add support for XTPUSHSGR / XTPOPSGR #1978.

PR Checklist

Closes SGR 0 doesn't work correctly when combined with meta attributes #5341
CLA signed.
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: Improve the DECSC/DECRC implementation #3160 (comment)

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.

…y with SetAttributes.

…tribute constructor.

… dependence on internal details.

…ding GetColorTableEntry API.

… implementation.

DHowett-MSFT · 2020-05-05T23:09:47Z

i am so excited

DHowett-MSFT

This looks great to me. I want both @zadjii-msft and @miniksa to sign off, but if one of them isn't comfortable doing so alone I'll be the "third". 😄

DHowett-MSFT · 2020-05-07T00:11:45Z

src/buffer/out/OutputCellIterator.cpp

@@ -518,8 +518,7 @@ OutputCellView OutputCellIterator::s_GenerateView(const CHAR_INFO& charInfo) noe
        dbcsAttr.SetTrailing();
    }

-    TextAttribute textAttr;
-    textAttr.SetFromLegacy(charInfo.Attributes);
+    const TextAttribute textAttr(charInfo.Attributes);


tiny nit: it took me a while looking at this to get over the "most vexing parse"; can we use {} universal init syntax?

Yeah I'm a fan of the {} syntax too, but most of the surrounding code is using () initialization, at least in the areas I was modifying. So if we want to change that, I think it's best to handle that in a separate code cleanup PR that replaces everything consistently.

DHowett-MSFT · 2020-05-07T00:14:04Z

src/buffer/out/TextAttribute.hpp

+    bool HasIdenticalVisualRepresentationForBlankSpace(const TextAttribute& other, const bool inverted = false) const noexcept
    {
        // sneaky-sneaky: I'm using xor here
        // inverted is whether there's a global invert; Reverse is a local one.
        // global ^ local == true : the background attribute is actually the visible foreground, so we care about the foregrounds being identical
        // global ^ local == false: the foreground attribute is the visible foreground, so we care about the backgrounds being identical
-        const auto checkForeground = (inverted != _IsReverseVideo());
+        const auto checkForeground = (inverted != IsReverseVideo());


Wondering if we should move more of the getters into the header and mark them as constexprable instead of stripping it off. Thoughts?

I'm glad you brought this up, because I was actually meaning to propose moving more methods out of the header. I don't know if I'm missing something, but I couldn't see any reason why some methods were defined inline and others weren't - it seemed to me to be entirely arbitrary. And other than maybe the constructors, I couldn't see any benefit in making these methods constexpr if they're not actually being evaluated as constants anywhere.

That said, I don't feel strongly about it either way, but I would like to understand the reasoning behind the decision. Like what criteria do we use to decide whether a method should be defined inline in the header or not?

DHowett-MSFT · 2020-05-07T00:15:26Z

src/host/getset.cpp

 {
    RETURN_HR_IF(E_INVALIDARG, index >= 256);
    try
    {
        Globals& g = ServiceLocator::LocateGlobals();
        CONSOLE_INFORMATION& gci = g.getConsoleInformation();

-        gci.SetColorTableEntry(index, value);
+        gci.SetColorTableEntry(::Xterm256ToWindowsIndex(index), value);


tear_of_joy.exe

Would love to get rid of this mapping entirely at some point, but for now it seems preferable to handle here, and keep the dispatch code cleaner.

DHowett-MSFT · 2020-05-07T00:16:34Z

src/host/ut_host/OutputCellIteratorTests.cpp

@@ -102,8 +102,7 @@ class OutputCellIteratorTests
    {
        SetVerifyOutput settings(VerifyOutputSettings::LogOnlyFailures);

-        TextAttribute attr;
-        attr.SetFromLegacy(FOREGROUND_RED | BACKGROUND_BLUE);
+        const TextAttribute attr(FOREGROUND_RED | BACKGROUND_BLUE);


(nit: "most vexing parse" comment again)

As I mentioned above, this is just the style of the surrounding code. Notice the settings variable above, and the iterator and view a couple lines below.

DHowett-MSFT · 2020-05-07T00:18:49Z

src/terminal/adapter/adaptDispatchGraphics.cpp

-// - <none>
-void AdaptDispatch::s_ApplyColors(WORD& attr, const WORD applyThis, const bool isForeground) noexcept
-{
-#pragma warning(suppress : 26496) // SA is wrong. This variable is assigned more than once.


I love that this SA comment is just.. entirely incorrect. It's assigned once. LOL.

That'd be me. I had the arguments that were const vs. not in WI_UpdateFlagsInMask incorrect in my head. Woops.

DHowett-MSFT · 2020-05-07T00:19:33Z

src/terminal/adapter/adaptDispatchGraphics.cpp

-// - isForeground - Location to place whether or not the parsed color is for the foreground or not.
-// - optionsConsumed - Location to place the number of options we consumed parsing this option.
+// - attr - The attribute that will be updated with the parsed color.
+// - isForeground - Whether or not the parsed color is for the foreground or not.


Suggested change

// - isForeground - Whether or not the parsed color is for the foreground or not.

// - isForeground - Whether or not the parsed color is for the foreground.

I don't trust github not to screw up the line endings if I accept this here, but I'll make another commit with your suggested comment changes.

DHowett-MSFT · 2020-05-07T00:24:30Z

src/terminal/adapter/adaptDispatchGraphics.cpp

+            optionsConsumed = 2;
+            const size_t tableIndex = til::at(options, 1);
+            COLORREF rgbColor;
+            if (_pConApi->PrivateGetColorTableEntry(tableIndex, rgbColor))
            {


Suggested change

{

{

// TODO GH#xxxx: Decouple xterm-256color indexed storage from RGB storage

(I don't know if this is best covered by 2661.)

Yeah, this can't be done until the color tables are unified (essentially issue #1223). I've got a followup PR in the works that will address this, but I'll add a TODO comment in the meantime.

DHowett-MSFT · 2020-05-07T00:26:16Z

src/terminal/adapter/adaptDispatchGraphics.cpp

+const BYTE BLUE_ATTR      = 0x01;
+const BYTE GREEN_ATTR     = 0x02;
+const BYTE RED_ATTR       = 0x04;
+const BYTE BRIGHT_ATTR    = 0x08;
+const BYTE DARK_BLACK     = 0;
+const BYTE DARK_RED       = RED_ATTR;
+const BYTE DARK_GREEN     = GREEN_ATTR;
+const BYTE DARK_YELLOW    = RED_ATTR | GREEN_ATTR;
+const BYTE DARK_BLUE      = BLUE_ATTR;
+const BYTE DARK_MAGENTA   = RED_ATTR | BLUE_ATTR;
+const BYTE DARK_CYAN      = GREEN_ATTR | BLUE_ATTR;
+const BYTE DARK_WHITE     = RED_ATTR | GREEN_ATTR | BLUE_ATTR;
+const BYTE BRIGHT_BLACK   = BRIGHT_ATTR;
+const BYTE BRIGHT_RED     = BRIGHT_ATTR | RED_ATTR;
+const BYTE BRIGHT_GREEN   = BRIGHT_ATTR | GREEN_ATTR;
+const BYTE BRIGHT_YELLOW  = BRIGHT_ATTR | RED_ATTR | GREEN_ATTR;
+const BYTE BRIGHT_BLUE    = BRIGHT_ATTR | BLUE_ATTR;
+const BYTE BRIGHT_MAGENTA = BRIGHT_ATTR | RED_ATTR | BLUE_ATTR;
+const BYTE BRIGHT_CYAN    = BRIGHT_ATTR | GREEN_ATTR | BLUE_ATTR;
+const BYTE BRIGHT_WHITE   = BRIGHT_ATTR | RED_ATTR | GREEN_ATTR | BLUE_ATTR;


we can probably call these static constexpr; static so they don't appears as global symbols, and constexpr because it feels most right

Again I'm in agreement with you on this - I prefer constexpr - but I just copied the format from the TerminalDispatchGraphics implementation for consistency (see here). And note that both const and constexpr have internal linkage in C++ (at least by default), so static shouldn't be necessary as I understand it.

Also, I'm rather hoping we can rid of these constants in the long term anyway. Or at least unify the two sets into a shared header somewhere. It's just that we've got this weird situation at the moment where the conhost color table is in a different order to the one in Windows Terminal. Ideally we'll get those in sync at some point.

const and constexpr have internal linkage

Fine by me! I'm all for "unify these later and clean them up at the same time". THanks!

We do have src/inc/conattrs.h, which might be an appropriate place for them 🤔?

Problem is the constants in AdaptDispatch are currently different from the ones in TerminalDispatch. Once we can unify them, assuming that's possible, I'd be happy to move them to conattrs. I wanted to leave that for a later PR, though, because I think it might be complicated, and it's not strictly necessary to get to #2661.

DHowett-MSFT · 2020-05-07T00:27:44Z

None of my comments surfaced above nit level. As always, excellent work.

j4james · 2020-05-07T12:51:32Z

@DHowett-MSFT Btw I'm sorry if it seems like I'm rejecting all of your suggestions, but I just wanted to clarify why I took the approach I did in each of those cases. If you (or anyone else) feels strongly about any of these things, do feel free to push me to change them. I won't mind.

j4james · 2020-05-07T13:08:10Z

New misspellings found, please review:

Bitfields

🤔 I'm not sure why I'm suddenly getting the blame for that misspelling. It looks like it's been there since the beginning of time. It's not even a file I've touched. Should I be trying to fix this?

github-actions · 2020-05-07T13:21:18Z

New misspellings found, please review:

Bitfields

To accept these changes, run the following commands

remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
bitfield
Emojis
HREF
memcpying
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
Bitfields
href
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/1513de7607d75f067093cee2831770c7940c2a6e.txt'

✏️ Contributor please read this

If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
If you need to use a specific token in one place and it shouldn't generally be used, you can
add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

DHowett-MSFT · 2020-05-07T15:37:24Z

Btw I'm sorry if it seems like I'm rejecting all of your suggestions

Hey, that's why they're nits. Don't worry about! Your reasoning makes sense, afterall.

DHowett-MSFT · 2020-05-07T15:40:04Z

Paging @jsoref - we're seeing the spelling action flag something that already existed and isn't in a whitelist, but can't figure out why it's suddenly complaining. Any ideas?

jsoref · 2020-05-07T16:01:12Z

It used to whitelist bitfield, there's logic that groups plurals together. Apparently you removed bitfield:

-// - Small helper to help mask off the appropriate foreground/background bits in the colors bitfield.

So now there are only misspellings for Bitfields.

There's a hint about that (admittedly, not stellar) in the to apply bits:

remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
bitfield
...
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
Bitfields
href
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/1513de7607d75f067093cee2831770c7940c2a6e.txt'

jsoref · 2020-05-07T16:03:53Z

I suspect you'll want to move bitfield into dictionary/ so that it can come and go, since from your perspective it's a word, not an exception.

jsoref · 2020-05-07T16:18:34Z

Essentially, the goal of the whitelist (soon to be expect -- I need to make a release ...) is allow the fewest non-dictionary items. So if it can suggest a stricter list than the current list, it will. And it does that by removing things which are no longer "misspelled".

On the other hand, the dictionary/ is a place where you put things which are just words, things you can use or not use at will.
Someone should probably go through the whitelist and determine which should be promoted into the dictionary.

An example would be a whitelist for unknwns and unknwn -- if you had both misspellings of the unknown word family, then initially you whitelist unknwn and it will tolerate both. But if you correct all the instances of unknwn to unknown but leave the instances of unknwns, then it suggests replacing the whitelist entry for unknwn with one for unknwns -- that will allow you to continue using unknwns without introducing new instances of unknwn.

Note to self: I need to fix it so that it doesn't call them new misspellings, because, you're right, they aren't new. They're just requests to update the whitelist... I'll need to think about how to do that.

Gory implementation detail:

The magic it uses starts by grouping things if the singular base is present. (This is partially a compression algorithm, otherwise the whitelist would be much larger, having to contain all the plurals / case variations / ....)
What happens here is the singular base is no longer present, can't be grouped to the missing singular base, and thus it's seen as a new word.

j4james · 2020-05-07T17:23:55Z

I'm not really sure what I need to be doing here, so I could do with some clarification on a few points:

I've run the script, and it's removed a bunch of words from the whitelist.txt file which I assume are no longer needed. Should I now be committing that updated list?
The script also created a file with a random filename containing the words Bitfields and href. Am I right in assuming those are the words I'm now expected to deal with manually?
Assuming the answer to the second question is yes, what exactly should I do? You say we can add bitfield to the dictionary, and that should fix Bitfields, but what about href? Is that something I need to deal with too? Should I add it back to the whitelist in lowercase (it was previously included as HREF, but the script has now removed that)?

DHowett-MSFT · 2020-05-07T17:33:15Z

For now, let's just go with the easier solution of putting "bitfields" into the dictionary and not trimming the whitelist/fixing href. We can do that in a later commit so as to not complicate the history of this PR 😄

DHowett-MSFT · 2020-05-07T17:33:24Z

(Thanks for dealing with this.)

jsoref · 2020-05-07T17:40:44Z

That file could be git added (~~I need to fix the script to suggest adding it~~ I've written the code for that -- it's in prerelease) -- if there's only one expect file, it just updates it, if there are multiple, it picks a random filename (based on the commit sha) to avoid running into merge conflicts between multiple pending PRs (since as you've seen it will automatically prune from all of them, and thus doesn't really care about their names).

Alternatively, bitfield, update: and bitfields, and href could be added to:
https://github.com/microsoft/terminal/blob/master/.github/actions/spell-check/dictionary/apis.txt

You can create a branch w/ that additional commit and push it to your repository w/o pushing it to this PR and let the action run, the action should run (ideally pass) or comment (if it doesn't like things). That way you can avoid churn to the Azure bots attached to the PR.

jsoref · 2020-05-07T17:42:21Z

Note: href is probably in the same boat. The repository had HREF (and apparently no unignored instances of href?!)... If you're going to update dictionary, I'd definitely add both. (Sorry, I didn't send the earlier comment earlier. I decided to try to fix the suggestion code...)

zadjii-msft

Man this is what I only dreamed this code could be 😄 I really don't have any other complaints or concerns about this. I don't really care one way or another about the constexpr stuff - I think we in the past had a push to move as much code to constexpr as possible, but we've kinda abandoned that pursuit for now.

zadjii-msft · 2020-05-08T14:13:22Z

src/terminal/adapter/adaptDispatchGraphics.cpp

+const BYTE BLUE_ATTR      = 0x01;
+const BYTE GREEN_ATTR     = 0x02;
+const BYTE RED_ATTR       = 0x04;
+const BYTE BRIGHT_ATTR    = 0x08;
+const BYTE DARK_BLACK     = 0;
+const BYTE DARK_RED       = RED_ATTR;
+const BYTE DARK_GREEN     = GREEN_ATTR;
+const BYTE DARK_YELLOW    = RED_ATTR | GREEN_ATTR;
+const BYTE DARK_BLUE      = BLUE_ATTR;
+const BYTE DARK_MAGENTA   = RED_ATTR | BLUE_ATTR;
+const BYTE DARK_CYAN      = GREEN_ATTR | BLUE_ATTR;
+const BYTE DARK_WHITE     = RED_ATTR | GREEN_ATTR | BLUE_ATTR;
+const BYTE BRIGHT_BLACK   = BRIGHT_ATTR;
+const BYTE BRIGHT_RED     = BRIGHT_ATTR | RED_ATTR;
+const BYTE BRIGHT_GREEN   = BRIGHT_ATTR | GREEN_ATTR;
+const BYTE BRIGHT_YELLOW  = BRIGHT_ATTR | RED_ATTR | GREEN_ATTR;
+const BYTE BRIGHT_BLUE    = BRIGHT_ATTR | BLUE_ATTR;
+const BYTE BRIGHT_MAGENTA = BRIGHT_ATTR | RED_ATTR | BLUE_ATTR;
+const BYTE BRIGHT_CYAN    = BRIGHT_ATTR | GREEN_ATTR | BLUE_ATTR;
+const BYTE BRIGHT_WHITE   = BRIGHT_ATTR | RED_ATTR | GREEN_ATTR | BLUE_ATTR;


We do have src/inc/conattrs.h, which might be an appropriate place for them 🤔?

miniksa · 2020-05-08T17:11:48Z

@DHowett-MSFT Btw I'm sorry if it seems like I'm rejecting all of your suggestions, but I just wanted to clarify why I took the approach I did in each of those cases. If you (or anyone else) feels strongly about any of these things, do feel free to push me to change them. I won't mind.

Hey look @j4james, you're pulling a me when @DHowett-MSFT piles nits all over my PRs! 🤣

src/terminal/adapter/ut_adapter/Adapter.UnitTests.vcxproj

miniksa

This is so much better. Thank you very much!

miniksa · 2020-05-08T19:53:23Z

src/cascadia/TerminalCore/TerminalApi.cpp

-    if (boldOn)
-    {
-        attrs.Embolden();
-    }
-    else
-    {
-        attrs.Debolden();
-    }


Mildly sad to see the terminology of "embolden" and "debolden" go. It was always amusing when I came across it.

DHowett-MSFT · 2020-05-08T21:48:02Z

Argh, we conflicted on the file you didn't want to have to change. I'm sorry about that. :|

I'm happy to fix this up if I have maintainer-write access to yourbranch

jsoref · 2020-05-08T21:57:38Z

Sorry.

I'm open to suggestions for how to deal w/ that. It's why I switched to having the script generate random filenames for the whitelist. -- It's also why the cleanup script rewrites the files instead of offering a patch, patches are way too brittle.

I suppose the contributor recommendation could be the same: use a SHA, and then at a later point the maintainers could merge the dictionary files together.

j4james · 2020-05-08T22:34:05Z

Argh, we conflicted on the file you didn't want to have to change. I'm sorry about that. :|

I'm happy to fix this up if I have maintainer-write access to yourbranch

Sorry, I thought I'd already ticked that. Please feel free to fix.

j4james · 2020-05-08T22:40:26Z

Hmmm... I'm not sure checking that "allow edits by maintainers" box works. Every time I refresh this page, it's unchecked again. I'll try resolve the conflicts myself.

DHowett-MSFT · 2020-05-08T23:04:23Z

Thanks so much for this.

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

ghost · 2020-06-18T17:02:17Z

🎉Windows Terminal Preview v1.1.1671.0 has been released which incorporates this pull request.:tada:

Handy links:

This is essentially a rewrite of the `TerminalDispatch::SetGraphicsRendition` method, bringing it into closer alignment with the `AdaptDispatch` implementation, simplifying the `ITerminalApi` interface, and making the code easier to extend. It adds support for a number of attributes which weren't previously implemented. REFERENCES * This is a mirror of the `AdaptDispatch` refactoring in PR #5758. * The closer alignment with `AdaptDispatch` is a small step towards solving issue #3849. * The newly supported attributes should help a little with issues #5461 (italics) and #6205 (strike-through). DETAILS I've literally copied and pasted the `SetGraphicsRendition` implementation from `AdaptDispatch` into `TerminalDispatch`, with only few minor changes: * The `SetTextAttribute` and `GetTextAttribute` calls are slightly different in the `TerminalDispatch` version, since they don't return a pointless `success` value, and in the case of the getter, the `TextAttribute` is returned directly instead of by reference. Ultimately I'd like to move the `AdaptDispatch` code towards that way of doing things too, but I'd like to deal with that later as part of a wider refactoring of the `ConGetSet` interface. * The `SetIndexedForeground256` and `SetIndexedBackground256` calls required the color indices to be remapped in the `AdaptDispatch` implementation, because the conhost color table is in a different order to the XTerm standard. `TerminalDispatch` doesn't have that problem, so doesn't require the mapping. * The index color constants used in the 16-color `SetIndexedForeground` and `SetIndexedBackground` calls are also slightly different for the same reason. VALIDATION I cherry-picked this code on top of the #6506 and #6698 PRs, since that's only way to really get the different color formats passed-through to the terminal. I then ran a bunch of manual tests with various color coverage scripts that I have, and confirmed that all the different color formats were being rendered as expected. Closes #6725

DHowett · 2020-07-02T20:58:34Z

🎉 Once again, thanks for the contribution!

This pull request was included in a set of conhost changes that was just
released with Windows Insider Build 20161.

j4james added 8 commits May 5, 2020 22:43

Get rid of unused legacy methods in ATTR_ROW and OutputCell.

dbf25c4

Replace unnecessary usage of TextAttributeRun::SetAttributesFromLegac…

562bdec

…y with SetAttributes.

Replace unnecessary usage of TextAttribute::SetFromLegacy with TextAt…

aeb1c7f

…tribute constructor.

Add methods to the TextAttribute class to update attributes with less…

0f08581

… dependence on internal details.

Update SetColorTableEntry API to take XTerm indices and add correspon…

249003f

…ding GetColorTableEntry API.

Rewrite the AdaptDispatch::SetGraphicsRendition method with a simpler…

9534337

… implementation.

Update adapter tests to account for the changes to the ConGetSet API.

3c96482

Remove all the APIs that aren't needed anymore.

ee517f6

ghost added Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase labels May 5, 2020

j4james marked this pull request as ready for review May 6, 2020 23:31

DHowett-MSFT approved these changes May 7, 2020

View reviewed changes

Add suggested comment changes and TODO note.

1513de7

This comment has been minimized.

Sign in to view

Add bitfield(s) and href to the apis dictionary.

9252ea2

j4james force-pushed the refactor-sgr-dispatch branch from 99f7703 to 9252ea2 Compare May 7, 2020 18:03

zadjii-msft approved these changes May 8, 2020

View reviewed changes

miniksa reviewed May 8, 2020

View reviewed changes

src/terminal/adapter/ut_adapter/Adapter.UnitTests.vcxproj Show resolved Hide resolved

miniksa approved these changes May 8, 2020

View reviewed changes

Merge branch 'master' into refactor-sgr-dispatch

a0dd0b6

DHowett-MSFT merged commit e7a2732 into microsoft:master May 8, 2020

j4james deleted the refactor-sgr-dispatch branch May 10, 2020 18:38

ghost mentioned this pull request Jun 18, 2020

SGR 0 doesn't work correctly when combined with meta attributes #5341

Closed

This was referenced Jun 30, 2020

Refactor the TerminalDispatch graphics rendition to match AdaptDispatch #6725

Closed

Refactor TerminalDispatch (graphics) to match AdaptDispatch #6728

Merged

	// - isForeground - Whether or not the parsed color is for the foreground or not.
	// - isForeground - Whether or not the parsed color is for the foreground.

	{
	{
	// TODO GH#xxxx: Decouple xterm-256color indexed storage from RGB storage

Refactor the SGR implementation in AdaptDispatch #5758

Refactor the SGR implementation in AdaptDispatch #5758

Conversation

j4james commented May 5, 2020 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

DHowett-MSFT commented May 5, 2020

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented May 7, 2020

j4james commented May 7, 2020

j4james commented May 7, 2020

New misspellings found, please review:

github-actions bot commented May 7, 2020

New misspellings found, please review:

⚠️ Reviewers

DHowett-MSFT commented May 7, 2020

DHowett-MSFT commented May 7, 2020

This comment has been minimized.

jsoref commented May 7, 2020

jsoref commented May 7, 2020

jsoref commented May 7, 2020

j4james commented May 7, 2020

DHowett-MSFT commented May 7, 2020

DHowett-MSFT commented May 7, 2020

jsoref commented May 7, 2020 • edited Loading

jsoref commented May 7, 2020 • edited Loading

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa commented May 8, 2020

miniksa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented May 8, 2020

jsoref commented May 8, 2020

j4james commented May 8, 2020

j4james commented May 8, 2020

DHowett-MSFT commented May 8, 2020

ghost commented Jun 18, 2020 • edited by miniksa Loading

DHowett commented Jul 2, 2020

j4james commented May 5, 2020 •

edited

Loading

jsoref commented May 7, 2020 •

edited

Loading

jsoref commented May 7, 2020 •

edited

Loading

ghost commented Jun 18, 2020 •

edited by miniksa

Loading