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.

Sign up for GitHub

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

support multi-root for 'Add Configuration' #8066

Closed
wants to merge 1 commit into from
Closed

support multi-root for 'Add Configuration' #8066

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
52 changes: 46 additions & 6 deletions packages/debug/src/browser/debug-configuration-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import URI from '@theia/core/lib/common/uri';
import { Emitter, Event, WaitUntilEvent } from '@theia/core/lib/common/event';
import { EditorManager, EditorWidget } from '@theia/editor/lib/browser';
import { MonacoEditor } from '@theia/monaco/lib/browser/monaco-editor';
import { PreferenceService, StorageService } from '@theia/core/lib/browser';
import { PreferenceService, StorageService, LabelProvider } from '@theia/core/lib/browser';
import { QuickPickService } from '@theia/core/lib/common/quick-pick-service';
import { WorkspaceService } from '@theia/workspace/lib/browser/workspace-service';
import { DebugConfigurationModel } from './debug-configuration-model';
Expand All @@ -37,6 +37,7 @@ import { DebugConfiguration } from '../common/debug-common';
import { WorkspaceVariableContribution } from '@theia/workspace/lib/browser/workspace-variable-contribution';
import { FileSystem, FileSystemError } from '@theia/filesystem/lib/common';
import { PreferenceConfigurations } from '@theia/core/lib/browser/preferences/preference-configurations';
import { PreferenceScope } from '@theia/core/lib/browser/preferences/preference-service';

export interface WillProvideDebugConfiguration extends WaitUntilEvent {
}
Expand Down Expand Up @@ -68,6 +69,9 @@ export class DebugConfigurationManager {
@inject(WorkspaceVariableContribution)
protected readonly workspaceVariables: WorkspaceVariableContribution;

@inject(LabelProvider)
protected readonly labelProvider: LabelProvider;

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

Expand Down Expand Up @@ -189,7 +193,26 @@ export class DebugConfigurationManager {
}
}
async addConfiguration(): Promise<void> {
const { model } = this;
const thismodel = this.model;
if (thismodel) {
console.log(thismodel);
}
let model: DebugConfigurationModel | undefined;
if (this.models.size > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this check is not reliable: I was able to construct a case where I'm not asked which folder I would like to configure:

  1. Open an unrelated folder
  2. Select "Open Workspace" and open a folder looking like this:
   Third
     .vscode
       launch.json
  1. Add a second folder to the workspace looking like this (don't save to a file)
   Second
     untitled.txt
  1. Hit F1->Debug: Add Configuration
  2. Observe: I'm not asked which workspace folder to use: the config goes to Third

I've been able to trace the problem to the fact that the listener for preference changes is never called when a new workspace root is added: theia/packages/debug/src/browser/debug-configuration-manager.ts#88

I am not really sure what the right fix is, though: should we listen to workspace folder changes or should the preferences system send a change?

Copy link
Contributor

Choose a reason for hiding this comment

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

@akosyakov I'd like your opinion on that one.

Copy link
Member

Choose a reason for hiding this comment

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

It sounds fine to listen to workspace root changes, since they are input to updateModels function and seems to be easy fix.

But that we don't fire preference events when roots are changed bother me too. We should check what VS Code extensions will expect. Could you file an issue to investigate?

model = await this.quickPick.show((await this.workspaceService.roots).map(rootFileStat => {
const modelValue = this.models.get(rootFileStat.uri);
const rootUri = new URI(rootFileStat.uri);
return {
label: this.labelProvider.getName(rootUri),
description: this.labelProvider.getLongName(rootUri),
value: modelValue
};
}), {
placeholder: 'Select the workspace folder to add the configuration to'
});
} else {
model = this.model;
}
if (!model) {
return;
}
Expand Down Expand Up @@ -254,21 +277,27 @@ export class DebugConfigurationManager {

protected async doOpen(model: DebugConfigurationModel): Promise<EditorWidget> {
let uri = model.uri;
if (!uri) {
const configFileUri = await this.getFolderConfigurationUri(model);
if (!await this.filesystem.exists(configFileUri.toString())) {
Copy link
Member

Choose a reason for hiding this comment

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

why previous check was not good? if uri is missing, that already means that model does not exist.

uri = await this.doCreate(model);
}
return this.editorManager.open(uri, {
return this.editorManager.open(uri!, {
Copy link
Member

Choose a reason for hiding this comment

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

! why is it needed?, if branch should ensure that uri exists

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 agree with you while the linter doesn't.

without ! it gives me this error

Argument of type 'URI | undefined' is not assignable to parameter of type 'URI'.
  Type 'undefined' is not assignable to type 'URI'.ts(2345)

and

   'Exit with failure status (1): concurrently -n compile,lint -c blue,green "theiaext compile" "theiaext lint"\nerror Command failed with exit code 1.\n',

Copy link
Member

Choose a reason for hiding this comment

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

We should sort out other comments, I don't think that so many changes to doCreate function are required. It seems there are regressions with PreferenceService.

mode: 'activate'
});
}
protected async doCreate(model: DebugConfigurationModel): Promise<URI> {
await this.preferences.set('launch', {}); // create dummy launch.json in the correct place
// create dummy launch.json in the correct place
if (this.workspaceService.isMultiRootWorkspaceOpened) {
await this.preferences.set('launch', {}, PreferenceScope.Folder, model.workspaceFolderUri.toString());
} else {
await this.preferences.set('launch', {});
}
const { configUri } = this.preferences.resolve('launch'); // get uri to write content to it
let uri: URI;
if (configUri && configUri.path.base === 'launch.json') {
uri = configUri;
} else { // fallback
uri = new URI(model.workspaceFolderUri).resolve(`${this.preferenceConfigurations.getPaths()[0]}/launch.json`);
uri = await this.getFolderConfigurationUri(model);
}
const debugType = await this.selectDebugType();
const configurations = debugType ? await this.provideDebugConfigurations(debugType, model.workspaceFolderUri) : [];
Expand All @@ -287,6 +316,17 @@ export class DebugConfigurationManager {
return uri;
}

private async getFolderConfigurationUri(model: DebugConfigurationModel): Promise<URI> {
const paths = this.preferenceConfigurations.getPaths();
for (const configPath of paths) {
const configUri = new URI(model.workspaceFolderUri).resolve(`${configPath}/launch.json`);
if (await this.filesystem.exists(configUri.toString())) {
Copy link
Member

@akosyakov akosyakov Jun 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

why do we need it? if something exist already we should not override it at all, previous fallback was fine to me

doCreate method should assume that it is called when nothing exists

Copy link
Contributor

Choose a reason for hiding this comment

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

The previous version of the PR did not work in the cases where a launch.json existed, but was under .vscode instead of .theia

Copy link
Member

Choose a reason for hiding this comment

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

that sounds like a bug in the preference service, DebugConfigurationModel.uri used to point to a config from which data was loaded or nothing

return configUri;
}
}
return new URI(model.workspaceFolderUri).resolve(`${paths[0]}/launch.json`);
}

protected async provideDebugConfigurations(debugType: string, workspaceFolderUri: string | undefined): Promise<DebugConfiguration[]> {
await this.fireWillProvideDebugConfiguration();
return this.debug.provideDebugConfigurations(debugType, workspaceFolderUri);
Expand Down