[Settings UI] [Colour schemes page] Redesign Colour Buttons #8997
Conversation
|
@msftbot make sure @cinnamon-msft signs off on this |
ghost
added
the
AutoMerge
|
Hello @zadjii-msft! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
There was a problem hiding this comment.
I think I'm fine with this, but I'd be more comfortable if @cinnamon-msft and @carlos-zamora were the two signoffs
| @@ -117,64 +117,64 @@ | |||
| <resheader name="writer"> | |||
| <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value> | |||
| </resheader> | |||
| <data name="ColorScheme_Background.Text" xml:space="preserve"> | |||
| <data name="ColorScheme_Background.[using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip" xml:space="preserve"> | |||
There was a problem hiding this comment.
why is this one (and others) [using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip while the rest are just ToolTip?
There was a problem hiding this comment.
Because that one is linked to the "Background" button (x:Uid="identifier"), while the rest are linked through a XAML binding.
| <TextBlock x:Uid="ColorScheme_Foreground" | ||
| Style="{StaticResource ColorHeaderStyle}"/> | ||
| <Button Background="{Binding Color, ElementName=ForegroundPicker, Converter={StaticResource ColorToBrushConverter}, Mode=OneWay}"> | ||
| <Button x:Uid="ColorScheme_Foreground" |
There was a problem hiding this comment.
@cinnamon-msft Do we want to keep the labels for Foreground, Background, Selection Color and Cursor Color?
|
I'm all for keeping the UI as clean as possible, but there could be accessibility implications, and you lose the ability to look at the page and associate the chosen colours with the original intent. |
|
Big-picture, one of the other things we're planning on is making the table look more like this:
I'll agree that as of this pr, the table is pretty illegible. My hope is that once we modify the page more to look like an actual color table, this will make more sense again |
How would that reflow for a narrow window?
|
|
I actually really like that. That looks even better |
|
I agree, I love @mdtauk's design. I do feel the current design of this PR is inaccessible, because you can't tell what color you're looking at unless you hover over/use the keyboard to select the color. |
|
Thank you, @mdtauk. I also quite like your design. I'll see if I can make that. |
|
Okay, I don't understand how the colour table works. Could someone please explain to me how we can reorganize the buttons like @mdtauk's? |
I am not a C/C++ coder, so I apologise if I am not understanding this. It looks like you have an array of "Colors" that make up a scheme. td::array<hstring, 16> TableColorNamesCould this become an array of arrays, so you can group the Bright and normal colours? Then each row can be a grid, with three columns, the last two set at the width of the button The other array/colour list is for the selection, cursor, foreground, and background colours? That can be set to the right as is, and moved below on window resize (StateTrigger?) |
|
@mdtauk Thank you for your prompt reply. I’ll take a look at it tomorrow as it is late here in Vancouver. |
|
To add, the array is split into two halves - the first 8 (0-7) are the "normal" colors (red, black, etc), and the second 8 (8-15) are the "bright" colors (bright red, bright black, etc). |
Does the order of those colours affect anything other than the order they show up in the settings UI? Or are they also tied to the writing out of Settings.JSON and other code elements? |
|
Yes, the order absolutely matters. Commandline client applications (traditionally) can't say "I'd like the text in red". They say "I'd like the text in whatever color 1 is". The client isn't in control of what the terminal decides color 1 should look like. It's standard that everyone agrees that color 1 is a red-like color, but that's not a guarantee. Case in point: "Batman" doesn't have a red. Instead, color 1 is just one of the yellows. |
Hmm that will possibly complicate things a bit. unless there is some array index manipulation to choose what goes where in the UI |
|
I mean, the order you have it in the screenshot looks perfectly good to me... |
How you would do that in Xaml, would require changing how the ItemsControl lists it's items I think. assuming each row is an item |
|
@Chips1234 Here are some tips that are hopefully helpful:
<ItemsControl ItemsSource="{x:Bind CurrentBrightColorTable, Mode=OneWay}"
ItemTemplate="{StaticResource ColorTableEntryDataTemplate}">
<ItemsControl.ItemsPanel>
<ItemsPanelTemplate>
<ItemsStackPanel Orientation="Vertical"/>
</ItemsPanelTemplate>
</ItemsControl.ItemsPanel>
</ItemsControl>
Let me know if there's anything I can do to help! We can use this thread to see your progress and give you feedback as you go. This is a really exciting change! |
DHowett
removed
the
AutoMerge
|
Thank you. |
|
@carlos-zamora, Sorry, but could you elaborate on the "Separating the colours" part? I am not sure how the |
Sure! Could you elaborate on what you need help with? In ColorSchemes.h/idl, look at any reference to |
Ah. Push your changes so I can take a look then haha. That'll help a bit. As for how to fix your problem, you'll want to populate that list with only the colors you care about. So, in a function called 'UpdateColorTable' or something like that, we populate the color table. There, update the logic to include the non-bright colors in one list and the bright colors in the other. |
Note: This is largely design, it is not complete yet!
|
Okay, I have published the changes. |
There was a problem hiding this comment.
Hoping these comments can help get the ball rolling. If you manage to get that done, here's some bonus points:
- since
8is a pretty important number here, I'd declare it as astatic constexprat the top of the cpp. Just to make the code look nicer. Give it a name likeColorTableDividermaybe?
| _CurrentColorTable.Append(entry); | ||
| _CurrentNonBrightColorTable.Append(entry); | ||
| _CurrentBrightColorTable.Append(entry); |
There was a problem hiding this comment.
Ah. So here, we're iterating over the TableColorNames and adding them to the two color tables right? But the first 8 are non-bright colors, and the remaining ones are bright colors. So, codify that with something like this:
if (i < 8)
{
// add to the non-bright color table
}
else
{
// add to the bright color table
}
| for (uint8_t i = 0; i < TableColorNames.size(); ++i) | ||
| { | ||
| _CurrentColorTable.GetAt(i).Color(colorScheme.Table()[i]); | ||
| _CurrentNonBrightColorTable.GetAt(i).Color(colorScheme.Table()[i]); | ||
| _CurrentBrightColorTable.GetAt(i).Color(colorScheme.Table()[i]); | ||
| } |
There was a problem hiding this comment.
A note about the architecture. A color scheme object has a Table property (see terminal settings model for more details). The Table is a 16-sized array of colors. Each color in that table aligns with the name in TableColorNames.
Here, the old code was taking the color from the color scheme object's table, and throwing it into the _CurrentColorTable list. In the new code, we want to have the non-bright colors go in the non-bright table, and the bright colors to go in the bright table. Again, 8 is the magic number here. Anything less than that is non-bright, anything greater is bright. Codify that.
| { | ||
| auto index = winrt::unbox_value<uint8_t>(tag); | ||
| CurrentColorScheme().SetColorTableEntry(index, args.NewColor()); | ||
| _CurrentColorTable.GetAt(index).Color(args.NewColor()); | ||
| _CurrentNonBrightColorTable.GetAt(index).Color(args.NewColor()); | ||
| _CurrentBrightColorTable.GetAt(index).Color(args.NewColor()); | ||
| } |
There was a problem hiding this comment.
(read this comment last. I suggest you figure out the other two comments first.)
So, each ColorTableEntry has an index property. This index is the index in the color table. So if you look at TableColorNames, this should help you out. A color table entry with the index of 0 is our representation of "black", 1st is "red", etc...
So here we have to do that 8 trick backwards. The 0th color in _CurrentNonBrightColorTable is black, which is the 0th color of the color scheme's color table. 1st non-bright is red --> 1st in color scheme color table. Whereas the 0th in the bright color table is actually the 8th in the color scheme's color table (bright black needs to map to bright black). etc.
There was a problem hiding this comment.
Okay, I've got the others down, but I'm not quite sure I understand this one. Would you please be more specific? Thanks in advance
There was a problem hiding this comment.
@carlos-zamora Here's my current code, however, it doesn't seem to work:
void ColorSchemes::ColorPickerChanged(IInspectable const& sender,
ColorChangedEventArgs const& args)
{
if (auto picker = sender.try_as<ColorPicker>())
{
if (auto tag = picker.Tag())
{
auto index = winrt::unbox_value<uint8_t>(tag);
if (index < 8)
{
CurrentColorScheme().SetColorTableEntry(index, args.NewColor());
}
else if (index > 8)
{
CurrentColorScheme().SetColorTableEntry(index + 8, args.NewColor());
}
_CurrentNonBrightColorTable.GetAt(index).Color(args.NewColor());
_CurrentBrightColorTable.GetAt(index).Color(args.NewColor());
}
}
}There was a problem hiding this comment.
So, we shouldn't really be changing our interaction with CurrentColorScheme() in this PR. CurrentColorScheme is the color scheme object from the settings model. This function gets called when the user picks a color from the flyout (i.e. changing "black" to #FFFFFF). So, here, our interaction with the CurrentColorScheme is that we're updating the settings model.
_CurrentColorTable, on the other hand, is solely a part of the Settings UI. Here, we're updating which color to show in the UI. Before, it was all one color table. But now, we've split the table into 2 (brights and non-brights). So the if-statements should determine which of those color tables to modify. Whereas the settings model can stay the same as it was before because the index is the same.
There was a problem hiding this comment.
@carlos-zamora Thank you, do you mean something like this (The two different colour pickers will point to the appropriate method)?
void ColorSchemes::NonBrightColorPickerChanged(IInspectable const& sender,
ColorChangedEventArgs const& args)
{
if (auto picker = sender.try_as<ColorPicker>())
{
if (auto tag = picker.Tag())
{
auto index = winrt::unbox_value<uint8_t>(tag);
CurrentColorScheme().SetColorTableEntry(index, args.NewColor());
_CurrentNonBrightColorTable.GetAt(index).Color(args.NewColor());
}
}
}
void ColorSchemes::BrightColorPickerChanged(IInspectable const& sender,
ColorChangedEventArgs const& args)
{
if (auto picker = sender.try_as<ColorPicker>())
{
if (auto tag = picker.Tag())
{
auto index = winrt::unbox_value<uint8_t>(tag);
CurrentColorScheme().SetColorTableEntry(index, args.NewColor());
_CurrentBrightColorTable.GetAt(index).Color(args.NewColor());
}
}
}There was a problem hiding this comment.
Sure, something like that is close.
There's a small bug in the BrightColorPickerChanged function. Remember, all of the bright colors have an index >= 8. So bright black is 8, bright red is 9, etc. Since the tag for bright colors will always be >= 8, you won't be interacting with any entries < 8 in _CurrentBrightColorTable. Instead, offset it so that bright black is on index 0 for _CurrentBrightColorTable, bright red is on 1, etc...
Some meta advice: please try implementing your solutions and debugging through them yourself. I'm happy to help when you get stuck, but I've explained how the color table works a few times in this thread alone now.
There was a problem hiding this comment.
@carlos-zamora Sorry if I was a bit annoying with all the questions. I do understand how the colour table works, but I'm stuck on how to offset it. Thank you very much for all your help.
ghost
added
the
Needs-Author-Feedback
|
Thanks, I'll take a look. |
ghost
removed
the
Needs-Author-Feedback
|
Hey @Chips1234. Have you made any more progress on this issue? Now that #8919 merged, I'd like to go into the color schemes page and polish the accessibility and keyboard navigation. But I don't want that work to go to waste if we're doing a redesign. Any chance you could get this done by next Monday (2/15)? |
|
@carlos-zamora Sorry, I've been a bit busy the last few days. I'll see if I can get this done by the 15th of February. |
|
@carlos-zamora Hello, I have split the tables into two. Could you please review the code and see what is wrong? I can't seem to figure out how to fix the bright colour table (see screenshot below). And, everytime I test it, there's an error: " |
| for (uint8_t i = 0; i < TableColorNames.size(); ++i) | ||
| { | ||
| _CurrentColorTable.GetAt(i).Color(colorScheme.Table()[i]); | ||
| if (i < ColorTableDivider) | ||
| { | ||
| _CurrentNonBrightColorTable.GetAt(i).Color(colorScheme.Table()[i]); | ||
| } | ||
| else | ||
| { | ||
| _CurrentBrightColorTable.GetAt(i).Color(colorScheme.Table()[i]); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think you made a mistake here.
Lets say you have 8 ColorNames, your devider is set to 4 and you have split the 8 colors in two arrays where each array has 4 colors. Then you get an IndexOutOfBounds-Exception when you try to get index 4 to 7 from the second array, because each array has only an index of 0 - 3. Keep in mind you have splitted 8 colours in two arrays.
I think you have to count each array on it's own. The first from 0 to Devider - 1 and the second from 0 to ColorNames.Count - Devider - 1.
ATTENTION: Don't use Devider - 1 as max count for both arrays. If the second array has more colors because the ColorNames.Count - Diveder don't result in the same number of colors for both arrays, they get a different length.
There was a problem hiding this comment.
Maybe it's a better way to split the ColorNames array in two arrays too. Then you avoid mistakes on adding other colors because of moving/changing the order (especially on adding non-brigth colors).
There was a problem hiding this comment.
@htcfreek Thank you, I'll look into it.
There was a problem hiding this comment.
@htcfreek Sorry, I'm not quite sure I understand you. Would you please elaborate on what I should do to fix this? Thank you.
There was a problem hiding this comment.
@htcfreek Sorry, I'm not quite sure I understand you. Would you please elaborate on what I should do to fix this? Thank you.
Sure. I'll answer you then tomorrow.
There was a problem hiding this comment.
@htcfreek Your first comment. Thank you.
There was a problem hiding this comment.
Have looked into it and tomorrow/later this day I will post sample code with explaination.
There was a problem hiding this comment.
Have looked into it and tomorrow/later this day I will post sample code with explaination.
Thanks
There was a problem hiding this comment.
@Chips1234
Sorry for let you waiting. But I hope I can help you, and you understand what I mean with the explanations at the end of this comment.
My suggested code idea
⚠ Attention: I haven't run the code, and you have to check if the syntax of the mathematical operation is correct. (I am not so familiar with this.)
If you use this code please don't remove the comments, so that other people can read the information later too.
for (uint8_t i = 0; i < ColorTableDivider; ++i)
{
// Here, the code is taking the color from the color scheme object's table, and throwing it for non-bright colors into the "_CurrentNonBrightColorTable" list and for bright colors into the "_CurrentBrightColorTable" list.
// We have 16 colors, whereas 8 of them are non-bright and 8 of them are bright.
// At the beginning of this file we set a constant variable ("ColorTableDivider") to the number of elements (colors) where we split the color table list.
// Because of these facts we can use the value of "ColorTableDivider" as stop condition in our for loop.
// And we can use the loop's index as index for the element in the "_CurrentNonBrightColorTable" list and the "_CurrentBrightColorTable" list we are working on at each iteration.
// To get the correct elements from the color scheme object's table we can set/calculate the index of the non-bright color and the bright color mathematical (based on index and "ColorTableDivider") on each iteration.
// Fill the element of "_CurrentNonBrightColorTable" with the non-brigth color (lower index on the color scheme object's table: 0-7).
int indexNonBrightColor = i;
_CurrentNonBrightColorTable.GetAt(i).Color(colorScheme.Table()[indexNonBrightColor]);
// Fill the element of "_CurrentBrightColorTable" with the brigth color (higher index on the color scheme object's table: 8-15).
int indexBrightColor = i + ColorTableDivider;
_CurrentBrightColorTable.GetAt(i).Color(colorScheme.Table()[indexBrightColor]);
}Explanation
Your code
In your code you made the mistake to iterate based on the 16 colors in "TableColorNames". This leads to an index from 0 to 15. But at the beginning of this file you separate the 16 colors into two lists with 8 colors on each list. So your condition is incorrect.
for (uint8_t i = 0; i < TableColorNames.size(); ++i)Within the loop (iteration from 0 to 15) you first tried to fill the "_CurrentNonBrightColorTable" with the first 8 colors (iteration/index 0-7) and that works because your "_CurrentNonBrightColorTable" has 8 elements (0-7).
But then you tried to fill the "_CurrentBrightColorTable" with colors based on the remaining iterations (index 8 - 15). And this CAN'T work because your "_CurrentBrightColorTable" list has only 8 elements (0-7). Remember you have split the 16 colors into two lists of 8 elements per list.
if (i < ColorTableDivider)
{
_CurrentNonBrightColorTable.GetAt(i).Color(colorScheme.Table()[i]);
}
else
{
_CurrentBrightColorTable.GetAt(i).Color(colorScheme.Table()[i]);
}My code idea
See the complete code at the beginning of this comment!
We iterate the loop based on the number of colors of each type (8 non-bright and 8 bright colors) and use the variable "ColorTableDivider" as condition. This leads to 8 iterations with an index form 0 - 7. Then we don't get a higher index as we have elements in each of the "current color" lists.
for (uint8_t i = 0; i < ColorTableDivider; ++i)Within the loop we can use the loop's index to get the elements of the "current color" lists.
_CurrentNonBrightColorTable.GetAt(i).Color(
_CurrentBrightColorTable.GetAt(i).Color(Additionally, we know that we have 8 non-bright colors and 8 bright colors in the color scheme object's table. And we know that the bright color's element index is +8 elements higher as the related non-bright color's element index. So we can calculate the index for the bright color element based on the loop's index and the value of "ColorTableDivider". For the non-bright color element we can simply use the loop's index.
Example:
- Index=0; non-bright color element=0; bright color element (index + ColorTableDivider)=8
- Index=7; non-bright color element=7; bright color element (index + ColorTableDivider)=15
int indexNonBrightColor = i;
.Color(colorScheme.Table()[indexNonBrightColor]);
int indexBrightColor = i + ColorTableDivider;
.Color(colorScheme.Table()[indexBrightColor]);There was a problem hiding this comment.
@htcfreek, great, thank you for your thorough explanation! I’ll take a look soon.
|
@Chips1234 , @mdtauk , @carlos-zamora |
I originally thought the Tooltips would be enough, but I have no objection to the word "Bright" being added to the right column as a header, if others agree. |
|
@mdtauk This could be a special problem for people with eye problems like people with a red-green weakness. |
Excellent idea. I think “normal” should be fine. |
@htcfreek Maybe the tool tips should display “Display red as...” instead? @carlos-zamora what are your thoughts? |
| @@ -284,6 +319,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |||
|
|
|||
| // Function Description: | |||
| // - Updates the currently modifiable color table based on the given current color scheme. | |||
| // There are 7 non-bright colours and 7 bright colours. | |||
There was a problem hiding this comment.
This is false. we have 8+8 colors. But the list index goes from 0-7. If you count from 0-7 you count 8 times.
| for (uint8_t i = 0; i < TableColorNames.size(); ++i) | ||
| { | ||
| _CurrentColorTable.GetAt(i).Color(colorScheme.Table()[i]); | ||
| // Fill the element of "_CurrentNonBrightColorTable" with the non-bright colors (lower index on the color scheme object's table: 0-7). | ||
| int indexNonBrightColor = i; | ||
| // Fill the element of "_CurrentBrightColorTable" with the bright colors (higher index on the color scheme object's table: 8-15). | ||
| int indexBrightColor = i + ColorTableDivider; | ||
|
|
||
| _CurrentNonBrightColorTable.GetAt(i).Color(colorScheme.Table()[indexNonBrightColor]); | ||
| _CurrentBrightColorTable.GetAt(i).Color(colorScheme.Table()[indexBrightColor]); | ||
| } |
There was a problem hiding this comment.
Have you really tried this code?
I think this can't fix the error because you haven't used the whole code. (See my review before.)
-
You didn't changed the condition of the loop which leads imo to to many iterations. (We only need 8 iterations from 0 to 7 and not 16 iterations from 0 to 15.)
-
Please copy all my comments in the code suggestion. Now there are information that are missing.
ghost
added
the
Needs-Author-Feedback
One point. You can't replace |
|
@carlos-zamora |
This PR performs a large overall polish of the color schemes page: - Ensures keyboard navigation is holistically improved (i.e. fully accessible, no lost focus, etc...) - Adds tooltips and automation properties to all controls - Redesigns the page according to @mdtauk's approved design ([link](#8997 (comment))). Note, there are some minor modifications to the design that were approved by @cinnamon-msft. - Automatically reflow's the color buttons when they do not fit in horizontal mode ## Detailed Description of the Pull Request / Additional comments - Redesign - a data template was introduced to make color representation consistent and straightforward - `ContentControl` is used to hold a reference to the `ColorTableEntry` and represent it properly using the aforementioned data template. - The design is mainly a StackPanel holding two grids: color table & functional colors. - The color table is populated via code. After much thought, this seems to be the easiest way to correctly bind 16 controls that are very similar. - The functional colors are populated via XAML manually. - We need a grid to separate the text and the buttons. This allows for scenarios like "selection background is the longest string" to force the buttons and text to be aligned. - Reflow - A `VisualStateManager` uses an `AdaptiveTrigger` to change the orientation of the color tables' stack panel. The adaptive trigger was carefully calculated to align with the navigation view's breakpoint. - Keyboard Navigation - (a lesson from `SettingContainer`) `ContentControl` can be focused as opposed to the content inside of it. We don't want that, so we set `IsTabStop` to false on it. That basically fixes all of our keyboard navigation issues in this new design. - Automation Properties and ToolTips - As in my previous PRs, I can't seem to figure out how to bind to a control's automation property to its own tooltip. So I had to do this in the code and add a few `x:Name` around. ## Validation Steps Performed - Manually tested... - tab navigation - accessibility insights - NVDA - changing color schemes updates color table - specific scenario: - change a color table color and a functional color - navigate to a different color scheme - navigate back to the first color scheme - if the colors persist, the changes were propagated to the settings model References #8997 - Based on the work from @Chips1234 References #6800 - Settings UI Epic Closes #8765 - Polish Color Schemes page Closes #8899 - Automation Properties Closes #8768 - Keyboard Navigation
|
Closing in favor of #9196 |
Summary of the Pull Request
Gets rid of labels in favour of tooltips for the colour buttons. Tested manually.
References
Fixes an issue mentioned in #8765:
PR Checklist
Detailed Description of the Pull Request / Additional comments
Tighten margins and make rows narrower.