Skip to content
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

Class names for code scopes are changed? #18357

Closed
smlombardi opened this issue Jan 10, 2017 · 24 comments
Closed

Class names for code scopes are changed? #18357

smlombardi opened this issue Jan 10, 2017 · 24 comments
Assignees
Labels
languages-basic Basic language support issues *question Issue represents a question, should be posted to StackOverflow (VS Code)
Milestone

Comments

@smlombardi
Copy link

  • VSCode Version: Code - Insiders 1.9.0-insider (31997b3, 2017-01-10T09:18:34.083Z)
  • OS Version: Darwin x64 16.3.0
  • Extensions:
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
Copy link
Author

smlombardi commented Jan 10, 2017

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 mjbvz added the languages-basic Basic language support issues label Jan 10, 2017
@mjbvz
Copy link
Collaborator

mjbvz commented Jan 10, 2017

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
Copy link
Contributor

joshpeng commented Jan 10, 2017

@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
Copy link
Author

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
Copy link
Contributor

@smlombardi I used this #17933 (comment). 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
Copy link
Author

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
Copy link
Member

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:

  • correctly matching themes with scopes according to TM scope matching rules (and not CSS)
  • a 25-30% reduction in token memory usage because we now only store 64 bits per token:
    • one 32 bit uint for the token start offset and
    • one 32 bit uint for the token metadata
      • 8 bits for language id
      • 3 bits for standard token type (string, comment, regex or other)
      • 3 bits for the font style (underline, italic or bold)
      • 9 bits for the foreground color id, and
      • 9 bits for the background color id.
    • we no longer hold on to the scopes/tokens, in fact they technically don't get "produced" as before, applying of the theme rules happens while tokenizing via a nice trie built from the theme rules.
  • due to immediately applying theme rules (while tokenizing a line), tokenization is actually faster overall, whereas before we would "loop" three times over a line, once to create the tokens, then we would loop over the produced tokens to "flatten" them, and third we would loop over them to binary encode and dedup the token types.
  • there is less GC`ing, the three tokenization engines the editor supports all return the tokens encoded as an Uint32Array, which gets immediately set in the model (no post processing needed)
  • the rendering times of a frame are faster because:
    • the generated size of the HTML for a line has reduced significantly
    • the number of class names needed to be matched by CSS is reduced significantly
  • and finally, having the resolved color information in JavaScript means we can begin looking into building a minimap.

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:

  • metadata used for rendering and operations such as comment line, etc.
  • the theme rule that matches and gives the foreground color to the token
  • the list of scopes the grammar produces for the token

image

@alexdima alexdima added this to the January 2017 milestone Jan 10, 2017
@alexdima alexdima added the *question Issue represents a question, should be posted to StackOverflow (VS Code) label Jan 10, 2017
@smlombardi
Copy link
Author

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
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

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

@joshwiens
Copy link

@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
Copy link
Member

@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
Copy link
Member

@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
Copy link

@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
Copy link
Member

That would be awesome! ❤️

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
Copy link
Author

smlombardi commented Jan 26, 2017

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
Copy link
Member

alexdima commented Jan 27, 2017

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
Copy link
Contributor

akamud commented Feb 3, 2017

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
Copy link
Member

alexdima commented Feb 3, 2017

@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
Copy link
Contributor

akamud commented Feb 3, 2017

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
Copy link

gerane commented Mar 2, 2017

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

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
languages-basic Basic language support issues *question Issue represents a question, should be posted to StackOverflow (VS Code)
Projects
None yet
Development

No branches or pull requests

8 participants