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

Fix theme for new scoping in VSCode 1.9 #35

Closed
akamud opened this issue Feb 3, 2017 · 17 comments
Closed

Fix theme for new scoping in VSCode 1.9 #35

akamud opened this issue Feb 3, 2017 · 17 comments

Comments

@akamud
Copy link
Owner

akamud commented Feb 3, 2017

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: microsoft/vscode#18357

@akamud
Copy link
Owner Author

akamud commented Feb 3, 2017

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

@akamud
Copy link
Owner Author

akamud commented Feb 5, 2017

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

akamud commented Feb 7, 2017

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

vkbansal commented Feb 7, 2017

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

akamud commented Feb 7, 2017

Thanks for reporting!

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

@akamud
Copy link
Owner Author

akamud commented Feb 8, 2017

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

vkbansal commented Feb 8, 2017

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

akamud commented Feb 8, 2017

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

vkbansal commented Feb 8, 2017

Thanks for detailed explanation!

@iddan
Copy link

iddan commented Feb 9, 2017

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

@akamud
Copy link
Owner Author

akamud commented Feb 9, 2017

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

iddan commented Feb 9, 2017

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

akamud commented Feb 9, 2017

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

@iddan
Copy link

iddan commented Feb 9, 2017

image
Another bugs:

  • braces and semi-colons in yellow
  • decorator @ in grey
  • Variables inconsistent colouring (or is it intended because they "become" keys?)
    code:
@connect(
    ({ artists }) => ({ artists }),
    dispatch => bindActionCreators({ getArtists, getArtist, addArtist, editArtist, removeArtist }, dispatch)
)
export default class AdminArtists extends Component {
    
	componentWillMount = () => {
		this.props.getArtists();
	}

	render = () => {
		console.log(this.props.artists);
		return <div>

        </div>;
	}
}

@iddan
Copy link

iddan commented Feb 9, 2017

@akamud It's JS React

@akamud
Copy link
Owner Author

akamud commented Feb 9, 2017

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

Please use #36 to follow this discussion.

@akamud
Copy link
Owner Author

akamud commented Feb 9, 2017

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

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

@akamud akamud closed this as completed Feb 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants