Skip to content

Conversation

@JoeRobich
Copy link
Member

An experiment for adding new theme colors for some of the classifications added in #31231. The option to enable/disable the experiment will be in the Tools>Options Preview Features page.
image

If the user has customized any of the target colors then, this experiment will not apply colors. If the user has customized their theme but wishes to enable enhanced colors then, they can use the 'Use Defaults' button on the Tools>Options Fonts and Colors page to get their theme into a state where enhanced colors can be applied (a restart may be required).
image

The goal was to choose colors and use them in a similar fashion as the VS Code Light+ and Dark+ themes.
image

Light Theme Sample
image

Dark Theme Sample
image

@JoeRobich JoeRobich requested a review from a team as a code owner December 21, 2018 02:06
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chatted extensively with @JoeRobich on this in person, there's a threading problem here that we need to sort out. If the option changes, right now if the option changes this is kicking off background work which uses DTE. That's a violation of usual vs-threading rules, but since it's not being waited for anywhere it won't deadlock.

It, however, does risk crashes if that background work actually runs for a few seconds and VS shuts down while the process is happening. Even if it doesn't crash, it might result in a torn state which is also strange where some colors have changed and others haven't.

My proposal was to first measure how expensive those DTE operations actually are. If they're reasonably fast, it's probably easiest just to do a single thing on the UI thread in one chunk; and if we shutdown just cancel that work so it either all runs or all doesn't. Otherwise, if we want to continue with the "do lots of calls from the background thread" approach, the correct pattern to do is to have the MEF component JTF .Join() the background work so we ensure it's completed.

The "what about shutdown" is something we pretty regularly violate in Roslyn where we often launch stuff on the background fire-and-forget and don't correctly Join it if we're in the middle of shutdown. Now that we have JTF, I think it's good for us to follow prescribed patterns assuming that it's easy to do an the pattern is at least encapsulated in a single class. Obviously complicated stuff like "analyzers might be running, we don't wait for them" is something we've instead guaranteed that since we're a snapshot model that's OK to run until the CLR kills your thread, because you're just operating on pure managed state.

@CyrusNajmabadi
Copy link
Member

image

VS name for VB is 'Basic'. This can be seen here:

image

colorItemMap[ClassificationTypeNames.MethodName].Foreground == LightThemeMethodYellow &&
colorItemMap[ClassificationTypeNames.ExtensionMethodName].Foreground == LightThemeMethodYellow &&
colorItemMap[ClassificationTypeNames.OperatorOverloaded].Foreground == LightThemeMethodYellow &&
colorItemMap[ClassificationTypeNames.ControlKeyword].Foreground == LightThemeControlKeywordPurple;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so if the user changes a single color, it's no longer enhanced?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well they aren't using only our enhanced colors. The way I was viewing it is that a users owns their colors once they make a change. I am hesitant to overwrite their changes. After disabling the experiment in the preview options, the user could click 'Use Defaults' from Fonts and Colors to revert their changes and our changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. That makes more sense. Possibly worth a comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There won't even be an option to use enhanced colors in case they want to opt in?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Neme12 All of these classifications will show up in the Fonts and Colors dialog regardless of this experimental flag. For this experiment we have a set of colors we will try to set automatically, if the user has not made their own changes to these particular classifications. If the user wishes to opt-in to the experiment and have their own colors for these classifications, they will need to reset the individual classifications to 'Default color' or they can set their entire theme using the 'Use Defaults' button in Fonts and Colors.

@JoeRobich
Copy link
Member Author

VS name for VB is 'Basic'

@CyrusNajmabadi I'll get that corrected. Thanks!

@jinujoseph jinujoseph added this to the 16.0.P2 milestone Jan 3, 2019
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is something busted with stopping flights: that won't fetch _settingsManager, which means we'll then blow up trying to fetch the theme ID.

Fix that, and actually test it. 😄

// -1 -> never use enhanced colors.
var useFlightValue = useEnhancedColorsSetting == 0;
var applyEnhancedColors = (useFlightValue && _inUseEnhancedColorsFlight) || useEnhancedColorsSetting == 1;
var removeEnhancedColors = !applyEnhancedColors || _inStopEnhancedColorsFlight;
Copy link
Member

@jasonmalinowski jasonmalinowski Jan 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kinda a mind screw to me that applyEnhancedColors and removeEnhancedColors don't end up just being opposites of each other. And in the case of _inStopEnhancedColorsFlight, we did all of that fancy Boolean logic to ignore it. Can we just write this as:

if (_inStopEnhancedColorsFlight || useEnhancedColorsSetting == -1)
{
    _colorApplier.TrySetDefaultColors(currentThemeId);
}
else if (useEnhancedColorsSettings == 1 || (useEnhancedColorsSetting == 0 && _inUseEnhancedColorsFlight))
{
    _colorApplier.TrySetEnhancedColors(currentThemeId);
}

In my case it also means if you're not in the flight we just won't do anything at all, rather than doing a bunch of DTE querying that will ultimately result in nothing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We always need to try and set colors. There is the case where they are not part of the flight but opt-in then later set the checkbox back to the dithered state (0). We would need to remove enhanced colors.

@JoeRobich
Copy link
Member Author

@jinujoseph for approval

@JoeRobich
Copy link
Member Author

Integration tests timed out. Failures due to loss of focus issues.

@JoeRobich JoeRobich merged commit f63d50f into dotnet:master Jan 4, 2019
@CyrusNajmabadi
Copy link
Member

I'm so excited for this! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants