microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.98k stars 29.53k forks source link

Class names for code scopes are changed? #18357

Closed smlombardi closed 7 years ago

smlombardi commented 7 years ago
Extension Author Version
material-icon-theme PKief 1.2.0
code-settings-sync Shan 2.4.2
sort-lines Tyriar 1.1.0
html-css-class-completion Zignd 1.0.3
Bookmarks alefragnani 0.10.1
project-manager alefragnani 0.12.2
vscode-angular-files alexiv 1.3.7
one-monokai azemoh 0.2.4
vscode-icontheme-nomo-dark be5invis 1.2.5
vscode-eslint dbaeumer 1.2.2
githistory donjayamanne 0.1.4
tslint eg2 0.8.1
vscode-great-icons emmanuelbeziat 1.1.37
Angular2 johnpapa 1.0.2
theme-karyfoundation-themes karyfoundation 10.2.0
ftp-sync lukasz-wronski 0.3.2
HTMLHint mkaufman 0.3.3
vscode-csscomb mrmlnc 4.0.0
vscode-postcss-sorting mrmlnc 2.1.0
vscode-stylefmt mrmlnc 2.2.0
Theme-1337 ms-vscode 0.1.2
angular2-inline natewallace 0.0.17
nonbreakingspace smlombardi 0.0.1
vscode-icons robertohuertasm 6.0.0
stylelint shinnn 0.21.2
darcula-extended smlombardi 1.2.0
theme-tesla smlombardi 5.3.10
bootstrap-3-snippets wcwhitehead 0.0.9
change-case wmaurer 1.0.0

Steps to Reproduce:

  1. open developer tools
  2. inspect some code

screen shot 2017-01-10 at 8 21 59 am

where the code spans had classes like "punctuation definition entity html", now they are things like "mtk8".

The theme I created still has the colors I applied the old way, but making edits to my tmTheme file as before and refreshing has no effect. Did you change the theme format? If so, is there documentation for us theme authors?

smlombardi commented 7 years ago

In fact, many of my theme's colors are now incorrect -- as well as other 3rd-party themes. Colors have switched and many scopes are eliminated (things that were colored differently are now not).

mjbvz commented 7 years ago

I believe the change to the token classes is expected from @alexandrudima's work on #17933

@smlombardi Do you have any examples where the highlighting has regressed so that we can investigate?

Thanks

joshpeng commented 7 years ago

@mjbvz I've done a lot of work on my One Dark theme that was impacted by that merge. Please see the last 9 commits on this branch for examples of what used to work, but now doesn't. (Note: not all of my changes were for a direct 1:1 fix, but you should be able to find plenty of examples that were in there)

Most of the issues were with scopes starting with a language because the language is now at the end.

Ex: VSC1.8.1

html.entity.other.attribute-name

VSC1.9 Insiders

entity.other.attribute-name.html

Any author that used devtool's inspect to find scopes for tokens and created themes based off of exact scope sequencing would have their themes broken now. There are mainly two types of issues:

  1. Pre-flattening, scopes were sequenced in an odd, non-standard manner. If a tmTheme relied on VSC's exact sequence displayed as classes, those are now non-functional since, post-flattening, the proper sequence has surfaced. Author would need to change tmTheme to reflect the new, correct sequence.
  2. Pre-flattening, erroneous mixing&matching of scopes were possible. If a tmTheme relied on a combined scope, it is now broken since, post-flattening, the grammar rule hierarchy is kept clean. Ex: Grammar rule 1 - meta.paragraph.markdown Grammar rule 2 - meta.link.inline.markdown If the tmTheme used paragraph.link.inline.markdown it would no longer be able to match anything post-flattening. Author would need to change it to this in order to capture the rule hierarchy.
    meta.paragraph.markdown meta.link.inline.markdown

    All in all, I think this is going to be a growing pain that will break a number of themes.

Oh, one thing that is 100% "broken" now though is detected-link. tmThemes used to be able to piggyback onto this VSC injected class to stylize all URL links, but post-flattening, the detected-link "scope" is only applied after the flattening. tmTheme has no chance of capturing it and thus can no longer style it.

smlombardi commented 7 years ago

How do you inspect/find out what the new scopes are? The Developer Tools no longer show the scope, e.g. entity.other.attribute-name.html. So how did you discover what to change it to?

joshpeng commented 7 years ago

@smlombardi I used this https://github.com/Microsoft/vscode/pull/17933#issuecomment-271515251. Whatever scope is higher up on the bullet list will win any hierarchy conflicts. The current winning grammar is shown in the section above the bullet list.

smlombardi commented 7 years ago

I did not even know there was a scope inspector in VSCode ☺️

I'm used to using the ones in Sublime and TextMate though. Thanks.

alexdima commented 7 years ago

I am sorry for the breakage and sincerely thank you for looking into aligning the themes you are maintaining.

Please let us know if there is anything we can do on our side to help.


The changes in #17933 are finally aligning and correcting the way TM themes rules are applied to produced scopes. Most notably, it aligns VSCode's theme matching with that of TextMate and Sublime (see #3008 for a collection of previous broken themes in VSCode).

Before #17933, VSCode would split all scope names when a dot is encountered and would "collapse all scopes" in an array of tokens. It would then leave it up to CSS to match a theme's rules with a token. However, the rules of CSS matching (i.e. selector ranking) are not the same as the rules of TM themes scope selectors.

The changes in #17933 bring in total a number of advantages:

The TM themes selector rules and precedence rules are quite well explained here: https://manual.macromates.com/en/themes https://manual.macromates.com/en/scope_selectors.html

image


We have added a new widget to help inspect the scopes of a token and the matching theme rule. It is under F1 > Developer Tools: Inspect TM Scopes

This will show you the token you are on and three sections:

image

smlombardi commented 7 years ago

Thanks. Why can't Developer Tools: Inspect TM Scopes be assigned a keystroke? I was going to assign shift-ctrl-p but it's not listed as assignable.

alexdima commented 7 years ago

@smlombardi You can add the following to the keybindings.json:

{
    "key": "ctrl+shift+p",
    "command": "editor.action.inspectTMScopes",
    "when": "editorTextFocus"
}

It does not get a keyboard shortcut out of the box as IMHO it is a special action used only by advanced users / extension authors debugging either themes or grammars (same as other Developer: actions)

joshpeng commented 7 years ago

@alexandrudima I've gotten everything straightened out on my end except for the detected-link item I mentioned. Do you have any thoughts on that? Thanks.

alexdima commented 7 years ago

Colours for built-in decorations should be customizable via a theme section which does not have a scope.

e.g. https://github.com/Microsoft/vscode/blob/master/extensions/theme-monokai/themes/Monokai.tmTheme#L26

I see we do expose activeLinkForeground in https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/services/themes/electron-browser/stylesContributions.ts#L36

@sandy081 Is there any reason why we don't also expose detected-link ?

alexdima commented 7 years ago

We once put together a page containing these decorations at https://github.com/Microsoft/vscode/wiki/Editor-decorations-&-colors

@joshpeng Is there anything besides detected-link that you would like to target?

joshpeng commented 7 years ago

@alexandrudima Thanks for the link. Very helpful. detected-link is it for me.

joshwiens commented 7 years ago

@alexandrudima - In your opinion Is the theming approaching the point where we could start contributing to a wiki doc for theme authors without it being nightmarish to maintain?

As I am sure you and others know, the process is currently a bit, shall we say taxing in regards to creating & maintaining high quality editor themes.

sandy081 commented 7 years ago

@alexandrudima Some how I missed this notification. With respect to detected-link I think this is foreground color and AFIK we cannot contribute foreground colors other than for tokens. Hence this was not handled then.

alexdima commented 7 years ago

@d3viant0ne I agree there are bits of information spread everywhere and aligning our implementation with TM themes matching semantics (same as TM and Sublime) is a breaking change. I can only be thankful to you for adopting the TM themes matching semantics in the custom themes you've built and hope that you'll appreciate the extra few ms in each frame paint time and the extra available RAM :).

Point taken. We will definitely update our documentation on themes in time for 1.9.x, our current strategy is to have the docs always point to the current stable version. i.e. https://code.visualstudio.com/docs/customization/themes

joshwiens commented 7 years ago

@alexandrudima - I'm sure you could get more than a few of us, myself included to contribute to more robust theme documentation if you are interested in help & the theming is stable enough to warrant everyones time authoring & reviewing.

alexdima commented 7 years ago

That would be awesome! :heart:

Our documentation is also on github --- https://github.com/Microsoft/vscode-docs/blob/vnext/docs/customization/themes.md

I didn't get around to document the new inspect widget or the new linkForeground property (pushed 5 minutes ago) in there yet so PR definitely welcome.

Also, any other doc contributions would be appreciated. @gregvanl is the final reviewer there 👍

smlombardi commented 7 years ago

Question: how can we set the color of the gutter numbers in the editor? Or is that not possible?

Another question: I notice VSCode does not support background colors like Textmate; is this planned? For example:

            <dict>
                <key>name</key>
                <string>Integers</string>
                <key>scope</key>
                <string>constant.numeric</string>
                <key>settings</key>
                <dict>
                    <key>background</key>
                    <string>#F3F2FF</string>
                    <key>foreground</key>
                    <string>#8257BE</string>
                </dict>
            </dict>
alexdima commented 7 years ago

That is funny. It seems indeed we haven't exposed the line numbers color to themes. Created #19490 to track that.

We currently don't render the background colors on the text. It has to do with how the editor is structured into layers and the text is rendered on top of the decorations, so we cannot just start rendering them today because the background color of a token would obstruct the selection, the find matches, etc. Definitely something to investigate.

akamud commented 7 years ago

This completely broke my theme. I think such a major and breaking change should be warned directly to extension authors in advance so we can prepare.

Until I pretty much re-do the whole theme (which won't be fast) anyone downloading this will think it has horrible support.

alexdima commented 7 years ago

@akamud I am sorry for this breakage and for not reaching out to you.

At this time, there is no easy way for me to get all extension authors emails and just drop them an email (which in a way is a good thing, privacy-wise). The only early warning system we have for such changes is the Insiders Release Channel, that gets daily releases and stabilizes 2 weeks before a release (i.e. very rare to get new features or breaking changes in the final 2 weeks before a stable release).

@seanmcbreen @chrisdias @waderyan How can we improve in this area --- communicating possible breaking changes to extension authors? (preferably in this case the subset of extension authors that contribute themes)

akamud commented 7 years ago

I understand the privacy concerns and the Insiders Release Channel, which I tend to follow. But this is a big breaking change and very unexpected, since it looks like something stable that wouldn't change that much, I was away for 2 weeks and was taken completely by surprise.

@alexandrudima thanks for the feedback on this, I'm complaining so we can work on ways to avoid this in the future.

I think it is great that you are constantly rethinking the way you do things. I personally wouldn't mind being contacted regarding things like this. If I can contribute to improve this kind of communication just let me know. I would be very glad to help.

gerane commented 7 years ago

Edit: I've edited this to tone down my reply. I just had a newborn and lack of sleep and everything else that goes with it made me more agitated than I am normally would have been. I also think many of the issues I've been encountering might actually be bugs. Many of my themes are actually going from looking just like a sublime theme, to now looking nothing like the sublime themes. It seems scopes are being lost. I need more time to adequately debug this and determine what is actually breaking.

@alexandrudima I maintain nearly 300 themes. Breaking changes without warning is unacceptable. I think code needs a deprecation notification system. I haven't actively used Atom in well over a year, but I still get issues and PRs for those extensions for patches to fix upcoming deprecations before they actually go live. The users get notifications letting them know about upcoming breaking changes well ahead of time and I believe they have links to open issues in them (may be mistaken, it's been a while). You could either have a new tab open with the information, trigger problems in the problem matcher, or have it show up as an output window that shows a roadmap of a deprecation and which installed extensions are impacted. Giving a link to the associated repository would be a huge plus.

CC @tyriar