[tasks] add the display of configured tasks when executing 'configure tasks...' #5472

vince-fugnitto · 2019-06-14T12:26:23Z

What it does

Fixed #5468

added configured tasks when executing the command and menu item for configure tasks...
previously, only provided tasks where being displayed which is inconsistent with VSCode
better multi-root support for configured tasks (display the root as a description in the quick-open)
ensure that provided and configured tasks are unique (no duplicates found, configured and duplicate tasks should not overlap)

How to test

use the npm vscode-builtin-extension plugin when testing
open a workspace (ex: `theia/packages/console)
execute the command Run Task..., notice the correct source npm for the tasks
execute the command Configure Task, notice the same correct source npm for tasks
with the Configure Task quick-open menu present, configure a task (this should open the tasks.json file with a new entry
execute the command Run Task... and verify the newly configured task is found under 'configured' tasks
excute the command Configure Task, only one entry for the previously configured task should be present
repeat the steps for a multi-root workspace

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

Signed-off-by: Vincent Fugnitto vincent.fugnitto@ericsson.com

vince-fugnitto · 2019-06-17T17:18:32Z

Just force-pushed to re-trigger the Gitpod build :)

elaihau · 2019-06-17T17:36:22Z

Did a quick test.

it worked properly when I opened a folder as my workspace, however
it didn't not work in the multi root setting.

vince-fugnitto · 2019-06-17T17:39:52Z

Did a quick test.

it worked properly when I opened a folder as my workspace, however

it didn't not work in the multi root setting.

I'll try on master, perhaps it never worked in a multi-root use case since it doesn't know where to look for the tasks.json :(

RomanNikitenko · 2019-06-17T19:14:46Z

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

            this.items.push(new QuickOpenItem({
                label: 'No tasks found',
                run: (_mode: QuickOpenMode): boolean => false
            }));
        }

-        providedTasks.forEach(task => {
+        [...configuredTasks, ...providedTasks].forEach(task => {


I think about use case when at first user has some detected task, then user configures the task.
Will 'Configure Tasks' action display both tasks after that?
So two tasks with the same label will be displayed.
Is it expected behavior?

@RomanNikitenko what do you believe is the correct behavior?
The configure tasks... menu should display all configured tasks, and provided (detected tasks) which have yet to be configured?

This means if the provided task is configured, it'll no longer show in the menu as detected?

vscode doesn't display both configurations for this case.
"Run Task" menu of Theia displays only configured configuration if both are present (configured and detected)

Thanks! I'll wait for your PR #5313 to resolve before continuing.

RomanNikitenko · 2019-06-17T19:42:28Z

Did a quick test.

it worked properly when I opened a folder as my workspace, however

it didn't not work in the multi root setting.

I'll try on master, perhaps it never worked in a multi-root use case since it doesn't know where to look for the tasks.json :(

Unfortunately you are right #4919

elaihau · 2019-07-24T03:32:23Z

this one should be unblocked once #5777 is merged.

vince-fugnitto · 2019-07-24T12:15:49Z

this one should be unblocked once #5777 is merged.

Sounds good @elaihau, once #5777 is merged I'll revisit this PR again to re-confirm if it works and update it if required :)

vince-fugnitto · 2019-08-01T12:18:34Z

@elaihau I've rebased the PR to take advantage of the changes you made :)
Do you mind re-testing for me?

RomanNikitenko · 2019-08-01T14:25:46Z

I tested the changes after updating the PR.
Looks like type, not source field is used at displaying of configurations

Before the changes:

vince-fugnitto · 2019-08-01T15:18:25Z

Looks like type, not source field is used at displaying of configurations

@RomanNikitenko thank you for reviewing, I now perform the same operation to determine the label as the TaskRunQuickOpenItem.

RomanNikitenko · 2019-08-06T08:57:10Z

@vince-fugnitto
I see that you updated the PR, but for me the items are displayed in the same way as before updating. Please see the video: https://youtu.be/FodCrEOrtw0

Is it something wrong with my assembly?
Could you check it?
Thank you!

vince-fugnitto · 2019-08-06T11:29:10Z

I see that you updated the PR, but for me the items are displayed in the same way as before updating.

@RomanNikitenko thank you for pointing it out to me! I've updated the code and now get the following behavior:

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

RomanNikitenko · 2019-08-06T13:18:50Z

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

@@ -321,7 +324,9 @@ export class TaskConfigureQuickOpenItem extends QuickOpenGroupItem {
    }

    getLabel(): string {
-        return `${this.task._source}: ${this.task.label}`;
+        return this.task._source


I guess for a configured task the name of the root folder will be used for displaying.
Please see here
Is it expected behavior?

I mean if you add some configuration to tasks.json file:

{ "label": "Run my test task", "type": "shell", "command": "./task-long-running" }

For Configure Tasks the task is displayed :

For Run Task the same task is displayed:

RomanNikitenko · 2019-08-06T13:21:47Z

@vince-fugnitto
Could you test the case described here to avoid of displaying tasks with the same label twice

vince-fugnitto · 2019-08-06T13:25:49Z

@RomanNikitenko sure thank you for pointing it out, I'll investigate it more closely.

vince-fugnitto · 2019-08-07T15:50:08Z

I'm still not happy with the support present in a multi-root workspace.
The items should be grouped by root, have a groupLabel present, and have a divider.

Currently

VSCode

vince-fugnitto · 2019-08-12T17:12:20Z

@elaihau do you mind reviewing the PR for me when you get the chance?

akosyakov · 2019-08-13T06:39:35Z

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

    ) {
        super();
+        const stat = this.workspaceService.workspace;
+        this.isMulti = stat ? !stat.isDirectory : false;


you should not cache such things, but always compute them from WorkspaceService to make sure that you read up-to-date state, please review others places in the PR

Generally try to have a single source for the some state and read it from there, don't replicate it. Replication sometimes helpful, usually between processes, but introduces with keeping replica in sync and invalidating it properly.

WorkspaceService has isMultiRootWorkspaceOpened, just call it everywhere

akosyakov · 2019-08-13T06:41:02Z

@vince-fugnitto is not clear from the description how to test, please align with our template and assign more people for review

Fixed #5468 Added `configured` tasks when executing the command and menu item for `configure tasks...`. Previously, only `provided` tasks were displayed which is inconsistent with vscode, and our own implementation present in the command `run task...` which permits configuring all tasks using the `configure` icon. In order to be consistent, and align with vscode and our own implementations, `configured` tasks should also be added to the menu. Triggering the `configure` for any given task opens the `task.json`. - fixed the deprecated import statements - fixed the deprecated unused injection. Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>

vince-fugnitto · 2019-08-13T12:51:16Z

@vince-fugnitto is not clear from the description how to test, please align with our template and assign more people for review

Thanks, I update the PR description based on the new template.

RomanNikitenko

I tested the changes using npm extension:

configured tasks are displayed correctly: comment is fixed
the case described here is fixed
configured and detected tasks are displayed for Configure Tasks action
tried the cases for multi-root workspace

It works well for me! @vince-fugnitto thank you!

lmcbout · 2019-08-13T14:23:47Z

Tested only the single root for now, I found the following when using the procedure "How to test" describe above:
If I delete the file: .theia/task.json --> the task remain in "Configure task" not in "detected task" unless I reload the Front-end

When the file "task.json" is deleted, unable to recreate the file with Configure task unless I reload the Front-end

vince-fugnitto · 2019-08-13T14:26:44Z

Tested only the single root for now, I found the following when using the procedure "How to test" describe above...

That is a behavior currently present in master and is not addressed in the scope of the PR.

lmcbout · 2019-08-13T15:44:31Z

Question:
When we already have some "Task" configured, should we show the list of "Task" as "Configured" and "Detected" when opening the dialog? Or may be have a separator between the already "configured" and the "detected" task ?

vince-fugnitto · 2019-08-13T15:47:11Z

When we already have some "Task" configured, should we show the list of "Task" as "Configured" and "Detected" when opening the dialog? Or may be have a separator between the already "configured" and the "detected" task ?

I assume you mean when executing the Configure Task command which triggers the quick-open menu. VSCode does not display a separator between configured and detected tasks in this menu.

lmcbout · 2019-08-13T15:55:10Z

I assume you mean when executing the Configure Task command which triggers the quick-open menu. VSCode does not display a separator between configured and detected tasks in this menu.

Yes in the Configure Task quick-open menu. I just tested with VSCode, you are right, but still I think a separator would make it better

elaihau · 2019-08-15T02:21:48Z

I tested the following things:

both configured and detected tasks can be configured
no duplications in tasks.json if a detected task is configured twice or more
recent tasks can be configured
works properly in a multi root workspace

elaihau

Thank you for the work !

vince-fugnitto · 2019-08-15T10:27:21Z

Thank you @RomanNikitenko @elaihau for the review.

vince-fugnitto added enhancement issues that are enhancements to current functionality - nice to haves tasks issues related to the task system labels Jun 14, 2019

vince-fugnitto requested review from RomanNikitenko and elaihau June 14, 2019 12:26

vince-fugnitto force-pushed the vf/GH-5468 branch 3 times, most recently from 9963b15 to 1d1c2b1 Compare June 17, 2019 17:01

RomanNikitenko reviewed Jun 17, 2019

View reviewed changes

This was referenced Jun 18, 2019

[tasks] Fix execution of Configured tasks #5313

Closed

display source folder name for detected tasks in quick open #5550

Merged

vince-fugnitto force-pushed the vf/GH-5468 branch from 1d1c2b1 to d87ecf2 Compare August 1, 2019 12:16

vince-fugnitto force-pushed the vf/GH-5468 branch from d87ecf2 to 1beac5e Compare August 1, 2019 15:17

vince-fugnitto force-pushed the vf/GH-5468 branch from 1beac5e to 94305d5 Compare August 6, 2019 11:27

RomanNikitenko reviewed Aug 6, 2019

View reviewed changes

vince-fugnitto force-pushed the vf/GH-5468 branch from 94305d5 to d7283f9 Compare August 7, 2019 15:47

vince-fugnitto force-pushed the vf/GH-5468 branch from d7283f9 to b11fe53 Compare August 7, 2019 15:52

vince-fugnitto force-pushed the vf/GH-5468 branch 2 times, most recently from 0022aa6 to 7ab3104 Compare August 7, 2019 16:38

akosyakov reviewed Aug 13, 2019

View reviewed changes

vince-fugnitto force-pushed the vf/GH-5468 branch from 7ab3104 to eae6569 Compare August 13, 2019 12:34

vince-fugnitto requested a review from lmcbout August 13, 2019 12:51

RomanNikitenko approved these changes Aug 13, 2019

View reviewed changes

elaihau approved these changes Aug 15, 2019

View reviewed changes

vince-fugnitto merged commit e8a29ca into master Aug 15, 2019

vince-fugnitto deleted the vf/GH-5468 branch August 15, 2019 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tasks] add the display of configured tasks when executing 'configure tasks...' #5472

[tasks] add the display of configured tasks when executing 'configure tasks...' #5472

vince-fugnitto commented Jun 14, 2019 •

edited

Loading

vince-fugnitto commented Jun 17, 2019

elaihau commented Jun 17, 2019

vince-fugnitto commented Jun 17, 2019

RomanNikitenko Jun 17, 2019 •

edited

Loading

vince-fugnitto Jun 18, 2019

RomanNikitenko Jun 19, 2019

vince-fugnitto Jun 20, 2019

RomanNikitenko commented Jun 17, 2019

elaihau commented Jul 24, 2019

vince-fugnitto commented Jul 24, 2019

vince-fugnitto commented Aug 1, 2019

RomanNikitenko commented Aug 1, 2019

vince-fugnitto commented Aug 1, 2019

RomanNikitenko commented Aug 6, 2019

vince-fugnitto commented Aug 6, 2019

RomanNikitenko Aug 6, 2019

RomanNikitenko Aug 6, 2019

RomanNikitenko commented Aug 6, 2019 •

edited

Loading

vince-fugnitto commented Aug 6, 2019

vince-fugnitto commented Aug 7, 2019

vince-fugnitto commented Aug 12, 2019

akosyakov Aug 13, 2019

akosyakov Aug 13, 2019

akosyakov commented Aug 13, 2019

vince-fugnitto commented Aug 13, 2019

RomanNikitenko left a comment

lmcbout commented Aug 13, 2019

vince-fugnitto commented Aug 13, 2019

lmcbout commented Aug 13, 2019

vince-fugnitto commented Aug 13, 2019

lmcbout commented Aug 13, 2019

elaihau commented Aug 15, 2019

elaihau left a comment

vince-fugnitto commented Aug 15, 2019

[tasks] add the display of configured tasks when executing 'configure tasks...' #5472

[tasks] add the display of configured tasks when executing 'configure tasks...' #5472

Conversation

vince-fugnitto commented Jun 14, 2019 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

vince-fugnitto commented Jun 17, 2019

elaihau commented Jun 17, 2019

vince-fugnitto commented Jun 17, 2019

RomanNikitenko Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

vince-fugnitto Jun 18, 2019

Choose a reason for hiding this comment

RomanNikitenko Jun 19, 2019

Choose a reason for hiding this comment

vince-fugnitto Jun 20, 2019

Choose a reason for hiding this comment

RomanNikitenko commented Jun 17, 2019

elaihau commented Jul 24, 2019

vince-fugnitto commented Jul 24, 2019

vince-fugnitto commented Aug 1, 2019

RomanNikitenko commented Aug 1, 2019

vince-fugnitto commented Aug 1, 2019

RomanNikitenko commented Aug 6, 2019

vince-fugnitto commented Aug 6, 2019

RomanNikitenko Aug 6, 2019

Choose a reason for hiding this comment

RomanNikitenko Aug 6, 2019

Choose a reason for hiding this comment

RomanNikitenko commented Aug 6, 2019 • edited Loading

vince-fugnitto commented Aug 6, 2019

vince-fugnitto commented Aug 7, 2019

vince-fugnitto commented Aug 12, 2019

akosyakov Aug 13, 2019

Choose a reason for hiding this comment

akosyakov Aug 13, 2019

Choose a reason for hiding this comment

akosyakov commented Aug 13, 2019

vince-fugnitto commented Aug 13, 2019

RomanNikitenko left a comment

Choose a reason for hiding this comment

lmcbout commented Aug 13, 2019

vince-fugnitto commented Aug 13, 2019

lmcbout commented Aug 13, 2019

vince-fugnitto commented Aug 13, 2019

lmcbout commented Aug 13, 2019

elaihau commented Aug 15, 2019

elaihau left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Aug 15, 2019

vince-fugnitto commented Jun 14, 2019 •

edited

Loading

RomanNikitenko Jun 17, 2019 •

edited

Loading

RomanNikitenko commented Aug 6, 2019 •

edited

Loading