Retrieving launch configurations using Plugin API #4743

akurinnoy · 2019-03-28T08:13:27Z

This PR aims to fix #4188. In order to fix the issue following improvements are introduced:

[plugin-ext] getting and inspecting configuration for a resource
[preferences] handling preference properties that are not valid at the time of reading and re-validating them later on new preference schema has been set.
[preferences] handling launch.json to retrieve launch configurations
[debug] handling changes of launch configuration in order to build new preference schema

This PR also fixes #2290

akosyakov · 2019-03-28T10:19:50Z

@akurinnoy tests are red

akurinnoy · 2019-03-28T13:38:35Z

@akosyakov tests are green, please have a look

akosyakov · 2019-03-28T14:21:40Z

@akurinnoy i will test tomorrow morning

akosyakov · 2019-03-29T05:58:57Z

packages/core/src/browser/preferences/preference-contribution.ts

@@ -53,6 +54,7 @@ export class PreferenceSchemaProvider extends PreferenceProvider {

    protected readonly preferences: { [name: string]: any } = {};
    protected readonly combinedSchema: PreferenceDataSchema = { properties: {}, patternProperties: {} };
+    private remoteSchemas: IJSONSchema[] = [];


What is special about remoteSchema? Why cannot be incorporated in combinedSchema and should be validated separately?

After reading code it does not seem to affect combined schemas, but just passed here to reuse validate function for launch folder providers?

PreferenceSchemaProvider is responsible for providing a schema for settings files, validating it and used for getting default values of settings. It does not have anything to do with launch preferences. I think we should have a separate Ajv validator for launch file not aware of settings schema.

After reading code it does not seem to affect combined schemas, but just passed here to reuse validate function for launch folder providers?

You're right, but not only for launch folder provider but for any preference provider. Each time a launch configuration is added, renamed or removed we need new schema which restricts possible names of configuration available for compound sub-section.

PreferenceSchemaProvider is responsible for providing a schema for settings files, validating it and used for getting default values of settings. It does not have anything to do with launch preferences.

User might have a global or workspace wide launch configuration placed in corresponding settings file, and a project might contain no launch file at all. That is why I treated launch configuration as any other property.

User might have a global or workspace wide launch configuration placed in corresponding settings file

Wow, did not know it, ok, will test how it works. So i should get content assist in settings.json for launch property and registered debug configurations similar to VS Code?

There is no content assist support for launch property and launch configurations in 'settings.json` as in VS Code. Looking at the code this property should be provided by https://github.com/akurinnoy/theia/blob/397f360422dae3dc590e58665fd526d04d7810c7/packages/debug/src/browser/debug-schema-updater.ts#L77-L88 not by remote schemas.

I'm still confused why we need remoteSchemas here? How do they affect settings schema and values?

Thinking of content assist I meant this:

I'm still confused why we need remoteSchemas here? How do they affect settings schema and values?

That's the point. Remote schema doesn't affect setting a values. It affects only validation. Remote schema only makes things easier allowing to replace a part of preference schema without calling doSetSchema(), thus allows to easily update validation function.

The main part of launch preference schema is set only once. Since then only remote part of schema become updated.
https://github.com/theia-ide/theia/pull/4743/files#diff-2dd1a479f035ae6ce8c1517929d4568dR70

I will try again tomorrow. Tried the same today and did not get any content assist. The problematic part with it that these settings not respected by DebugConfigurationWidget and we don't have an implementation for compounds anywhere. We either have to implement it or keep content assist out of scope of this PR.

packages/core/src/browser/preferences/preference-provider.ts

akosyakov · 2019-03-29T06:04:58Z

packages/debug/src/browser/debug-schema-updater.ts

    }
 }

 // debug general schema
-const defaultCompound = { name: 'Compound', configurations: [] };
+export const defaultCompound = { name: 'Compound', configurations: [] };


Does it mean that we support compound configurations now? Does a user can get a false impression via UI that we do?

No, it doesn't mean that. It's only about providing preference schema and validating a launch configuration.

Which UI do you mean?

content assist in launch.json should not provide what is not implemented

If a property is not valid then user can't retrieve it using plugin API.

Could we instead provide a diagnostics for content assist for compounds property key to warn user that compound configurations are not supported?

When it should be stubbed somehow in the plugin ext for now without leaking anything to UI and adding half-baked code to other extensions.

Could It be done in the following way?

At this point
https://github.com/theia-ide/theia/blob/397f360422dae3dc590e58665fd526d04d7810c7/packages/debug/src/browser/debug-schema-updater.ts#L113
clone the schema. Then one of schemas could go to provide content assist. And the second one could be populated with configuration names and used for validation purpose.

This allows to validate preferences and will not affect UI.

packages/preferences/src/browser/abstract-resource-preference-provider.ts

akosyakov · 2019-03-29T06:20:29Z

packages/debug/src/browser/abstract-launch-preference-provider.ts

+        this.toDispose.push(this.onDidLaunchChangedEmitter);
+        this.toDispose.push(
+            this.preferenceProvider.onDidNotValidPreferencesRead(prefs => {
+                if (!prefs || !GlobalLaunchConfig.is(prefs.launch)) {


Why should it go here and cannot happen in launch preference provider? then we don't need a new event onDidNotValidPreferencesRead and can do GlobalLaunchConfig.is check in one place.

Also wondering after reading code, looks like if you don't use the validation function aware of setting schema for launch folder preferences maybe you won't get some false possible validation results and getting rid of an event would be easier.

Why should it go here and cannot happen in launch preference provider? then we don't need a new event onDidNotValidPreferencesRead and can do GlobalLaunchConfig.is check in one place.

Event onDidNotValidPreferencesRead triggers creating new preference schema for launch property, so I don't see how we can't get rid of it.

looks like if you don't use the validation function aware of setting schema for launch folder preferences maybe you won't get some false possible validation results and getting rid of an event would be easier.

I'm not sure I understood what you meant... Could you elaborate on this?

Why do we need to create preferences from invalid data? If it is valid then it should be part of onDidPreferencesChanged. Could you elaborate on an example when preferences are not valid but we still want to use them?

It's because how launch preference schema is defined.

https://github.com/theia-ide/theia/blob/397f360422dae3dc590e58665fd526d04d7810c7/packages/debug/src/browser/debug-schema-updater.ts#L186-L189

In order to build this schema I need to get all configuration names to fill the enum. On the other hand, to get them launch configurations need to be validated against launch preference schema which is not built yet.

Imagine, you have launch configuration

"launch": { "confugurations": [ { "name": "Launch Backend", ...}, { "name": "Launch Frontend", ...} ], "compounds": [ { "name": "Compound", "configurations": [ "Launch Backend", // valid "Launch Frontend", // valid "Run Tests" // not valid ] } ] }

And you need to restrict possible values of configurations in compounds section to only available configuration names.

Do you mean if someone add Run Tests to compounds.configurations then the whole launch preference marked as invalid and not available to a user?
Do you also mean that we reparse launch json twice, once to create a schema and second time to validate data?

How about having custom validation for launch preferences to exclude only bogus compound configuration, break a cycle between schema and data, avoid parsing data twice and introducing a new event? I don't think you need to check each property in this case of each configuration just top-level structure, like here https://github.com/akurinnoy/theia/blob/397f360422dae3dc590e58665fd526d04d7810c7/packages/debug/src/browser/debug-configuration-model.ts#L74

I wonder whether launch launch configs in user settings makes sense at all? Should not we just stub it with default values? Docs does not say anything about user settings:

Note: Workspace and Workspace Folder configurations contains launch and tasks settings.

Have you seen default settings and explanation near launch property?

I think launch config in user settings shouldn't be stabbed, and Theia should use a global launch config if a project doesn't have its own.

Does it mean that it's stubbed with such value and data from user settings json are ignored? Do I read it correctly?

I'm wrong. My user settings already had launch section that's why i got such results. If i try to changes it changes are reflected into expectations, so we should read launch section user settings as well.

I will update tests with expectations that user settings does not contain launch section.

akosyakov · 2019-03-29T06:23:09Z

I don't see that DebugConfigurationWidget is getting launch from preferences now, but continues using own logic.

akosyakov · 2019-03-29T06:24:52Z

packages/preferences/src/browser/folder-launch-preference-provider.ts

+    async getUri(): Promise<URI | undefined> {
+        this.folderUri = new URI(this.options.folder.uri);
+        if (await this.fileSystem.exists(this.folderUri.toString())) {
+            const uri = this.folderUri.resolve('.theia').resolve('launch.json');


It should respect launch.json from vscode as well. See logic in DebugConfigurationWidget, if we move it here it should be properly reimplemented.

https://github.com/theia-ide/theia/blob/master/packages/debug/src/browser/debug-configuration-manager.ts#L74

I think this should be separate issue and another pull request because there are some things to discuss. For example, if a project has one launch configurations for Theia and one for VScode which names are same but they differ in other fields. Which one should be returned if a user gets configuration by name? How a user should update or delete a concrete launch configuration?

@akosyakov do you mind if I open an issue?

It should work as on master. If a folder has .theia then theia settings is used, if not then VS Code settings.

packages/preferences/src/browser/folders-preferences-provider.ts

packages/preferences/src/browser/folder-preference-provider.ts

Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>

This allows to re-validate such properties when a corresponding schema has been set. Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>

Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>

…schema `UserLaunchProvider`, `WorkspaceLaunchProvider` and `FoldersLaunchProvider` handle changing of 'launch' section of preferences without paying attention whether configuration is valid or not. Once it happens a new preference schema for launch configurations builds. Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>

Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>

akosyakov · 2019-04-02T05:43:27Z

With which VS Code extension it's possible to reproduce issues like access compound configurations and so on?

akosyakov · 2019-04-02T05:12:02Z

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## v0.6.0

 - [filesystem] added the menu item `Upload Files...` to easily upload files into a workspace
+- [preferences] changed signature for methods `getProvider`, `setProvider` and `createFolderPreferenceProvider` of `FoldersPreferenceProvider`.


breaking changes should go in its own section, see below for 0.5.0

It would be nice also cover what for they were changed, see examples in breaknig changes for other releases

akosyakov · 2019-04-02T05:33:25Z

packages/debug/src/browser/abstract-launch-preference-provider.ts

+        this.toDispose.push(this.onDidLaunchChangedEmitter);
+        this.toDispose.push(
+            this.preferenceProvider.onDidNotValidPreferencesRead(prefs => {
+                if (!prefs || !GlobalLaunchConfig.is(prefs.launch)) {


Why do we need to create preferences from invalid data? If it is valid then it should be part of onDidPreferencesChanged. Could you elaborate on an example when preferences are not valid but we still want to use them?

akosyakov · 2019-04-02T05:35:02Z

packages/debug/src/browser/debug-schema-updater.ts

    }
 }

 // debug general schema
-const defaultCompound = { name: 'Compound', configurations: [] };
+export const defaultCompound = { name: 'Compound', configurations: [] };


When it should be stubbed somehow in the plugin ext for now without leaking anything to UI and adding half-baked code to other extensions.

akosyakov · 2019-04-02T05:40:22Z

packages/preferences/src/browser/folders-preferences-provider.ts

@@ -21,87 +21,113 @@ import { FolderPreferenceProvider, FolderPreferenceProviderFactory } from './fol
 import { FileSystem, FileStat } from '@theia/filesystem/lib/common';
 import URI from '@theia/core/lib/common/uri';

+export const LAUNCH_PROPERTY_NAME = 'launch';
+export type ResourceKind = 'settings' | 'launch';


please use unique naming, see naming conventions

i.e. FolderPreferenceKind

Resource already has a meaning in Theia, i.e git and file are kind of resources

akosyakov · 2019-04-02T05:54:44Z

packages/core/src/browser/preferences/preference-contribution.ts

@@ -53,6 +54,7 @@ export class PreferenceSchemaProvider extends PreferenceProvider {

    protected readonly preferences: { [name: string]: any } = {};
    protected readonly combinedSchema: PreferenceDataSchema = { properties: {}, patternProperties: {} };
+    private remoteSchemas: IJSONSchema[] = [];


There is no content assist support for launch property and launch configurations in 'settings.json` as in VS Code. Looking at the code this property should be provided by https://github.com/akurinnoy/theia/blob/397f360422dae3dc590e58665fd526d04d7810c7/packages/debug/src/browser/debug-schema-updater.ts#L77-L88 not by remote schemas.

I'm still confused why we need remoteSchemas here? How do they affect settings schema and values?

packages/debug/src/browser/abstract-launch-preference-provider.ts

mmorhun · 2019-04-02T09:37:12Z

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

@@ -215,7 +221,11 @@ export class PreferenceRegistryExtImpl implements PreferenceRegistryExt {
        const defaultConfiguration = this.getConfigurationModel(data[PreferenceScope.Default]);
        const userConfiguration = this.getConfigurationModel(data[PreferenceScope.User]);
        const workspaceConfiguration = this.getConfigurationModel(data[PreferenceScope.Workspace]);
-        return new Configuration(defaultConfiguration, userConfiguration, workspaceConfiguration);
+        const folderConfigurations = {} as { [resource: string]: ConfigurationModel };


I think it would be better to have a named type if this construction is used in several files.

mmorhun · 2019-04-02T13:23:32Z

packages/preferences/src/browser/folders-preferences-provider.ts

@@ -113,20 +139,34 @@ export class FoldersPreferencesProvider extends PreferenceProvider {
        return provider;
    }

-    protected createFolderPreferenceProvider(folder: FileStat): FolderPreferenceProvider {
-        const provider = this.folderPreferenceProviderFactory({ folder });
+    protected setProvider(provider: FolderPreferenceProvider, kind: ResourceKind): void {


To me addProvider suits better here

Got my explanation

tsmaeder · 2019-04-09T07:35:14Z

This PR is also required for finishing eclipse-che/che#12446: we cannot enable logging and debugging in jdt.ls right now.

akosyakov · 2019-04-11T09:10:24Z

@tsmaeder I understand and also want to see it merged, but the current state does not look good to me. If you don't mind we can collaborate on this PR, i.e. i will push commits to finish it up.

tsmaeder · 2019-04-11T11:40:04Z

If you don't mind we can collaborate on this PR, i.e. i will push commits to finish it up.

@akosyakov as I am not the author of the PR, I don't really have a say in that ;-). I cross-linked the other issue to make the link for planning purposes.
👍 for working towards making the PR better. That's what reviews are for.

akosyakov · 2019-04-15T10:20:52Z

@akurinnoy let me know if it would be fine with you to work together on this PR

akurinnoy · 2019-04-15T11:24:59Z

@akosyakov I'm OK to work on this PR together. How do we split the work?

akosyakov · 2019-04-15T11:56:19Z

I could work on integration with Theia UI, you with VS Code extensions.

I can look into hooking DebugConfigurationWidget first and preferring vscode launch config over theia if latter is missing, maybe in support compound configurations if it's easy. You can make sure that launch configuration are read and stored in the way how VS Code expects it.

I can open a PR against this PR for my changes.

akosyakov · 2019-04-16T15:16:37Z

packages/preferences/src/browser/folders-preferences-provider.ts

-        if (resourceUri && this.providers.length > 0) {
-            const provider = this.getProvider(resourceUri);
+        if (resourceUri) {
+            const resourceKind = preferenceName === LAUNCH_PROPERTY_NAME ? 'launch' : 'settings';


I wonder why do we check both? settings can provide as well, can't they?

AFAIK VS Code doesn't provide launch from folder's settings in multi-root scope

akosyakov · 2019-04-16T15:18:48Z

packages/preferences/src/browser/folders-preferences-provider.ts

+        const launchProvider = this.getProvider(resourceUri, 'launch');
+        const launch = launchProvider ? launchProvider.getPreferences() : {};
+
+        const result = Object.assign({}, prefs, launch);


Should not the last arg be { launch } and only if launch is defined?

No it shouldn't, see https://github.com/theia-ide/theia/pull/4743/files#diff-ef3f813f6d83ce1df009cf90a504ac82R33

akosyakov · 2019-04-16T15:33:14Z

packages/preferences/src/browser/folders-preferences-provider.ts

 @injectable()
 export class FoldersPreferencesProvider extends PreferenceProvider {

    @inject(WorkspaceService) protected readonly workspaceService: WorkspaceService;
    @inject(FileSystem) protected readonly fileSystem: FileSystem;
    @inject(FolderPreferenceProviderFactory) protected readonly folderPreferenceProviderFactory: FolderPreferenceProviderFactory;

-    private providers: FolderPreferenceProvider[] = [];
+    private providersByKind: Map<ResourceKind, FolderPreferenceProvider[]> = new Map();
+    private resourceKinds: ResourceKind[] = ['launch', 'settings'];

    @postConstruct()
    protected async init(): Promise<void> {
        await this.workspaceService.roots;


I've started rewriting this code. It's unnecessary complicated.

I've started reworking code as we decided earlier, so it will be much simpler.

benoitf · 2019-04-18T09:18:33Z

hello, any news on when we're able to merge this PR ?
21 days seems to be too long.

akosyakov · 2019-04-18T09:24:46Z

It cannot be merged. It does not pass any VS Code compatibility test and breaks Theia UI. I've strarted reworking it here: https://github.com/theia-ide/theia/tree/ak/laung_preferences will open a PR then it is finished

akurinnoy requested review from benoitf, evidolob, akosyakov, mmorhun and elaihau March 28, 2019 08:13

akurinnoy changed the title ~~Allow retrieving launch configurations using Plugin API~~ Retrieving launch configurations using Plugin API Mar 28, 2019

akurinnoy force-pushed the launch-configuration branch from c28e82b to a206590 Compare March 28, 2019 13:05

akosyakov reviewed Mar 29, 2019

View reviewed changes

packages/core/src/browser/preferences/preference-provider.ts Outdated Show resolved Hide resolved

akosyakov reviewed Mar 29, 2019

View reviewed changes

packages/preferences/src/browser/abstract-resource-preference-provider.ts Outdated Show resolved Hide resolved

akosyakov reviewed Mar 29, 2019

View reviewed changes

packages/preferences/src/browser/abstract-resource-preference-provider.ts Outdated Show resolved Hide resolved

akosyakov reviewed Mar 29, 2019

View reviewed changes

packages/preferences/src/browser/folders-preferences-provider.ts Outdated Show resolved Hide resolved

akosyakov reviewed Mar 29, 2019

View reviewed changes

packages/preferences/src/browser/folder-preference-provider.ts Outdated Show resolved Hide resolved

akurinnoy added 5 commits April 1, 2019 13:34

Added ability to retrieve configuration for a resource.

90265e8

Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>

Handle properties that are not valid at the time of reading.

4dd3b91

This allows to re-validate such properties when a corresponding schema has been set. Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>

Added preference provider to handle 'launch.json'

f9634e8

Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>

fixes

397f360

Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>

akurinnoy force-pushed the launch-configuration branch from a206590 to 397f360 Compare April 1, 2019 12:41

akurinnoy mentioned this pull request Apr 2, 2019

Plugin User Preferences are ignored #4773

Closed

akosyakov reviewed Apr 2, 2019

View reviewed changes

tsmaeder previously requested changes Apr 2, 2019

View reviewed changes

packages/debug/src/browser/abstract-launch-preference-provider.ts Show resolved Hide resolved

l0rd mentioned this pull request Apr 2, 2019

Che 7 GA eclipse-che/che#12696

Closed

mmorhun approved these changes Apr 2, 2019

View reviewed changes

evidolob mentioned this pull request Apr 4, 2019

IDE2-2019-04-30 (Sprint: 165) eclipse-che/che#13054

Closed

50 tasks

akosyakov reviewed Apr 16, 2019

View reviewed changes

benoitf mentioned this pull request Apr 18, 2019

plugins jebbs.plantuml not support #4899

Closed

ashumilova mentioned this pull request Apr 23, 2019

IDE2-2019-05-21 (Sprint: 166) eclipse-che/che#13212

Closed

25 tasks

svor mentioned this pull request Apr 23, 2019

Update vscode-java-debug to latest eclipse-che/che#13216

Closed

akosyakov mentioned this pull request Apr 26, 2019

launch configs as preferences #4947

Merged

11 tasks

akosyakov closed this in #4947 May 2, 2019

akurinnoy deleted the launch-configuration branch July 30, 2019 11:38

Retrieving launch configurations using Plugin API #4743

Retrieving launch configurations using Plugin API #4743

Conversation

akurinnoy commented Mar 28, 2019 • edited Loading

akosyakov commented Mar 28, 2019

akurinnoy commented Mar 28, 2019

akosyakov commented Mar 28, 2019

akosyakov Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Mar 29, 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

akosyakov Mar 29, 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

akosyakov Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

akurinnoy Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

akosyakov Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

akurinnoy Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akurinnoy Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akurinnoy Apr 16, 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

akosyakov commented Mar 29, 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

akosyakov commented Apr 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

akosyakov Apr 2, 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

tsmaeder commented Apr 9, 2019

akosyakov commented Apr 11, 2019 • edited Loading

tsmaeder commented Apr 11, 2019

akosyakov commented Apr 15, 2019

akurinnoy commented Apr 15, 2019

akosyakov commented Apr 15, 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

Choose a reason for hiding this comment

benoitf commented Apr 18, 2019

akosyakov commented Apr 18, 2019

akurinnoy commented Mar 28, 2019 •

edited

Loading

akosyakov Mar 29, 2019 •

edited

Loading

akosyakov Mar 29, 2019 •

edited

Loading

akosyakov Mar 29, 2019 •

edited

Loading

akosyakov Apr 2, 2019 •

edited

Loading

akurinnoy Apr 2, 2019 •

edited

Loading

akosyakov Mar 29, 2019 •

edited

Loading

akurinnoy Mar 29, 2019 •

edited

Loading

akurinnoy Apr 2, 2019 •

edited

Loading

akurinnoy Apr 16, 2019 •

edited

Loading

akosyakov Apr 2, 2019 •

edited

Loading

akosyakov Apr 2, 2019 •

edited

Loading

akosyakov commented Apr 11, 2019 •

edited

Loading

akosyakov commented Apr 15, 2019 •

edited

Loading