-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More themes #2906
More themes #2906
Conversation
This starts to look good, so I'm marking it as ready for review. There are 6 themes in total: 3 light and 3 dark. I left the existing ones as-is, and added 4 more inspired by VS Code and ReSharper. I left the theme selector combo box in the toolbar (at least for now), as it's easier to compare themes that way. |
a9572d1
to
711de7a
Compare
Also add a fallback mechanism for colors: if a color definition is empty, another one can be used instead.
711de7a
to
7055182
Compare
Colors won't be inherited from the xshd files in order to avoid mistakes.
This looks pretty good, would like to see this be approved. |
# Conflicts: # ILSpy/TextView/ILAsm-Mode-Dark.xshd # ILSpy/TextView/ILAsm-Mode.xshd
|
||
public void ApplyTo(HighlightingColor color) | ||
{ | ||
color.Foreground = Foreground is { } foreground ? new SimpleHighlightingBrush(foreground) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a small nit: why use pattern matching here? The lhs of the pattern is already a correctly typed variable. Wouldn't Foreground is not null ? new SimpleHighlightingBrush(Foreground) : null
be easier to read?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No good reason, I think I just started to use pattern matching a bit more than necessary recently. 😅
LGTM! Thanks! Sorry for taking so long to get to this... |
No worries, that's not exactly a small change. Thanks for reviewing/accepting this. 🙂 |
Unfortunately it looks like this PR is the cause of some performance issues (#2956). Investigating "who's calling CreateWindow there" lead us to this stack:
We may end up reverting this PR due to these issues. |
This is very surprising given that this PR didn't change the way the light/dark theme switch worked - it works on the same principle. Also, I didn't notice any perf regression. 😕 @siegfriedpammer told me he found a solution, but I'll be happy to help if needed. |
We currently suspect that iterating recursively over the MergedDictionaries (see stack trace) ends up eagerly instantiating parts of AvalonDock that we otherwise don't use, and that were not instantiated prior to this PR. |
In particular, with @siegfriedpammer's proposed hack:
then ILSpy loads seemingly without ever creating an instance of |
Oh, I see. I thought those dictionaries only instantiated some brushes. Oops. 🤦♂️ |
Fixes #2904
Problem
I'd like to have more theme choices than just light/dark.
Solution
Any comments on the approach taken, its consistency with surrounding code, etc.
Basic approach:
Additional changes:
Which part of this PR is most in need of attention/improvement?
TextTokenWriter.IsDefinition
I'm not 100% sure about, as I'm still unfamiliar with the AST.At least one test covering the code changed
This is a visual change only.