use task definition as a reference to compare tasks

- Although task users have much flexibility to define tasks through contributing to the Task definitions, TaskConfiguration.equals() and ContributedTaskConfiguration.equals() compare certain properties of task configs, and thus are not reliable enough. With changes in this pull request, Theia uses task definitions as a reference to decide which properties in task configs should be considered in comparison. - fixed #5878 Signed-off-by: Liang Huang <[email protected]>
eclipse-theia · Aug 21, 2019 · 528ded1 · 528ded1
1 parent d059f9e
commit 528ded1
Show file tree

Hide file tree

Showing 8 changed files with 44 additions and 29 deletions.
diff --git a/packages/cpp/src/browser/cpp-task-provider.ts b/packages/cpp/src/browser/cpp-task-provider.ts
@@ -145,6 +145,7 @@ export class CppTaskProvider implements TaskContribution, TaskProvider, TaskReso
     private registerTaskDefinition(): void {
         this.taskDefinitionRegistry.register({
             taskType: CPP_BUILD_TASK_TYPE_KEY,
+            source: 'cpp',
             properties: {
                 required: ['label'],
                 all: ['label']

diff --git a/packages/plugin-ext/src/hosted/node/scanners/scanner-theia.ts b/packages/plugin-ext/src/hosted/node/scanners/scanner-theia.ts
@@ -189,7 +189,7 @@ export class TheiaPluginScanner implements PluginScanner {
         }
 
         if (rawPlugin.contributes!.taskDefinitions) {
-            contributions.taskDefinitions = rawPlugin.contributes!.taskDefinitions!.map(definitionContribution => this.readTaskDefinition(definitionContribution));
+            contributions.taskDefinitions = rawPlugin.contributes!.taskDefinitions!.map(definitionContribution => this.readTaskDefinition(rawPlugin.name, definitionContribution));
         }
 
         if (rawPlugin.contributes!.problemMatchers) {
@@ -372,9 +372,10 @@ export class TheiaPluginScanner implements PluginScanner {
         return result;
     }
 
-    private readTaskDefinition(definitionContribution: PluginTaskDefinitionContribution): TaskDefinition {
+    private readTaskDefinition(pluginName: string, definitionContribution: PluginTaskDefinitionContribution): TaskDefinition {
         return {
             taskType: definitionContribution.type,
+            source: pluginName,
             properties: {
                 required: definitionContribution.required,
                 all: Object.keys(definitionContribution.properties)

diff --git a/packages/task/src/browser/quick-open-task.ts b/packages/task/src/browser/quick-open-task.ts
@@ -16,7 +16,7 @@
 
 import { inject, injectable } from 'inversify';
 import { TaskService } from './task-service';
-import { ContributedTaskConfiguration, TaskInfo, TaskConfiguration } from '../common/task-protocol';
+import { TaskInfo, TaskConfiguration } from '../common/task-protocol';
 import { TaskDefinitionRegistry } from './task-definition-registry';
 import URI from '@theia/core/lib/common/uri';
 import { TaskActionProvider } from './task-action-provider';
@@ -207,23 +207,23 @@ export class QuickOpenTask implements QuickOpenModel, QuickOpenHandler {
 
         const filteredRecentTasks: TaskConfiguration[] = [];
         recentTasks.forEach(recent => {
-            const exist = [...configuredTasks, ...providedTasks].some(t => TaskConfiguration.equals(recent, t));
+            const exist = [...configuredTasks, ...providedTasks].some(t => this.taskDefinitionRegistry.compareTasks(recent, t));
             if (exist) {
                 filteredRecentTasks.push(recent);
             }
         });
 
         const filteredProvidedTasks: TaskConfiguration[] = [];
         providedTasks.forEach(provided => {
-            const exist = [...filteredRecentTasks, ...configuredTasks].some(t => ContributedTaskConfiguration.equals(provided, t));
+            const exist = [...filteredRecentTasks, ...configuredTasks].some(t => this.taskDefinitionRegistry.compareTasks(provided, t));
             if (!exist) {
                 filteredProvidedTasks.push(provided);
             }
         });
 
         const filteredConfiguredTasks: TaskConfiguration[] = [];
         configuredTasks.forEach(configured => {
-            const exist = filteredRecentTasks.some(t => TaskConfiguration.equals(configured, t));
+            const exist = filteredRecentTasks.some(t => this.taskDefinitionRegistry.compareTasks(configured, t));
             if (!exist) {
                 filteredConfiguredTasks.push(configured);
             }
@@ -253,10 +253,8 @@ export class TaskRunQuickOpenItem extends QuickOpenGroupItem {
     }
 
     getLabel(): string {
-        if (this.taskDefinitionRegistry && !!this.taskDefinitionRegistry.getDefinition(this.task)) {
-            return `${this.task._source}: ${this.task.label}`;
-        }
-        return `${this.task.type}: ${this.task.label}`;
+        const type = this.task.taskType || this.task.type;
+        return `${type}: ${this.task.label}`;
     }
 
     getGroupLabel(): string {
@@ -283,7 +281,11 @@ export class TaskRunQuickOpenItem extends QuickOpenGroupItem {
             return false;
         }
 
-        this.taskService.run(this.task._source, this.task.label);
+        if (this.taskDefinitionRegistry && !!this.taskDefinitionRegistry.getDefinition(this.task)) {
+            this.taskService.run(this.task.source, this.task.label);
+        } else {
+            this.taskService.run(this.task._source, this.task.label);
+        }
         return true;
     }
 }

diff --git a/packages/task/src/browser/task-configurations.ts b/packages/task/src/browser/task-configurations.ts
@@ -329,9 +329,16 @@ export class TaskConfigurations implements Disposable {
                     if (this.rawTaskConfigurations.has(rootFolderUri)) {
                         this.rawTaskConfigurations.delete(rootFolderUri);
                     }
-                    const tasks = rawTasks['tasks'].map((t: TaskCustomization | TaskConfiguration) =>
-                        Object.assign(t, { _source: t.source || this.getSourceFolderFromConfigUri(uri) })
-                    );
+                    const tasks = rawTasks['tasks'].map((t: TaskCustomization | TaskConfiguration) => {
+                        if (this.isDetectedTask(t)) {
+                            const def = this.getTaskDefinition(t);
+                            return Object.assign(t, {
+                                _source: def!.source,
+                                _scope: this.getSourceFolderFromConfigUri(uri)
+                            });
+                        }
+                        return Object.assign(t, { _source: this.getSourceFolderFromConfigUri(uri) });
+                    });
                     this.rawTaskConfigurations.set(rootFolderUri, tasks);
                     return tasks;
                 }
@@ -362,7 +369,7 @@ export class TaskConfigurations implements Disposable {
 
         const configFileUri = this.getConfigFileUri(sourceFolderUri);
         const configuredAndCustomizedTasks = await this.getTasks();
-        if (!configuredAndCustomizedTasks.some(t => ContributedTaskConfiguration.equals(t, task))) {
+        if (!configuredAndCustomizedTasks.some(t => this.taskDefinitionRegistry.compareTasks(t, task))) {
             await this.saveTask(configFileUri, task);
         }
 

diff --git a/packages/task/src/browser/task-definition-registry.spec.ts b/packages/task/src/browser/task-definition-registry.spec.ts
@@ -22,6 +22,7 @@ describe('TaskDefinitionRegistry', () => {
     let registry: TaskDefinitionRegistry;
     const definitonContributionA = {
         taskType: 'extA',
+        source: 'extA',
         required: ['extensionType'],
         properties: {
             required: ['extensionType'],
@@ -30,6 +31,7 @@ describe('TaskDefinitionRegistry', () => {
     };
     const definitonContributionB = {
         taskType: 'extA',
+        source: 'extA',
         properties: {
             required: ['extensionType', 'taskLabel', 'taskDetailedLabel'],
             all: ['extensionType', 'taskLabel', 'taskDetailedLabel']

diff --git a/packages/task/src/browser/task-definition-registry.ts b/packages/task/src/browser/task-definition-registry.ts
@@ -78,4 +78,17 @@ export class TaskDefinitionRegistry {
         this.definitions.set(taskType, [...this.getDefinitions(taskType), definition]);
         this.onDidRegisterTaskDefinitionEmitter.fire(undefined);
     }
+
+    compareTasks(one: TaskConfiguration, other: TaskConfiguration): boolean {
+        const oneType = one.taskType || one.type;
+        const otherType = other.taskType || other.type;
+        if (oneType !== otherType) {
+            return false;
+        }
+        const def = this.getDefinition(one);
+        if (def) {
+            return def.properties.all.every(p => p === 'type' || one[p] === other[p]) && one._scope === other._scope;
+        }
+        return one.label === other.label && one._source === other._source;
+    }
 }
diff --git a/packages/task/src/browser/task-service.ts b/packages/task/src/browser/task-service.ts
@@ -27,7 +27,6 @@ import { ProblemManager } from '@theia/markers/lib/browser/problem/problem-manag
 import { WorkspaceService } from '@theia/workspace/lib/browser/workspace-service';
 import { VariableResolverService } from '@theia/variable-resolver/lib/browser';
 import {
-    ContributedTaskConfiguration,
     ProblemMatcher,
     ProblemMatchData,
     TaskCustomization,
@@ -206,7 +205,7 @@ export class TaskService implements TaskConfigurationClient {
         const configuredTasks = await this.getConfiguredTasks();
         const providedTasks = await this.getProvidedTasks();
         const notCustomizedProvidedTasks = providedTasks.filter(provided =>
-            !configuredTasks.some(configured => ContributedTaskConfiguration.equals(configured, provided))
+            !configuredTasks.some(configured => this.taskDefinitionRegistry.compareTasks(configured, provided))
         );
         return [...configuredTasks, ...notCustomizedProvidedTasks];
     }
@@ -225,7 +224,7 @@ export class TaskService implements TaskConfigurationClient {
         if (Array.isArray(tasks)) {
             tasks.forEach(task => this.addRecentTasks(task));
         } else {
-            const ind = this.cachedRecentTasks.findIndex(recent => TaskConfiguration.equals(recent, tasks));
+            const ind = this.cachedRecentTasks.findIndex(recent => this.taskDefinitionRegistry.compareTasks(recent, tasks));
             if (ind >= 0) {
                 this.cachedRecentTasks.splice(ind, 1);
             }

diff --git a/packages/task/src/common/task-protocol.ts b/packages/task/src/common/task-protocol.ts
@@ -33,12 +33,6 @@ export interface TaskConfiguration extends TaskCustomization {
     /** A label that uniquely identifies a task configuration per source */
     readonly label: string;
 }
-export namespace TaskConfiguration {
-    export function equals(one: TaskConfiguration, other: TaskConfiguration): boolean {
-        return (one.taskType || one.type) === (other.taskType || other.type) &&
-            one.label === other.label && one._source === other._source;
-    }
-}
 
 export interface ContributedTaskConfiguration extends TaskConfiguration {
     /**
@@ -53,11 +47,6 @@ export interface ContributedTaskConfiguration extends TaskConfiguration {
      */
     readonly _scope: string | undefined;
 }
-export namespace ContributedTaskConfiguration {
-    export function equals(one: TaskConfiguration, other: TaskConfiguration): boolean {
-        return TaskConfiguration.equals(one, other) && one._scope === other._scope;
-    }
-}
 
 /** Runtime information about Task. */
 export interface TaskInfo {
@@ -139,6 +128,7 @@ export interface TaskClient {
 
 export interface TaskDefinition {
     taskType: string;
+    source: string;
     properties: {
         required: string[];
         all: string[];