`Find All references` doesn't exist for typescript plugin #14856 #7055

vrubezhny · 2020-02-03T13:10:55Z

Signed-off-by: Victor Rubezhny vrubezhny@redhat.com

What it does

The fix adds the required API and fixes required to make possible to run VSCode Reference View extension plug-in on Theia.

How to test

Tested manually using references-view-0.0.47 (https://github.com/microsoft/vscode-references-view) downloaded into plugins/ Theia folder,

Review checklist

[*] as an author, I have thoroughly tested my changes and carefully followed the review guidelines
Check that VSCode Reference View is loaded and shown in Theia Plugins View
Test by executing Find All References context menu action and revisiting the result tree of the References View
Test by executing Find All Implementations context menu action and revisiting the result tree of the References View

Reminder for reviewers

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

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

akosyakov · 2020-02-04T08:37:21Z

I can have a look but later this week. If someone else can test and review it would help. That's cool btw :)

Tree styling on the screenshot looks bad.

tsmaeder

Looks good in general, but I have a couple of suggestions/questions

packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts

packages/plugin-ext/src/common/index.ts

packages/plugin-ext/src/main/browser/view/plugin-view-registry.ts

tsmaeder · 2020-02-06T11:00:26Z

packages/plugin-ext/src/main/browser/view/tree-views-main.ts

@@ -108,6 +108,23 @@ export class TreeViewsMainImpl implements TreeViewsMain, Disposable {
        }
    }

+    async $setMessage(treeViewId: string, message: string): Promise<void> {
+        this.viewRegistry.getView(treeViewId).then(viewPanel => {


Do we need some error handling here? What if the tree view does not exist?

Not sure if we can do anything here (with $setMessage or $setTitle) in case of tree view doesn't exist. Add an error record to Log?

Yes, this indicates something is wrong, needs to got to the log even if we can't handle it.

When I try to print an error by console.error(...) TSLint says: Calls to 'console.error' are not allowed. (no-console)tslint(1).
What is the correct way o report an error here?

tsmaeder · 2020-02-06T11:20:30Z

packages/plugin-ext/src/plugin/known-commands.ts

-    mappings['goToNextReference'] = ['goToNextReference', MONACO_CONVERSION_IDENTITY];
-    mappings['goToPreviousReference'] = ['goToPreviousReference', MONACO_CONVERSION_IDENTITY];
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    const VSCODE_CONVERSION_IDENTITY = (args: any[] | undefined) => {


Also, this is not an identity, it changes stuff. Needs a better name.

tsmaeder · 2020-02-06T12:58:09Z

packages/plugin-ext/src/plugin/type-converters.ts

+    if (!thing) {
+        return false;
+    }
+    return ('startLineNumber' in thing) && ('startColumn' in thing) &&


Shouldn't we be checking the types are correct?

How? I use these methods (isModelLocation(), isModelRange() and isUriComponents()) in result conversion in order to detect if I can convert an object into a theia.Location, theia.Range etc. Here I have a json-like object with properties. No type nor type name available...

still, doesn't startLineNumber have to be of type number?

added checks for property types

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

tsmaeder · 2020-02-06T13:04:09Z

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

@@ -390,7 +411,8 @@ export class Range {
        return new Range(start, end);
    }

-    static isRange(thing: {}): thing is theia.Range {
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    static isRange(thing: any): thing is theia.Range {


Why is this change necessary?

The same reason: to make this class to be defined exactly as it's done in VS Code. Probably it's not necessary change.

I can rollback the changes in types-impl.ts as they aren't necessary and do not bring any fixes/improvements regarding this issue. Rolledback in fixup

tsmaeder · 2020-02-06T13:34:23Z

I built your branch and added the references extension from the marketplace. I noticed the following problems:

The View has no icon in the view sidebar
the search term ("CallHierarchyCommands") is not highlighted
The icon is not the typescript file icon, but a "folder" icon.
There is a line break in the rendering of the file entry ("callhierarchy-contribution.ts\npackages...")
The message "4 results in 1 file is not indented, which looks weird.
When I do "Show All References" from the context menu in an editor, the references view does not open, I need to go through the menu bar "view" menu.

vince-fugnitto · 2020-02-06T13:44:47Z

The View has no icon in the view sidebar

For that specific issue, there is a bug tracking icons which are not rendered in Electron: #7040

akosyakov · 2020-02-10T06:08:25Z

There is a line break in the rendereing of the file entry ("callhierarchy-contribution.ts\npackages...")

It is a regression: #7109

vrubezhny · 2020-02-12T14:31:49Z

I've uploaded a new version of PR. Most of requests to change were reworked, but not all. For example, #7055 (comment) - to improve KnownCommands.map()...
Also, I found that I still have problem to show a search message on very first run of 'Find References/Implementations' - the message isn't shown... Looking into why this happens.
But still, please, review

vrubezhny · 2020-02-14T02:26:29Z

I've pushed a new version of PR:

Fixed very first command invocation search message problem: now the search message is displayed correctly on the first run
Added check for type when converting from model.* to types.* for Location, Range and UriComponents
An error is reported to console in $setTitle and $setMessage API in case of Tree Widget cannot be found

@tsmaeder Please review.

packages/plugin-ext/src/main/browser/view/plugin-view-registry.ts

packages/plugin-ext/src/main/browser/view/tree-view-widget.tsx

akosyakov · 2020-02-14T08:28:44Z

packages/plugin-ext/src/main/browser/view/tree-views-main.ts

+
+        if (widget instanceof TreeViewWidget) {
+            widget.message = message;
+        } else {


i'm not sure about logging errors in else cases, other methods don't do it. It could be that message is resolved by the time when a user already closed the tree, there is nothing bogus to ignore message then.

It was requested by @tsmaeder. I also think the logging is not needed here.
During the run this error appears several times, while not always indicate a real problem with absent widget.
It was discussed here: #7055 (comment)

So what we're trying to guard here is the widget being present, not that it might be there but not a TreeWidget? Then why not do a if (widget) It has the same effect, but IMO would be better express the meaning.

If we do like here #7055 (comment) then we should log error if only metadata is missing completely (such view is not registered). The same for title.

packages/plugin-ext/src/main/browser/view/tree-views-main.ts

packages/plugin-ext/src/plugin/command-registry.ts

akosyakov · 2020-02-14T08:35:42Z

packages/plugin-ext/src/plugin/known-commands.ts

-import { cloneAndChange } from './objects';
-import { Position, Range, Location } from '../plugin/types-impl';
-import { fromPosition, fromRange, fromLocation } from '../plugin/type-converters';
+import { cloneAndChange } from '../common/objects';


Could someone elaborate except moving this file and renaming things what was fixes in this file? It is used for all commands triggered by the plugin host process would be bad to break it.

For vscode.executeReferenceProvider and vscode.executeImplementationProvider, as they return location array as result, it is required to convert the result from model.* objects to theia.* objects in order to be accepted by VSCode Reference View plugin.
So, optional property is added to the mapping to specify the converter function that is to be used to convert the results. As it is optional, there is no need to add this property to all the other existing mappings. Currently, CONVERT_MONACO_TO_VSCODE is added to convert the results array into an array of theia.Location objects. It's used to convert the results of vscode.executeReferenceProvider and vscode.executeImplementationProvider command execution.
The only change for the old mappings is that MONACO_CONVERSION_IDENTITY function used for arguments conversion is renamed to CONVERT_VSCODE_TO_MONACO (in order to indicate the fact that it really changes arguments - requested by @tsmaeder )

Unfortunately, the github diff algorithm is being really stupid here: there are loads of single line changes in the command definitions, but GH picks it up as a two entirely changed blocks of code 🤷‍♂

packages/plugin-ext/src/plugin/tree/tree-views.ts

akosyakov · 2020-02-14T08:37:27Z

packages/plugin-ext/src/plugin/tree/tree-views.ts

@@ -159,6 +176,10 @@ class TreeViewExtImpl<T> implements Disposable {
    private readonly onDidChangeVisibilityEmitter = new Emitter<TreeViewVisibilityChangeEvent>();
    readonly onDidChangeVisibility = this.onDidChangeVisibilityEmitter.event;

+    protected updateMessage = debounce(() => {


Why to debounce? Did you observe any performance or usability issues?

At first invocation of Find All References or Find All Implementations command, the Reference View widget is not yet created, so I cannot set a message on it. So, when you ask for the references or implementation for the first time the references tree will be shown without that search summary message. Debouncing solves this problem.

No, we cannot just "do it later". The problem here is that the state of the tree view can be changed before the widget in the UI is actually created. This just means that we need to transfer the state at the time the widget is created. But this can be 5 minutes after creation of the tree view, so the debounce() is not reliable.
IMO, we have to pass message and title to TreeViewsMain#reveal(). I think that is the only way a plugin can make a tree view visible, right?

Note sure where to put the message when the widget is not created yet. @akosyakov any suggestions how to do that?

Had a discussion on Friday and will investigate what the behaviour of createTreeView should be: if it is supposed to open the widget, the set message should come afterwards and succeed.

The widget creation happens in several steps and is distributed in several events and backend/frontend communications.
During this process $setMessage and $setTitle are invoked with default values message=undefined and title="Results" in order to indicate that the reference tree is empty during the search for the references (the results are not received yet) - trying to process this initial call is the reason of debounce appearance.
And it is still possible that $setMessage with a valuable message will be called before the widget is finally initialized and added to the view on the first request for references/implementations.

After the widget creation these methods are called once again with the real (not empty) values.

I think it should be part of tree view metadata which initially loaded from package.json. When $setMessage or $setTitle are called then metadata should be updated and used during creation. If a view already present then we update it as well.

Actually for title is even stated it in js-doc:

The tree view title is initially taken from the extension package.json Changes to the title property will be properly reflected in the UI in the title of the view.

We should respect it and make sure that initial title and message from metadata are installed properly. It does not seem to happen in the PR.

vrubezhny · 2020-02-14T10:03:19Z

2. the search term ("CallHierarchyCommands") is  not highlighted

When I test it with Electron, in some cases the result items are highlighted, in other cases are not. In case such an item is not highlighted it's enough to mouse click on editor area (so the editor receives focus) - the search item immediately gets highlighted. Exactly the same happens when running Browser version.

6. When I do "Show All References" from the context menu in an editor, the references view does not open, I need to go through the menu bar "view" menu.

Yes, it is. In my case I even didn't have View->References menu item, it looks like it appeared after I tried to invoke Find All Referencess or Find All Implementations command.
But the following invocation of Find All Referencess/Find All Implementations commands, in my case, successfully opens References View (if it's not opened) even after I restarted Electron application several times.

akosyakov · 2020-02-21T09:46:42Z

Do I get correctly that the reference widget extension already makes use of the call hierarchy API? And we can replace our call hierarchy Theia extension by using it?

vrubezhny · 2020-02-21T10:40:02Z

Do I get correctly that the reference widget extension already makes use of the call hierarchy API? And we can replace our call hierarchy Theia extension by using it?

No.
Yes it uses CallHierarchy API, but I really never could make it work ]from the commands provided by the current version of the extension] on VSCode when I was testing it. The plugin's REAAD.me doesn't have any information on CallHierarchy commands... So, I suppose it either just doesn't work with typescript or there are some other limitations.

The support for vscode.prepareCallHierarchy, vscode.provideIncomingCalls and vscode.provideOutgoingCalls commands is included in order to fully support vscode-reference-view existing capabilities

vrubezhny · 2020-02-21T14:05:42Z

The <view.id>.focus command registration is made to not have UI contributions.

akosyakov · 2020-02-21T15:23:20Z

I have errors in logs when try to activate it with keybindings:

akosyakov · 2020-02-21T15:23:58Z

There is no enough spacing around message and the color is bogus:

And in VS Code:

BTW what is wrong with icons, folders instead of files?

Not sure why description color is wrong as well. We need to make it look decent at least and then file issues for everything else if you don't want to address them in this PR.

packages/plugin-ext/src/main/browser/view/plugin-view-widget.ts

packages/plugin-ext/src/main/browser/view/tree-view-widget.tsx

packages/plugin-ext/src/main/browser/view/plugin-view-widget.ts

vrubezhny · 2020-02-21T22:16:08Z

There is no enough spacing around message and the color is bogus:

And in VS Code:

I'll fix spacing

BTW what is wrong with icons, folders instead of files?

Probably we should look into Label Provider: https://github.com/eclipse-theia/theia/blob/master/packages/core/src/browser/label-provider.ts#L203
I'd like to create a separate issue for this.

Not sure why description color is wrong as well. We need to make it look decent at least and then file issues for everything else if you don't want to address them in this PR.

What do you mean by "description color"? Is it a X results in Y files (message) on top of the tree?
As for me, the View colors in Chrome (for Browser example) and in VSCode are pretty the same (Fedora 29, Gnome):

The only that message is different (grey instead of white).
I don't have orange backgrounded words (looks like a word that was searched for) - but, imho, it's better not to have it colored on the tree side.
But I don't understand why some lines in your VSCode have orange foreground and a labeled like 9+ on the right... Is it a something in the UI specially made for MacOS?

vrubezhny · 2020-02-21T22:28:26Z

I have errors in logs when try to activate it with keybindings:

I don't have any errors in log when I use Alt + Shift + F12 to find references. The hotkey combination is not shown in context menu, but it still works fine for me.
What version you use to test - Browser or Electron?

akosyakov · 2020-02-24T12:35:17Z

@vrubezhny Could you please do #7153 (comment) it will easy testing of this PR and it would be good to have this extension on master after merging.

vrubezhny · 2020-02-24T15:21:29Z

I've updated the PR with:

Margins and foregroung color are fixed for the Tree View Message
PR Add vscode-references-view plugin to Theia #7153 is merged into this one (adds vscode-references-view plugin to the list of plugins to be downloaded from https://download.jboss.org at Theia build, so making it available for use in run time)

package.json

akosyakov · 2020-02-26T09:47:28Z

#7055 (comment)

@vrubezhny I can reproduce it reliably for browser. Please make sure to reset the layout before that it is not opened.

packages/plugin-ext/src/main/browser/style/tree.css

akosyakov

Probably we should look into Label Provider: https://github.com/eclipse-theia/theia/blob/master/packages/core/src/browser/label-provider.ts#L203
I'd like to create a separate issue for this.

Ok, please create.

But I don't understand why some lines in your VSCode have orange foreground and a labeled like 9+ on the right... Is it a something in the UI specially made for MacOS?

We don't support tree item highlights VS Code API: https://github.com/vrubezhny/theia/blob/c9370d3de85b225419d7946938ac6d3ac5624a21/packages/plugin/src/theia-proposed.d.ts#L213 It would be good to file a new issue for this.

akosyakov · 2020-02-26T10:09:31Z

It looks good except keybindings don't work for me.

akosyakov · 2020-02-26T10:14:34Z

@vrubezhny Don't care about keybindings. I know there is the issue and it will be fixed in: dae6865#diff-0291e1b92ff01c13c0dc1c98263ac426 Keybindings don't call command handlers properly, so the plugin system gets never notified about executed commands.

akosyakov

I think it is good, there are some issues but they can be resolved separately. Please take care though about color variable comment before merging.

Fixes: #14856 Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

i believe all comments were addressed

akosyakov reviewed Feb 4, 2020

View reviewed changes

packages/plugin-ext/src/plugin/types-impl.ts Outdated Show resolved Hide resolved

akosyakov added typescript issues related to the typescript language vscode issues related to VSCode compatibility labels Feb 4, 2020

akosyakov requested review from azatsarynnyy, benoitf, elaihau and vince-fugnitto February 4, 2020 08:09

akosyakov mentioned this pull request Feb 4, 2020

Inconsistent vscode.workspace.onDidChangeTextDocument behaviour #6895

Open

tsmaeder previously requested changes Feb 6, 2020

View reviewed changes

vrubezhny force-pushed the et14856 branch from 0821c2d to 99c027a Compare February 12, 2020 14:19

vrubezhny requested a review from tsmaeder February 12, 2020 14:21

vrubezhny force-pushed the et14856 branch from 99c027a to ea84e78 Compare February 14, 2020 02:16