Fix SGR indexed colors to distinguish Indexed256 color (and more) #5834

j4james · 2020-05-10T19:39:44Z

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
render: defer conversion of TextColor into COLORREF until actual rendering time (avoid ConPTY narrowing bugs) #2661.
This is technically a fix for issue Should the bold/bright attribute apply to SGR 38/48 indexed colors? #5384, but that won't be apparent
until render: defer conversion of TextColor into COLORREF until actual rendering time (avoid ConPTY narrowing bugs) #2661 is complete.

PR Checklist

Closes [host] Changing a color table value above 16 does not update the color of other cells with that index #1223
CLA signed.
Tests added/passed
Requires documentation to be updated
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.

…an be brightened), and the 256 color palette (which can't).

…8:5 and 48:5 escape sequences.

…g the tables around.

…:5 and 48:5 escape sequences.

…xed colors.

DHowett-MSFT · 2020-05-12T21:48:43Z

(Is this a draft because of the description or the implementation? If the implementation, we can start preliminary review 😄)

j4james · 2020-05-12T22:03:25Z

Well partially the description, and partially because I wanted to do more testing of the different places that can interact with the color tables (registry, properties dialog, shortcuts, etc.). The problem I had was that the shortcut saving would fail randomly, and I've been trying to figure it why. I still don't know the answer to that, but I have now reached the conclusion that it's probably an existing bug. In short, you can start a preliminary review if you want, and I'll try and get the description finished tonight if I can.

DHowett-MSFT

I've said it before, and I will say it again: This work is excellent, and I appreciate having somebody digging through our terrifying legacy to make things better.

DHowett-MSFT · 2020-05-12T03:32:52Z

src/buffer/out/TextAttribute.hpp

+        _foreground{ gsl::narrow_cast<BYTE>(wLegacyAttr & FG_ATTRS), true },
+        _background{ gsl::narrow_cast<BYTE>((wLegacyAttr & BG_ATTRS) >> 4), true },


Concerned about the difference between console color and ANSI color indices; also, should this be false since the console 16 are equivalent to the ANSI 16?

(Right now, I believe (?) a round trip from legacy attribute to TextAttribute back to legacy attribute will fail because of the brighten check on line R67.)

It is possibly debatable whether legacy colors should map to Index16 or Index256, but see the PR description for my reasoning behind the decision. Legacy roundtrips are never going to be perfect once you mix them with VT attributes, but if you're sticking to console APIs, they should be fine.

Oh, I think I understand. We store INTENSITY in the meta field, and it’ll come back out during ReadConsoleOutput or GetConsoleTextAttribute, but GetLegacyAttributes will not explicitly set it or clear it. That might be worth a comment- what do you think?

(How does this comport with BACKGROUND_INTENSITY, and its potential impact on whether conpty should generate 10x or 40x? Am I mixing concerns here and worrying about the wrong thing?)

No, the INTENSITY bit of legacy colors is not stored as a meta attribute, it's just one of the 4 bits making up the index value. There is a bold extended attribute, but that it not set by legacy colors, and has no effect on their rendering. The FOREGROUND_INTENSITY that you see being added in the GetLegacyAttributes method is just to handle the conversion of VT ANSI colors (which can be brightened), to an equivalent legacy value.

As for the conpty rendering, under these rules legacy colors would almost always be rendered with ISO/ITU sequences. Remember they're stored with the IsIndex256 type. The only time we'd map them to SGR 3x/4x sequences is if we were using one of the 16-color renderers that didn't have any other option.

Whoops, thanks for the reminder about the intensity bit being on the color. That all makes sense.

As for the conpty rendering, under these rules legacy colors would almost always be rendered with ISO/ITU sequences.

Just rereading what I wrote here, I should clarify that I'm talking about the future version of the vtengine that I'm working on for issue #2661. The current vtengine doesn't yet take these index types into account.

src/interactivity/win32/menu.cpp

DHowett-MSFT · 2020-05-12T23:07:10Z

src/host/ut_host/ScreenBufferTests.cpp


-    auto expectedAttr = TextAttribute(XtermToLegacy(1, 12));
+    auto expectedAttr = TextAttribute{};
+    expectedAttr.SetIndexedForeground((BYTE)XtermToWindowsIndex(1));
+    expectedAttr.SetIndexedBackground((BYTE)XtermToWindowsIndex(12));
    stateMachine.ProcessString(L"\x1b[31;104m");


(It seems like this needed to change because of the treatment of legacy as 256=true in TextAttribute.hpp)

Yep. But as I said, this was a deliberate choice. I didn't think it was appropriate to have legacy dark red map to ansi red, because I don't think it should be possible to brighten a legacy attribute.

src/host/conattrs.cpp

DHowett-MSFT · 2020-05-12T23:15:46Z

src/terminal/adapter/ut_adapter/adapterTest.cpp

@@ -1286,42 +1286,42 @@ class AdapterTest
        case DispatchTypes::GraphicsOptions::ForegroundBlack:
            Log::Comment(L"Testing graphics 'Foreground Color Black'");
            _testGetSet->_attribute = FOREGROUND_RED | FOREGROUND_GREEN | FOREGROUND_BLUE | FOREGROUND_INTENSITY;
-            _testGetSet->_expectedAttribute = 0;
+            _testGetSet->_expectedAttribute.SetIndexedForeground(0);


This one stands out to me as a change in test behavior -- on the left side it's replacing and on the right side it's additive, right?

This might not be a meaningful change, of course, I just can't tell from here

Yeah, this is relying on the fact that the _expectedAttribute is initialized in PrepData. I was trying to keep the changes minimal, but it would probably be clearer what's going on if I set _expectedAttribute to _attribute before updating the foreground index, the same way I've done the background tests below this. Would that make sense to you?

I’m fine with this honestly. Just so long as we understand where we’re changing the behavior and that it’s not impactful.

Idly, I wonder if our internal APIs should eventually move towards immutable structs and “WithXxx” functions that return a copy. That’s not work for this PR, but maybe good for me to file as a CodeHealth issue. Dunno. It’s germane here because it would become _attribute.WithIndexedFG()

That's an interesting thought. It would certainly make these test more readable, but I'd be curious to see how well something like that optimises and how well the pattern works in the main code. I wouldn't want to add those methods exclusively for use by the unit tests, because that means the tests aren't actually testing the real code - they'd just be testing their own private code path.

Just for fun, I threw something silly into godbolt to see how it optimizes.

https://godbolt.org/z/UZik2K

void usingWith(void) PROC ; usingWith, COMDAT $LN6: sub rsp, 40 ; 00000028H call QWORD PTR __imp_TextAttribute CantOptimizeMe_Sucker(void) movzx ecx, al shr ax, 8 or cl, 16 mov BYTE PTR $T1[rsp+1], al mov BYTE PTR $T1[rsp], cl lea rcx, QWORD PTR newAttribute$[rsp] movzx eax, WORD PTR $T1[rsp] mov WORD PTR newAttribute$[rsp], ax call QWORD PTR __imp_void CantOptimizeThis_Either(TextAttribute const &) add rsp, 40 ; 00000028H ret 0 void usingWith(void) ENDP

It does end up a little more verbose than the alternative (setting it directly):

void usingSetter(void) PROC ; usingSetter, COMDAT $LN6: sub rsp, 40 ; 00000028H call QWORD PTR __imp_TextAttribute CantOptimizeMe_Sucker(void) mov WORD PTR secondOldAttr$[rsp], ax lea rcx, QWORD PTR secondOldAttr$[rsp] or BYTE PTR secondOldAttr$[rsp], 16 call QWORD PTR __imp_void CantOptimizeThis_Either(TextAttribute const &) add rsp, 40 ; 00000028H ret 0 void usingSetter(void) ENDP

Inconclusive, and probably micro-optimization, but we also don't need to do the With() work now. 😁

clang optimizes them to the same thing. LOL.

EDIT: clang optimizes them such that With... is slightly cheaper because it doesn't load the two bytes as half registers and instead just manipulates the whole attribute as a 16-bit value (it was 16 in my tests)

Note that the With... version does tend to get a worse the more fields you add though. Well except in clang, which is clearly using some kind of alien technology. 👽

Anyway, for now I've checked in a commit that initialises _expectedAttribute to _attribute before calling SetIndexedForeground. I think that makes things a little clearer, and is also more consistent with the SetIndexedBackground test cases.

…xpected attribute before overriding the foreground index.

DHowett-MSFT · 2020-05-15T00:04:24Z

(reformatted the description to be in the gitcommit 80-column style -- content not changed -- and marked for additional review. GitHub doesn't send e-mails about PRs moving out of Draft)

DHowett-MSFT

Having read over this in its entirety before publication, and wrestled with some of the broader implications on legacy compatibility, I'm comfortable with it.

It breaks a very specific case that I ~~think~~ hope nobody was ever using in production (and if they were, I'd like to have some words with them about proper etiquette!)

	SetConsoleTextAttribute(o, FOREGROUND_RED);
	printf("DarkRed\x1b[1mBrightRed\x1b[m\n");

(WHY)

I chatted with @miniksa about this on Teams yesterday, and he suggests that he may also be comfortable with it. I'm not signing off on his behalf, of course, so this is simply informative.

I'm comfortable with us making the call that the bottom 16 xterm-256 indices are simply not brightenable.

j4james · 2020-05-15T00:21:20Z

It breaks a very specific case that I ~~think~~ hope nobody was ever using in production (and if they were, I'd like to have some words with them about proper etiquette!)
	SetConsoleTextAttribute(o, FOREGROUND_RED);
	printf("DarkRed\x1b[1mBrightRed\x1b[m\n");

It's probably not a big deal to change this behaviour if we don't like it, but I think this is the right choice. If you want bright red, you set the FOREGROUND_INTENSITY attribute. And applying SGR 1 in this situation could still be used to select a bold font weight one day, assuming anyone really wanted to mix legacy and VT code.

miniksa · 2020-05-27T20:48:07Z

Adding @DHowett because he changed himself from the MS-specific account to a unified one.

Going to read this today. Sorry for the delay.

DHowett

Approved as my real account. Sorry/thanks!

miniksa · 2020-05-27T21:01:47Z

It breaks a very specific case that I ~~think~~ hope nobody was ever using in production (and if they were, I'd like to have some words with them about proper etiquette!)
	SetConsoleTextAttribute(o, FOREGROUND_RED);
	printf("DarkRed\x1b[1mBrightRed\x1b[m\n");
It's probably not a big deal to change this behaviour if we don't like it, but I think this is the right choice. If you want bright red, you set the FOREGROUND_INTENSITY attribute. And applying SGR 1 in this situation could still be used to select a bold font weight one day, assuming anyone really wanted to mix legacy and VT code.

I agree with this.

miniksa

I've got nothing that Dustin didn't already cover. Excellent work. I agree with your assessment on the "breaking change". Let's do this. Thanks again.

ghost · 2020-05-27T22:34:42Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

…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.

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

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

Handy links:

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

🎉 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 10, 2020 01:20

Distinguish between indexed colors from the 16 color palette (which c…

d5aabb4

…an be brightened), and the 256 color palette (which can't).

Make the terminal dispatcher use the 256 color index format for SGR 3…

414c727

…8:5 and 48:5 escape sequences.

Update unit tests to account for the updated index color formats.

6ca9015

Avoid directly copying the color table with memmove.

1814a58

Remove some unnecessary functions that made the code harder to follow.

c06c575

Combine the 16 and 256 color tables, and use string_views when passin…

a4c23a6

…g the tables around.

Make the conhost dispatcher use the 256 color index format for SGR 38…

f321051

…:5 and 48:5 escape sequences.

Update unit tests to account for the corrected interpretation of inde…

020b0d1

…xed colors.

ghost added 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 labels May 10, 2020

DHowett-MSFT reviewed May 12, 2020

View reviewed changes

Make the GraphicsSingleTests clearer by explicitly initialising the e…

beeb0c9

…xpected attribute before overriding the foreground index.

j4james marked this pull request as ready for review May 14, 2020 15:05

DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label May 15, 2020

ghost requested review from zadjii-msft, miniksa, carlos-zamora and leonMSFT May 15, 2020 00:03

DHowett-MSFT approved these changes May 15, 2020

View reviewed changes

miniksa requested a review from DHowett May 27, 2020 20:47

DHowett approved these changes May 27, 2020

View reviewed changes

miniksa approved these changes May 27, 2020

View reviewed changes

DHowett changed the title ~~Fix SGR indexed colors~~ Fix SGR indexed colors to distinguish Indexed256 color (and more) May 27, 2020

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 27, 2020

ghost merged commit fa7c1ab into microsoft:master May 27, 2020

j4james deleted the fix-indexed-colors branch June 4, 2020 22:59

ghost mentioned this pull request Jun 18, 2020

[host] Changing a color table value above 16 does not update the color of other cells with that index #1223

Closed

lhecker mentioned this pull request Jul 9, 2024

A minor ConPTY refactoring: Goodbye VtEngine Edition #17510

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SGR indexed colors to distinguish Indexed256 color (and more) #5834

Fix SGR indexed colors to distinguish Indexed256 color (and more) #5834

j4james commented May 10, 2020 •

edited by DHowett-MSFT

Loading

DHowett-MSFT commented May 12, 2020

j4james commented May 12, 2020

DHowett-MSFT left a comment

DHowett-MSFT May 12, 2020

DHowett-MSFT May 12, 2020

j4james May 13, 2020

DHowett-MSFT May 13, 2020

DHowett-MSFT May 13, 2020

j4james May 13, 2020

DHowett-MSFT May 13, 2020

j4james May 15, 2020

DHowett-MSFT May 12, 2020

j4james May 13, 2020

DHowett-MSFT May 12, 2020

j4james May 13, 2020

DHowett-MSFT May 13, 2020

j4james May 13, 2020

DHowett-MSFT May 13, 2020

DHowett-MSFT May 13, 2020 •

edited

Loading

j4james May 14, 2020

DHowett-MSFT commented May 15, 2020

DHowett-MSFT left a comment

j4james commented May 15, 2020

miniksa commented May 27, 2020

DHowett left a comment

miniksa commented May 27, 2020

miniksa left a comment

ghost commented May 27, 2020

ghost commented Jun 18, 2020 •

edited by miniksa

Loading

DHowett commented Jul 2, 2020

		_foreground{ gsl::narrow_cast<BYTE>(wLegacyAttr & FG_ATTRS), true },
		_background{ gsl::narrow_cast<BYTE>((wLegacyAttr & BG_ATTRS) >> 4), true },

Fix SGR indexed colors to distinguish Indexed256 color (and more) #5834

Fix SGR indexed colors to distinguish Indexed256 color (and more) #5834

Conversation

j4james commented May 10, 2020 • edited by DHowett-MSFT Loading

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

DHowett-MSFT commented May 12, 2020

j4james commented May 12, 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

DHowett-MSFT May 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented May 15, 2020

DHowett-MSFT left a comment

Choose a reason for hiding this comment

j4james commented May 15, 2020

miniksa commented May 27, 2020

DHowett left a comment

Choose a reason for hiding this comment

miniksa commented May 27, 2020

miniksa left a comment

Choose a reason for hiding this comment

ghost commented May 27, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Jun 18, 2020 • edited by miniksa Loading

DHowett commented Jul 2, 2020

j4james commented May 10, 2020 •

edited by DHowett-MSFT

Loading

DHowett-MSFT May 13, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost commented Jun 18, 2020 •

edited by miniksa

Loading