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

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

Open
zledas opened this issue Feb 23, 2020 · 22 comments
Open

Comments

@zledas
Copy link

zledas commented Feb 23, 2020

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

zledas commented Mar 20, 2020

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

@ephread
Copy link
Collaborator

ephread commented Apr 28, 2020

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

zledas commented Apr 29, 2020

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

ephread commented May 7, 2020

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

zledas commented May 8, 2020

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

ephread commented May 8, 2020

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

ephread commented May 8, 2020

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

@zledas
Copy link
Author

zledas commented May 8, 2020

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

ephread commented May 8, 2020

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

zledas commented May 8, 2020

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

zledas commented May 8, 2020

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

ephread commented May 9, 2020

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

zledas commented May 11, 2020

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

ephread commented May 11, 2020

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

ephread commented May 11, 2020

0.2.3 should fix the issue. 🙂

@zledas
Copy link
Author

zledas commented May 12, 2020

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

ephread commented May 12, 2020

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

zledas commented May 15, 2020

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

@ephread
Copy link
Collaborator

ephread commented May 19, 2020

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

ephread commented May 19, 2020

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

zledas commented May 20, 2020

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

ephread commented May 20, 2020

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!

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

2 participants