GH-715: Implemented the semantic highlighting support. by kittaakos · Pull Request #746 · eclipse-jdtls/eclipse.jdt.ls

kittaakos · 2018-08-07T11:33:27Z

This PR contains the following change:

Enables the semantic highlighting for the JDT LS.
Switches from LSP4J 0.4.0 to 0.5.0.

Static:

Member vs. Parameter:

Deprecated:

Auto (un)boxing:

Closes #715.

Signed-off-by: Akos Kitta kittaakos@typefox.io

fbricon · 2018-08-07T13:29:56Z

@kittaakos thanks for the PR that looks promising! I'll review as soon as I can

kittaakos · 2018-08-09T14:48:18Z

I will shrink the size of this PR once eclipse-lsp4j/lsp4j#223 will be accepted and merged.

snjeza · 2018-08-10T23:10:07Z

test this please

snjeza · 2018-08-10T23:59:33Z

I will shrink the size of this PR once eclipse-lsp4j/lsp4j#223 will be accepted and merged.

@kittaakos eclipse-lsp4j/lsp4j#223 has been merged.

snjeza · 2018-08-12T19:43:27Z

The target platform is built from a fork of the original LSP4J source to enable the semantic highlighting capabilities.

@kittaakos Do I need to use any particular versions of VS Code?

kittaakos · 2018-08-14T07:02:17Z

Do I need to use any particular versions of VS Code?

Currently, VS Code does not expose a theming API. See here. You can try it out in Eclipse Theia; it already uses the forked JDT language server.

snjeza

It works fine in Eclipse Theia.

fbricon · 2018-08-20T15:48:38Z

Can we rename the org.eclipse.jdt.ls.core.internal.semantichighlight package to org.eclipse.jdt.ls.core.internal.highlighting? semantichighlight burns my eyes :-)

kittaakos · 2018-08-21T06:11:55Z

semantichighlight burns my eyes :-)

Sure. I do the updates here, once there is a new release of lsp4j. So that I can get rid of a bunch of classes, custom target platform files and so on. The release will happen either this or next week.

kittaakos · 2018-08-27T14:12:10Z

I did the followings:

Switched to 0.5.0.M1 of LSP4J. (Adapted the codeAction and documentSymbol API to compile.)
Got rid of a bunch of obsolete utility classes and the custom target platform.
Renamed org.eclipse.jdt.ls.core.internal.semantichighlight to org.eclipse.jdt.ls.core.internal.highlighting.

fbricon · 2018-08-28T14:12:39Z

@kittaakos could you extract the launcher/debugger stuff in a separate PR please, it's orthogonal to the issue and will be faster to get in

kittaakos · 2018-08-28T14:21:26Z

could you extract the launcher/debugger stuff in a separate PR please

Sure.

kittaakos · 2018-08-30T14:41:22Z

could you extract the launcher/debugger stuff in a separate PR please

@fbricon, I got rid of the non-highlighting related changes.

kittaakos · 2018-09-05T06:45:21Z

Are there any updates, feedback on this PR?

svenefftinge · 2018-09-12T07:12:07Z

Would be really nice to get this merged. It works nicely with Theia now.
And we currently need to ship Theia with a fork.

fbricon

Except for the copyright headers to fix, I only have a few minor remarks. But that's a pretty good patch overall.

Anyways, we'll have to open a CQ for that, which may take a few days to get approved.

fbricon · 2018-09-12T13:55:20Z

        <location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="true" type="InstallableUnit">
-            <unit id="org.eclipse.lsp4j.sdk.feature.group" version="0.4.0.v20180425-1137"/>
-            <repository location="http://download.eclipse.org/lsp4j/updates/releases/"/>
+            <unit id="org.eclipse.lsp4j.sdk.feature.group" version="0.5.0.v20180822-0752"/>


TP is pointing to 0.5.0.M1 in master

fbricon · 2018-09-12T13:58:29Z

@@ -0,0 +1,245 @@
+/*******************************************************************************
+ * Copyright (c) 2018 Red Hat Inc. and others.


s/Red Hat Inc./TypeFox/g

fbricon · 2018-09-12T14:00:29Z

-				IDocument document = JsonRpcHelpers.toDocument(unit.getBuffer());
-				edit.apply(document, TextEdit.NONE);
+
+				// Avoid any computation iff the `SemanticHighlightingService#isEnabled` is `false`.


Just for completeness with the source:
"[...]In logic and related fields such as mathematics and philosophy, if and only if (shortened iff) is a biconditional logical connective between statements.[...]"

Since it was not obvious whether it is a typo or not, I apply the recommended change. Thanks for the feedback.

fbricon · 2018-09-12T14:03:47Z

@@ -27,6 +27,7 @@
 import java.util.concurrent.Executors;


lsp4j 0.5.0 compat changes already available in master

I do not understand this. Wha do you mean by this comment?

Just that those changes aren't necessary anymore, as they ve already been taken care of. Just needs to rebase against master

fbricon · 2018-09-12T14:12:34Z

@@ -0,0 +1,72 @@
+/*******************************************************************************
+ * Copyright (c) 2018 TypeFox and others.


if file was copied from jdt, then it should retain the original copyright from IBM.

Add Contributors section:

* * Contributors: * IBM Corporation - initial API and implementation * TypeFox - port to jdt.ls

fbricon · 2018-09-12T21:37:38Z

+			infos.put(line, new SemanticHighlightingTokens.Token(character, length, scope));
+		}
+		//@formatter:off
+		return FluentIterable.from(infos.asMap().entrySet())


Java Streams instead?

fbricon · 2018-09-12T21:39:06Z

+import org.eclipse.lsp4j.VersionedTextDocumentIdentifier;
+import org.eclipse.lsp4j.util.SemanticHighlightingTokens;
+
+import com.google.common.base.Preconditions;


eclipse Assert?

fbricon · 2018-09-12T21:44:32Z

@@ -0,0 +1,117 @@
+/*******************************************************************************
+ * Copyright (c) 2018 TypeFox and others.


if file was copied from jdt, then it should retain the original copyright from IBM.

Add Contributors section:

* * Contributors: * IBM Corporation - initial API and implementation * TypeFox - port to jdt.ls

fbricon · 2018-09-12T21:48:02Z

+
+	public boolean isSemanticHighlightingSupported() {
+		TextDocumentClientCapabilities textDocument = capabilities.getTextDocument();
+		if (textDocument == null) {


use v3supported instead

kittaakos · 2018-09-13T08:17:09Z

Thank you for the review, @fbricon. I have updated this PR based on your feedback.

fbricon · 2018-09-13T23:21:48Z

Opened CQ https://dev.eclipse.org/ipzilla/show_bug.cgi?id=17560

fbricon · 2018-09-28T23:15:59Z

@jjohnstn is everything available in jdt.core now so that we can drop the copied files?

Closes eclipse-jdtls#715. Signed-off-by: Akos Kitta <kittaakos@typefox.io>

fbricon · 2018-10-19T18:56:40Z

test this please

jjohnstn · 2018-10-19T19:30:13Z

@jjohnstn is everything available in jdt.core now so that we can drop the copied files?

Still in review. https://git.eclipse.org/r/#/c/129807/

kittaakos changed the title ~~[WIP] Initial changes to support semantic highlighting~~ Initial changes to support semantic highlighting Aug 7, 2018

kittaakos force-pushed the GH-715 branch from 41a6186 to 6cfca4d Compare August 7, 2018 13:15

kittaakos changed the title ~~Initial changes to support semantic highlighting~~ GH-715: Implemented the semantic highlighting support. Aug 7, 2018

fbricon requested a review from snjeza August 10, 2018 22:59

snjeza reviewed Aug 15, 2018

View reviewed changes

kittaakos force-pushed the GH-715 branch 3 times, most recently from 364f971 to c8ce2b8 Compare August 27, 2018 14:02

kittaakos force-pushed the GH-715 branch 4 times, most recently from 720550d to fae85af Compare August 30, 2018 14:40

kittaakos mentioned this pull request Sep 11, 2018

Java: Consume che-ls-jdt as jdt.ls extension eclipse-theia/theia#2849

Closed

fbricon requested changes Sep 12, 2018

View reviewed changes

kittaakos force-pushed the GH-715 branch from fae85af to 86c2f8f Compare September 13, 2018 08:12

fbricon added the CQ required label Sep 13, 2018

kittaakos force-pushed the GH-715 branch from 86c2f8f to b7ffc74 Compare September 20, 2018 12:19

eclipse-jdtlsGH-715: Implemented the semantic highlighting support.

948f1cf

Closes eclipse-jdtls#715. Signed-off-by: Akos Kitta <kittaakos@typefox.io>

kittaakos force-pushed the GH-715 branch from b7ffc74 to 948f1cf Compare October 12, 2018 06:10

fbricon approved these changes Oct 19, 2018

View reviewed changes

fbricon merged commit b834f12 into eclipse-jdtls:master Oct 19, 2018

fbricon mentioned this pull request Oct 19, 2018

Delete copied code for Semantic Highlighting #833

Closed

kittaakos deleted the GH-715 branch October 23, 2018 06:11

yyoncho mentioned this pull request Oct 31, 2018

Support semantic highlighting emacs-lsp/lsp-java#72

Closed

kittaakos mentioned this pull request Nov 9, 2018

GH-3397: Implemented the HTTP-based authentication for Git in Electron. eclipse-theia/theia#3426

Merged

4 tasks

Eskibear mentioned this pull request Jan 20, 2021

Remove legacy Semantic Highlighting implementation #1649

Merged

		@@ -0,0 +1,245 @@
		/*******************************************************************************
		* Copyright (c) 2018 Red Hat Inc. and others.

		@@ -0,0 +1,72 @@
		/*******************************************************************************
		* Copyright (c) 2018 TypeFox and others.

		@@ -0,0 +1,117 @@
		/*******************************************************************************
		* Copyright (c) 2018 TypeFox and others.

Uh oh!

Conversation

kittaakos commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fbricon commented Aug 7, 2018

Uh oh!

kittaakos commented Aug 9, 2018

Uh oh!

snjeza commented Aug 10, 2018

Uh oh!

snjeza commented Aug 10, 2018

Uh oh!

snjeza commented Aug 12, 2018

Uh oh!

kittaakos commented Aug 14, 2018

Uh oh!

snjeza left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbricon commented Aug 20, 2018

Uh oh!

kittaakos commented Aug 21, 2018

Uh oh!

kittaakos commented Aug 27, 2018

Uh oh!

fbricon commented Aug 28, 2018

Uh oh!

kittaakos commented Aug 28, 2018

Uh oh!

kittaakos commented Aug 30, 2018

Uh oh!

kittaakos commented Sep 5, 2018

Uh oh!

svenefftinge commented Sep 12, 2018

Uh oh!

fbricon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kittaakos commented Sep 13, 2018

Uh oh!

fbricon commented Sep 13, 2018

Uh oh!

fbricon commented Sep 28, 2018

Uh oh!

fbricon commented Oct 19, 2018

Uh oh!

jjohnstn commented Oct 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

kittaakos commented Aug 7, 2018 •

edited

Loading

snjeza left a comment •

edited

Loading