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

Semantic tokens from LS available in the Java editor #1161

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

BoykoAlex
Copy link
Contributor

Based on the latest addition of semantic tokens extension for Java editor the support is added for having semantic tokens from the language server in the Java editor.
There is a preference add to turn off semantic tokens support in the Java editor. The preference is defined but the UI is not and is expected to come from 3rd party plugins using this functionality (i.e. spring boot ls plugin) otherwise it's unclear where to fit this preference in the UI at the moment

@@ -9,7 +9,7 @@ org.eclipse.jdt.core.compiler.annotation.nonnullbydefault.secondary=
org.eclipse.jdt.core.compiler.annotation.notowning=org.eclipse.jdt.annotation.NotOwning
org.eclipse.jdt.core.compiler.annotation.nullable=org.eclipse.jdt.annotation.Nullable
org.eclipse.jdt.core.compiler.annotation.nullable.secondary=
org.eclipse.jdt.core.compiler.annotation.nullanalysis=enabled
org.eclipse.jdt.core.compiler.annotation.nullanalysis=disabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you not prefer to follow the other projects in this settings? What is the reason for disabling it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would... but I'm lost with JDT Null check annotations... I see that LSP4E plugin project has lots of errors coming from JDT related to Null checks but the maven build goes through. The lsp4e.jdt project also has a bunch of same origin errors however maven build fails with compilation errors for the JDT null check issues... I wasn't able to figure out where the difference between lsp4e and lsp4e.jdt projects comes from... Disabling the null checking in the preference solved it for the lsp4e.jdt for me...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that I need to add @NonNull annotations in JDT UI in ISemanticTokensProvider and the plugin there depend on org.eclipse.jdt.annotation plugin... Otherwise without ignoring null violation analysis i can't get it to compile :-\

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long delay.

You can annotate libraries externally with annotations, in LSP4E we use the library https://github.com/vegardit/no-npe instead of doing it directly in our repo. You should be able to fix it by adding annotations in https://github.com/vegardit/no-npe/tree/main/libs/eea-eclipse-platform/src/main/resources/org/eclipse

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I didn't see eaa-lsp4e in there... Or did you mean I should add lsp4e in there?

@@ -9,7 +9,7 @@
* Contributors:
* - Martin Lippert (Pivotal Inc.) - Initial implementation
*******************************************************************************/
package org.eclipse.lsp4e.jdt;
package org.eclipse.lsp4e.jdt.internal;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we not rather do this move in different PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can do that. I'll revert moving classes. I did that because had to export a package and didn't mean to export more than needed... Wonder if perhaps we could have a preference page for LSP4E JDT bit and expose the preference I'm introducing on that new preference page... This way we can completely avoid exposing anything as API...

@@ -13,7 +13,7 @@
<location includeAllPlatforms="false" includeConfigurePhase="true" includeMode="planner" includeSource="true" type="InstallableUnit">
<unit id="org.eclipse.ui.tests.harness" version="0.0.0"/>
<unit id="org.eclipse.sdk.ide" version="0.0.0"/>
<repository location="https://download.eclipse.org/eclipse/updates/latest/"/>
<repository location="https://download.eclipse.org/eclipse/updates/4.35-I-builds/"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct in guessing that this change means that the PR cannot be merged before 4.35 has been released?

Copy link
Contributor Author

@BoykoAlex BoykoAlex Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it'd need 4.35. Are LSP4E releases come out as new Eclipse being released? When does LSP4E adopt changes in the platform? When the new Eclipse is out already? Or a few months/weeks before new Eclipse is out?

@@ -14,4 +17,9 @@ Require-Bundle: org.eclipse.core.runtime;bundle-version="3.12.0",
org.eclipse.lsp4e;bundle-version="0.18.13",
org.eclipse.jdt.core;bundle-version="3.20.0",
org.eclipse.jdt.ui;bundle-version="3.20.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version should be bumped to the 1st version that introduces org.eclipse.jdt.ui.text.java.ISemanticTokensProvider

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will do that

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

Successfully merging this pull request may close these issues.

3 participants