Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retrieving launch configurations using Plugin API #4743

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


## v0.5.0

Expand Down
24 changes: 22 additions & 2 deletions packages/core/src/browser/preferences/preference-contribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { inject, injectable, interfaces, named, postConstruct } from 'inversify'
import { ContributionProvider, bindContributionProvider, escapeRegExpCharacters, Emitter, Event } from '../../common';
import { PreferenceScope } from './preference-scope';
import { PreferenceProvider, PreferenceProviderPriority, PreferenceProviderDataChange } from './preference-provider';
import { IJSONSchema } from '../../common/json-schema';

import {
PreferenceSchema, PreferenceSchemaProperties, PreferenceDataSchema, PreferenceItem, PreferenceSchemaProperty, PreferenceDataProperty, JsonType
Expand Down Expand Up @@ -53,6 +54,7 @@ export class PreferenceSchemaProvider extends PreferenceProvider {

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

@akosyakov akosyakov Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@akosyakov akosyakov Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking of content assist I meant this:

launch-content-assist

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@inject(ContributionProvider) @named(PreferenceContribution)
protected readonly preferenceContributions: ContributionProvider<PreferenceContribution>;
Expand Down Expand Up @@ -182,7 +184,7 @@ export class PreferenceSchemaProvider extends PreferenceProvider {
}

protected updateValidate(): void {
this.validateFunction = new Ajv().compile(this.combinedSchema);
this.validateFunction = new Ajv({ schemas: this.remoteSchemas }).compile(this.combinedSchema);
}

validate(name: string, value: any): boolean {
Expand All @@ -193,12 +195,30 @@ export class PreferenceSchemaProvider extends PreferenceProvider {
return this.combinedSchema;
}

setSchema(schema: PreferenceSchema): void {
setSchema(schema: PreferenceSchema, remoteSchema?: IJSONSchema): void {
const changes = this.doSetSchema(schema);
if (remoteSchema) {
this.doSetRemoteSchema(remoteSchema);
}
this.fireDidPreferenceSchemaChanged();
this.emitPreferencesChangedEvent(changes);
}

protected doSetRemoteSchema(schema: IJSONSchema): void {
// remove existing remote schema if any
const existingSchemaIndex = this.remoteSchemas.findIndex(s => !!s.$id && !!s.$id && s.$id !== s.$id);
if (existingSchemaIndex) {
this.remoteSchemas.splice(existingSchemaIndex, 1);
}

this.remoteSchemas.push(schema);
}

setRemoteSchema(schema: IJSONSchema): void {
this.doSetRemoteSchema(schema);
this.fireDidPreferenceSchemaChanged();
}

getPreferences(): { [name: string]: any } {
return this.preferences;
}
Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/browser/preferences/preference-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ export abstract class PreferenceProvider implements Disposable {
protected readonly onDidPreferencesChangedEmitter = new Emitter<PreferenceProviderDataChanges | undefined>();
readonly onDidPreferencesChanged: Event<PreferenceProviderDataChanges | undefined> = this.onDidPreferencesChangedEmitter.event;

protected readonly onDidInvalidPreferencesReadEmitter = new Emitter<{ [key: string]: any }>();
readonly onDidInvalidPreferencesRead: Event<{ [key: string]: any }> = this.onDidInvalidPreferencesReadEmitter.event;

protected readonly toDispose = new DisposableCollection();

/**
Expand Down
157 changes: 157 additions & 0 deletions packages/debug/src/browser/abstract-launch-preference-provider.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
/********************************************************************************
* Copyright (C) 2019 Red Hat, Inc. and others.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0.
*
* This Source Code may also be made available under the following Secondary
* Licenses when the conditions for such availability set forth in the Eclipse
* Public License v. 2.0 are satisfied: GNU General Public License, version 2
* with the GNU Classpath Exception which is available at
* https://www.gnu.org/software/classpath/license.html.
*
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
********************************************************************************/

import { injectable, postConstruct } from 'inversify';
import { PreferenceScope, PreferenceProvider } from '@theia/core/lib/browser';
import { Emitter, Event } from '@theia/core';
import { Deferred } from '@theia/core/lib/common/promise-util';
import { DisposableCollection } from '@theia/core';
import { Disposable } from '@theia/core';

export interface GlobalLaunchConfig {
version: string;
compounds?: LaunchCompound[];
configurations: LaunchConfig[];
}

export namespace GlobalLaunchConfig {
/* tslint:disable-next-line:no-any */
export function is(data: any): data is GlobalLaunchConfig {
return !data || (!!data.version && (!data.compounds || Array.isArray(data.compounds)) && Array.isArray(data.configurations));
}
}

export interface LaunchConfig {
type: string;
request: string;
name: string;

/* tslint:disable-next-line:no-any */
[field: string]: any;
}

export interface LaunchCompound {
name: string;
configurations: (string | { name: string, folder: string })[];
}

export const LaunchPreferenceProvider = Symbol('LaunchConfigurationProvider');
export interface LaunchPreferenceProvider {

readonly onDidLaunchChanged: Event<void>;

ready: Promise<void>;

getConfigurationNames(withCompounds: boolean, resourceUri?: string): string[];

}

export const FolderLaunchProviderOptions = Symbol('FolderLaunchProviderOptions');
export interface FolderLaunchProviderOptions {
folderUri: string;
}

export const LaunchProviderProvider = Symbol('LaunchProviderProvider');
export type LaunchProviderProvider = (scope: PreferenceScope) => LaunchPreferenceProvider;

@injectable()
export abstract class AbstractLaunchPreferenceProvider implements LaunchPreferenceProvider, Disposable {
tsmaeder marked this conversation as resolved.
Show resolved Hide resolved

protected readonly onDidLaunchChangedEmitter = new Emitter<void>();
readonly onDidLaunchChanged: Event<void> = this.onDidLaunchChangedEmitter.event;

protected preferences: GlobalLaunchConfig | undefined;

protected _ready: Deferred<void> = new Deferred<void>();

protected readonly toDispose = new DisposableCollection();

protected readonly preferenceProvider: PreferenceProvider;

@postConstruct()
protected init(): void {
this.preferenceProvider.ready
.then(() => this._ready.resolve())
.catch(() => this._ready.resolve());

this.updatePreferences();
if (this.preferences !== undefined) {
this.emitLaunchChangedEvent();
}

this.toDispose.push(this.onDidLaunchChangedEmitter);
this.toDispose.push(
this.preferenceProvider.onDidInvalidPreferencesRead(prefs => {
if (!prefs || !GlobalLaunchConfig.is(prefs.launch)) {
Copy link
Member

@akosyakov akosyakov Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@akurinnoy akurinnoy Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@akurinnoy akurinnoy Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@akurinnoy akurinnoy Apr 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you seen default settings and explanation near launch property?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
}
if (!prefs.launch && !this.preferences) {
return;
}
this.preferences = prefs.launch;
this.emitLaunchChangedEvent();
})
);
this.toDispose.push(
this.preferenceProvider.onDidPreferencesChanged(prefs => {
if (!prefs || !prefs.launch) {
return;
}
this.updatePreferences();
this.emitLaunchChangedEvent();
})
);
}

protected updatePreferences(): void {
const prefs = this.preferenceProvider.getPreferences();
if (GlobalLaunchConfig.is(prefs.launch)) {
this.preferences = prefs.launch;
}
}

protected emitLaunchChangedEvent(): void {
this.onDidLaunchChangedEmitter.fire(undefined);
}

get ready(): Promise<void> {
return this._ready.promise;
}

dispose(): void {
this.toDispose.dispose();
}

getConfigurationNames(withCompounds = true, resourceUri?: string): string[] {
const config = this.preferences;
if (!config) {
return [];
}

const names = config.configurations
.filter(launchConfig => launchConfig && typeof launchConfig.name === 'string')
.map(launchConfig => launchConfig.name);
if (withCompounds && config.compounds) {
const compoundNames = config.compounds
.filter(compoundConfig => typeof compoundConfig.name === 'string' && compoundConfig.configurations && compoundConfig.configurations.length)
.map(compoundConfig => compoundConfig.name);
names.push(...compoundNames);
}

return names;
}

}
12 changes: 11 additions & 1 deletion packages/debug/src/browser/debug-frontend-module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { ContainerModule, interfaces } from 'inversify';
import { DebugConfigurationManager } from './debug-configuration-manager';
import { DebugWidget } from './view/debug-widget';
import { DebugPath, DebugService } from '../common/debug-service';
import { WidgetFactory, WebSocketConnectionProvider, FrontendApplicationContribution, bindViewContribution, KeybindingContext } from '@theia/core/lib/browser';
import { WidgetFactory, WebSocketConnectionProvider, FrontendApplicationContribution, bindViewContribution, KeybindingContext, PreferenceScope } from '@theia/core/lib/browser';
import { DebugSessionManager } from './debug-session-manager';
import { DebugResourceResolver } from './debug-resource';
import {
Expand All @@ -44,6 +44,10 @@ import './debug-monaco-contribution';
import { bindDebugPreferences } from './debug-preferences';
import { DebugSchemaUpdater } from './debug-schema-updater';
import { DebugCallStackItemTypeKey } from './debug-call-stack-item-type-key';
import { LaunchProviderProvider, LaunchPreferenceProvider } from './abstract-launch-preference-provider';
import { WorkspaceLaunchProvider } from './workspace-launch-provider';
import { UserLaunchProvider } from './user-launch-provider';
import { FoldersLaunchProvider } from './folders-launch-provider';

export default new ContainerModule((bind: interfaces.Bind) => {
bind(DebugCallStackItemTypeKey).toDynamicValue(({ container }) =>
Expand Down Expand Up @@ -85,5 +89,11 @@ export default new ContainerModule((bind: interfaces.Bind) => {
bind(DebugSessionContributionRegistryImpl).toSelf().inSingletonScope();
bind(DebugSessionContributionRegistry).toService(DebugSessionContributionRegistryImpl);

bind(LaunchPreferenceProvider).to(UserLaunchProvider).inSingletonScope().whenTargetNamed(PreferenceScope.User);
bind(LaunchPreferenceProvider).to(WorkspaceLaunchProvider).inSingletonScope().whenTargetNamed(PreferenceScope.Workspace);
bind(LaunchPreferenceProvider).to(FoldersLaunchProvider).inSingletonScope().whenTargetNamed(PreferenceScope.Folder);
bind(LaunchProviderProvider).toFactory(ctx => (scope: PreferenceScope) =>
ctx.container.getNamed(LaunchPreferenceProvider, scope));

bindDebugPreferences(bind);
});
4 changes: 2 additions & 2 deletions packages/debug/src/browser/debug-preferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,14 @@ export class DebugConfiguration {
export const DebugPreferences = Symbol('DebugPreferences');
export type DebugPreferences = PreferenceProxy<DebugConfiguration>;

export function createDebugreferences(preferences: PreferenceService): DebugPreferences {
export function createDebugPreferences(preferences: PreferenceService): DebugPreferences {
return createPreferenceProxy(preferences, debugPreferencesSchema);
}

export function bindDebugPreferences(bind: interfaces.Bind): void {
bind(DebugPreferences).toDynamicValue(ctx => {
const preferences = ctx.container.get<PreferenceService>(PreferenceService);
return createDebugreferences(preferences);
return createDebugPreferences(preferences);
}).inSingletonScope();

bind(PreferenceContribution).toConstantValue({ schema: debugPreferencesSchema });
Expand Down
Loading