GH-858: [LS] Added support for the hierarchical document symbols. #862

kittaakos · 2018-09-06T12:59:44Z

Signed-off-by: Akos Kitta [email protected]

cdietrich · 2018-09-06T13:19:20Z

org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/symbol/IDocumentSymbolService.xtend

+@ImplementedBy(Noop)
+interface IDocumentSymbolService {
+
+	def List<Either<SymbolInformation, DocumentSymbol>> getSymbols(Document document, XtextResource resource,


this is one of the apis where i am not sure if i like it. e.g. this could not be used to produce an eclipse outline without using lsp4j

I am not against modifying, adjusting it, but I would like to understand what is wrong with the lsp4j model API? If I understood you correctly, you would like to introduce another API in xtext-core that would be used by xtext-eclipse and will be transformed into the IOutlineNode, right? But xtext-eclipse already has lsp4j as a dependency, right? Then why to add another API?

no it does not thats the point

=> for api that might be useful toi be reused in eclipse it might help to have a lsp4j independent api e.g. as we do it for content assist

=> we should decide that at per api level if it can be usefully used in more than the lsp4j context. currently we are inconsistent with that cc @tivervac

i can live with that if we dont offer this as api to be used in eclipse

i can live with that if we dont offer this as api to be used in eclipse

Should I remove the @since 2.16 from the source? How should we proceed with this PR?

Although the PR is not super urgent, I would like to get this done if you do not have any objections. Thanks!

One thing that we have to keep in mind before we merge is the LSP4J API. See here eclipse-lsp4j/lsp4j#252.

we marked provisional api as @Beta before. correct @szarnekow @svenefftinge ?

Yes, we did that in some places.

cdietrich · 2018-09-06T13:21:11Z

...se.xtext.ide/src/org/eclipse/xtext/ide/server/symbol/HierarchicalDocumentSymbolService.xtend

+		val rootSymbols = newArrayList;
+		val itr = getAllContents(resource);
+		while (itr.hasNext) {
+			operationCanceledManager.checkCanceled(cancelIndicator);


i wonder if we can build a lsp4j independent tree and transform it to a lsp4j conformant later on

Sure, we can do this but let's move this conversation to here. I think they are related.

kthoms · 2018-09-19T14:13:25Z

org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/DocumentExtensions.xtend

@@ -75,6 +75,12 @@ class DocumentExtensions {
 		return resource.newLocation(textRegion)
 	}

+	def Location newFullLocation(EObject object) {


missing @since 2.16 here

kthoms · 2018-09-19T14:14:25Z

org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/LanguageServerImpl.xtend

@@ -406,6 +408,22 @@ import static org.eclipse.xtext.diagnostics.Severity.*
 		]
 	}

+	protected def IDocumentSymbolService getIDocumentSymbolService(IResourceServiceProvider serviceProvider) {


also add @since for new protected methods in public API packages

kthoms · 2018-09-19T14:15:51Z

org.eclipse.xtext.testing/xtend-gen/org/eclipse/xtext/testing/DocumentSymbolConfiguraiton.java

@@ -31,11 +33,11 @@ public void setExpectedSymbols(final String expectedSymbols) {
  }

  @Pure
-  public Procedure1<? super List<? extends SymbolInformation>> getAssertSymbols() {
+  public Procedure1<? super List<Either<SymbolInformation, DocumentSymbol>>> getAssertSymbols() {


add @since

kthoms · 2018-09-19T14:15:59Z

org.eclipse.xtext.testing/xtend-gen/org/eclipse/xtext/testing/DocumentSymbolConfiguraiton.java

    return this.assertSymbols;
  }

-  public void setAssertSymbols(final Procedure1<? super List<? extends SymbolInformation>> assertSymbols) {
+  public void setAssertSymbols(final Procedure1<? super List<Either<SymbolInformation, DocumentSymbol>>> assertSymbols) {


add @since

kittaakos · 2018-09-19T14:19:09Z

Thanks for the feedback on this PR. One general question, shall we use @Beta or @since. Thanks!

kthoms · 2018-09-19T15:18:29Z

I'd say a class once made public available can't be @Beta, and adding API methods can't either. And if I see this correctly, @Beta is only used on type level in the codebase.

So adding API methods to API classes can't be beta, thus we have to be carefully.

cdietrich · 2018-09-19T15:19:38Z

No Beta says available but not API

kthoms · 2018-09-19T15:19:48Z

org.eclipse.xtext.testing/src/org/eclipse/xtext/testing/AbstractLanguageServerTest.xtend

@@ -682,7 +702,7 @@ class DocumentHighlightConfiguration extends TextDocumentPositionConfiguration {
 @Accessors
 class DocumentSymbolConfiguraiton extends TextDocumentConfiguration {


Typo in class name :(

cdietrich · 2018-09-19T15:25:20Z

The main problem is that the whole of lsp and how ms deals with it and how Java works vs typescript works in incompatible with eclipse api rules. So marking as beta can give users a hint on that

kittaakos · 2018-09-19T15:27:34Z

So marking as beta can give users a hint on that

👍

Can we agree that I should get rid of @since and use @Beta instead?

kthoms · 2018-09-19T15:30:09Z

So API rule incompatibility is blocking this change? If so, please veto @cdietrich .

cdietrich · 2018-09-19T15:31:12Z

No it’s new code. Simply mark it as beta

kittaakos · 2018-09-19T15:32:11Z

No it’s new code. Simply mark it as beta

This.

Thanks, guys! I apply the changes soon.

Closes: eclipse#858. Signed-off-by: Akos Kitta <[email protected]>

kittaakos · 2018-09-19T16:01:22Z

I made the changes.

kittaakos · 2018-09-20T08:21:26Z

Can someone please look into (or restart) the build, I do not have problems running it locally:

git rev-parse --short HEAD && git clean -fxdq && ./gradlew -q clean build
65b1f89a6
Note: Some input files use or override a deprecated API.
...
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
Akoss-MacBook-Pro:xtext-core akos.kitta$

Thank you!

kittaakos · 2018-09-25T06:37:39Z

If this PR is good to merge, please push the button. Thank you!

cdietrich requested a review from spoenemann September 6, 2018 13:05

cdietrich added this to the Release_2.16 milestone Sep 6, 2018

cdietrich reviewed Sep 6, 2018

View reviewed changes

spoenemann removed their request for review September 14, 2018 07:12

svenefftinge approved these changes Sep 19, 2018

View reviewed changes

kthoms reviewed Sep 19, 2018

View reviewed changes

eclipseGH-858: [LS] Added support for the hierarchical document symbols.

65b1f89

Closes: eclipse#858. Signed-off-by: Akos Kitta <[email protected]>

kittaakos force-pushed the GH-858 branch from 0d49baa to 65b1f89 Compare September 19, 2018 15:59

svenefftinge merged commit 9e46f30 into eclipse:master Sep 25, 2018

kittaakos deleted the GH-858 branch September 25, 2018 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-858: [LS] Added support for the hierarchical document symbols. #862

GH-858: [LS] Added support for the hierarchical document symbols. #862

kittaakos commented Sep 6, 2018

cdietrich Sep 6, 2018

kittaakos Sep 6, 2018

cdietrich Sep 6, 2018

cdietrich Sep 6, 2018

cdietrich Sep 6, 2018

cdietrich Sep 7, 2018

svenefftinge Sep 7, 2018

kittaakos Sep 14, 2018

cdietrich Sep 14, 2018

svenefftinge Sep 14, 2018

cdietrich Sep 6, 2018

kittaakos Sep 6, 2018

kthoms Sep 19, 2018

kthoms Sep 19, 2018

kthoms Sep 19, 2018

kthoms Sep 19, 2018

kittaakos commented Sep 19, 2018

kthoms commented Sep 19, 2018

cdietrich commented Sep 19, 2018

kthoms Sep 19, 2018

cdietrich commented Sep 19, 2018

kittaakos commented Sep 19, 2018

kthoms commented Sep 19, 2018

cdietrich commented Sep 19, 2018

kittaakos commented Sep 19, 2018

kittaakos commented Sep 19, 2018

kittaakos commented Sep 20, 2018

kittaakos commented Sep 25, 2018

		@@ -682,7 +702,7 @@ class DocumentHighlightConfiguration extends TextDocumentPositionConfiguration {
		@Accessors
		class DocumentSymbolConfiguraiton extends TextDocumentConfiguration {

GH-858: [LS] Added support for the hierarchical document symbols. #862

GH-858: [LS] Added support for the hierarchical document symbols. #862

Conversation

kittaakos commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kittaakos commented Sep 19, 2018

kthoms commented Sep 19, 2018

cdietrich commented Sep 19, 2018

Choose a reason for hiding this comment

cdietrich commented Sep 19, 2018

kittaakos commented Sep 19, 2018

kthoms commented Sep 19, 2018

cdietrich commented Sep 19, 2018

kittaakos commented Sep 19, 2018

kittaakos commented Sep 19, 2018

kittaakos commented Sep 20, 2018

kittaakos commented Sep 25, 2018