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

[tasks] Fix execution of Configured tasks #5313

Closed
wants to merge 1 commit into from
Closed

[tasks] Fix execution of Configured tasks #5313

Show file tree
Hide file tree
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
42 changes: 23 additions & 19 deletions packages/task/src/browser/quick-open-task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ export class QuickOpenTask implements QuickOpenModel, QuickOpenHandler {
async init(): Promise<void> {
const recentTasks = this.taskService.recentTasks;
const configuredTasks = this.taskService.getConfiguredTasks();
const providedTasks = await this.taskService.getProvidedTasks();
const detectedTasks = await this.taskService.getProvidedTasks();

const { filteredRecentTasks, filteredConfiguredTasks, filteredProvidedTasks } = this.getFilteredTasks(recentTasks, configuredTasks, providedTasks);
const { filteredRecentTasks, filteredConfiguredTasks, filteredDetectedTasks } = this.getFilteredTasks(recentTasks, configuredTasks, detectedTasks);
const stat = this.workspaceService.workspace;
const isMulti = stat ? !stat.isDirectory : false;
this.items = [];
Expand All @@ -84,7 +84,7 @@ export class QuickOpenTask implements QuickOpenModel, QuickOpenHandler {
: index === 0 ? true : false
)
})),
...filteredProvidedTasks.map((task, index) =>
...filteredDetectedTasks.map((task, index) =>
new TaskRunQuickOpenItem(task, this.taskService, isMulti, {
groupLabel: index === 0 ? 'detected tasks' : undefined,
showBorder: (
Expand Down Expand Up @@ -187,36 +187,40 @@ export class QuickOpenTask implements QuickOpenModel, QuickOpenHandler {
return `Task id: ${task.taskId}, label: ${task.config.label}`;
}

private getFilteredTasks(recentTasks: TaskConfiguration[], configuredTasks: TaskConfiguration[], providedTasks: TaskConfiguration[])
: { filteredRecentTasks: TaskConfiguration[], filteredConfiguredTasks: TaskConfiguration[], filteredProvidedTasks: TaskConfiguration[] } {
private getFilteredTasks(recentTasks: TaskConfiguration[], configuredTasks: TaskConfiguration[], detectedTasks: TaskConfiguration[])
: { filteredRecentTasks: TaskConfiguration[], filteredConfiguredTasks: TaskConfiguration[], filteredDetectedTasks: TaskConfiguration[] } {

const filteredRecentTasks: TaskConfiguration[] = [];
recentTasks.forEach(recent => {
const exist = [...configuredTasks, ...providedTasks].some(t => TaskConfiguration.equals(recent, t));
if (exist) {
filteredRecentTasks.push(recent);
for (const recent of recentTasks) {
const taskConfig = configuredTasks.find(task => recent.label === task.label) || detectedTasks.find(task => TaskConfiguration.equals(task, recent));
Copy link
Contributor

@elaihau elaihau Jun 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

like i mentioned in the other comment, using label comparison would cause issues that tasks from different sources are indentified as the same task.

This is what happened with your change:

Peek 2019-06-27 09-38

What i did was running test task from test-resources_A and test-resources_B respectively.
We ended up having only one test task in recently used tasks

============================

For your reference, this is what happened on master branch

Peek 2019-06-27 09-36

Copy link
Contributor

Choose a reason for hiding this comment

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

We used to use TaskConfiguration.equals() which compares the label and _source - I guess you might have a good reason to use the label comparison instead. Could you please explain ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you, I'll check mentioned case

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 tried to fix the case like:

  1. User runs detected task (the task moves to 'recently used' section)
  2. User configures the task, edits it (adds some args for command, for example) and tries to run the task.
  3. It's expected for me that modified task will be running, not detected task.
    So at first I try to find configured task by label for ‘recently used’ section. Why by label? Because
    according to the doc for configured task source is the root folder, while for a detected task, it is the name of the provider - after configuring the task has another source - so I can not use equals method for this case. Is it correct?
    Then I use equals method to find task among detected tasks.

But you are right, for a multi-root use case it's not working, so we need to improve filtering tasks for 'recently used' section.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. You are right: When the detected task is written into the tasks.json, it becomes a configured task. We need a machanism to differentiate the configured detected task from the configured task. And to be honest, I am not sure what is the best way to do.

if (!taskConfig) {
continue;
}
});

const filteredProvidedTasks: TaskConfiguration[] = [];
providedTasks.forEach(provided => {
const exist = [...filteredRecentTasks, ...configuredTasks].some(t => TaskConfiguration.equals(provided, t));
const exist = filteredRecentTasks.some(task => TaskConfiguration.equals(task, taskConfig));
if (!exist) {
filteredProvidedTasks.push(provided);
filteredRecentTasks.push(taskConfig);
}
});
}

const filteredConfiguredTasks: TaskConfiguration[] = [];
configuredTasks.forEach(configured => {
const exist = filteredRecentTasks.some(t => TaskConfiguration.equals(configured, t));
const exist = filteredRecentTasks.some(recent => TaskConfiguration.equals(configured, recent));
if (!exist) {
filteredConfiguredTasks.push(configured);
}
});

return {
filteredRecentTasks, filteredConfiguredTasks, filteredProvidedTasks
};
const filteredDetectedTasks: TaskConfiguration[] = [];
detectedTasks.forEach(detected => {
const exist = filteredRecentTasks.some(recent => TaskConfiguration.equals(detected, recent)) ||
configuredTasks.some(configured => configured.label === detected.label);
if (!exist) {
filteredDetectedTasks.push(detected);
}
});

return { filteredRecentTasks, filteredConfiguredTasks, filteredDetectedTasks };
}
}

Expand Down
7 changes: 5 additions & 2 deletions packages/task/src/browser/task-configurations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,10 @@ export class TaskConfigurations implements Disposable {
console.error(`Error parsing ${uri}: error: ${e.error}, length: ${e.length}, offset: ${e.offset}`);
}
} else {
return this.filterDuplicates(tasks['tasks']).map(t => Object.assign(t, { _source: t.source || this.getSourceFolderFromConfigUri(uri) }));
return this.filterDuplicates(tasks['tasks']).map(task => {
const { _source, _scope, ...configuration } = task;
return { ...configuration, _source: this.getSourceFolderFromConfigUri(uri) };
});
}
} catch (err) {
console.error(`Error(s) reading config file: ${uri}`);
Expand Down Expand Up @@ -267,7 +270,7 @@ export class TaskConfigurations implements Disposable {
await this.fileSystem.createFile(configFileUri);
}

const { _source, $ident, ...preparedTask } = task;
const { _source, _scope, $ident, ...preparedTask } = task;
try {
const response = await this.fileSystem.resolveContent(configFileUri);
const content = response.content;
Expand Down