textmate / javascript.tmbundle

TextMate support for JavaScript
88 stars 54 forks source link

Wrong support.constant.dom.js #37

Open igor10k opened 10 years ago

igor10k commented 10 years ago

I've noticed that that TM wrongly highlights some constants. That's how it currently looks

{
    name = 'support.constant.dom.js';
    comment = 'HTML 5 (http://www.w3.org/TR/html5/single-page.html#window)';
    match = '(?<!\.|\$)\b(applicationCache|closed|console|crypto|document|...)\b(?!\$)';
}

screen shot 2014-09-10 at 19 55 14

It should be vise versa, var top should not have highlight, but window.top should.

In short I think that match should look like this

'(?<=\.)\b(applicationCache|closed|console|crypto|...)\b(?!\$)';

Am I missing something?

igor10k commented 10 years ago

Actually I do miss something. Those constants should be separated into two types: objects and properties. Objects like window, console, etc should stay with the old match regexp

'(?<!\.|\$)\b(console|window...)\b(?!\$)';

and properties like log, error, top, etc should go with the new one

'(?<=\.)\b(log|error|top|...)\b(?!\$)';
whitlockjc commented 10 years ago

I'll look into it. I agree with what you're saying, something I've noticed as well but I just haven't gotten to it yet.

whitlockjc commented 10 years ago

I started looking into this and I don't think I can use your suggestion as-is. For example, the window object's properties should always be accessible without window. in the browser, since the global this is a window. So requiring a . before top or name or others is not ideal. I do think we should break things up into objects and properties where possible to have a better experience.

Until we have per-runtime (Browser, Node.js, Common.js, Rhino, ...) JavaScript grammars, it will be impossible to get everything right. Let me see what I can come up with.

igor10k commented 10 years ago

Per-runtime grammars sounds like a bad idea since I don't see any way to automatically distinguish Browser and node.js code for example.

whitlockjc commented 10 years ago

I agree. It's not ideal, I'm just saying that we're close to "anomalies" being because we have one syntax for all of JavaScript. Maybe we could do something like JSHint or emacs where you can throw a special comment in your file to "choose" browser vs. Node.js vs. ... Thoughts?

igor10k commented 10 years ago

Project can contain thousands of JS files. Throwing a comment in each one of them doesn't sound like a solution. It's even easier then to use .tm_properties and choose grammar there by path as usually nobody mixes up different runtime js within the same folder, but this still doesn't sound like a solution.

I'd say we either highlight browser's default globals everywhere or we highlight them only as a property of window. I'd shoot with the latter as it's better to not have something highlighted than highlighting something that shouldn't be.

Even within the browser's js in cases like var location = '' word location should not be highlighted.

whitlockjc commented 10 years ago

Well, you'd only need the comment when you open the file for editing so it's not like you'd have to go in and alter all files just because. That said, I do get your drift and I also think a .tm_properties entry could be better. Still not ideal, I get it, so we'll keep thinking.

As for when we highlight browser global variables, we're already doing it everywhere and that's why you've filed the bug. The problem is for us to just not highlight it when it's not a reference to the variable is that we don't really parse JavaScript using a JavaScript parser. We use regex patterns, that's how TextMate works. So it's nearly impossible to go line-by-line and tell a token's context because matching is line by line. I could special case lines starting with var but that wouldn't handle all cases.

Let me think a little more and if you have any suggestions, let me know.