akamud / vscode-theme-onedark

VSCode Theme based on Atom's One Dark theme
MIT License
293 stars 200 forks source link

Fix theme for new scoping in VSCode 1.9 #35

Closed akamud closed 7 years ago

akamud commented 7 years ago

VSCode 1.9 completely changed the way it handles tmTheme scopes for coloring.

The whole theme is broken at the moment and needs to be pretty much re-done from zero.

More information: https://github.com/Microsoft/vscode/issues/18357

akamud commented 7 years ago

I'll be working on this ASAP. Hold tight everyone and sorry for the inconvenience :(

akamud commented 7 years ago

I just released a version 1.0.0, it brings minimal support for version 1.9.

Some languages simply don't have good support in VS Code and won't allow for good coloring (e.g. Java).

I still have a lot of work in front of me. I believe I can achieve the same support I had before 1.9, some languages (e.g. Python) are still really bad at the moment compared to the last version. I'm sorry about that, I'm working on it.

I'll update this issue as I make progress.

akamud commented 7 years ago

To anyone waiting, I believe version 1.1.0 now has the same coloring it had before version 1.9. You can just update on VSCode Marketplace.

Some languages got improved with the new scopes, like Ruby and JS/TS but if you find any inconsistency, specially one that did not exist prior to 1.9, please report.

I believe the only languages missing now are the ones that depend on plugins on VSCode that I supported before 1.9, like Rust. I'll work on them over the week and close this issue when they are released.

Thanks for the patience :)

vkbansal commented 7 years ago

I found the following inconsistency.

Language: JavaScript

Before:

screen shot 2017-02-07 at 7 33 48 pm

After:

screen shot 2017-02-07 at 7 34 08 pm

The problem seems to be with brackets and code inside them.

The sample code is

export default class Definition {
    constructor(def) {
        if (invalidParams(def)) {
            throw new Error('Invalid arguments provided to Lang constructor');
        }

        this.__def = def instanceof Map ? def : new Map(def);
    }

    clone = () => new Definition(new Map(this.__def));

    extend = (def) => {
        if (invalidParams(def)) {
            throw new Error(`extend requires Map`);
        }

        const extendedLang = new Map(this.__def);

        for (const [key, value] of def) {
            extendedLang.set(key, value);
        }

        return new Definition(extendedLang);
    }
}
akamud commented 7 years ago

Thanks for reporting!

I'll take a look at this tonight and get back to you.

akamud commented 7 years ago

Fixed. I'll hold until I have the remaining work done so I don't flood users with a new version everyday (received a few complainings about that in the past). This will be released in version 1.2.0.

For comparison,

Atom:

screen shot 2017-02-07 at 23 35 12

VSCode:

screen shot 2017-02-07 at 23 35 56

The constructor keyword unfortunately can't be selected without changing the colors of many other items together... Const declarations in JS also have this same problem, no unique scope for them. If VSCode ever improves that I'll update fixing this.

Thanks again for contributing :)

vkbansal commented 7 years ago

Thanks for the quick turnaround!

One question though: The const declarations worked correctly before 1.9 and also if you look, the const declaration in for loop is working correctly. So why not the one above it?

akamud commented 7 years ago

The [key, value] inside the for loop are array variables declaration, so VSCode provides unique scopes for them, which I used.

The extendedLang const is detected as a regular variable though. As shown in the picture below:

screen shot 2017-02-07 at 23 48 59

That is the most specific scope it has. And as you can see, none of the other scopes detect it as a const declaration. So if I color it now, every variable will be red too.
When I hit these conflicts I have to make a choice, so usually the most common scope or the color that is more easy on the eyes wins. In this case, variables are way more common and making them red to match consts would cause much more differences from Atom's original theme.

As to why this worked before, VSCode now exposes scopes that are totally different from previous versions, probably I had a way to differentiate variables declaration from consts in this case.

You can see consts and variables now have the exact same scopes:

screen shot 2017-02-07 at 23 56 35

Atom exposes a constant.other.js scope so it can provide custom coloring.

vkbansal commented 7 years ago

Thanks for detailed explanation!

iddan commented 7 years ago

Can you make an option for coloring all variables types consistently? (in red I guess)

akamud commented 7 years ago

I try to follow the Atom theme as close as I can. Atom does not color variables (var or let), they are gray. So I'll keep it that way.

Atom:

screen shot 2017-02-09 at 00 09 26

You can always fork the project and customize it if you want, it is MIT licensed :)

iddan commented 7 years ago

Anyway here is a bug: image code:

import * as styles from './mission.scss';

export default function MissionIcon({ key, title }) {
    return <i class={classnames(
        styles['mission__icon'],
        {
            [styles['mission__icon-letter']]: title.match(A_HEBREW_LETTER),
            [styles['mission__icon-first']]: !key,
        }
    )}>{title}</i>;
}

Thank you!

akamud commented 7 years ago

Can you provide a little more information? Is this a JS or TS file? Is this React?

iddan commented 7 years ago

image Another bugs:

iddan commented 7 years ago

@akamud It's JS React

akamud commented 7 years ago

I'll create a new issue to track this JS React issues.

Please use #36 to follow this discussion.

akamud commented 7 years ago

Version 1.2.0 now has the same support for plugins as before.

If you find any other inconsistency, please create a new issue.