[tasks] Fix execution of Configured tasks #5313

RomanNikitenko · 2019-05-30T14:22:18Z

Change way of filtering task configurations:

'configured' tasks have higher priority than 'detected' at adding to 'recently used' section
do not display 'detected' task for running if 'configured' task is exist with the same label

Do not set ‘source’ of ‘detected’ task for configuration of ‘configured’ task - after saving the task is citizen of tasks.json file, so according to the doc should has corresponding source (path of root folder that the task config comes from)
Do not save '_scope' of 'detected' task at 'configure task' action - this field is not supposed to be used in tasks.json

Related issues: #5064 and #5067

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

benoitf · 2019-06-18T12:35:56Z

@elaihau @akosyakov is it fine for you ?

elaihau · 2019-06-18T14:42:56Z

@vince-fugnitto this one is related to your work in #5472

elaihau · 2019-06-18T15:25:05Z

Sorry for my misleading comments (which has been removed). I am playing with this change in my local env.

elaihau

Worked well when I opened one folder as my workspace.

When i tested in the setting where there are multiple folders in the same workspace, the task list looks different from that of vsCode.

In Theia:

And in vsCode:

elaihau · 2019-06-18T15:51:00Z

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

+    }
+
+    private findConfig(label: string, configs: TaskConfiguration[]): TaskConfiguration | undefined {
+        return configs.find(task => label === task.label);


Not sure if the label comparison is enough. What if we have tasks

from different sources

and with the same label
?

All configurations from tasks.json file have the same source.
I tried to add a configuration with the same label to tasks.json file and got error:

Detected tasks from different sources can have the same label - you are right.
I'll try improve this behavior, thank you!

fixed, please review

i am testing right now

RomanNikitenko · 2019-06-19T05:05:44Z

Worked well when I opened one folder as my workspace.

When i tested in the setting where there are multiple folders in the same workspace, the task list looks different from that of vsCode.

Maybe it's related to known issue #4919 and it never worked in a multi-root use case.
I'll check on master branch

RomanNikitenko · 2019-06-20T17:07:03Z

@elaihau

Worked well when I opened one folder as my workspace.

When i tested in the setting where there are multiple folders in the same workspace, the task list looks different from that of vsCode.

Hello,
I can see that you have:

for Theia case: one recently used, two detected tasks and a few configured
for VS Code case: one recently used, two configured tasks and a lot of detected

Looks like the tasks are different for the cases and maybe it's quickly on gif...

I'm trying to reproduce it and catch the difference, could you help me?
Could you share your test project or the main steps?

Thanks in advance!

elaihau · 2019-06-20T21:38:30Z

Worked well when I opened one folder as my workspace.
When i tested in the setting where there are multiple folders in the same workspace, the task list looks different from that of vsCode.

Maybe it's related to known issue #4919 and it never worked in a multi-root use case.
I'll check on master branch

i did more testing and looks like the problem i found is different from what i initially thought:

let me use {theia}/packages/task/test-resources to make an example
i copied it and renamed it into test-resources_A and test-resources_B. Here is what happened on the master branch:

As you see, only the names of the folders (instead of full paths) are displayed as the source.

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

Things started going wrong when i added "detected tasks":

Full paths show up as the source, which is inconsistent.
To reproduce what the 2nd GIF shows, please ensure you have npm extension (from vs code) installed as a Theia plugin.

So this is not a bug introduced by this pull request.

elaihau · 2019-06-20T22:32:50Z

I fixed it in #5550

RomanNikitenko · 2019-06-21T09:09:04Z

@elaihau thank you very much for your investigation, this was very helpful!

elaihau · 2019-06-26T18:19:20Z

@RomanNikitenko do you want to resolve this comment and merge this PR in near future? I see other PRs dependent on this one. Thanks !

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

RomanNikitenko · 2019-06-27T00:49:33Z

@RomanNikitenko do you want to resolve this comment and merge this PR in near future? I see other PRs dependent on this one. Thanks !

I fixed the comment here.
I guess that tasks.json file can not contain tasks with the same label, so first I’m trying to find configured task by label for ‘recently used’ section. Then I use equals method to find task among detected tasks.

Also I separated changes related to fix task converting, please review.

It should be noted that ‘run task’ items for ‘configured’ tasks don’t look the same as for VS Code, so maybe it makes sense to create the issue and align this behavior.
@akosyakov @elaihau WDYT?
If I’m not mistaken, in master branch they look the same because they are considered not as ‘configured’ here, so 'source' + 'label' are used to name an item.

elaihau · 2019-06-27T13:42:00Z

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

-            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));


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:

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

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 ?

thank you, I'll check mentioned case

I tried to fix the case like:

User runs detected task (the task moves to 'recently used' section)

User configures the task, edits it (adds some args for command, for example) and tries to run the task.

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.

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.

RomanNikitenko · 2019-07-19T09:35:03Z

The behavior for master branch was changed: after configuring of detected task, this one is not present in configured tasks section.
For more details please see #5747
so I'm closing the PR.

RomanNikitenko requested review from benoitf and evidolob as code owners May 30, 2019 14:22

RomanNikitenko added the tasks issues related to the task system label May 30, 2019

RomanNikitenko requested review from elaihau and akosyakov May 30, 2019 14:23

l0rd mentioned this pull request Jun 4, 2019

Che 7 GA eclipse-che/che#12696

Closed

mmorhun mentioned this pull request Jun 11, 2019

Allow not persistent changes for task before run #5312

Closed

vparfonov mentioned this pull request Jun 12, 2019

IDE-2019-07-03 (Sprint: 168) eclipse-che/che#13524

Closed

14 tasks

elaihau reviewed Jun 18, 2019

View reviewed changes

RomanNikitenko mentioned this pull request Jun 19, 2019

Export configurations of Che tasks to config file eclipse-che/che-theia#295

Merged

vince-fugnitto mentioned this pull request Jun 20, 2019

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

Merged

1 task

elaihau mentioned this pull request Jun 20, 2019

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

Merged

[tasks] Fix execution of Configured tasks

e08f734

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

RomanNikitenko force-pushed the fixConfiguredTasks branch from 774f02e to e08f734 Compare June 27, 2019 00:34

elaihau reviewed Jun 27, 2019

View reviewed changes

vparfonov mentioned this pull request Jul 15, 2019

IDE-2019-07-23 (Sprint: 169) eclipse-che/che#13845

Closed

8 tasks

RomanNikitenko closed this Jul 19, 2019

RomanNikitenko deleted the fixConfiguredTasks branch July 19, 2019 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tasks] Fix execution of Configured tasks #5313

[tasks] Fix execution of Configured tasks #5313

RomanNikitenko commented May 30, 2019 •

edited

Loading

benoitf commented Jun 18, 2019

elaihau commented Jun 18, 2019

elaihau commented Jun 18, 2019

elaihau left a comment

elaihau Jun 18, 2019

RomanNikitenko Jun 19, 2019

RomanNikitenko Jun 27, 2019

elaihau Jun 27, 2019

RomanNikitenko commented Jun 19, 2019

RomanNikitenko commented Jun 20, 2019

elaihau commented Jun 20, 2019 •

edited

Loading

elaihau commented Jun 20, 2019

RomanNikitenko commented Jun 21, 2019

elaihau commented Jun 26, 2019

RomanNikitenko commented Jun 27, 2019 •

edited

Loading

elaihau Jun 27, 2019 •

edited

Loading

elaihau Jun 27, 2019

RomanNikitenko Jul 2, 2019

RomanNikitenko Jul 3, 2019

elaihau Jul 4, 2019

RomanNikitenko commented Jul 19, 2019

[tasks] Fix execution of Configured tasks #5313

[tasks] Fix execution of Configured tasks #5313

Conversation

RomanNikitenko commented May 30, 2019 • edited Loading

benoitf commented Jun 18, 2019

elaihau commented Jun 18, 2019

elaihau commented Jun 18, 2019

elaihau left a 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

RomanNikitenko commented Jun 19, 2019

RomanNikitenko commented Jun 20, 2019

elaihau commented Jun 20, 2019 • edited Loading

elaihau commented Jun 20, 2019

RomanNikitenko commented Jun 21, 2019

elaihau commented Jun 26, 2019

RomanNikitenko commented Jun 27, 2019 • edited Loading

elaihau Jun 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

RomanNikitenko commented Jul 19, 2019

RomanNikitenko commented May 30, 2019 •

edited

Loading

elaihau commented Jun 20, 2019 •

edited

Loading

RomanNikitenko commented Jun 27, 2019 •

edited

Loading

elaihau Jun 27, 2019 •

edited

Loading