More themes #2906
Merged
More themes #2906
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
|
I added a theme based on the Light+ theme from VS Code, here's a preview:
|
|
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. |
ltrzesniewski
force-pushed
the
themes
branch
2 times, most recently
from
a9572d1 to
711de7a
Compare
Also add a fallback mechanism for colors: if a color definition is empty, another one can be used instead.
ltrzesniewski
force-pushed
the
themes
branch
from
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.
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.
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. 🙂 |
Merged
|
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. 🤦♂️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.IsDefinitionI'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.