Add ability to configure task #4714

RomanNikitenko · 2019-03-26T09:18:28Z

Add ability to configure Task:

provides 'Configure task' action for each 'TaskRunQuickOpenItem'
adds 'Configure Tasks' menu action

'Configure Tasks' menu action displays only provided tasks.
'Run Task' action displays both: configured and filtered provided tasks. Provided tasks are filtered by label: for example, when we have configured and provided tasks with the same label - only configured task is displayed (provided task is still available from 'Configure Tasks' menu)

'Configure task' action allows to copy task configuration to tasks.json file and provides ability to edit this one. Config file will be created if this one does not exist.

Please see a short demo: https://youtu.be/FKkSnWJy2ac

Related issue: eclipse-che/che#12712

Signed-off-by: Roman Nikitenko rnikiten@redhat.com

packages/core/src/browser/quick-open/quick-open-action.ts

akosyakov · 2019-03-26T10:05:49Z

packages/monaco/src/typings/monaco/index.d.ts

@@ -743,8 +743,125 @@ declare module monaco.quickOpen {
        setShowBorder(showBorder: boolean): void;
        getEntry(): QuickOpenEntry | undefined;
    }
+
+    export interface INextIterator<T> {


please expose only what is used and remove everything what is not used in this PR

akosyakov · 2019-03-26T10:07:13Z

packages/monaco/src/typings/monaco/index.d.ts

+        next(): T | null;
+    }
+
+    export interface ITree {


Could we avoid exposing it?

RomanNikitenko · 2019-03-27T09:49:11Z

@akosyakov I removed all extra objects and updated the PR. Could you review?

akosyakov · 2019-03-27T10:34:55Z

@RomanNikitenko monaco code looks good, thank you 👍

will test in the afternoon, @elaihau could you test as well when you have a chance, you know more about tasks

akosyakov · 2019-03-27T10:39:55Z

packages/task/src/browser/quick-open-task.ts

        );
+
+        this.actionProvider = this.items.length ? this.taskActionProvider : this.noActionProvider;


Do we need NoActionProvider? How about assigning undefined instead?

I used NoActionProvider to avoid potential NPE, but I reviewed code - we have check, so I removed this entry.

akosyakov · 2019-03-27T12:57:48Z

@RomanNikitenko Do I understand correctly that it's basically cover this section of VS Code: https://code.visualstudio.com/Docs/editor/tasks#_customizing-autodetected-tasks

So I can test it against grunt built-in extension for example?

akosyakov · 2019-03-27T13:45:23Z

It works nicely for Run Task, but missing for task prefix in the quick palette. It seems to be the same widget in VS Code in both cases. Could we do the same?

VS Code:

Theia:

Also icon is different, is there a way to reuse from VS Code? You don't need to wait for CQ to be approved, but have to file one if you copy an icon. It would be approved eventually, everything in VS Code is under MIT.

akosyakov · 2019-03-27T13:51:24Z

There are also visual differences to VS Code. It would be nice to align, but separately. cc @elaihau

VS Code:

Theia:

placeholder is different
recently used tasks is missing
configured -> configured tasks
provided -> detected tasks
grouping of configured and detected tasks to separate them
there is not point to show a workspace for a task description if there is only one root
if a workspace root is shown it should be aligned for configured and detected tasks

akosyakov · 2019-03-27T14:00:57Z

packages/task/src/browser/task-action.ts

+import { QuickOpenBaseAction, QuickOpenActionProvider, QuickOpenAction } from '@theia/core/lib/browser/quick-open/quick-open-action';
+
+@injectable()
+export class ConfigureTaskAction extends QuickOpenBaseAction {


please keep file named after main class, i.e. task-action-provider.ts, it is a part of naming conventions for Theia to simplify searching classes by file names

please have a look at other files and rename where it is appropriate

task-action.ts ---> task-action-provider.ts
quick-open-action.ts ---> quick-open-action-provider.ts

elaihau · 2019-03-27T14:02:15Z

It works nicely for Run Task, but missing for task prefix in the quick palette. It seems to be the same widget in VS Code in both cases. Could we do the same?

Yes we can. Sorry I am in the team training today, will test once it is done.

akosyakov · 2019-03-27T14:05:38Z

packages/core/src/browser/quick-open/quick-open-action.ts

+    protected actionRadio: boolean;
+
+    constructor(
+        @unmanaged() id: string,


you could use Options pattern like for QuickOpenItem: https://github.com/theia-ide/theia/blob/7edb5f2e9a94272360404d0f48f9a1b88989e9b1/packages/core/src/browser/quick-open/quick-open-model.ts#L54

akosyakov · 2019-03-27T14:06:42Z

packages/core/src/browser/quick-open/quick-open-action.ts

+    enabled: boolean;
+    checked: boolean;
+    radio: boolean;
+    // tslint:disable-next-line:no-any


run(event?: { item?: QuickOpenItem }): PromiseLike<any>; would it be correct type?

Done: I added MonacoQuickOpenAction to provide ability to use QuickOpenItem here

akosyakov · 2019-03-27T14:07:02Z

packages/core/src/browser/quick-open/quick-open-action.ts

+}
+
+export interface QuickOpenActionProvider {
+    // tslint:disable-next-line:no-any


hasActions(item: QuickOpenItem): boolean; getActions(item: QuickOpenItem): Promise<QuickOpenAction[]>;

Would it be correct item type?

akosyakov · 2019-03-27T14:07:51Z

packages/task/src/browser/task-action.ts

+
+import { injectable, inject } from 'inversify';
+import { TaskService } from './task-service';
+import { QuickOpenBaseAction, QuickOpenActionProvider, QuickOpenAction } from '@theia/core/lib/browser/quick-open/quick-open-action';


It would be nice to reexport them in @theia/core/lib/browser/quick-open.

akosyakov · 2019-03-27T14:08:11Z

packages/task/src/browser/task-action.ts

+        super('configure:task', '', 'fa fa-cog');
+    }
+
+    // tslint:disable-next-line:no-any


async run(event?: { item: QuickOpenItem }): Promise<void> { if (event && event.item instanceof TaskRunQuickOpenItem) {

?

akosyakov · 2019-03-27T14:09:09Z

packages/task/src/browser/task-frontend-contribution.ts

+        registry.registerCommand(
+            TaskCommands.TASK_CONFIGURE,
+            {
+                isEnabled: () => true,


bwt isEnabled is true by default as well as isVisible

akosyakov · 2019-03-27T14:10:45Z

packages/task/src/browser/task-configurations.ts

@@ -56,7 +58,11 @@ export class TaskConfigurations implements Disposable {

    constructor(
        @inject(FileSystemWatcher) protected readonly watcherServer: FileSystemWatcher,
-        @inject(FileSystem) protected readonly fileSystem: FileSystem
+        @inject(FileSystem) protected readonly fileSystem: FileSystem,
+        @inject(WorkspaceService) protected readonly workspaceService: WorkspaceService,


please add new dependenices always vis property injection, otherwise it is a breaking change, see inversify conventions : https://github.com/theia-ide/theia/wiki/Coding-Guidelines#property-injection

Fixed, thank you!

elaihau · 2019-03-27T15:35:43Z

one thing that i found was, the "configure task" button put everything in the task object into task.json, regardless of the place that the task comes from.

this is what vscode does for configuring detected tasks:

And this is theia (same workspace & same task):

in spite of the difference / inconsistency, i would say we could probably leave it to the future PRs to resolve, as we are not supporting the problem matchers, problem patterns, or task definitions at the moment. we will need to pass more information through plugin-ext, and i am currently working on it.

akosyakov · 2019-03-27T15:48:00Z

@elaihau ok, would be good to have follow-up issues for #4714 (comment), #4714 (comment) and #4714 (comment) that all these things get fixed eventually

RomanNikitenko · 2019-03-28T07:39:45Z

@akosyakov @elaihau thank you for your comments, I will fix and update the PR today.

RomanNikitenko · 2019-04-01T09:12:47Z

There are also visual differences to VS Code. It would be nice to align, but separately. cc @elaihau

VS Code:

Theia:

placeholder is different

recently used tasks is missing

configured -> configured tasks

provided -> detected tasks

grouping of configured and detected tasks to separate them

there is not point to show a workspace for a task description if there is only one root

if a workspace root is shown it should be aligned for configured and detected tasks

Changes:

placeholder ---> Select the task to run
configured -> configured tasks
provided -> detected tasks
VS Code icon is used for dark and light themes

please see the short video https://youtu.be/Hhlls33wqmQ

btw, where I should create CQ for VS code icon? Do you have some template/sample?
thank you!

akosyakov · 2019-04-01T09:31:59Z

@RomanNikitenko you can follow https://github.com/theia-ide/theia/wiki/Registering-CQs#case-3rd-party-project-code-copiedforked-from-another-project-into-eclipse-theia-maintained-by-us to file a CQ

akosyakov · 2019-04-01T09:32:58Z

CHANGELOG.md

@@ -5,6 +5,7 @@
 - [cpp] added new clang-tidy and clang-tidy-checks preferences to lint cpp program when clangd v9+ is used.
 - [plugin] `workspace.openTextDocument` API now respects the contributed `FileSystemProviders`
 - [search-in-workspace] added a new preference `search.lineNumbers` to control whether to show line numbers for search results.
+- [task] added support to configure tasks


please move to 0.6.0

akosyakov · 2019-04-01T09:40:11Z

packages/task/src/browser/quick-open-task.ts

+
+    getDescription(): string {
+        if (this.task._scope) {
+            return new URI(this.task._scope).path.toString();


use LabelProvider.getLongName to get human readable representation of a URI relative the workspace

akosyakov · 2019-04-01T09:45:37Z

packages/task/src/browser/task-configurations.ts

+
+        const resource = await this.resourceProvider(new URI(configFileUri));
+        if (resource.saveContents) {
+            await resource.saveContents(JSON.stringify({ tasks: preparedTasks }, undefined, 4));


We should try to preserve formatting, like here: https://github.com/theia-ide/theia/blob/3886397750b34e6cb65976578c4eb848639adfae/packages/workspace/src/browser/workspace-service.ts#L363-L364

You can use Resource.save and provide delta as well as full context. It knows when apply what.

just tested, changing tasks.json and then configuring a new task via Run Task breaks user formatting

akosyakov

it works well, except breaking formatting #4714 (comment)

Please take case about it and merge. I would open an issue for remaining UI improvements.

@elaihau is it fine from your side?

akosyakov · 2019-04-01T10:24:22Z

Follow-up issues: #4768, #4769 and #4770

elaihau

looks good to me, thank you !

Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>

RomanNikitenko requested review from azatsarynnyy, akosyakov and elaihau March 26, 2019 09:18

RomanNikitenko requested review from benoitf and evidolob as code owners March 26, 2019 09:18

akosyakov reviewed Mar 26, 2019

View reviewed changes

packages/core/src/browser/quick-open/quick-open-action.ts Outdated Show resolved Hide resolved

akosyakov reviewed Mar 26, 2019

View reviewed changes

RomanNikitenko force-pushed the configureTask branch from 68884dc to 4e53d4e Compare March 27, 2019 09:04

akosyakov reviewed Mar 27, 2019

View reviewed changes

akosyakov mentioned this pull request Mar 27, 2019

tasks contributed by VS Code extensions don't work #4216

Closed

akosyakov reviewed Mar 27, 2019

View reviewed changes

akosyakov reviewed Apr 1, 2019

View reviewed changes

akosyakov approved these changes Apr 1, 2019

View reviewed changes

elaihau approved these changes Apr 1, 2019

View reviewed changes

l0rd mentioned this pull request Apr 2, 2019

Che 7 GA eclipse-che/che#12696

Closed

RomanNikitenko force-pushed the configureTask branch 2 times, most recently from 2d4537e to 6ee2014 Compare April 2, 2019 14:17

azatsarynnyy approved these changes Apr 2, 2019

View reviewed changes

Add ability to configure task

1047237

Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>

RomanNikitenko force-pushed the configureTask branch from 6ee2014 to 1047237 Compare April 3, 2019 05:47

RomanNikitenko merged commit 0dab9af into master Apr 3, 2019

RomanNikitenko deleted the configureTask branch April 3, 2019 06:12

RomanNikitenko mentioned this pull request Jun 4, 2019

When editing the pre-defined tasks, all tasks should be displayed in the tasks.json file eclipse-che/che#13392

Closed

		);

		this.actionProvider = this.items.length ? this.taskActionProvider : this.noActionProvider;

Add ability to configure task #4714

Add ability to configure task #4714

Conversation

RomanNikitenko commented Mar 26, 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

RomanNikitenko commented Mar 27, 2019

akosyakov commented Mar 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov commented Mar 27, 2019 • edited Loading

akosyakov commented Mar 27, 2019 • edited Loading

akosyakov commented Mar 27, 2019 • edited Loading

akosyakov Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elaihau commented Mar 27, 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

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 27, 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

elaihau commented Mar 27, 2019 • edited Loading

akosyakov commented Mar 27, 2019

RomanNikitenko commented Mar 28, 2019

RomanNikitenko commented Apr 1, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov commented Apr 1, 2019

elaihau left a comment

Choose a reason for hiding this comment

RomanNikitenko commented Mar 26, 2019 •

edited

Loading

akosyakov commented Mar 27, 2019 •

edited

Loading

akosyakov commented Mar 27, 2019 •

edited

Loading

akosyakov commented Mar 27, 2019 •

edited

Loading

akosyakov Mar 27, 2019 •

edited

Loading

akosyakov Mar 27, 2019 •

edited

Loading

elaihau commented Mar 27, 2019 •

edited

Loading

RomanNikitenko commented Apr 1, 2019 •

edited

Loading