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

[JavaScript] Syntax highlight - how to get different colors for object literal key and a function value? #154

Closed
ahwayakchih opened this issue Feb 10, 2016 · 12 comments

Comments

@ahwayakchih
Copy link

As mentioned in #141, keys of object literals will get different color if value is a function.
I managed to modify Monokai Extended theme to prevent that, but i do not know how to keep key white, but function name colored.

Here's theme rule i added:

        <dict>
            <key>name</key>
            <string>JS: Object Key for Function Name</string>
            <key>scope</key>
            <string>meta.function.json.js entity.name.function.js, meta.function.json.arrow.js entity.name.function.js</string>
            <key>settings</key>
            <dict>
                <key>fontStyle</key>
                <string></string>
                <key>foreground</key>
                <string>#ffffff</string>
            </dict>
        </dict>

Here's test code:

var obj = {
    one: () => 1,
    two: function two () {
        return 2;
    }
}

Here's result:

screenshot from 2016-02-10 19 10 21

Is there a way to make function name colored, but keep object literal key color unchanged? It would be great if i could get something like this mockup:

screenshot from 2016-02-10 19 10 21-mockup

@jonschlinkert
Copy link

can you paste the code in too? I'll play around with it and see if I can get what you want.

fwiw, honestly the first one (before) seems like the most desirable, given that the value is a function for both properties. but that's just my own preference

@FichteFoll
Copy link
Collaborator

I took a quick look and it is currently impossible to specifically target functions defined as object keys without also matching "normal" function definitions.

The syntax definition needs to be changed.

@ahwayakchih
Copy link
Author

@jonschlinkert i've updated post, to include code.

It does not look bad in case of example above, but it starts to look messy in something like this:

var obj = {
    one: 1,
    two: () => 2,
    three: "3",
    four: function four () {
        return 4;
    }
}

But that's just a visual preference, and one could argue that such object literals should not be defined anyway. Thing is that the key is not highlighted if a function is passed through a variable, like this:

var one = () => 1;
var obj = {
    one: one
}

obj.one is still a function (or a reference to a function), but highlighter does not know about that. Which kinda breaks rule.
I understand that, for highlighter to know which variable is a function, it would require a lot more understanding of the code (which would complicate things a lot) for not much gain.
So, i'd rather keep things simple and have object literal keys all in the same color, than have them colored differently for some functions (and in cases when they are colored, i can see that there is a function expression right there anyway).

@FichteFoll i never edited syntax files before, but maybe we could add a marker while keeping backwards compatibility?

In JavaScript.sublime-syntax file there is literal-function-labels and it defines two matches. Each of them has \s*([_$a-zA-Z][$\w]*)?\s* as last part of matching regex. Would it be OK to change it by adding another parenthesis like this: \s*(([_$a-zA-Z][$\w]*))?\s* and then adding something like entity.name.function.label.js as last item of captures?

Here's modified part:

  literal-function-labels:
    - match: |-
        (?x)
        \b([_$a-zA-Z][$\w]*)
        \s*(:)
        \s*(?:(async)\s+)?
        \s*(function)(?:\s*(\*)|(?=\s|[(]))
        \s*(([_$a-zA-Z][$\w]*))?\s*
      captures:
        1: entity.name.function.js
        2: punctuation.separator.key-value.js
        3: storage.type.js
        4: storage.type.function.js
        5: keyword.generator.asterisk.js
        6: entity.name.function.js
        7: entity.name.function.label.js
      push:
        - meta_scope: meta.function.json.js
        - match: (?<=\))
          pop: true
        - include: function-declaration-parameters
    - match: |-
        (?x)
        (?:
          ((')((?:[^']|\\')*)('))|
          ((")((?:[^"]|\\")*)("))
        )
        \s*(:)
        \s*(?:(async)\s+)?
        \s*(function)(?:\s*(\*)|(?=\s|[(]))
        \s*(([_$a-zA-Z][$\w]*))?\s*
      captures:
        1: string.quoted.single.js
        2: punctuation.definition.string.begin.js
        3: entity.name.function.js
        4: punctuation.definition.string.end.js
        5: string.quoted.double.js
        6: punctuation.definition.string.begin.js
        7: entity.name.function.js
        8: punctuation.definition.string.end.js
        9: punctuation.separator.key-value.js
        10: storage.type.js
        11: storage.type.function.js
        12: keyword.generator.asterisk.js
        13: entity.name.function.js
        14: entity.name.function.label.js
      push:
        - meta_scope: meta.function.json.js
        - match: (?<=\))
          pop: true
        - include: function-declaration-parameters

Of course entity.name.function.label.js name is just an example - i'm not sure about the naming convention for captures and what would be best name for function name in this case.

I tested changes and they work great, once i've added entity.name.function.label.js to list of scopes in JS: Function Name of Monokai Extended theme.

If you think that such change to syntax file is ok, i can create pull request.

@ahwayakchih
Copy link
Author

One more thing: allowing for separate color of function name could allow themes to "greyout" whole thing to minimize visual impact, but it would require additional captures also in literal-function part of syntax file.

@FichteFoll
Copy link
Collaborator

The thought behind your proposed patch is correct, but it should be executed differently. Instead of adding another capture group you can just adjust the existing captures' scope names to include the additional identifier after entity.name.function. You should also probably add one for all function matches.

@FichteFoll
Copy link
Collaborator

Unfortunately it seems that some color schemes (like monokai extemded) define specific colors to the entity.name.function.js scope, which would not match anymore afaik. I can't test if it would still work, but I have to question why such a specific highlight is necessary and why source.js entity.name.function wouldn't suffice.

@ahwayakchih
Copy link
Author

@FichteFoll thanks. So captures support comma separated identifiers? For example:

captures:
        1: string.quoted.single.js, some.other.identifier.js

You should also probably add one for all function matches

But this is only about function names in case of function X () {} (and when it is assigned as value either to variable or key in object literal).

Unfortunately it seems that some color schemes (like monokai extemded) define specific colors to the entity.name.function.js scope

Yes, that is why i wanted to just add another, more specific identifier, without removing previous one - to keep backward compatibility (and because this case does work as both regular entity.name.function.js and more specific "original name of function").

I have to question why such a specific highlight is necessary and why source.js entity.name.function wouldn't suffice.

Because currently entity.name.function is used for both sides of expression, i.e.:

var entityNameFunction = function entityNameFunction () {};

And they're not exactly the same. I understand that historically they often were colored the same, but it would allow themes/highlighters for a lot more flexibility if they could be treated differently.

It started from what i described above as issue, but i can imagine a theme where function entityNameFunction part would be made less visible, to make most important parts of function expression more visible.

@jonschlinkert
Copy link

I have to question why such a specific highlight is necessary and why source.js entity.name.function wouldn't suffice.

IMHO that's a bit like asking "why isn't red good enough?" lol.

Also I retract my earlier comment. I'm finding it difficult to apply syntax highlighting with the same amount of specificity as before the patch.

And they're not exactly the same.

they're not at all the same. One is a variable and one is a function name. e.g. I agree.

@FichteFoll
Copy link
Collaborator

So captures support comma separated identifiers?

No. Just make the capture define two scopes by separating with a space.

But this is only about function names in case of function X () {} (and when it is assigned as value either to variable or key in object literal).

I mean that a special scope should be added for the other type of functions as well (anonymous function assigned to an object key).

IMHO that's a bit like asking "why isn't red good enough?" lol.

Well, the thing is that tech-depth imposed by third-party package authors is seemingly holding us back here. That's pretty frustrating, if you ask me.

@ahwayakchih
Copy link
Author

@FichteFoll thanks for help. I've prepared pull request for function label entities. As for anonymous functions, that will require a lot more changes to syntax file, because right now function "label" is optional everywhere, so to differentiate between anonymous and named functions, we would have to create separate matches for them. Unless there is a way to declare conditional meta scopes in syntax file?

@wbond
Copy link
Member

wbond commented Feb 16, 2016

This should be addressed now in f6c42c7. PR #181 has a comment showing an example of color scheme modifications to get the desired color for objects keys that have functions set to them.

@wbond wbond closed this as completed Feb 16, 2016
@ahwayakchih
Copy link
Author

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants