Replace Enhanced Colors with Color Schemes#39792
Conversation
|
Are you going to provide enhanced default colors to distinguish more, especially for light theme? |
dpoeschl
left a comment
There was a problem hiding this comment.
Round 1 of review complete.
| BindToOption(Editor_color_scheme, ColorSchemeOptions.ColorScheme) | ||
| End Sub | ||
|
|
||
| Protected Overrides Sub OnRender(drawingContext As DrawingContext) |
There was a problem hiding this comment.
See same new questions as C#. 😄
b03a619 to
fed209e
Compare
dpoeschl
left a comment
There was a problem hiding this comment.
Looking better. Mostly interested in how the user will understand what's happening (Theme vs. Scheme, what about pre-customized colors, etc.).
@huoyaoyuan Sorry I didn't see this earlier. This PR is just to change the underlying mechanism by which we switch colors between our two color schemes. Feel free to open an issue to enhance the light theme further with additional syntax colors. |
| <Foreground Type="CT_RAW" Source="FFDCDCAA" /> | ||
| </Color> | ||
| <!-- | ||
| <Color Name="property name"> |
There was a problem hiding this comment.
why are there multiple of this same block commented out? (also this one is at a different indentation)
Same applies to the other color scheme files
There was a problem hiding this comment.
These classifications inherit their colors (based on the classification hierarchy). I wanted to leave all the classification names in the file in case colors are one day specified, but leaving it without a foreground entry causes the color compiler to error.
jasonmalinowski
left a comment
There was a problem hiding this comment.
Leaving comments so far. So far biggest concern is mostly maintainability with having both the .xml files and essentially a representation of it also in code. It'd be nice if we could at least get rid of one.
| { | ||
| public const string SettingKey = "TextEditor.Roslyn.ColorScheme"; | ||
|
|
||
| public const string Enhanced = nameof(Enhanced); |
There was a problem hiding this comment.
If we ever make a "more enhanced" what's the plan here? We'd introduce a new key (VisualStudio2019, say), and users who are on "Enhanced" continue to get the enhancement?
There was a problem hiding this comment.
I feel like I set us up for failure with this one. I assume because of our work the C++ team has adopted the scheme names "Visual Studio 2017" and "Enhanced". Which I now feel compelled to match. I think the way out would be for unified cross-language "scheme" support in the platform. At which time someone can reset all the names to something more sensible.
There was a problem hiding this comment.
I guess my question was more how we would keep syncing stuff: if we create a new scheme, we'll at least internally call this one "enhanced" and then name the newer one "enhanced++" or something?
There was a problem hiding this comment.
If it is incremental improvements to the enhanced theme then I don't think there is a need for a new scheme. If there is a new scenario that we wanted to use highlighting to imrpove (C++ team have an "Enhanced Globals vs Locals" theme i believe) then we could name it something appropriate.
|
@jasonmalinowski This PR is ready for another review when you have time. Thanks! |
jasonmalinowski
left a comment
There was a problem hiding this comment.
At this point, I suspect I've found all the issues I ever will find with this. 😄 A few general comments:
- Something is funky with the nullable handling in ColorSchemeApplier.ColorSchemeReader.cs. If we were on an annotated framework you'd have the compiler flagging a few things here.
- It'd be nice if we just used __VSCOLORTYPE rather than explicitly passing around ints everywhere, since there's a few places the use of the enum feels pretty magic.
- Rather than importing VisualStudioWorkspace and then changing options through that, can we just import IGlobalOptionService and change things through that? That avoids loading the rest of the Workspace stuff which isn't exactly cheap -- that loads the solution crawler and such. It's nice if we can avoid the coupling if possible, which may also help as we move more stuff OOP. If @mavasani gave opposite guidance as far as if IGlobalOptionService is still acceptable to use then ignore this for now and we'll figure out how to deal with that.
- When we change the theme, do we have to do a global broadcast message of WM_THEMECHANGE, or can we just post it to the VS window? Otherwise we're doing a system-wide refresh potentially which seems a bit overkill...
| { | ||
| public const string SettingKey = "TextEditor.Roslyn.ColorScheme"; | ||
|
|
||
| public const string Enhanced = nameof(Enhanced); |
There was a problem hiding this comment.
I guess my question was more how we would keep syncing stuff: if we create a new scheme, we'll at least internally call this one "enhanced" and then name the newer one "enhanced++" or something?
| <Color Name="type parameter name"> | ||
| <Foreground Type="CT_RAW" Source="FF2B91AF" /> | ||
| </Color> | ||
| <!-- |
There was a problem hiding this comment.
Should the comments be removed now? Keeping the comment there with "these are things we aren't setting" also seems reasonable too; this more smelled of "I commented this out temporarily and forgot about it".
This reverts commit 646489a.
This reverts commit 646489a.
Revert "Replace Enhanced Colors with Color Schemes (#39792)"
In VS 16.0 we introduced Enhanced Colors for C# and Basic. This worked by updating the users configured colors to match either the Enhanced scheme or the Visual Studio 2017 (Classic) theme. The user would choose whether to use the Enhanced colors by setting a checkbox in Tools > Options > Text Editor > C# | Basic > Advanced. We would then attempt to apply the correct colors on VS start and when the VS theme was changed.
One problem with this approach is that the default colors from the Fonts and Colors options page were always the VS2017 colors. This would cause confusion when users had opted-in to the Enhanced colors but tried to reset custom colors that they had applied.
Another problem is that we tried to Apply the colors on top of the user's configured colors. There are several open feedbacks reporting application of colors either failed or, since they were applied, are now synching over affecting the user's other VS installs in unexpected ways.
In 16.3 the C++ team implemented Color Schemes support. Their approach was different from ours in that they wrote the colors into the VS registry overwriting defaults for each of the 4 in-box themes (Light, Blue, Blue (Extra Contrast), and Dark), meaning 'Use Defaults' in Fonts and Colors worked. They also never changed the users configured colors which avoided any potential partial application or synching issues.
For 16.4 we want to adopt this same method of controlling the editor's color scheme for Roslyn. There is no longer a checkbox for 'Use enhanced colors for C# and Basic'. Instead we have a dropdown where a user will select either Enhanced or Visual Studio 2017. For new VS users this will default to Enhanced. For users of previous versions of 16, we will migrate over the user's choice from the checkbox experience with one exception.
Previously when the user had customized any of the classification colors that we were interested in, we would not apply colors. To retain this behavior, on first launch of this build during the migration we will set the Color Scheme to Visual Studio 2017 even if Use Enhanced colors was checked.
As a consequence of moving from the old approach (updating user colors) to this new approach (updating default colors) is that we want the user's colors to be defaulted so that the default colors can come through. To account for this, the first time each of the in-box themes are loaded we will walk the user's colors and default the classifications that match the scheme's colors. After this initial defaulting, we will make no attempts at updating user colors and any incomplete migration would be resolved by using the 'Use Defaults' button in Fonts and Colors.
For VS themes that have been customized in Fonts and Colors a warning will be displayed.
For custom VS themes a warning will be displayed and the dropdown will be hidden.
This change involves removing our classification colors from the VS repo and moving them into Roslyn. At the time this is inserted we will replace our colors in the VS EditorColors.xml with a comment about where they have moved.