Repository: microsoft/terminal (language: cpp)

Issue:
RGB colour values for CMD colours don't update as you select new colours.
Moved from MSFT:20246999


RGB colour values for CMD colours don't update as you select new colours.

They  don't match the selected colour when you change the item (like  foreground or background). They only change when you pick a new colour  from the swatch. See repro screenshots.


This is not a regression (from RS4, even), and is a UI issue only.






Discussion / hints:
@microsoft/windows-console-team Can we attach the "repro screenshots" described above here?
Here's the deal:

```sh
printf '\e[38;5;134mColor_134_Before\e[m\n'
printf '\e]4;134;rgb:ff/00/ff\e\'
printf '\e[38;5;134mColor_134_after\e[m\n'
printf '\e]4;134;rgb:00/00/ff\e\'
printf '\e[38;5;134mColor_134_after_2\e[m\n'
```

These five lines do the following:

1. Print something in color 134 using CSI m
1. Change the definition of color 134 to be `#FF00FF` using OSC 4
1. Print something in color 134
1. Change the definition of color 134 to be `#0000FF`
1. Print something in color 134

It looks like this:

![image](https://user-images.githubusercontent.com/14316954/64140922-0e0fb680-cdbb-11e9-804f-60b2bf94f066.png)

This is actually wrong! Other terminal emulators remember that all three lines are in "color 134", and when color 134 _changes definition_, the existing lines change too!

This is what it looks like in xterm:

![image](https://user-images.githubusercontent.com/14316954/64141050-85dde100-cdbb-11e9-80d5-b71af804c424.png)



So using a UWP/XAML metaphor...

You believe the console colours should be treated like ThemeResources and not Static ones.  So they update as the "theme"/colour scheme changes?

What kind of perf impact would that have?
> You believe ...

Well, xterm (the reference implementation) does it this way, so it is less a matter of belief and more a matter of correctness. :wink:

We already store color indices for runs using indices 0-16; it should be a fairly minimal perf impact to broaden that to indices 0-255 and trigger a frame repaint when the color table changes.

In general, I think it may not have been the best choice for us to always store the RGB color in the attribute run -- we should have only done so for `38;2` and `48;2` (true color) runs. It unnecessarily couples the color table and attribute storage.
But yeah, actually, ThemeResource is an apt analogy. You’ve also helped me figure out what a ThemeResource is meant to do!
@DHowett-MSFT  And I learned that there is an existing spec that Windows Terminal is working towards, other than Bash/Unix compatibility for the WSL profiles.
Whoops, I totally messed up on this bug.  'm going to move the comments later to the _correct_ issue for OSC 4.

@carlos-zamora the actual issue is this one:

![image](https://user-images.githubusercontent.com/14316954/64144910-75356700-cdcb-11e9-92ae-120fab345db6.png)

These radio buttons don't change the RGB boxes, but they do seemingly select the correct color swatch below.
I was going to say, the issue Dustin's talking about is #1223, which is related, but _different_. I have a fix ready for #1223, which is almost exactly what Dustin suggested. 



Fix patch:
diff --git a/src/propsheet/ColorsPage.cpp b/src/propsheet/ColorsPage.cpp
index 6441bab42b3..a5e223bc982 100644
--- a/src/propsheet/ColorsPage.cpp
+++ b/src/propsheet/ColorsPage.cpp
@@ -186,6 +186,11 @@ INT_PTR WINAPI ColorDlgProc(HWND hDlg, UINT wMsg, WPARAM wParam, LPARAM lParam)
                 InvalidateRect(hWndOld, nullptr, TRUE);
             }
 
+            // update color value fields to reflect the new color
+            UpdateItem(hDlg, IDD_COLOR_RED, GetRValue(AttrToRGB(ColorArray[iColor])));
+            UpdateItem(hDlg, IDD_COLOR_GREEN, GetGValue(AttrToRGB(ColorArray[iColor])));
+            UpdateItem(hDlg, IDD_COLOR_BLUE, GetBValue(AttrToRGB(ColorArray[iColor])));
+
             return TRUE;
 
         case IDD_COLOR_RED:


Test patch: