Binaryify / OneDark-Pro

Atom's iconic One Dark theme for Visual Studio Code
https://marketplace.visualstudio.com/items?itemName=zhuangtongfa.Material-theme
MIT License
1.52k stars 289 forks source link

PHP highlighting of $this keyword #294

Closed ylorant closed 5 years ago

ylorant commented 5 years ago

Hello,

Since a few releases (about 3 weeks ago ?) I have a change that happened on the highlighting for my PHP files. The "$this" variable (referring to the current instance in an object method) stopped being highlighted in the same way as other variables, and started being highlighted as a type/class name :

image

Is it due to a change in the theme, a change on VSC's highlighting engine, or something else ? And is there a way for me to put the highlighting back to how it was before ($this being highlighted as $raffles in this example) ?

Thank you.

JonGarbayo commented 5 years ago

Hi @ylorant!

Short answer here: https://github.com/Binaryify/OneDark-Pro/pull/287#issue-257669204 Long answer tomorrow ;)

ylorant commented 5 years ago

Hmmm, I see. However, I don't agree with that way for the this keyword, because here $this (or this in other languages) behaves like a regular variable (referencing an instance of an object), and not a type, hence it should be colored like a variable.

But I guess it's just a way of thinking and I'm not on the majority.

I'll just change it for myself in my config following what you did in your PR. Thanks for the info.

EDIT: For people looking to do that in the future, here's the config that I've added:

"editor.tokenColorCustomizations":{
    "[One Dark Pro]": {
        "textMateRules": [
            {
                "scope": "variable.language",
                "settings": {
                    "foreground": "#e06c75"
                }
            }
        ]
    }
}
JonGarbayo commented 5 years ago

Hi @ylorant!

Sorry for this late answer, I didn't have as time as I thought.

I understand your point of view, and I agree with it. But remember: colors are not unlimited in a theme. It's nearly impossible to have something good looking and a specific color for each piece of syntax at the same time. Look at extended class names in PHP or JavaScript: they're green with One Dark, just as strings.

For me and for a lot of people, it's important to identify where this or $this is used at a glance. As you said, it's a variable. But it's a special one, as it's value cannot be changed by the developer. So it needs some sort of highlight. this was yellow in JavaScript yet, so for consistency, it was logic to apply it in every language πŸ˜„

What you did with custom settings is great if you want to revert back to the previous behavior. But what do you think if this was at least highlighted in italic (in the theme, for everyone, not only in your custom settings I mean)? It's less visible than a color, but it could be nice for people in the same case than you ☺️ It was the italic in JavaScript, but I deleted that rule in my pull request 😐

ylorant commented 5 years ago

Hello,

I understand you point of view, and it has good arguments too.

I think having it in italic could be a great idea, it encompasses both our points of view I think, putting the variable in a different styling than regular variables, but still keeping it close enough to convey the idea that it behaves likely (which was my main complaint originally, having it shown like a static type was kind of disturbing for PHP, where static calls and dynamic calls are done differently, this could mislead some people into thinking their calls aren't made correctly).

The best thing to do here is to give the users the choice of either, but I don't know/think that configuration like that is possible. If it would be for choosing a definitive solution, I would go for the "same color, different styling" solution of course, but I don't want to be the person choosing for the huge amount of people using this theme. Do you have any idea on how to choose a solution that would please the most people ? On my side I've already put the italic modification to experiment with it and see how it fits, and I don't see any issue with that.