inkle / ink-tmlanguage

TextMate grammar files for Ink. (VS Code, Sublime Text, and Atom)
MIT License
20 stars 6 forks source link

Some changes to work better with Visual Studio 2019 plus TODO highlighting #2

Open zledas opened 4 years ago

zledas commented 4 years ago

Hello,

I tried to use it with Visual Studio 2019, but there were some problems. The main one, is that VS2019 somehow reads to the end of the line AND consumes line ending if \s* is used. So next line is not highlighted properly. So I changed \s* to [^\S\n\r]* in many places where it was used to read till the end of line.

Also I changed variable.other.XXX (and similar) to entity.name.variable.other.XXX so it gets coloured.

What is more, I added TODO highlighting, but I am not sure if I inserted it in all the needed places.

I am attaching .patch file with my changes, if that would help: changes.txt

P. S. I edited xml directly, so don't have YAML for these changes...

zledas commented 4 years ago

Today I saw, that TODO highlighting is not good enough – it does not catch some cases...

ephread commented 4 years ago

Thanks for the changes @zledas!

I'm sorry it took so long to respond, I completely missed this issue.

I won't be able to use your patch as-is, since, as you pointed out, the source of truth is the YAML. But, if changing \s* to [^\S\n\r]* is all you did, then I can probably update the YAML next time I'm touching the grammar. I'll also add entity.name.variable.other.XXX to all patterns using variable.other.XXX so that they can be both matched.

I'll have a look at the TODO changes, but a lot of corner cases can't be matched with the Textmate Grammar. So not handling these cases might be perfectly okay.

zledas commented 4 years ago

Thanks for responding.

Yes, \s* and variable.other.XXX changes are the main ones!

And it would be perfect if at least main cases of TODO highlighting would be available as well :)

ephread commented 4 years ago

I've resumed working on the grammar and made significant progress. There were a lot of other issues, beyond the line-ending thing.

Would you be able to try the latest version and tell me if you still encounter the same issues? (If you do, it would be nice to have a sample of the breaking code, so that I can write a text case against it!)

Thanks a lot for your help!

zledas commented 4 years ago

Tested new version – it works! :)

Didn't find any breakages, but our formatting is quite strict, so not much variation.

On the other hand, variables and function calls are still not coloured. From what I see, variable.other.XXX (and similar) still do not have prefix (entity.name.variable.other.XXX), so if it would be possible to add that (as an additional match?) – it would be perfect.

ephread commented 4 years ago

Tested new version – it works! :) Didn't find any breakages, but our formatting is quite strict, so not much variation.

Awesome!

if it would be possible to add that (as an additional match?) – it would be perfect.

Of course, I missed it during the refactoring. I'll add the scopes right away.

ephread commented 4 years ago

0.2.2 is out with the scope extensions. Let me know if it works or if I missed something else!

zledas commented 4 years ago

Tested it – extended variable scopes work great.

BUT I found another problem. I think this did not work before as well. Attaching screenshot and repro case (repro.ink.txt).

AND while writing this report I noticed that + [{EXAMINE}] is not highlighted in the right screenshot. Not sure if this is one of the limitations of text mate parsing.

2020-05-08_2123

ephread commented 4 years ago

Tested it – extended variable scopes work great.

I'm happy to hear that!

Thanks for providing an example, I'm unable to reproduce the problem with the latest grammar, sadly (attached here, in case I messed up the release: Ink.tmLanguage.zip). I tested with Textmate 2, SublimeText and VS Code, see below.

AND while writing this report I noticed that + [{EXAMINE}] is not highlighted in the right screenshot. Not sure if this is one of the limitations of text mate parsing.

You're right, it should be coloured. But I'm assuming it's related to the other issue.

Which editor are you using?

ink-test
zledas commented 4 years ago

Thanks for checking it out. I am using Visual Studio 2019 (Community edition).

What I just noticed is that { and } are not highlighted if they are right after true or false keywords. This can be seen in my previously attached screenshot as well. So I think it is line break problem – line break is consumed and so next line is not treated as new line...

zledas commented 4 years ago

Ahhh, dammit... Turns out that when I first checked 0.2.0 version, Visual Studio still used my older, modified .tmLanguage file, so I didn't see that it was broken :( Sorry for that.

So line break problem still exists. Basically all \s* that read till the end of line work differently in VS compared to other editors for some reason. VS reads to the end of the line AND consumes line ending if \s* is used. So next line is not highlighted properly. I can solve it if I replace \s* with [^\S\n\r]* (this explicitly tells not to consume line break symbols).

So I have to change such (and similar) places: <string>\s*(false|true)\s*</string> to <string>\s*(false|true)[^\S\n\r]*</string>

ephread commented 4 years ago

It's really strange that Visual Studio doesn't behave like other editors. I replaced most \s occurrences by [^\S\n\r] and it doesn't seem to have any negative impacts on other editors. Sorry I didn't make this change sooner, I wanted to make sure there wasn't any other underlying issues.

Here's the latest version, can you give it a try and let me know how it goes?

Ink.tmLanguage.zip

zledas commented 4 years ago

Yes, it was very strange to me as well. Tested this new one – looks like it works good. Thanks for fixing all these issues!

On the other hand, I found one not handled TODO case. I am copying it here, but if it would be better to have separate issue,I can move it to separate one:

===events===

=exit
    {
    - (variable != 4):
        TODO: ...
        M: Hi.
    }
    -> the_end
ephread commented 4 years ago

Tested this new one – looks like it works good. Thanks for fixing all these issues!

All good!

On the other hand, I found one not handled TODO case. I am copying it here, but if it would be better to have separate issue,I can move it to separate one:

It's fine, thanks for providing an example, I'll have a look.

ephread commented 4 years ago

0.2.3 should fix the issue. 🙂

zledas commented 4 years ago

Thank you for looking into it and fixing.

And I am really embarased now :( Looks like I was testing with the wrong version again (previous TODO bug happened to be present at my modification of .tmLanguage as well, so lucky coincidence)...

Visual Studio is really annoying that it somehow caches .tmLanguage and does not reload if I change it, so I tested with wrong files. Really sorry for that.

So, now I tested with the newest version (0.2.3). Definitely with it :) And some things are weird.

1) Gathers are highlighted differently. Is this expected? gathers

2) First tag # symbol is highlited differently than all folowing ones (I think this might be a deeper problem than just color...) tags

3) Strings are broken :( Any string ending is not matched, not only in this example... strings

4) Should function calls and -> DONE be highlited? If yes, then they are not :( function calls

I start to feel really bad because I am causing so much trouble because of these Visual Studio differences :( Sorry for that.

ephread commented 4 years ago

And I am really embarased now

All good, we all spend 30% of our time fightings tools, 60% of our time wondering why things don't work and 10% actually coding. 😄

  1. Gathers are highlighted differently. Is this expected?

Probably not, can you provide a gist? Seems that they might get eaten by something else.

  1. First tag # symbol is highlited differently than all folowing ones (I think this might be a deeper problem than just color...)

That one is an oversight, I forgot tags could be chained. It's easy to fix! I also noticed the * was a false positive, I'll try to fix that.

Note that # being a different color from the tag itself is done on purpose, it's because it's kind of a mix between a comment and a string. Dunno if it makes sense, but it can be changed later.

  1. Strings are broken :( Any string ending is not matched, not only in this example...

Ouch. I'm okay with some things not being highlighted properly, but NOT when it breaks the next lines.

Looking at the source, I think the regex for strings is wrong actually. Maybe Visual Studio is stricter than other editors and it's actually a good thing. I'll update the regex, let me know if you see a difference.

  1. Should function calls and -> DONE be highlited? If yes, then they are not :(

Yeah they should but I'd tend to believe it's the same issue you had with variables. Your current theme might highlight neither variable.function nor support.constant. I imagine that adding entity.name.variable.function would solve the issue for the first case, but I have no idea what scope would be required to highlight language constants.

The naming conventions give a lot of room for interpretation, so it's always a bit tricky to find the right scopes.

I start to feel really bad because I am causing so much trouble because of these Visual Studio differences :( Sorry for that.

Nah, don't, you've been super helpful! Visual Studio might have its quirks, but it helped me uncover a lot of small issues in the grammar. I'm happy to fix if you're happy to test!

I'll push a new grammar soon.

zledas commented 4 years ago

Sorry, can't find enough time to check it this week... Will try to do it next.

ephread commented 4 years ago

No worries, I'll be working a bit in Visual Studio on Windows in the near future, so I'm going to do a full sweep and try to fix as many issues as I can see!

ephread commented 4 years ago

So, I had a look with Visual Studio. The good news is that it's definitely got a stricter engine and that's a good thing. I fixed most parsing issues (I think), the remaining ones have to do with expressions. It's notable with list declarations, for instance, where syntax highlighting breaks. I'll fix that in the future.

The bad news is that while Visual Studio does detect the scopes properly, the default theme chooses to not highlight them most of the time. I double checked and it's in line with how C# is highlighted by defaults. For instance variables and functions are not highlighted when they are invoked (hence the need to use entity.name.variable instead of variable.other even though it's somewhat incorrect). That's the root of a lot of highlighting issues and it's going to be quite hard to please every themes in every editors.

I'm trying to find a middle ground that is semantically accurate (I've already removed some of the crazy scopes I previously added). I'm not opposed to create a custom version for Visual Studio but I think providing custom tmThemes or customising the Visual Studio editor theme would be a better option.

For instance, I noticed that changing the color of Identifier would change the color of tokens matched by variable.function. I still have to look further though, but it looks promising. I'll keep you posted!

zledas commented 4 years ago

This is great news (at least for me :) ) that you are able to test with Visual Studio!

Yeah, highlighting in VS looks complicated. I could not find a good way to customise only for .ink files (might be my problem not finding, but I think it is not possible). To my mind, it is best to prepare a theme that looks good with defaults, but I totally understand your reasoning about semantic accuracy!

And sorry once more – I was hoping to have some time this week, but looks like I won't, and not sure if I'll be able to find enough time next week as well.

ephread commented 4 years ago

Yeah, highlighting in VS looks complicated. I could not find a good way to customise only for .ink files (might be my problem not finding, but I think it is not possible).

Yeah, that's my understanding as well after fiddling with the editor. The default theming capabilities apply to all languages (and are a bit weird IMO, it looks like they deal with a ton of legacy there). Identifer matches basically everything in a C# file, so it's not ideal to change its default color.

Good thing though, is that as far as I understand, Visual Studio as a strict number of configuration variables for the theme. So side effects are limited, it's not as wild as TextMate scopes!

I've added override capabilities in the build system. Meaning that I can now output a VS-only version without too much hassle and without cluttering the base grammar! The current version in master uses these overrides for now:

"variable.other.ink" -> "entity.name.variable.other.ink"
"variable.function.ink" -> "entity.name.function.ink"

I'm going to fix the remaining matching issues and test it with Visual Studio over the next few days, but I should have something by the end of the week.

And sorry once more – I was hoping to have some time this week, but looks like I won't, and not sure if I'll be able to find enough time next week as well.

All good, we're not in a hurry!