Skip to content

First TS keyword in file misses last character of bold highlighting #139

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

Closed
johnnyreilly opened this issue Mar 2, 2015 · 23 comments
Closed
Assignees

Comments

@johnnyreilly
Copy link
Member

See screenshot illustrating below - look out for module and interface

screenshot 2015-03-02 17 05 54

This only seems to be an issue if the first word in a file is TS keyword. You can see here it's not a problem:

screenshot 2015-03-02 17 10 26

@basarat
Copy link
Member

basarat commented Mar 2, 2015

@johnnyreilly what theme's (ui + syntax) + atomts version are you using. I don't seem to be getting this :

image

Note: In your example { also gets eaten up i.e. you have module controllers instead of module controllers {. This is a clear example of Grammar tokenization gone wrong (a bug in our or TypeScript language service code). I'd be very interested in investigating. Can you post an example .ts file?

@johnnyreilly
Copy link
Member Author

I hadn't spotted the { vanishing - you have keener eyes than I! 😆

The files in the shot are these:

https://github.com/johnnyreilly/proverb-offline/blob/master/Proverb.Web/app/sayings/sayings.ts
https://github.com/johnnyreilly/proverb-offline/blob/master/Proverb.Web/app/app.ts

My UI and syntax themes are Atom Light. I'm using [email protected]

@basarat
Copy link
Member

basarat commented Mar 3, 2015

I suspect this is a file encoding issue. Two bytes off feels like a BOM marker not getting detected.

@johnnyreilly
Copy link
Member Author

Some odd behaviour to report. I just fired up atom again and app.ts wasn't illustrating the problem behaviour whilst sayings.ts was.

Both files are UTF-8 according to atom.

@basarat
Copy link
Member

basarat commented Mar 3, 2015

Having a BOM in the file does throw it off. : https://github.com/TypeStrong/atom-typescript-examples/tree/master/bomtest furthermore its how the typescript classifier classifies it. It completely ignores the BOM.

@johnnyreilly
Copy link
Member Author

Interesting. What do you make of both files being UTF-8 but only one illustrating the behaviour? (I'm assuming that right clicking in the editor and clicking "change encoding" is an adequate test?

screenshot 2015-03-03 11 27 55

@basarat
Copy link
Member

basarat commented Mar 3, 2015

(I'm assuming that right clicking in the editor and clicking "change encoding" is an adequate test

No. The quickest way I find is to save with "Advanced save options" in visual studio using an encoding that has "with signature" marked. e.g.
image

@johnnyreilly
Copy link
Member Author

By Jingo you're right! Atom has lied to me 😨

screenshot 2015-03-03 11 38 48
screenshot 2015-03-03 11 39 11

So is the conclusion that UTF-8 is a problem? (Given that sayings.ts which is UTF-8 demonstrates the problem and app.ts which is not doesn't)

@basarat basarat closed this as completed in c09b8cf Mar 3, 2015
@basarat
Copy link
Member

basarat commented Mar 3, 2015

UTF-8 is a problem

A combination of how Atom + TypeScript classifies BOM. TypeScript ignores it (a bad thing ... I would have liked it to classify it as whitespace). Atom doesn't want other's to classify it (this makes the bad thing by TypeScript a good thing), except that atom will still give you the string with the BOM and expect you (the grammar author) to not return it as a part of your classification. So finally we still need to offset our classification by the BOM amount.

You can see the commit for details ❤️

@basarat
Copy link
Member

basarat commented Mar 3, 2015

published as v0.78.0

@johnnyreilly
Copy link
Member Author

Cool - so the fix was in atom-typescript rather than elsewhere.

@basarat
Copy link
Member

basarat commented Mar 3, 2015

yes 💓

@basarat
Copy link
Member

basarat commented Mar 3, 2015

BTW check out them fancy panels in the next version ;)
image

Off to bed for me

@johnnyreilly
Copy link
Member Author

Nice - that's pretty clear. I remembered seeing discussions around TypeScript and encoding elsewhere but had completely forgotten the details. I'm just upgrading to 0.78.0 now - will report back....

@johnnyreilly
Copy link
Member Author

Sleep well chap! Good work 👏

@johnnyreilly
Copy link
Member Author

I've just given it a test with 0.78 and there's good and bad news.

The good news is the highlighting now looks different:

screenshot 2015-03-03 12 07 20

But unfortunately module doesn't appear to be bold now? Or at least not as bold as it was before....

The slightly worse news is that the TypeScript build doesn't seem to work at all now. Both saving and ctrl+shift+b result in this:

No Build. Press (ctrl+shift+b / cmd+shift+b ) to start a build for an active TypeScript file's project.

@basarat
Copy link
Member

basarat commented Mar 3, 2015

Quick note : the shortcut only works if your focus cursor is in an editor
window.

If that fixes it for you we can make the shortcut more global later.

@basarat basarat reopened this Mar 3, 2015
@basarat basarat closed this as completed in cc4d6ca Mar 4, 2015
@basarat
Copy link
Member

basarat commented Mar 4, 2015

The build shortcut + goto definition shortcut are a bit more global so they should just work. Try 0.79.0

@basarat
Copy link
Member

basarat commented Mar 4, 2015

Regarding it not appearing as bold : it seems to be tokenize correctly as you can see :

image

I am assigning module the category storage.modifier (an atom specific name) and not keyword because of previous work that does the same thing : https://github.com/fdecampredon/brackets-typescript/blob/master/src/main/mode.ts#L101 (def is brackets specific)

module will look at the same as class and var. How it looks will depend on your theme. I should do the same with interface. These decisions are somewhat arbitrary on my part. ❤️ I'll clear those up later but there is no unintentional side effect that needs to be further investigated here 👍

@johnnyreilly
Copy link
Member Author

Hey @basarat,

module will look at the same as class and var . How it looks will depend on your theme.

Yup the theme lends it almost no visibility - maybe I'll switch! But it seems to share the same presentation as class and var as you suggest.

I'm still getting no compilation but I'll try and clean install of Atom and atom-typescript and see if that resolves it. (Imagine trying that with Visual Studio - would take days!)

If the compilation issue still presents I'll open new issue.

@johnnyreilly
Copy link
Member Author

Completely clean install and I can't compile at all. However I think it's related to some problem with my system - when I roll back to [email protected] I still get no compilation (even though tsc on the command line works fine)....

I'm not about to rebuild this machine so I'm going to look at this as an opportunity to try out atom on Linux...

@johnnyreilly
Copy link
Member Author

And after all that it was a return of module: null in tsconfig.json... sigh. Fixed now.

@basarat
Copy link
Member

basarat commented Mar 4, 2015

Sorry about that. I've made this my main task : #138 will have it sorted today after work. Hopefully before you get to read this in your timezone ❤️

@TypeStrong TypeStrong locked and limited conversation to collaborators Jan 20, 2019
angelestelar5z added a commit to angelestelar5z/atom-typescript that referenced this issue Aug 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants