Color variable.language differently from other variables

[Ben3eeE]

Issue or RFC Endorsed by Atom's Maintainers

N/A

Description of the Change

Both textmate and tree-sitter scopes language variables (this, super, arguments) as variable.language in JavaScript (After atom/language-javascript#620 is merged). These should be colored differently from other variables.

Looks like this:

Alternate Designs

• Different color. I just picked one at random that was already defined. Ideas for a color is welcome.
• Move this to base so it applies to every language and not just javascript where we have the only variable.language with tree-sitter after Add more scopes to the tree-sitter grammar language-javascript#620 is merged. It's possible we will add more variable.language to other languages shortly and they already exist in textmate.

Possible Drawbacks

People that disabled tree-sitter likes the red color:

Verification Process

I checked that this is colored differently with tree-sitter enabled and atom/language-javascript#620 merged

Release Notes

• Changed the color of language variables in JavaScript to be different from other variables

/cc: @maxbrunsfeld @simurai

[simurai]

I just picked one at random

That's what I usually do too. 😂 Sometimes I look where the same color is already used. In this case it would be the same as support.class and a few more. Maybe ok, since they don't clash?

Move this to base so it applies to every language and not just javascript

👍 Yeah, that might be a good idea. Especially since variable.language isn't in use already, it wouldn't break anything. In general, I think we should try to move to _base as much as possible and only tweak it on a per language basis if it doesn't fit. Then other language packages can start to use it too.

[maxbrunsfeld]

I just picked one at random

I'm a little worried that this particular color choice (the yellow-ish one) will be too big of a change for right now. After releasing 1.32, we've gotten a lot of feedback that people find color changes really disruptive.

Currently, normal variables are just the base text color: light gray in one-light-syntax, and black in one-dark-syntax. I think I'd be more comfortable with the change if we were to make these "special language variables" a subtle variation of that color. So on one-light, something close to black, and on one-dark, something close to light gray. Does that make sense?

It may be too disruptive to introduce a whole new color though.

[maxbrunsfeld]

Another option would be to make language.variable the same color as the base text, except bold-face.

[maxbrunsfeld]

@simurai, @Ben3eeE

Overall, when making color choices, I'd like to start thinking it something like this:

• Functions have a base color (in one-light, blue).
  • Special, built-in functions (e.g. require) have a related color.
• Types have a base color (in one-light, blueish-green).
  • Special, built-in types (e.g. int) have a related color.
• Property names have a base color (in one-light, red).
  • Special, built-in property names (e.g. __proto__) have a related color.
• Variables have a base color (in one-light, black).
  • Special, built-in variables (e.g. this) have a related color.
• Strings have a base color (in one-light, green)
  • Special kinds of strings (e.g. regexes) have a related color.
• Constructors have a base color (in one-light, yellow)
  • Special kinds of constructors have a related color.

And so on.

Coming from TextMate, we didn't always have clear enough information to follow such a framework, but now we do. Does that way of thinking about it make sense?

[Ben3eeE]

@maxbrunsfeld Thanks for the review 💯 I understand your vision a lot more now. This will help me out a lot when making tree-sitter scope changes 👍 Maybe we should document this somewhere 💭

@simurai Thanks for the review 🙇 I added a commit to address both points of review feedback:

• Color it differently
• Move it to base

In this commit I removed coloring red for any .variable and added a new shade of mono for these variables.
I also made it bold. It's entirely up to you if you want to keep this or not. Feel free to commit directly here or comment with what you would like me to change 🙂 I am just suggesting and I'm really bad at making things look good 😅

I chatted with @maxbrunsfeld on slack to suggest holding off on merging the language-javascript PR until this is reviewed and approved. Because we can't ship one fix without the other. Making it red.red again just feels wrong.

[Ben3eeE]

This change as it proposed now is gonna require some thorough test plan and verification process to go over all languages and how they scope variable.

Some prerequsite changes to better support this PR:
atom/language-javascript#620 & https://github.com/atom/language-typescript/tree/b3-sync-with-javascript
atom/language-ruby#250

The latest commit scopes variable.language and variable.support with the same color. The difference is that variable.language is bold.

[simurai]

Coming from TextMate, we didn't always have clear enough information to follow such a framework, but now we do. Does that way of thinking about it make sense?

Maybe we should document this somewhere

A guide would be great. When it comes to syntax highlighting, 2 parts are very clear:

1. Language packages define the scopes. E.g. variable.language
2. Themes define the colors. E.g. @mono-4: hsl(@syntax-hue, 14%, 60%);

But then there is this 3rd aspect: What scope should have what color? Or how do the scopes relate to each other? And here it's where things get very hand-wavy. Currently it's defined in the themes, but I think to really be able to decide, you need to understand, use and care about a language. Not sure if all theme authors do (me included 😇 ). Some options that might reduce the "randomness":

1. Like suggested above, have a guide/convention that themes hopefully follow.
2. Move the styling e.g. .syntax--support { color: @mono-4; } entirely into language packages. Then language authors can not only add the scopes, but also add the logic how the scopes relate to each other. Themes are left with defining only the colors.
3. Bring Atom core into the mix:
   • Themes define the color variables.
   • Atom core uses these variables to add a certain default. Think about moving _base.less into core. Then there is basic syntax highlighting for any language.
   • Language packages can customize, if the core styles aren't enough.

Option 2 + 3 are probably not possible without breaking a lot?

---

Anyways, back to this PR. 😄

Feel free to commit directly

I replaced mono-4 with mono-2. The difference seemed too small to introduce another shade of grey. We could add a 4th, but then maybe stretch it out more and make the others a bit lighter/darker. Here the example I used to test:

function a() {
  foo.aw    // default            @mono-1
  window.aw // variable.support   @mono-2
  this.aw   // variable.language  @mono-2 + bold
}

One dark	One light

Using bold, is something the One themes don't really do, but maybe ok to start using bold more.

I removed coloring red for any .variable and added a new shade of mono for these variables.

Hmm.. that feels a bit too drastic. In Less it would be:

Before	After

People might say: "Why are the variables not colored anymore?".

[Ben3eeE]

People might say: "Why are the variables not colored anymore?".

Yeah. You're right it's gonna break textmate grammars and not make much difference to tree-sitter. We should definitely revert that change. It would also break keyword arguments that we restored color to in Atom 1.32.1 by adding the variable class to them.

Option 2 + 3 are probably not possible without breaking a lot?

Yeah I think these would break a lot of things. Having a guide/convention both for scopes that grammars should apply and how they should be colored in relation to each other would be nice to have. Scopes seem to be kinda standard for every core grammar, maybe it's because that's how they get colored or maybe there is some convention already that I don't know about. But there are some differences. For example php has a whole lot of red with one-dark because it decided to scope local variables as variable.other.php which is red in one-dark. Other languages don't scope local variables and they have the @mono-1 color.

In the WIP pr to add a tree-sitter php grammar I have currently removed this local variable scope but I am considering adding it back as with a different class that is not colored by one-syntax. local-variable maybe. This combined with some small changes I made to my personal stylesheet makes php look like other languages with one-syntax. But given the feedback we got from other changes we made it's likely that's gonna get a lot of complaints 😅

Using bold, is something the One themes don't really do, but maybe ok to start using bold more.

I don't mind if you want to change it. I just added bold because that was what was suggested 🙂

[chbk]

@sadick254 This can be closed, it's no longer relevant now that #20524 is merged. It would be good to also merge atom/language-javascript#690 to fully support reserved variables like this, super, etc.

[sadick254]

@Ben3eeE Closing this has it has been superseded by #20524. Thank you for your contribution.