Add symbol tags as proposed for LSP specification (e.g. visibility tags, static, abstract, etc.) #977

[travkin79]

Start using new symbol tags as proposed in my microsoft/language-server-protocol#2003, for example, adding private, package, protected, and public visibility tags as well as tags like static, final, abstract, read-only, nullable and non-null. Our motivation comes from our wish to add visibility and other symbol details to the outline view (see discussion #977), but maybe also to the call hierarchy and to the type hierarchy or to other related features / LSP operations.

The PR on the LSP specification requires at least one implementation in a language server and / or a client. We plan to finish a first language server implementation in clangd [1] [2] and a first client implementation in LSP4J, LSP4E, and CDT LSP (which is using clangd).

See microsoft/language-server-protocol#2003
and llvm/llvm-project#167536
and eclipse-lsp4j/lsp4j#856

The PR for updating LSP4E to LSP4J vers. 1.0.0 (#1421) has to be merged before this PR can be merged.

[rubenporras]

This would be great it the LSP gets improved and a new version in released.

[sebthom]

Hi @travkin79, just checking in - are you planning to finish this PR? It would be much appreciated if you could! 😊

[travkin79]

Hi @sebthom,
Yes, I'd be happy to finish this and related PRs.

The reason for the delay is that the LSP PR requires (in best case) a client implementation and a language server implementation. We planned to implement it for CDT LSP, LSP4E and clangd, but the person implementing the required changes on clangd side stopped working on her/his PR. But in the meanwhile, we found someone else who will finish the work. So, I hope to finish the related PRs, including this one, some time soon.

[travkin79]

Hi @jonahgraham, @sebthom, and @rubenporras,
It seems that LSP4J introduced some new Either types in the API vers. 1.0.0 and LSP4E didn't adapt the changes. Is someone working on that or can give me some hints how the new types should be handled in LSP4E?

Type changes in LSP4J vers. 1.0.0:

• TextEdit -> Either<TextEdit, SnippetTextEdit> in TextDocumentEdit class
• String -> Either<String, MarkupContent> in Diagnostic class

[jonahgraham]

TextEdit -> Either<TextEdit, SnippetTextEdit> in TextDocumentEdit class
String -> Either<String, MarkupContent> in Diagnostic class

Background:

Those are the new features in LSP 3.18 (beta support). See https://github.com/eclipse-lsp4j/lsp4j/blob/main/CHANGELOG.md#v100-tbd

See 3.18 spec for more details, which is also in the LSP4J javadocs

How to fix:

Because you are not turning on snippetEditSupport to enable the 3.18 feature (yet), the right thing to do is change things like .getEdits -> getEdits.getLeft.

LMK if that makes sense. It may be a better idea to have a separate PR for the 0.24 -> 1.0 LSP4J change, and I can help write that if the above isn't trivial.

[jonahgraham]

We have improved the API policy for 1.0 going forward to be properly semantic versions. This means the range can safely be:

Suggested change

	org.eclipse.lsp4j.jsonrpc;bundle-version="[1.0.0,1.1.0)",
	org.eclipse.lsp4j.jsonrpc;bundle-version="[1.0.0,2.0.0)",

[travkin79]

Hi @jonahgraham,

It may be a better idea to have a separate PR for the 0.24 -> 1.0 LSP4J change,...

Do we need an issue for the separate PR? I started PR #1421 for that purpose.

[jonahgraham]

Do we need an issue for the separate PR?

No IMHO - but I am not an active committer on LSP4E so I don't know if it is best to just create one to save yourself hassle later.

[travkin79]

OK. Then, @rubenporras, please let me know if you want me to create an issue for PR #1421 or if you prefer to create it yourself.

[travkin79]

Hi @jonahgraham,
Do we have to dispose the images cached here? It seems that images should be disposed when the Display used to create them is being disposed. What would be the best way to do that?

[jonahgraham]

The short answer is register with org.eclipse.swt.widgets.Display.disposeExec(Runnable) to dispose of images when the display is disposed.

You can also look at how org.eclipse.jface.resource.ImageRegistry handles it.

[jonahgraham]

PS I dodged the "Do we have to dispose the images cached here? " part of the question - but the answer is yes in theory, in practice if the test suites aren't making additional displays, chances are it never matters in practice. However the non-dispose listener may warn about non-disposed images. Perhaps it is already on colorToImageCache entries, but as it happens on shutdown no one paid attention/noticed?

Run with -Dorg.eclipse.swt.graphics.Resource.reportNonDisposed=true to get the warnings.

[travkin79]

Ok, I hope to have solved it by disposing the cached images on bundle / plug-in stop. I tried to start Eclipse with -Dorg.eclipse.swt.graphics.Resource.reportNonDisposed=true, but did not see any warning or error message if images were not disposed.

[FlorianKroiss]

@travkin79 I recently made some changes so that LSP4E uses mostly svg images (#1456). Can you please try to replace the pngs in this PR with their respective svg source?

[travkin79]

Hi @FlorianKroiss,

Thank you for your improvement suggestions and welcome in the LSP4E team!

@travkin79 I recently made some changes so that LSP4E uses mostly svg images (#1456). Can you please try to replace the pngs in this PR with their respective svg source?

I think, using SVG images has many advantages when handling various screen sizes, DPI, and zoom levels. Unfortunately, I don't have any SVG version of the images in this PR. They were all taken from other Eclipse plug-ins (like JDT, CDT, etc.) using the Plug-in Image Browser view. But I did not check yet if there are SVG versions available in the plug-ins' git repos. I'll try to find out.

[travkin79]

I'd like to go on with finalizing this PR. The prototype creates overlay icons in the icon corners for various additional details if they are provided by the language server. This includes the element's visibility (public, private, etc.), if it is static, deprecated, etc. Here is an example:

I'd like to hear your opinion on using overlay icons in the upper left corner of each symbol icon in contrast to JDT using differently shaped and colored symbol icons (at least for methods and fields) to express the visibility (see similar outline view examples from CDT and JDT in the first comment in discussion #977 and see the table below). Is it ok to use a visibility overlay icon (in the top left corner) for each symbol kind image instead of creating unique images for each combination of symbol kind and visibility, similar to JDT?

Icons used in JDT (classes look like overlay icons are used, methods and field have visibility-dependent icons):

Element	original icon	private	package visible (default)	protected	public
Class (same for interfaces and enums)
Method	-
Field	-

[FlorianKroiss]

I think that using an overlay in the top-left to indicate visibility is fine. It probably also makes future maintenance of this implementation easier.

I think that creating unique icons for each combination of symbolkind and visibilitytag is a lot of work, and also harder to maintain in the long run. The only benefit that this would get you, is that you free up the left corner to potentially display one more tag.

Disclaimer: I'm not a UI/UX person, so maybe my ideas here at not the best design decisions from a end-user perspective 😅

[FlorianKroiss]

PR overall looks good. My comments are mostly suggestions/nitpicks but nothing which must be addressed.

If you have any questions on my comments, feel free to ask,

[travkin79]

Hi @FlorianKroiss,

PR overall looks good. My comments are mostly suggestions/nitpicks but nothing which must be addressed.

If you have any questions on my comments, feel free to ask,

Thank you very much for your detailed and helpful review. I incorporated most of your suggestions. IMHO your suggestions definitely improve the code quality. I'm just not yet sure about the cache key that we should use.

Nevertheless, before finishing this PR, I think, we have to wait for the next LSP4J release, where the new symbol tags will be added which I use in this PR.

[travkin79]

Hi @FlorianKroiss,

Thank you for the feedback.

I think that using an overlay in the top-left to indicate visibility is fine. It probably also makes future maintenance of this implementation easier.

I think that creating unique icons for each combination of symbolkind and visibilitytag is a lot of work, and also harder to maintain in the long run. The only benefit that this would get you, is that you free up the left corner to potentially display one more tag.

Disclaimer: I'm not a UI/UX person, so maybe my ideas here at not the best design decisions from a end-user perspective 😅

I talked about that with my team mates. Some of them find it useful to reduce the number of overlay icons per document symbol in order to reduce "pollution" in the UI / make the icon overlays less distracting. Our idea was not to create different icons for each combination of a symbol kind and visibility tags, but only for fields and method and their combinations with visibility tags (i.e. 8 icons in total), since fields and methods are the elements that we see in views like the outline view and call hierarchy view most often.

We might also think about not showing certain details from symbol tags at all, maybe removing declaration and definition overlay icons. But we're not sure about that.

[rubenporras]

@travkin79 , I was planning to do a release for LSP4E tomorrow. Or would you like for me to wait a bit longer so that this PR can go in? Is your idea to finish this one quickly?

[travkin79]

Hi @rubenporras,

Thank you very much for asking. Indeed, I would like to finish this PR quite soon, but I think, my PR might still need some final review. In addition, I don't want to unnecessarily delay any release.

Could we just create another LSP4E release a few days / weeks later? In that case you could just go on with preparing the LSP4E release tomorrow.

[rubenporras]

Thanks. Let us then create a release now and another one later, that is something quick to do.

[FlorianKroiss]

LGTM, only a few minor points.

Do plan to still work on the TODOs in this PR or address them later?

Btw: I think this will be a great addition to the LSP and LSP4E :)

[travkin79]

Hi @FlorianKroiss,

Do plan to still work on the TODOs in this PR or address them later?

I was working on implementing visibility-dependent symbol icons for fields and methods, similar to JDT. In most cases that leads to less overlay icons used in the outline view (and call hierarchy / type hierarchy views), in other cases that allows us to show one more detail (SymbolTag) in overlay icons.

Instead of having many overlay icons in the outline view like here

we'll now have an outline view like here (field and method visibility is represented by the symbol icon instead of an additional overlay icon, declaration and definition details are omitted)

Now, we have the following visibility representing icons in LSP4E:

	fall-back icon	private	package-visible	protected	public	internal	file-visible
Field
Method
Constructor		*	*	*	*	*	*
Overlay icon	-

* with a "C" overlay icon in the upper right corner similar to the fall-back icon

While implementing that, I had to adapt the cache's HashMap key a little. In addition, when thinking about customizing the label providers' behavior for certain languages (e.g. showing a destructor overlay icon in CDT LSP), I decided to separate label-provider-related code from the LSPImages class and made most methods customizable by making them overridable (protected and non-static). All label providers in LSP4E are now sub-classes of a new abstract label provider class.

I hope, you and maybe other reviewers consider the changes as an improvement.

Now, I think, I'll clean up the code a little and then the PR should be ready.

[rubenporras]

@travkin79 , shall I merge?

[travkin79]

Hi @rubenporras,
I'm not sure if someone else like @jonahgraham would like to do a review before merging. In addition, I'd like to check a few things, but I don't have access to my computer at the moment. Maybe, we should wait a few days.

[jonahgraham]

I have only done a cursory review, @travkin79 it looks great to me and well done on persuing this - it looks near completion now with the changes adopted in llvm + lsp4j. I hope that once this is merged it can soon make it officially into LSP.

[travkin79]

Thanks @jonahgraham,
I'm looking forward to releasing this feature in LSP4E and getting the LSP specification PR merged, too.

Unfortunately, I'm still waiting for getting access to my workstation again, where I'd like to run final tests. (After incorporating the last feedback from @FlorianKroiss I was not able to test it properly on my workstation where I have set up a patched clangd version for this PR). As soon as I'm able to run the final tests, we can merge this PR.