[plugin] support `vscode.executeDocumentSymbol` command. #6291

svenefftinge · 2019-09-29T14:08:23Z

What it does

Registers the vscode.executeDocumentSymbol command, which is one of the commands vscode extensions might use.
See https://code.visualstudio.com/api/references/commands for the full list.

How to test

Install the emmet extension (https://registry.npmjs.org/@theia/vscode-builtin-emmet/-/vscode-builtin-emmet-0.2.1.tgz)

Start theia
change preferences, adding the following property:

"emmet.includeLanguages": {
    "typescript": "typescriptreact"
  }

open a TS file
start using content assist.

Without this change, there will be an error in the console.

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

akosyakov

Code-wise it looks good, but I have not tried whether it works. cc @AlexTugarev

AlexTugarev · 2019-09-30T06:37:18Z

testing right now...

AlexTugarev · 2019-09-30T06:50:19Z

Uncaught (in promise) Error: Unknown actor StorageExt

I don't think this is related, but it's a new error.

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

benoitf · 2019-09-30T06:58:31Z

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

+        // Register built-in language service commands
+        // see https://code.visualstudio.com/api/references/commands
+        // tslint:disable: no-any
+        const instantiationService = monaco.services.StaticServices.instantiationService.get();


I would avoid to use monaco object directly.
We have a theia/monaco package that should be used provide the communication to monaco.

Why and how would adding an indirection help? This is clearly dependening on monaco so obfuscating that will only add unnecessary code and make it harder to understand.

There is DI objects in @theia/monaco
Here you're asking for services and you're in the need of importing some monaco definition

By using @theia/monaco there is no need to add this index.d.ts and tracking of what is used is clearly identified.

The fact is there are too many command registries. If each service is managing its way (and internal way) to get a command registry it's not clean.

Those commands are internal monaco commands and only added to our command registry through this contribution. I don't want to add them raw to our command registry and then have this contribution adding them once more using the vscode.prefix and needed conversion. Maybe I don't understand what you mean, but adding this to @theia/monaco would do nothing other than exposing it through our API. I don't see how this would help. We do it like this in other places as well and especially here we are vscode-specific so directly using monaco makes perfect sense.

well, if I read the discussion from one year, there are still a lot of discussions about the command registries, mapping, wrappers, ranges, etc. so something is not clear.

https://spectrum.chat/theia/dev/plugins-two-problems-related-to-command-parameters~11b89e76-1695-4eb1-b73e-49fb62cfb3f4
#4050

and latest discussion #5590

so I would fix it for once.

I don't think I can or should fix all the confusion from those discussions in the PR.
But again if I change this PR so that

the internal language-specific commands from monaco (i.e. those starting with _execute) are registered in our command registry as part of @theia/monaco,

those commands are aliased with vscode._execute... and proper conversion in plugin-ext-vscode and

I remove the monaco.d.ts from plugin-ext-vscode

you are ok with that?

@benoitf I have updated the code accordingly.

@svenefftinge yes I think code is more maintainable

@svenefftinge thx a lot

benoitf · 2019-09-30T06:59:51Z

packages/plugin-ext-vscode/src/typings/monaco/index.d.ts

+
+/// <reference types='@theia/monaco/src/typings/monaco'/>
+
+/* expose internal APIs in @theia/monaco/src/typings/monaco */


it shouldn't be there, vscode is a client of theia-monaco, if there is something to be exposed, it should be exposed , it shouldn't use extra d.ts.

That is what it says: Don't expose here but do so in @theia/monaco/src/typings/monaco.

Actually, it is exactly how we do it elsewhere.

AlexTugarev · 2019-09-30T07:34:49Z

Just verified that without this change there are no emmet suggestions for typescriptreact:

with this PR emmet suggestions are available 🎉

AlexTugarev

I tested with the emmet builtin extension as described, and it was working nicely with typescript react. Though I couldn't reproduce the error to be logged on console.

svenefftinge · 2019-09-30T08:00:23Z

@benoitf Ok to merge?

Signed-off-by: Sven Efftinge <sven.efftinge@typefox.io>

benoitf · 2019-09-30T08:57:12Z

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

+                id: 'vscode.executeDocumentSymbolProvider'
+            },
+            {
+                execute: (resource: URI) => commands.executeCommand('monaco._executeDocumentSymbolProvider',


just a question: why not use async/await instead of the callbacks ?

sorry, I don't understand what you mean. Could you clarify?

callback :

commands.executeCommand(...).then((foo) => {return foo.something() })

vs

async/await:

const foo = await commands.executeCommand(...) return foo.something()

Ah, I see. No specific reason but as this is a single expression I find it more readable and it has less code this way.

svenefftinge requested review from akosyakov, evidolob and AlexTugarev September 29, 2019 14:08

svenefftinge force-pushed the se/plugin_vscode_executeDocumentSymbol branch from 8a3f2ca to f66dd74 Compare September 29, 2019 14:59

akosyakov approved these changes Sep 29, 2019

View reviewed changes

benoitf reviewed Sep 30, 2019

View reviewed changes

AlexTugarev approved these changes Sep 30, 2019

View reviewed changes

svenefftinge force-pushed the se/plugin_vscode_executeDocumentSymbol branch from f66dd74 to 3fa4936 Compare September 30, 2019 08:45

[plugin] support vscode.executeDocumentSymbol command.

67217ea

Signed-off-by: Sven Efftinge <sven.efftinge@typefox.io>

svenefftinge force-pushed the se/plugin_vscode_executeDocumentSymbol branch from 3fa4936 to 67217ea Compare September 30, 2019 08:47

benoitf approved these changes Sep 30, 2019

View reviewed changes

benoitf reviewed Sep 30, 2019

View reviewed changes

svenefftinge merged commit 821b9dc into master Sep 30, 2019

svenefftinge deleted the se/plugin_vscode_executeDocumentSymbol branch September 30, 2019 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[plugin] support `vscode.executeDocumentSymbol` command. #6291

[plugin] support `vscode.executeDocumentSymbol` command. #6291

svenefftinge commented Sep 29, 2019 •

edited

Loading

akosyakov left a comment

AlexTugarev commented Sep 30, 2019

AlexTugarev commented Sep 30, 2019

benoitf Sep 30, 2019

svenefftinge Sep 30, 2019

benoitf Sep 30, 2019

benoitf Sep 30, 2019

svenefftinge Sep 30, 2019

benoitf Sep 30, 2019

svenefftinge Sep 30, 2019 •

edited

Loading

svenefftinge Sep 30, 2019

benoitf Sep 30, 2019

benoitf Sep 30, 2019

benoitf Sep 30, 2019

svenefftinge Sep 30, 2019 •

edited

Loading

AlexTugarev commented Sep 30, 2019

AlexTugarev left a comment

svenefftinge commented Sep 30, 2019

benoitf Sep 30, 2019

svenefftinge Sep 30, 2019

benoitf Sep 30, 2019

svenefftinge Sep 30, 2019


		/// <reference types='@theia/monaco/src/typings/monaco'/>

		/* expose internal APIs in @theia/monaco/src/typings/monaco */

[plugin] support vscode.executeDocumentSymbol command. #6291

[plugin] support vscode.executeDocumentSymbol command. #6291

Conversation

svenefftinge commented Sep 29, 2019 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

akosyakov left a comment

Choose a reason for hiding this comment

AlexTugarev commented Sep 30, 2019

AlexTugarev commented Sep 30, 2019

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

svenefftinge Sep 30, 2019 • 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

svenefftinge Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

AlexTugarev commented Sep 30, 2019

AlexTugarev left a comment

Choose a reason for hiding this comment

svenefftinge commented Sep 30, 2019

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

[plugin] support `vscode.executeDocumentSymbol` command. #6291

[plugin] support `vscode.executeDocumentSymbol` command. #6291

svenefftinge commented Sep 29, 2019 •

edited

Loading

svenefftinge Sep 30, 2019 •

edited

Loading

svenefftinge Sep 30, 2019 •

edited

Loading