Menu to select default formatter for the document #8446

vitaliy-guliy · 2020-08-30T22:07:13Z

What it does

Adds quick pick menu to select document formatter.
The menu is appeared only when the extensions registered several formatters for the same language.
ID of selected formatter is saved to preferences, so the menu will not appear next time when formatting.

Fixes #8354

How to test

Option 1

build Thena and launch it with several extensions that register formatters for the same language

Option 2

use prepared script to build Theia from sources and launch it with Java and Quarkus extensions. Both of them register document formatters for java language.

To test

download the script somewhere on the file system
launch
open browser, go to http://localhost:3000/
open any java file, wait for the language server to being initialized
right click on the document and select 'Format Document'

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

azatsarynnyy · 2020-09-04T20:15:19Z

When I run Theia with the following plugins:

I've got

root ERROR [hosted-plugin: 95750] Promise rejection not handled in one second: TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received type object , reason: TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received type object
root ERROR [hosted-plugin: 95750] With stack trace: TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received type object
    at validateString (internal/validators.js:112:11)
    at Object.join (path.js:1039:7)
    at /tmp/vscode-unpacked/java-0.63.0-2222.vsix/extension/dist/extension.js:7:10673
    at new Promise (<anonymous>)
    at /tmp/vscode-unpacked/java-0.63.0-2222.vsix/extension/dist/extension.js:7:10643
    at Object.<anonymous> (/tmp/vscode-unpacked/java-0.63.0-2222.vsix/extension/dist/extension.js:7:11385)
    at Generator.next (<anonymous>)
    at o (/tmp/vscode-unpacked/java-0.63.0-2222.vsix/extension/dist/extension.js:7:8792)

azatsarynnyy · 2020-09-04T20:25:41Z

packages/plugin-ext/src/common/plugin-api-rpc.ts

@@ -1276,8 +1276,8 @@ export interface LanguagesMain {
    $registerQuickFixProvider(handle: number, pluginInfo: PluginInfo, selector: SerializedDocumentFilter[], codeActionKinds?: string[]): void;
    $clearDiagnostics(id: string): void;
    $changeDiagnostics(id: string, delta: [string, MarkerData[]][]): void;
-    $registerDocumentFormattingSupport(handle: number, pluginInfo: PluginInfo, selector: SerializedDocumentFilter[]): void;
-    $registerRangeFormattingProvider(handle: number, pluginInfo: PluginInfo, selector: SerializedDocumentFilter[]): void;


Why renaming these is needed?
Why not keep it in sync with the corresponding VS Code interface.

Aligned with VS Code, thanks!

vitaliy-guliy · 2020-09-07T10:03:01Z

When I run Theia with the following plugins:

https://download.jboss.org/jbosstools/vscode/3rdparty/vscode-java-debug/vscode-java-debug-0.26.0.vsix

https://download.jboss.org/jbosstools/static/jdt.ls/stable/java-0.63.0-2222.vsix

https://download.jboss.org/jbosstools/vscode/stable/vscode-quarkus/vscode-quarkus-1.5.0-324.vsix

I've got

How do you run Theia? Did you try to run the test script?

azatsarynnyy · 2020-09-10T17:24:03Z

How do you run Theia? Did you try to run the test script?

@vitaliy-guliy I used "Option 1" from "How to test" section.

vitaliy-guliy · 2020-09-11T10:26:23Z

How do you run Theia? Did you try to run the test script?

@vitaliy-guliy I used "Option 1" from "How to test" section.

Could you please describe the steps what exactly are you doing?

Did you use fork with fixes?
https://github.com/vitaliy-guliy/theia/tree/default-formatter-8354

azatsarynnyy · 2020-09-11T13:10:31Z

@vitaliy-guliy
I've built https://github.com/vitaliy-guliy/theia/tree/default-formatter-8354
Then, I've downloaded the following plugins into theia/plugins folder:

https://download.jboss.org/jbosstools/vscode/3rdparty/vscode-java-debug/vscode-java-debug-0.26.0.vsix
https://download.jboss.org/jbosstools/static/jdt.ls/stable/java-0.63.0-2222.vsix
https://download.jboss.org/jbosstools/vscode/stable/vscode-quarkus/vscode-quarkus-1.5.0-324.vsix

It would be nice if someone else could try it as well.

RomanNikitenko

@azatsarynnyy
I tested according to #8446 (comment)
I didn't see such errors.

@vitaliy-guliy
I tried to remove a formatter from preferences to see the dialog about preferred formatter again, but looks like it always uses my first choice and I can not change formatter then.
Could you try this cases?
Does it work for you?
Maybe something wrong on my side...

Update: the problem was on my side - now it works for me and I can select another formatter after removing the previous one.
The only thing which I noticed - quarkus formatter do nothing

vitaliy-guliy · 2020-09-14T08:46:22Z

@azatsarynnyy
I tested according to #8446 (comment)
I didn't see such errors.

@vitaliy-guliy
I tried to remove a formatter from preferences to see the dialog about preferred formatter again, but looks like it always uses my first choice and I can not change formatter then.
Could you try this cases?
Does it work for you?
Maybe something wrong on my side...

Update: the problem was on my side - now it works for me and I can select another formatter after removing the previous one.
The only thing which I noticed - quarkus formatter do nothing

After removing preference the menu must appear again.
About Quarkus formatter, it does not format the document. It seems something wrong with formatter directly.
Thanks for the review!

RomanNikitenko · 2020-09-14T08:47:17Z

After removing preference the menu must appear again.

Yes, now it works for me - I mentioned above that the problem was on my side

About Quarkus formatter, it does not format the document. It seems something wrong with formatter directly.

OK, thanks!

azatsarynnyy

After fixing my JDK_HOME env. var. everything works as expected. Thanks @RomanNikitenko for the hint!

azatsarynnyy

@vitaliy-guliy please, check the failed tests before merging

vince-fugnitto · 2020-09-14T13:55:33Z

packages/editor/src/browser/editor-preferences.ts

+    'editor.defaultFormatter': {
+        'type': 'string',
+        'default': 'default',
+        'description': 'Default formatter'
+    },


Is there a reason we do not want to support this same schema as vscode, especially the default value (null)?
I believe it will cause inconsistencies:

https://github.com/microsoft/vscode/blob/68410abc36030d3fb321145d2e4945f72f6e6bee/src/vs/workbench/contrib/format/browser/formatActionsMultiple.ts#L157-L164

PR is updated. For default value I used undefined, as we cannot use null. Thanks

lerna ERR! /home/vitaliy/projects/vitaliy-guliy-theia/packages/editor/src/browser/editor-preferences.ts lerna ERR! 82:20 error Use undefined instead of null no-null/no-null

We could use it if we disable the rule for it, I think in this case it is fine since we want compatibility:

// eslint-disable-next-line no-null/no-null

My only concern is when reading .vscode settings where they use null as a value since it is allowed.

Done with

// eslint-disable no-null/no-null

as null can be used also to initialize other properties

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>

vitaliy-guliy requested review from akosyakov, vince-fugnitto, AlexTugarev, spoenemann and svenefftinge August 31, 2020 09:47

akosyakov requested review from azatsarynnyy and RomanNikitenko September 2, 2020 08:01

akosyakov added monaco issues related to monaco vscode issues related to VSCode compatibility labels Sep 2, 2020

azatsarynnyy reviewed Sep 4, 2020

View reviewed changes

RomanNikitenko reviewed Sep 14, 2020

View reviewed changes

RomanNikitenko approved these changes Sep 14, 2020

View reviewed changes

vitaliy-guliy force-pushed the default-formatter-8354 branch from 0963cb1 to 90270ad Compare September 14, 2020 09:40

azatsarynnyy approved these changes Sep 14, 2020

View reviewed changes

azatsarynnyy reviewed Sep 14, 2020

View reviewed changes

vitaliy-guliy force-pushed the default-formatter-8354 branch from ec1bee8 to 0a204c4 Compare September 14, 2020 13:43

vince-fugnitto reviewed Sep 14, 2020

View reviewed changes

Add ability to set default formatter

d664c3e

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>

vitaliy-guliy force-pushed the default-formatter-8354 branch from add3986 to d664c3e Compare September 15, 2020 10:00

vitaliy-guliy merged commit a89d725 into eclipse-theia:master Sep 15, 2020

vitaliy-guliy deleted the default-formatter-8354 branch September 15, 2020 11:50

vince-fugnitto mentioned this pull request Nov 15, 2021

Theia should support editor.defaultFormatter key #7252

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Menu to select default formatter for the document #8446

Menu to select default formatter for the document #8446

vitaliy-guliy commented Aug 30, 2020

azatsarynnyy commented Sep 4, 2020

azatsarynnyy Sep 4, 2020

vitaliy-guliy Sep 7, 2020

vitaliy-guliy commented Sep 7, 2020

azatsarynnyy commented Sep 10, 2020

vitaliy-guliy commented Sep 11, 2020

azatsarynnyy commented Sep 11, 2020

RomanNikitenko left a comment •

edited

Loading

vitaliy-guliy commented Sep 14, 2020

RomanNikitenko commented Sep 14, 2020 •

edited

Loading

azatsarynnyy left a comment

azatsarynnyy left a comment

vince-fugnitto Sep 14, 2020

vitaliy-guliy Sep 14, 2020

vince-fugnitto Sep 14, 2020

vitaliy-guliy Sep 14, 2020

Menu to select default formatter for the document #8446

Menu to select default formatter for the document #8446

Conversation

vitaliy-guliy commented Aug 30, 2020

What it does

How to test

Review checklist

Reminder for reviewers

azatsarynnyy commented Sep 4, 2020

azatsarynnyy Sep 4, 2020

Choose a reason for hiding this comment

vitaliy-guliy Sep 7, 2020

Choose a reason for hiding this comment

vitaliy-guliy commented Sep 7, 2020

azatsarynnyy commented Sep 10, 2020

vitaliy-guliy commented Sep 11, 2020

azatsarynnyy commented Sep 11, 2020

RomanNikitenko left a comment • edited Loading

Choose a reason for hiding this comment

vitaliy-guliy commented Sep 14, 2020

RomanNikitenko commented Sep 14, 2020 • edited Loading

azatsarynnyy left a comment

Choose a reason for hiding this comment

azatsarynnyy left a comment

Choose a reason for hiding this comment

vince-fugnitto Sep 14, 2020

Choose a reason for hiding this comment

vitaliy-guliy Sep 14, 2020

Choose a reason for hiding this comment

vince-fugnitto Sep 14, 2020

Choose a reason for hiding this comment

vitaliy-guliy Sep 14, 2020

Choose a reason for hiding this comment

RomanNikitenko left a comment •

edited

Loading

RomanNikitenko commented Sep 14, 2020 •

edited

Loading