feat: make diagnostic marker messages configurable

[sebthom]

This PR extends the IMarkerAttributeComputer with a method computeMarkerMessage(Diagnostic) allowing language server providers to customize the message to be used for markers.

[rubenporras]

Thanks for the follow-up PR.

Should the default not be the empty String or still need enablement with some configuration property?

So far we only know about one report where this feature is desired, and we know nothing about the rest of the user base.

[sebthom]

@mickaelistria WDYT? I am leaning towards making/keeping this the default and let providers of language server with extraordinary long diagnostic codes overwrite the default if required.

[sebthom]

or we truncate or omit codes that exceed a certain length.

[travkin79]

Hi @sebthom,
it's great to see this PR, since I was working more or less on the same topic.

For CDT LSP, I'd like to add problem issue codes and problem description URLs (that the language server receives from clang-tidy, e.g. something like readability-magic-numbers and https://clang.llvm.org/extra/clang-tidy/checks/readability/magic-numbers.html) to the problems view and / or to the markers view. My idea was to add new columns Code and URL to these views. But I came across an extension issue and was working an a PR to fix it.

As long as that fix is not available, using the code as message suffix is a good idea. But it would be even better if the diagnostics code and maybe other details could be presented in separate columns in the problems / marker views. Clients could add these or even more marker fields to these views, depending on the marker type and language (or LS capabilities) and the user could show or hide these columns on demand.

What do you think?

[sebthom]

I think it would be a nice addition but not as part of this PR. If it doesn't exist already please open a dedicated issue.

[rubenporras]

Hi @travkin79 ,

great news. I think that to add new columns Code and URL to these views is the right solution to the problem (and since we do not have that, we built our own views for our product (as I commented). Adding the information as a textual string looks like a poor man solution instead.

@sebthom: Since we have better solution already prepared for as 2024-12, it would be really be better if this feature would be opt-in. This way we can remove it later when we can add the attributes to the view and most users will not see a change in the way the marker messages are presented, only an improvement in the view.

[travkin79]

Hi @sebthom,

I think it would be a nice addition but not as part of this PR.

Of course, not as part of this PR. 😄

If it doesn't exist already please open a dedicated issue.

It seems that issue #185 has a very similar goal. But it does not explicitly mention adding new columns "Code" and "Details URL" (or similar) to the problems view or to the marker view. Thus, I created a new issue #1070.

[sebthom]

@rubenporras I honestly don't see the point in making this an opt-in. realistically this will mean it will not get implemented for any LS in the near future. I still believe it is very valuable to see the actual diagnostic code directly in the hover message and not a distant list with unrelated messages. Esp. if the error code can be used to e.g. suppress warnings like in shellcheck using # shellcheck disable=SC2119,SC2120 or in Dart using // ignore: non_constant_identifier_names. Then you can simply F2 on the marker message and copy/paste the code into the comment.

[travkin79]

@rubenporras, could it also be an opt-out?

I think, in most cases the diagnostics code will not be too long.

[sebthom]

Currently it is an opt-out. But as I said we could also change the implementation to omit codes if they are too long (whatever the definition of too long would be).

@rubenporras I actually am very curious how long the codes are in that language server you are working on (and why they are so long).

[travkin79]

@sebthom,
Could you also fix the hover dialog's size (maybe in another PR)? In the following example it's to small.

If extend it to the actual size it becomes the following.

[rubenporras]

My reasoning for an opt-in is that if you have the attribute in the markers view, you can also copy from there, so the information is redundant, and it makes the hover bigger, which might be a problem. As a side note, as far as I can tell, the JDT does not add the code to a message, and I think VS Code also does not do it. There might be reasons for it :)

In any case, I can customize the behaviour, so if I am the only one that does not like it, you can merge it. Maybe @mickaelistria has an opinion on the usability?

Regarding your question: Our codes are long because we bundle a lot of plug-ins for many DSLs (company made), to assure uniqueness we have the <"fully qualified name of the plug-in">. .

[sebthom]

Ok, there is no need to rush this PR and incorporate something that ends up being dead code in a few months. We can also wait if someone implements the problems view extra column and see if it sufficient.

[rubenporras]

@sebthom, Could you also fix the hover dialog's size (maybe in another PR)? In the following example it's to small.

If he knows how to fix it or has time to look into it, by all means. Otherwise I am already investigating this since a couple of days. If @sebthom are interested into working on it, I can share what I have found so far.

@travkin79 , also another feature that seems to be broken for me (and I believe has the same root cause), is that if you apply the quicfix by clicking in the hyperlink, the hover does not disappear. Could you confirm this for you environment as well?

[sebthom]

@rubenporras I have no experience with fixing hover heights so far and I am not working on that. so if you find a solution please contribute :)

[travkin79]

Hi @rubenporras,

if you apply the quickfix by clicking in the hyperlink, the hover does not disappear. Could you confirm this for you environment as well?

Yes, it's the same issue in my environment. In addition, I cannot select the text from the diagnostics message in the hover dialog in the area highlighted in the following screenshot.

[travkin79]

Hi @sebthom,
Sorry for being a little off topic, but I don't know where to ask that:
I have a missing dependency in LSP4E after pulling from main branch (see screenshot). I think, you're most familiar with null checks. :-) Is there an update site that I can use to install the missing bundle to my environment?

[sebthom]

it comes from maven central. the maven location is defined in the target platform. maybe you need to update/refresh/reload the target

[travkin79]

I didn't use the target file, since I am testing a few additional plug-ins in the same workspace. Is there a simple way to add that library to Eclipse, maybe similar to the libs from orbit update sites?

[sebthom]

I didn't use the target file, since I am testing a few additional plug-ins in the same workspace. Is there a simple way to add that library to Eclipse, maybe similar to the libs from orbit update sites?

I have no idea. I only work with target files.

[travkin79]

I have no idea. I only work with target files.

Well, then it might be a feature request for your project. 😄
I found this on contributing a library to orbit simultaneous releases:
https://github.com/eclipse-orbit/orbit/blob/main/Add-bundle-in-5-minutes.md
I'd appreciate it if you would consider contributing your library to the orbit update sites.

[rubenporras]

I have no idea. I only work with target files.

Well, then it might be a feature request for your project. 😄 I found this on contributing a library to orbit simultaneous releases: https://github.com/eclipse-orbit/orbit/blob/main/Add-bundle-in-5-minutes.md I'd appreciate it if you would consider contributing your library to the orbit update sites.

Probably you need install the maven integration plugin (M2E) for the target file to work properly.

[rubenporras]

Ok, there is no need to rush this PR and incorporate something that ends up being dead code in a few months. We can also wait if someone implements the problems view extra column and see if it sufficient.

I will do most probably a release of LSP4E before that happens, given that it will need to wait for 2024-12, and since the PR that adds the code to the message is merged, I need some kind of configuration.

I would say I merge this PR tomorrow if no-one disagrees, as it is an improvement over the existing code in master in any case.

[travkin79]

Probably you need install the maven integration plugin (M2E) for the target file to work properly.

I have M2E installed. As soon as I activate the target platform, the issue disappears, but my other bundles do not compile anymore. That's why I need the library somewhere in Eclipse as an OSGi bundle. But since it is not required at runtime, I can work this way anyway.

Well, then it might be a feature request for your project. 😄
I found this on contributing a library to orbit simultaneous releases:
https://github.com/eclipse-orbit/orbit/blob/main/Add-bundle-in-5-minutes.md
I'd appreciate it if you would consider contributing your library to the orbit update sites.

@sebthom, I found a simpler solution. No need for putting your lib to orbit update sites (at least not for me). I just added the Maven dependency to a customized target definition in the preferences (without an explicit target definition file). That works for me.

[sebthom]

@mickaelistria based on the recent discussion, what do you think about this PR? should we merge it as is? make it opt-in or wait for another solution?

[mickaelistria]

If you think it's good to merge, and @rubenporras (and is "counter" use-case) agrees this PR is good as he did in #1066 (comment) , I don't see any reason why not merging it.