CallHierarchyService Plugin API #3765 #6924

tsmaeder · 2020-01-20T15:06:05Z

Signed-off-by: Thomas Mäder tmader@redhat.com

What it does

Introduces call hierarchy API into theia.d.ts (see https://code.visualstudio.com/api/references/vscode-api#languages). Support for SymbolTag has been omitted, since we do not support it for other features.
Only necessary changes have been done the UI. In particular, no support for "outgoing calls" has been added. In consequence (for lack of easy testing) the codepath producing "outgoing calls" has not been added.

Since the VS Code API is based on LanguageSelector, it was easier to break the API for CallHierarchyService. I believe there aren't any implementors outside of the typescript extension.

How to test

Build one of the standard examples, removing Java support from the package.json. Then add the latest vscode-java extension (https://github.com/redhat-developer/vscode-java/releases) to /plugins. Open a typescript project to test the extension API, open a Java project to test the VS Code API.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

packages/callhierarchy/src/browser/callhierarchy-service.ts

packages/plugin-ext/src/plugin/languages/call-hierarchy.ts

benoitf · 2020-01-21T08:19:35Z

wanted to test with https://github.com/rmi22186/vscodeextension/blob/926c5c045e3274a0572f4a07ca0adf6d2fdbd949/call-hierarchy-sample/README.md but failed to do (I think there is an error unrelated to this PR)

tsmaeder · 2020-01-21T11:47:09Z

wanted to test with https://github.com/rmi22186/vscodeextension/blob/926c5c045e3274a0572f4a07ca0adf6d2fdbd949/call-hierarchy-sample/README.md but failed to do (I think there is an error unrelated to this PR)

What is the error?

benoitf · 2020-01-21T13:22:35Z

error was Cannot read property 'readFile' of undefined

JanKoehnlein

LGTM

akosyakov

i've only reviewed that MS code does not spread over our codebase. Please keep in the plugin system as possible, if you need in callhierarchy move it there, but better use our implementations.

akosyakov · 2020-01-22T02:51:10Z

packages/core/src/common/language-selector/index.ts

+ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+ ********************************************************************************/
+
+export * from './language-selector';


please move ilanguage-selectro code to @theia/callhierarchy or to @theia/langauges from @theia/core, plugin system still can access it from there

core is only about the application framework and shell and anything which require to support it

I thought about where to put it and decided against "callhierarchy". It is really not directly related to call hierarchy, even in the plugin-ext usage it's not related to call hierarchy. We rely on monaco to do language-selector related stuff for other services like code completion. It's utility code related to language implementation. Maybe @theia/languages/common would fit?

ok, let's do @theia/languages/common

akosyakov · 2020-01-22T02:52:56Z

packages/core/src/common/language-selector/paths.ts

@@ -21,7 +21,7 @@
 *--------------------------------------------------------------------------------------------*/
 'use strict';


we have Path in core, please use it instead, we don't need another implementation

both path.ts and strings.ts are things that are used in code copied from VS Code as requirements for glob.ts. That is also why I copied that stuff to common/language-selector and did not put it in common. Using the Path object from core would have meant to rewrite a lot of code that his not related to this PR (I just moved it). Some of the code seems to do the same thing, but I feared there might be subtle differences that might break the glob code from VS Code.

ok, let's move it out of core to languages as well

akosyakov · 2020-01-22T02:53:39Z

packages/core/src/common/language-selector/strings.ts

@@ -14,7 +14,7 @@
 * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
 ********************************************************************************/

-// copied from https://github.com/Microsoft/vscode/blob/bf7ac9201e7a7d01741d4e6e64b5dc9f3197d97b/src/vs/base/common/strings.ts
+// based on https://github.com/Microsoft/vscode/blob/bf7ac9201e7a7d01741d4e6e64b5dc9f3197d97b/src/vs/base/common/strings.ts
 /*---------------------------------------------------------------------------------------------


there is already strings.ts file in core, could it be merged? I see a lot of utility code copied, we don't need it in core and it does not seem that all these code is used, could we remove all unused functions and types?

i've only reviewed that MS code does not spread over our codebase.

wouldn't merging the two files that go against that goal?

If we move out it form core, it would be fine with me

packages/plugin-ext/src/plugin/languages/call-hierarchy.ts

akosyakov · 2020-01-22T03:06:46Z

packages/plugin-ext/src/plugin/types-impl.ts

@@ -2009,3 +2009,36 @@ export enum WebviewPanelTargetArea {
    Right = 'right',
    Bottom = 'bottom'
 }
+export class CallHierarchyItem {


I hope this types are just copied without any modifications from VS Code codebase.

Actually, I shortened the code considerably using parameter properties. Since we already have documentation for the properties in theia.d.ts (where it's copied verbatim), no reason not to.

I hope to be able to put this file next to counterpart to VS Code and see differences like explained in this PR: https://github.com/eclipse-theia/theia/pull/6903/files#diff-905508836c18f47e2fc9e75198b465e8R21-R32

I would appreciate if we just keep types and don't change anything to simplify synching.

akosyakov · 2020-01-22T03:09:45Z

Addressed some commments by Florent Benoit …

there should not be commits fixing issues introduced by a PR: https://github.com/eclipse-theia/theia/blob/master/doc/pull-requests.md#checklist-commit-history

tsmaeder · 2020-01-22T10:54:34Z

there should not be commits fixing issues introduced by a PR: https://github.com/eclipse-theia/theia/blob/master/doc/pull-requests.md#checklist-commit-history

I usually keep the "fixup commits" until the comments are all addressed and then fold them back into the original commit. Is that acceptable?

tsmaeder · 2020-01-22T15:17:11Z

@akosyakov I have replied to a bunch of your comments and would need a response.

tsmaeder · 2020-01-24T10:32:32Z

@akosyakov I addressed your comments.
@AlexTugarev are you planning on reviewing this one?

akosyakov · 2020-01-27T02:09:12Z

@tsmaeder i will have a look this week
@benoitf @eclipse-theia/ecd-theia-committers maybe someone else can help testing it

benoitf · 2020-01-27T08:03:12Z

I tried with callhierarchy example (by fixing the example by removing stuff which is not yet implemented in theia about filesystem to initialize the txt file as it doesn't matter for this test as I can just copy/paste manually the text content)

but then it's not working:

custom vsix example in VS Code:

tsmaeder · 2020-01-27T08:31:04Z

@benoitf can you provide your fixed .visix? Also, can you provide some log output? The behaviour would be the same if the extension just never got activated.

benoitf · 2020-01-27T08:46:20Z

@tsmaeder sure, custom vsix is there:
https://gist.github.com/benoitf/8c7af767e142913bde38ad177bd58e3c/raw/ba6dc06b94e6cdf69ba045e95a2a3693eb436281/call-hierarchy-sample-0.0.1.vsix

I've removed showSampleText method and provide the sample.txt file manually https://github.com/microsoft/vscode-extension-samples/blob/master/call-hierarchy-sample/src/extension.ts#L17

in the log I can see

all-hierarchy-sample-0.0.1.vsix/extension/out/extension.js)
root INFO [hosted-plugin: 37501] Congratulations, your extension "call-hierarchy-sample" is now active!
root INFO [68304538-7366-4507-97da-69b9d6961580][vscode-samples-foo.call-hierarchy-sample]: Started plugin.

but nothing reported when clicking on call hierarchy

tsmaeder · 2020-01-27T08:49:51Z

@benoitf do you have any error messages in the browser logs?

benoitf · 2020-01-27T08:51:30Z

yes:

callhierarchy-tree-model.ts:15 Uncaught (in promise) Error: Invalid argument
    at DocumentDataExt.validatePosition (:3000/private/tmp/rev/theia/packages/plugin-ext/lib/plugin/document-data.js:196)
    at DocumentDataExt.getWordRangeAtPosition (:3000/private/tmp/rev/theia/packages/plugin-ext/lib/plugin/document-data.js:282)
    at Object.getWordRangeAtPosition (:3000/private/tmp/rev/theia/packages/plugin-ext/lib/plugin/document-data.js:92)
    at FoodPyramidHierarchyProvider.prepareCallHierarchy (:3000/private/var/folders/tg/_5rxbhmj4xncz4szvpgswrmc0000gn/T/vscode-unpacked/call-hierarchy-sample-0.0.1.vsix/extension/out/FoodPyramidHierarchyProvider.js:16)
    at CallHierarchyAdapter.<anonymous> (:3000/private/tmp/rev/theia/packages/plugin-ext/lib/plugin/languages/call-hierarchy.js:71)
    at step (:3000/private/tmp/rev/theia/packages/plugin-ext/lib/plugin/languages/call-hierarchy.js:47)
    at Object.next (:3000/private/tmp/rev/theia/packages/plugin-ext/lib/plugin/languages/call-hierarchy.js:28)
    at :3000/private/tmp/rev/theia/packages/plugin-ext/lib/plugin/languages/call-hierarchy.js:22
    at new Promise (<anonymous>)
    at __awaiter (:3000/private/tmp/rev/theia/packages/plugin-ext/lib/plugin/languages/call-hierarchy.js:18)

tsmaeder · 2020-01-27T09:07:19Z

Thx @benoitf

tsmaeder · 2020-01-27T16:03:20Z

@benoitf problem lies with document-data.ts#validatePosition():

if (!(position instanceof Position)) {
    throw new Error('Invalid argument');
}

where Position stands for types-impl.ts position. Not sure whether it would be better to check for theia.Position here. WDYT?

benoitf · 2020-01-27T16:17:44Z

@tsmaeder AFAIK we can only use interfaces from theia.d.ts and as Position is a class we can only use Position from types-impl.

Issue eclipse-theia#3765 Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com> Signed-off-by: Thomas Mäder <tmader@redhat.com>

benoitf

benoitf · 2020-01-28T13:24:29Z

@tsmaeder should you close #4146 ?

akosyakov

i have not tried, but code and dependency wise it looks good to me.

tsmaeder force-pushed the 3765_call_hierarchy_api branch from 013a6ac to 3f35234 Compare January 20, 2020 16:37

akosyakov requested review from JanKoehnlein and benoitf January 21, 2020 02:45

akosyakov added call-hierarchy issues related to the call-hierarachy vscode issues related to VSCode compatibility labels Jan 21, 2020

akosyakov requested review from evidolob and AlexTugarev January 21, 2020 02:45

benoitf reviewed Jan 21, 2020

View reviewed changes

packages/callhierarchy/src/browser/callhierarchy-service.ts Show resolved Hide resolved

benoitf reviewed Jan 21, 2020

View reviewed changes

packages/plugin-ext/src/plugin/languages/call-hierarchy.ts Outdated Show resolved Hide resolved

benoitf reviewed Jan 21, 2020

View reviewed changes

packages/plugin-ext/src/plugin/languages/call-hierarchy.ts Outdated Show resolved Hide resolved

tsmaeder force-pushed the 3765_call_hierarchy_api branch 2 times, most recently from 3eee615 to 615f7c8 Compare January 21, 2020 11:10

tsmaeder marked this pull request as ready for review January 21, 2020 11:20

JanKoehnlein approved these changes Jan 21, 2020

View reviewed changes

tsmaeder force-pushed the 3765_call_hierarchy_api branch 4 times, most recently from 16371d6 to 95dde3e Compare January 21, 2020 15:09

akosyakov reviewed Jan 22, 2020

View reviewed changes

tsmaeder mentioned this pull request Jan 22, 2020

Languages Sprint 178 eclipse-che/che#15629

Closed

35 tasks

tsmaeder mentioned this pull request Jan 23, 2020

Languages Sprint 179 eclipse-che/che#15796

Closed

36 tasks

tsmaeder force-pushed the 3765_call_hierarchy_api branch from f9897f9 to fe1ab01 Compare January 24, 2020 10:25

tsmaeder force-pushed the 3765_call_hierarchy_api branch from cd0a369 to 3b2d08c Compare January 27, 2020 17:13

CallHierarchyService Plugin API eclipse-theia#3765

a982e56

Issue eclipse-theia#3765 Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com> Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder force-pushed the 3765_call_hierarchy_api branch from 3b2d08c to a982e56 Compare January 28, 2020 07:45

benoitf approved these changes Jan 28, 2020

View reviewed changes

akosyakov approved these changes Jan 29, 2020

View reviewed changes

akosyakov mentioned this pull request Jan 29, 2020

eslint: use vscode extension compatible with Theia #6975

Closed

tsmaeder merged commit 764b740 into eclipse-theia:master Jan 29, 2020

vince-fugnitto mentioned this pull request Jan 31, 2020

[call-hierarchy] Implement proposed LSP extension (WIP) #4154

Closed

akosyakov mentioned this pull request Jul 5, 2020

fix #7100: remove @theia/languages and monaco-languageclient #8131

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CallHierarchyService Plugin API #3765 #6924

CallHierarchyService Plugin API #3765 #6924

tsmaeder commented Jan 20, 2020 •

edited

Loading

benoitf commented Jan 21, 2020

tsmaeder commented Jan 21, 2020

benoitf commented Jan 21, 2020

JanKoehnlein left a comment

akosyakov left a comment

akosyakov Jan 22, 2020

tsmaeder Jan 22, 2020

akosyakov Jan 22, 2020

akosyakov Jan 22, 2020 •

edited

Loading

tsmaeder Jan 22, 2020 •

edited

Loading

akosyakov Jan 22, 2020

akosyakov Jan 22, 2020 •

edited

Loading

tsmaeder Jan 22, 2020

tsmaeder Jan 22, 2020

akosyakov Jan 22, 2020

akosyakov Jan 22, 2020

tsmaeder Jan 22, 2020 •

edited

Loading

akosyakov Jan 22, 2020

akosyakov commented Jan 22, 2020

tsmaeder commented Jan 22, 2020

tsmaeder commented Jan 22, 2020

tsmaeder commented Jan 24, 2020

akosyakov commented Jan 27, 2020

benoitf commented Jan 27, 2020

tsmaeder commented Jan 27, 2020

benoitf commented Jan 27, 2020

tsmaeder commented Jan 27, 2020

benoitf commented Jan 27, 2020

tsmaeder commented Jan 27, 2020

tsmaeder commented Jan 27, 2020 •

edited by paul-marechal

Loading

benoitf commented Jan 27, 2020 •

edited

Loading

benoitf left a comment

benoitf commented Jan 28, 2020

akosyakov left a comment

		@@ -21,7 +21,7 @@
		--------------------------------------------------------------------------------------------/
		'use strict';

CallHierarchyService Plugin API #3765 #6924

CallHierarchyService Plugin API #3765 #6924

Conversation

tsmaeder commented Jan 20, 2020 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

benoitf commented Jan 21, 2020

tsmaeder commented Jan 21, 2020

benoitf commented Jan 21, 2020

JanKoehnlein left a comment

Choose a reason for hiding this comment

akosyakov left a 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

akosyakov Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

tsmaeder Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Jan 22, 2020 • edited Loading

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

tsmaeder Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov commented Jan 22, 2020

tsmaeder commented Jan 22, 2020

tsmaeder commented Jan 22, 2020

tsmaeder commented Jan 24, 2020

akosyakov commented Jan 27, 2020

benoitf commented Jan 27, 2020

tsmaeder commented Jan 27, 2020

benoitf commented Jan 27, 2020

tsmaeder commented Jan 27, 2020

benoitf commented Jan 27, 2020

tsmaeder commented Jan 27, 2020

tsmaeder commented Jan 27, 2020 • edited by paul-marechal Loading

benoitf commented Jan 27, 2020 • edited Loading

benoitf left a comment

Choose a reason for hiding this comment

benoitf commented Jan 28, 2020

akosyakov left a comment

Choose a reason for hiding this comment

tsmaeder commented Jan 20, 2020 •

edited

Loading

akosyakov Jan 22, 2020 •

edited

Loading

tsmaeder Jan 22, 2020 •

edited

Loading

akosyakov Jan 22, 2020 •

edited

Loading

tsmaeder Jan 22, 2020 •

edited

Loading

tsmaeder commented Jan 27, 2020 •

edited by paul-marechal

Loading

benoitf commented Jan 27, 2020 •

edited

Loading