tomblachut / svelte-intellij

Svelte components in WebStorm and friends
MIT License
482 stars 38 forks source link

Handle svelte label #43

Closed unlocomqx closed 5 years ago

unlocomqx commented 5 years ago

Solves #7

tomblachut commented 5 years ago

I've been working on #4 recently because those issues are IMO related. First I've met the dead end but then asked JB and now I'm thinking your approach for this is in fact the best one.

Thanks for the tests, much appreciated.

I didn't run this yet but from the top of my head, have you tried using HighlightingLexer or Annotator instead of HighlightingVisitor?

https://intellij-support.jetbrains.com/hc/en-us/community/posts/206124159-Difference-between-HighlightVisitor-and-Annotator-

I've found this topic that says Annotator is recommended for plugins, on the other hand lexers are always the fastest but IDK if this is suitable

unlocomqx commented 5 years ago

I didn't try Annotator or HighlightingLexer but I will give them a try

unlocomqx commented 5 years ago

The annotator is more efficient indeed https://github.com/tomblachut/svelte-intellij/pull/43/commits/1f3643ae8310ff27a1872da00dda2da354139dc2

tomblachut commented 5 years ago

Nice, looks cleaner!

tomblachut commented 5 years ago

Annotator stil does not work for me. When I change info annotation to e.g. error colors kick in but with info severity it stays white.

Notice that I've extended TextRange to include colon and it is being highlighted.

Screenshot 2019-08-09 at 10 01 23
unlocomqx commented 5 years ago

I was able to reproduce the annotator issue on the dracula theme screenshot_2

Light theme gave expected results (I made keywords red for better visibility) image

I will try to determine the cause and come up with a fix/workaround

unlocomqx commented 5 years ago

In the light theme, the label doesn't have a foreground color image

In dracula, it has image

Seems to be an issue of priority because the two annotations have severity = info I removed the label annotation in https://github.com/tomblachut/svelte-intellij/pull/43/commits/00cdccc79cf927d52994655380ee2dafcfb0be10

image

tomblachut commented 5 years ago

Excellent work. I've never checked Color Scheme settings myself, seems you can use it for debugging 🥇

Sadly this code is so hacky I'm afraid it can stop working between IntelliJ releases. There was already such issue between 2019.1 and 2019.2 in this project. Anyway let's keep it that way for now.

I've experimented a bit and learned a bunch about annotations along the way. Nothing else worked lol.

Built in label annotation is added by JSAnnotator, which indirectly calls JSSemanticHighlightingUtil.class:590.

I've had some success by adding annotations to the same element as JSAnnotator does but there is race condition. I thought adding order="last" to the entry in plugin.xml will fix that but it doesn't work as I thought it does.

I have last clue remaining: JSAnnotator is created in JSAnalysisHandlersFactory#createAnnotatingVisitor. This is an extension point, so we could modify the method that creates annotations for labels instead of using separate Annotator. This would I think require creating new JS dialect and I suppose there will be complications. Asking JetBrains is the best course of action now, I'll do it later.


This label is weird imo. 'let' and 'const' keywords don't have those. image

I've remembered about reactive statements now. You are searching for definitions also in them so the following will find a reference. Such code will throw in runtime because Svelte treats JS as strict mode but more importantly IDE won't find the reference if you remove the label so I think we should handle it the same way.

image

Lastly references are being found inside of all JSLabeledStatements, not only Svelte ones.

One additional issue is duplicate finding. I think it should be made in separate PR.

image

unlocomqx commented 5 years ago

image

This labeling is added by https://github.com/tomblachut/svelte-intellij/blob/00cdccc79cf927d52994655380ee2dafcfb0be10/src/main/java/dev/blachut/svelte/lang/codeInsight/SvelteAnnotator.kt#L19

I could remove it if you like I hope I understood you correctly regarding this point I will check other points as well

tomblachut commented 5 years ago

I know. Yes, remove it please

unlocomqx commented 5 years ago

I removed it but other than that, I agree, it's hacky and probably many unexpected edge cases will arise on the ref contributor side