Support plugin tasks using 'TaskScope.Workspace' #9032

alvsan09 · 2021-02-05T18:57:13Z

What it does

Fixes #7931

Aligns the TaskScope enum definition with vscode and Theia plugins API see issue Align TaskScope with API object #7931 and corresponding schemas for Theia [2] and vscode [3]
Supports processing of task configurations provided by plugins with
the scope set to TaskScope.Workspace.
Ignores processing of task configurations provided by plugins with the scope set to TaskScope.Global i.e. User tasks.

In addition to the general description above.

Updates quick-open-task to add Workspace tasks provided by plugins to the list of available task to configure (see main menu Terminal -> Configure Tasks ....
Workspace tasks provided by plugins are now stored under the workspace file (for multi-root projects) and on the first root tasks.json file for not multi-root projects.

How to test

Install the test plugin [1] under your plugins folder, or use one of your own.
Use a plugin task provider that contributes tasks configurations of scope Folder, Workspace and Global (e.g. [1] branch TaskScope.workspace, version 0.0.10) and make sure the tasks are presented when selecting from the main menu Terminal -> Configure Tasks...

Check that provided tasks of Scope Workspace are presented for projects using a workspace e.g. multi-root
Check that the selected and provided tasks of scope WorkSpace are stored in the tasks.json file in a root/.theia folder for non multi-root projects.
Note that Global tasks defined by the user are still available, but no Global tasks from the plugin provider shall be available as they are not supported via providers.

References

Plugin used to test, note: branch TaskScope.workspace, version 0.0.10
TaskScope, Theia plugin schema
TaskScope, vscode extensions schema
Make sure that selected tasks are configured and are runnable
Make sure that Global tasks defined by the user still work as before this change

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: Alvaro Sanchez-Leon alvaro.sanchez-leon@ericsson.com

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

vince-fugnitto · 2021-02-05T20:41:28Z

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

+                        this.taskNameResolver,
+                        this.workspaceService,
+                        isMulti,
+                        { showBorder: showBorder }


minor: it can be omitted since the variable has the same name as the option:

Suggested change

{ showBorder: showBorder }

{ showBorder }

No longer applicable in the latest commit, I will keep it in mind for next changes, thanks !

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

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

alvsan09 · 2021-02-12T13:34:32Z

Hi @colin-grant-work and @tsmaeder,
It would be great if you could review this PR,

Regards

colin-grant-work · 2021-02-12T14:59:52Z

It looks like this is going to conflict pretty massively with this PR and it may be worth a discussion to coordinate approach to task scoping.

RomanNikitenko · 2021-02-16T11:47:37Z

It looks like this is going to conflict pretty massively with this PR and it may be worth a discussion to coordinate approach to task scoping.

I see that @colin-grant-work and @alvsan09 are working on tasks related area very close, so just a proposal, If it's OK with everybody: the discussion could be started with the reviewing #8917 by @alvsan09
It might help to detect critical conflict places and discuss the different approaches for both PRs

@alvsan09 @colin-grant-work @vince-fugnitto
wdyt?

alvsan09 · 2021-02-17T01:41:08Z

@RomanNikitenko, did you mean start reviewing #8917 by @colin-grant-work ? or #9032 by @alvsan09 ?

I am opened to both options, mine (#9032 is smaller though), so it depends how we want to proceed, tackle the small or the big one first ? we need to merge a portion of it in any case.

Regards

RomanNikitenko · 2021-02-17T07:24:38Z

@alvsan09
The changes #8917 were reviewed and tested by 2 people, it has two approval.
So I suggested that you could review #8917.

After reviewing - if it's OK from your point of view - we could merge #8917.
If it's not - then within your review we are starting a discussion about different approaches.

It was my proposal, but it's up to you and @colin-grant-work.
You and @colin-grant-work could apply another way which works for you and @colin-grant-work to avoid the conflicts related to tasks area in your PRs.

thanks!

alvsan09 · 2021-02-17T23:49:19Z

Hello @RomanNikitenko and @colin-grant-work,
I am fine with the proposal, I have been pulled out by an internal task lately but I will concentrate on this tomorrow

Regards
/Alvaro

alvsan09 · 2021-03-02T20:31:04Z

Hello @RomanNikitenko and @colin-grant-work,
The PR #8917 is now merged,

Your comments on this implementation are very welcome !

colin-grant-work

Looks good to me for the most part, with a couple of minor requests.

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

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

packages/task/src/browser/task-name-resolver.ts

colin-grant-work · 2021-03-02T21:38:38Z

I am getting unexpected results when using a multi-root workspace. Using the Configure Tasks command brings up this quick pick:

The provided tasks are all being made available under one workspace root, and not for other roots or at the workspace scope. Am I doing something wrong with the plugin?

alvsan09 · 2021-03-02T22:37:07Z

The provided tasks are all being made available under one workspace root, and not for other roots or at the workspace scope. Am I doing something wrong with the plugin?

It seems like the version of the plugin is not right as I don't see any provided workspace tasks in the .gif, the plugin provides three workspace tasks and two folder tasks for the the first root.
I just pushed a commit so the folder scoped tasks could be configured and run foe each folder of a multi-root project, and kept the three workspace tasks.

I have also updated the version to 0.0.10 to validate the version of the example loaded on branch "TaskScope.workspace"

https://github.com/alvsan09/vscode-task-provider-example/tree/TaskScope.workspace

Questions are very welcome !

colin-grant-work · 2021-03-02T22:55:19Z

I have also updated the version to 0.0.10 to validate the version of the example loaded on branch "TaskScope.workspace"

This looks like it's working as intended. You may want to mention that branch and version in the instructions for testing.

colin-grant-work · 2021-03-02T23:00:40Z

With the new version of the plugin, everything seems to be working as expected. Very nice!

alvsan09 · 2021-03-02T23:03:08Z

You may want to mention that branch and version in the instructions for testing.

Good idea !, Done !

alvsan09 · 2021-03-02T23:04:01Z

With the new version of the plugin, everything seems to be working as expected. Very nice!

Sounds Good !, thanks for looking into it ! :-)

alvsan09 · 2021-03-03T00:47:23Z

Hi @colin-grant-work , @RomanNikitenko, @vince-fugnitto , @tsmaeder ,
The current implementation does have a difference in behavior compared to vscode,

Vscode does not save the workspace task in the workspace file but it does it in the first root directory it finds,
the advantage is that provided workspace tasks can always be configured under a root folder (With this approach, provided workspace tasks never get saved under the workspace file).

This implementation in Theia does configure workspace tasks in the workspace file in case of multi root projects, which is kind of expected for workspace tasks, however you don't get to configure them for not multi-root projects although you can still run them.

Any thoughts on that?
Regards
/Alvaro

colin-grant-work · 2021-03-03T16:16:10Z

This implementation in Theia does configure workspace tasks in the workspace file in case of multi root projects, which is kind of expected for workspace tasks, however you don't get to configure them for not multi-root projects although you can still run them.

Any thoughts on that?

Hm... I think it's not very intuitive to find tasks in the Tasks Run options that don't appear in the Tasks Configure options. I think there's a pretty straightforward workaround:

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

Lines 249 to 251 in 997074c

    
           if (this.workspaceService.saved) { 
        
               scopes.push(TaskScope.Workspace); 
        
           }

Here, if you modify the condition in l. 249 to this:

if (this.workspaceService.saved || groupedTasks.get(TaskScope.Workspace.toString())?.length)

detected workspace-scoped tasks will also appear in the Configure option set for a single-root workspace, and if the user selects one of those tasks, it will be written to the tasks.json of the workspace root. That seems consistent with the way workspace scope and folder scope collapse in single-root workspaces.

alvsan09 · 2021-03-03T21:39:30Z

Here, if you modify the condition in l. 249 to this:
if (this.workspaceService.saved || groupedTasks.get(TaskScope.Workspace.toString())?.length)

After the proposed update from @colin-grant-work, the behavior is now equivalent to vscode, Thanks !! :-)

I have pushed a new commit reflecting this change and addressed pending comments.

alvsan09 · 2021-03-04T00:42:48Z

Hi @colin-grant-work ,
I think I have addressed your comments, please let me know in case I missed anything.
Thanks !

colin-grant-work

Looks good to me. 👍

RomanNikitenko

About https://github.com/eclipse-theia/theia/pull/9032/files#r586799552

as long as it's coming from the detected task system, it'll have the property scope, and if the user chooses to add it to the configured task system, it'll have a ._scope

I think it makes sense to add such description to the corresponding fields of an interface. It should be really useful for people who work with tasks related area.
wdyt?

alvsan09 · 2021-03-09T00:22:15Z

as long as it's coming from the detected task system, it'll have the property scope, and if the user chooses to add it to the configured task system, it'll have a ._scope

I think it makes sense to add such description to the corresponding fields of an interface. It should be really useful for people who work with tasks related area.
wdyt?

Hi @RomanNikitenko,
I have added a couple of comments (see 1 and 2 below) to help clarify the uses of _scope and scope, although this may not be super clear as both properties don't belong to the same class / interface.
I would like to phrase it as you did, but I am afraid is not quite accurate as these can take basically three different values string, TaskScope.Global and TaskScope.Workspace, these values don't depend only on the origin (plugin provided vs user provided) so we can have combinations of them.

I hope you are OK with the additional clarifications.
Comments are very welcome !
/Alvaro

alvsan09 · 2021-03-09T01:13:17Z

Hi @colin-grant-work and @RomanNikitenko,
With this solution there is still a difference in behavior compared to vscode,
i.e. In vscode extension / plugin provided workSpace tasks always get configured in a root folder
with this implementation in theia, the plugin provided workspace tasks get configured in the workSpace file for a multi root project otherwise get configured in a root folder.

What do you guys think? should we try to follow vscode approach and always save provided workspace tasks under a root folder?

alvsan09 · 2021-03-09T18:26:16Z

Hi @colin-grant-work, @RomanNikitenko and @vince-fugnitto,
I have added a second commit, so the behavior in vscode is then replicated more closely as plugin provided workspace tasks get always configured under a root folder.

Let me know what you guys think !
Regards
/Alvaro

colin-grant-work · 2021-03-09T19:06:33Z

I have added a second commit, so the behavior in vscode is then replicated more closely as plugin provided workspace tasks get always configured under a root folder.

This falls into a hard category for me of things that VSCode does that don't make a lot of sense. Usually we want to do what VSCode does, but if we distinguish between Workspace and Folder scope, and its possible to contribute tasks for each scope, it also seems like we ought to maintain that distinction consistently: workspace-scoped tasks end up in the workspace file if there is one, folder-local tasks.json if there isn't a workspace file. If we do it the VSCode way, we allow workspace-scoped provided tasks, but then ignore the scope and arbitrarily choose the workspace root to put the task in.

I see a few of courses of action:

The current code with the new commit - treat workspace scope and first-folder scope.
Rather than treating workspace-scope tasks as belonging to only the first root, make them available for all roots.
Break with VSCode and actually use workspace scope if it's available.

I would favor either (2) or (3) over (1) - I can't see any good reason why workspace should mean a particular folder without the user being able to choose which folder it's going to be. I would favor (3) over (2), but I acknowledge that that would be a departure from VSCode's behavior, and so perhaps not consistent with what plugin creators intend - in particular they may be relying on folder-scoping to supply information about the CWD associated with a given task.

colin-grant-work · 2021-03-09T19:09:44Z

packages/task/src/browser/task-configuration-manager.ts

@@ -172,6 +172,11 @@ export class TaskConfigurationManager {
        return this.models.get(scope);
    }

+    findFirstWorkSpaceFolder(): string | undefined {
+        const firstWorkSpaceFolder = Array.from(this.models.keys()).find(mScope => typeof mScope === 'string');
+        return typeof firstWorkSpaceFolder === 'string' ? firstWorkSpaceFolder : undefined;


You should just be able to return the previous line. If nothing is found, the value is undefined; if something is found, it's guaranteed to pass the typeof ... === 'string' check because that was one of the find criteria.

You are right, that was my initial line, although I get the following issue:

Hm... you should always try out your own snippets before suggesting them 😉. To get the type inference to accept the code, you have to add an explicit type guard as your return value inside the find:

Array.from(this.models.keys()).find((mScope): mScope is string => typeof mScope === 'string');

I see, I won't be able to submit this change as I have removed the function as pointed out in another comment !
Thanks for the good information, I will keep it in mind for a next time.

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

alvsan09 · 2021-03-09T20:02:45Z

2. Rather than treating workspace-scope tasks as belonging to only the first root, make them available for all roots.

It seems like workspace tasks should apply to the whole project regardless of the number of roots, so I think one configured instance would be sufficient, so this would be my last choice.

I also like option 3, which relies on the first commit, although your concern is very valid as we may break expectations of the location of the 'cwd'.

@vince-fugnitto, @tsmaeder, @RomanNikitenko, if you can please give us your opinion !

colin-grant-work · 2021-03-09T20:20:09Z

It seems like workspace tasks should apply to the whole project regardless of the number of roots, so I think one configured instance would be sufficient, so this would be my last choice.

I agree that one instance should be enough, but I think it should be up to the user to decide where that goes - we shouldn't just stick it in the first workspace root.

vince-fugnitto · 2021-03-10T15:37:05Z

@vince-fugnitto ...if you can please give us your opinion !

@alvsan09 I believe that we should aim to have better multi-root support and not make the assumption that users want to configure their tasks in the first available root. Based on the docs I believe saving the task under the workspace file would be the best solution, while giving users the possibility to choose from available roots would be a fine alternative as well.

alvsan09 · 2021-03-11T21:28:36Z

Based on the options listed under 1), and previous comments from @colin-grant-work and @vince-fugnitto ,
it seems like we all accept option 3 as a good alternative despite the drawback of potentially causing a bit of rework in certain plugins to make them work for theia and vscode.

@colin-grant-work, @tsmaeder and @RomanNikitenko could you please confirm if this is ok to proceed with this option 3.

in essence:
Workspace tasks provided by vscode extensions always get configured in the root folder when used in vscode.

Commit 1 for this PR reflects the behavior listed as option 3 under comment 1)
and in summary:
Workspace tasks provided by vscode plugins in theia get configured in the workspace file for multi-root projects otherwise get configured under the .theia/tasks.json, this behavior will honor the description as per 2)

RomanNikitenko · 2021-03-12T10:30:28Z

Based on the docs I believe saving the task under the workspace file would be the best solution

Totally agree. From my point of view it's the best option for the current problem.

* Aligns the TaskScope enum definition with vscode see issue [1] and corresponding schemas for Theia [2] and vscode [3] * Supports processing of task configurations provided by plugins with the scope set to TaskScope.Workspace. * Ignores processing of task configurations provided by plugins with the scope set to TaskScope.Global i.e. User tasks. Fixes: eclipse-theia#7931 [1] eclipse-theia#7931 [2] https://github.com/eclipse-theia/theia/blob/2aa2fa1ab091ec36ef851c4e364b322301cddb40/packages/plugin/src/theia.d.ts#L8637 [3] https://github.com/microsoft/vscode/blob/50f907f0ba9b0c799a7c1d2f28a625bf30041636/src/vs/vscode.d.ts#L5923 Signed-off-by: Alvaro Sanchez-Leon <alvaro.sanchez-leon@ericsson.com>

vince-fugnitto

The changes look good to me 👍

vince-fugnitto · 2021-03-17T13:42:26Z

If there is no other feedback I'll merge the pull-request later on during the week 👍

RomanNikitenko

I tested the following:

Workspace and Folder scoped tasks provided by the test plugin are available from Configure Tasks and Run Task menu
I'm able to run the provided by test plugin tasks
Workspace scoped tasks are placed in the workspace config file for a multi-root workspace when I apply Configure Task action
Workspace scoped tasks are placed in the tasks.json file when I apply Configure Task action for a not multi-root workspace
User level tasks are still available for running

It works well for me, thanks!

vince-fugnitto added tasks issues related to the task system vscode issues related to VSCode compatibility labels Feb 5, 2021

vince-fugnitto reviewed Feb 5, 2021

View reviewed changes

alvsan09 force-pushed the AlignTaskScope branch 2 times, most recently from e9e23f5 to 37c5582 Compare February 9, 2021 18:31

alvsan09 changed the title ~~Align TaskScope with plugin API~~ Support plugin tasks using 'TaskScope.Workspace' Feb 9, 2021

vince-fugnitto requested review from tsmaeder, RomanNikitenko and colin-grant-work February 12, 2021 13:35

alvsan09 force-pushed the AlignTaskScope branch from 37c5582 to ef7e296 Compare February 23, 2021 14:33

alvsan09 force-pushed the AlignTaskScope branch 2 times, most recently from 33ac2ea to 8a8b052 Compare March 2, 2021 20:25

colin-grant-work assigned colin-grant-work and unassigned colin-grant-work Mar 2, 2021

colin-grant-work requested changes Mar 2, 2021

View reviewed changes

packages/task/src/browser/provided-task-configurations.ts Outdated Show resolved Hide resolved

packages/task/src/browser/task-configurations.ts Show resolved Hide resolved

packages/task/src/browser/task-name-resolver.ts Outdated Show resolved Hide resolved

alvsan09 force-pushed the AlignTaskScope branch from 8a8b052 to 87128d4 Compare March 3, 2021 21:27

colin-grant-work approved these changes Mar 4, 2021

View reviewed changes

RomanNikitenko reviewed Mar 5, 2021

View reviewed changes

alvsan09 force-pushed the AlignTaskScope branch 2 times, most recently from a3bc8f4 to 680ff5a Compare March 8, 2021 22:30

alvsan09 force-pushed the AlignTaskScope branch from 680ff5a to 493f088 Compare March 9, 2021 18:21

colin-grant-work reviewed Mar 9, 2021

View reviewed changes

alvsan09 force-pushed the AlignTaskScope branch from 493f088 to 37ff92b Compare March 9, 2021 19:51

alvsan09 force-pushed the AlignTaskScope branch from 37ff92b to 14574ba Compare March 16, 2021 13:35

alvsan09 force-pushed the AlignTaskScope branch from 14574ba to d198a28 Compare March 16, 2021 19:02

vince-fugnitto approved these changes Mar 16, 2021

View reviewed changes

RomanNikitenko approved these changes Mar 18, 2021

View reviewed changes

vince-fugnitto merged commit ca2f87c into eclipse-theia:master Mar 19, 2021

vince-fugnitto added this to the 1.12.0 milestone Mar 25, 2021

Support plugin tasks using 'TaskScope.Workspace' #9032

Support plugin tasks using 'TaskScope.Workspace' #9032

Conversation

alvsan09 commented Feb 5, 2021 • edited Loading

What it does

How to test

References

Review checklist

Reminder for reviewers

vince-fugnitto Feb 5, 2021

Choose a reason for hiding this comment

alvsan09 Feb 9, 2021

Choose a reason for hiding this comment

alvsan09 commented Feb 12, 2021 • edited Loading

colin-grant-work commented Feb 12, 2021

RomanNikitenko commented Feb 16, 2021

alvsan09 commented Feb 17, 2021 • edited Loading

RomanNikitenko commented Feb 17, 2021 • edited Loading

alvsan09 commented Feb 17, 2021

alvsan09 commented Mar 2, 2021

colin-grant-work left a comment

Choose a reason for hiding this comment

colin-grant-work commented Mar 2, 2021

alvsan09 commented Mar 2, 2021

colin-grant-work commented Mar 2, 2021

colin-grant-work commented Mar 2, 2021

alvsan09 commented Mar 2, 2021

alvsan09 commented Mar 2, 2021

alvsan09 commented Mar 3, 2021 • edited Loading

colin-grant-work commented Mar 3, 2021

alvsan09 commented Mar 3, 2021

alvsan09 commented Mar 4, 2021

colin-grant-work left a comment

Choose a reason for hiding this comment

RomanNikitenko left a comment • edited Loading

Choose a reason for hiding this comment

alvsan09 commented Mar 9, 2021

alvsan09 commented Mar 9, 2021 • edited Loading

alvsan09 commented Mar 9, 2021

colin-grant-work commented Mar 9, 2021

colin-grant-work Mar 9, 2021

Choose a reason for hiding this comment

alvsan09 Mar 9, 2021

Choose a reason for hiding this comment

colin-grant-work Mar 9, 2021

Choose a reason for hiding this comment

alvsan09 Mar 9, 2021

Choose a reason for hiding this comment

alvsan09 commented Mar 9, 2021 • edited Loading

colin-grant-work commented Mar 9, 2021

vince-fugnitto commented Mar 10, 2021

alvsan09 commented Mar 11, 2021

RomanNikitenko commented Mar 12, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Mar 17, 2021

RomanNikitenko left a comment • edited Loading

Choose a reason for hiding this comment

alvsan09 commented Feb 5, 2021 •

edited

Loading

alvsan09 commented Feb 12, 2021 •

edited

Loading

alvsan09 commented Feb 17, 2021 •

edited

Loading

RomanNikitenko commented Feb 17, 2021 •

edited

Loading

alvsan09 commented Mar 3, 2021 •

edited

Loading

RomanNikitenko left a comment •

edited

Loading

alvsan09 commented Mar 9, 2021 •

edited

Loading

alvsan09 commented Mar 9, 2021 •

edited

Loading

RomanNikitenko left a comment •

edited

Loading