-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable Workspace-Scoped Tasks #8917
Enable Workspace-Scoped Tasks #8917
Conversation
4aa6c90
to
c88b5db
Compare
@tsmaeder, I think you took the last thorough pass through the tasks system - I wonder if you'd be willing to take a look at this? I've made some changes to logic in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be working for me under the testing steps outlined and through experimenting with various other workspace configurations and setups. I haven't found any issues with your updates except that I'm not getting any sort of auto-suggest behavior inside the workspace file for tasks
. Tried to track it down but was unsuccessful, not sure why that behavior isn't being enabled.
Otherwise had some minor comments 👌
packages/preferences/src/browser/workspace-file-preference-provider.ts
Outdated
Show resolved
Hide resolved
@colin-grant-work am I understanding correctly that this would also enable workspace-level launch configurations, not just tasks? |
@tsmaeder, workspace-scoped launch configurations are actually already supported, but they're formatted differently than VSCode expects. Instead of
The current system will do something like this:
This PR would adjust the way launch preferences are handled to agree with the format of VSCode's workspace file. |
224091a
to
d205c81
Compare
@kenneth-marut-work, thanks for the review. I've implemented your suggestions and tracked down the bug with the auto-suggestion functionality.
Turned out there was a race condition in the schema update logic. I've implemented a queue to handle it, and you should see suggestions for |
@RomanNikitenko, good catch. That's a bit annoying. The good news is (and please confirm that you also see this behavior), that you can do it only one way at a time. If there are Arranging for either...or is much easier than arranging for both...and. I'll modify the code accordingly. |
d205c81
to
6008a18
Compare
I've pushed code that should replicate the behavior seen in VSCode: it will read sections ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested a little the changes and can confirm - I'm able to run tasks configurations defined in the workspace configuration file - @colin-grant-work thanks a lot for such cool feature!
I faced with some issues within testing the PR, for example:
- I got the error
Global task cannot be customized
at attempt to applyConfigure Task
action to a workspace level task. You could try reproduce it by pressing thegear
icon fromTerminal => Run task
menu - Looks like workspace level tasks are not available from
Terminal => Configure Tasks
menu
@colin-grant-work
could you take a look when you have a chance
I noticed some strange behavior related to launch configurations. Steps to reproduce:
As result I can see that the |
@RomanNikitenko, thanks for taking another look. I will take a closer look at what's happening with the
I'm seeing the same behavior on |
@colin-grant-work |
@danarad05, I'm having some trouble, and it looks like you're the most recent person to touch the code. It looks like #8761 added this code to the theia/packages/core/src/browser/preferences/preference-contribution.ts Lines 393 to 404 in 4204297
The |
6008a18
to
7d36f65
Compare
2e53715
to
0f9f87d
Compare
@kenneth-marut-work, thanks for taking another look. I think I've addressed the issue of the strange opening behavior. A funky workaround had been employed to trick the |
0f9f87d
to
d8924d5
Compare
That sounds reasonable, and it'll mean modifying the |
@colin-grant-work About the task type limitation for Please feel free to edit it or add other required info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the following for a multi-root workspace:
Open Workspace Configuration File
command- adding a task to the workspace config file
- task schema support for
Workspace
scoped tasks Problems
view when there is an error in aWorkspace
scoped task configurationConfigure Task
actionConfigure Task
=>Configure new task
- running a workspace task
- terminate a workspace task
- restart a workspace task
- modify a workspace task configuration and check if the changes is applied for running
I also tested launch
configs related logic:
- adding a launch configuration inside and outside of
settings
field of the workspace config file - schema support for launch configurations at a configuration creation
It works well for me, except #8987 and #9009. But it's out of the scope of the issue.
The PR also fixes #8991
@colin-grant-work thanks a lot!
@kenneth-marut-work |
@colin-grant-work Thanks for the thorough description of changes above. Ended up touching a lot of files, but excited for this new addition. |
@vince-fugnitto, @marechal-p, would one of you be willing to take a look at this to make sure I haven't made any unwise changes to API? The changes to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks pretty good, I just spotted a regression that should be easy to fix.
It would also be good if you re-base it to latest master, as new configurations will now include a label and problem matchers if applicable.
Regards
b597a78
to
42ceb5b
Compare
If there are no objections, I'll merge this tomorrow morning. @alvsan09, are you happy with it as is? |
Hi @colin-grant-work , |
42ceb5b
to
7e9bf9a
Compare
Signed-off-by: Colin Grant <colin.grant@ericsson.com>
7e9bf9a
to
b720460
Compare
protected readonly toDisposeOnDelegateChange = new DisposableCollection(); | ||
protected updateWorkspaceModel(): void { | ||
const newDelegate = this.workspaceService.saved ? this.workspacePreferences : this.folderPreferences; | ||
const effectiveScope = this.workspaceService.saved ? TaskScope.Workspace : this.workspaceService.tryGetRoots()[0]?.resource.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colin-grant-work
Hello!
Could you share what use case here is handled.
I thought that the use case is Untittled (Workspace)
- so unsaved:
but I get newDelegate = this.workspacePreferences
and effectiveScope = TaskScope.Workspace
for such use case.
What steps I should do to get:
- newDelegate = this.folderPreferences;
- effectiveScope = this.workspaceService.tryGetRoots()[0]?.resource.toString();
Thanks in advance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, @RomanNikitenko! As soon as a workspace file exists, the workspace is considered .saved
, even if you haven't officially named it and saved it yourself. The case in which the delegate is folder preference is in a single-root unsaved workspace - i.e. the case in which the user uses 'Open folder...' to open a single folder as the workspace.
There was a slight hiccup with this logic where the TaskConfigurationManager
wasn't switching to the workspace preferences delegate as soon as a workspace file was created, but it that was fixed recently in #9331. It looks like you're seeing that change in action: as soon as you have a workspace file, whether you've titled it or not, the TaskConfigurationManager
starts considering the workspace file to be the source of information for workspace-scoped tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for the quick answer!
one more question:
The case in which the delegate is folder preference is in a single-root unsaved workspace - i.e. the case in which the user uses 'Open folder...' to open a single folder as the workspace.
If user has a single folder - then tasks are considered on the folder level and should be handled here
So, sorry - I still don't see what use case we handle here.
I'm not saying that there is a problem - I'm investigating some area related to multi-root workspace and have found this place - so I just want to clarify it for myself.
Maybe you could provide steps to reproduce, like:
- open a folder
- ....
thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right that all folder-scoped tasks, in both saved and unsaved workspaces, are handled by the folder preference provider. In the case of a single-root unsaved workspace, workspace scope also points to folder scope for the single folder. This parallels the preference system, which also collapses folder and workspace scope in the case of a single-root unsaved workspace. Since I'm not sure what your use case is, I'm not sure what steps to provide to 'reproduce' any particular behavior, but if you want to see how it works, you can do something like this:
- Create a command that does something like this:
{
execute: () => {
console.log("User scope:", this.taskConfigurationManager.getTasks(TaskScope.Global));
console.log("Workspace scope:", this.taskConfigurationManager.getTasks(TaskScope.Workspace));
for (const root of this.workspaceService.tryGetRoots()) {
console.log(`Folder scope (${root.resource.path.base}):`, this.taskConfigurationManager.getTasks(root.resource.toString()));
}
}
}
Try that command in different kinds of workspaces. In a single-root unsaved workspace, the logs for workspace and folder scope should be the same (same _scope
fields, too). In a saved workspace, they should be different. There aren't very many services that interface directly with the TaskConfigurationManager
- mostly its output gets filtered through the TaskService
, which is able to identify the fact that workspace scope and folder scope are duplicated in a single-root workspace (because the label
and _scope
fields are identical), so consumers of the data end up seeing what they should see: one set of folder-scoped tasks.
Conceivably, in a single-root unsaved workspace, we could just return an empty array for workspace tasks, but I prefer to keep the behavior of the various preference-related functionality areas consistent. Since the preferences service collapses workspace scope and folder scope in single-root workspaces, I think tasks (and launch
), should do so, as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for the detailed answer.
I think I was confused for the single-root use case(user have opened just a single folder) because
before your explanation I thought that:
- for the single-root use case tasks should be handled here on the
Folder
level - there is no need to createTaskConfigurationModel
for theWorkspace
scope. - there should be no
Workspace
scoped tasks for the such use case - as they are duplicated - in the example that you provided abovetaskConfigurationManager
returns the same tasks for theWorkspace
scope and for theFolder
level
Thanks again for your time and efforts - now it's more clear for me!
What it does
Fixes #8519
Fixes #8991 (thanks for noting that, @RomanNikitenko)
This PR sets up the necessary infrastructure to read and write tasks to a workspace file. In particular it:
WorkspaceFilePreferenceProvider
reads and writes non-settings preferences. (breaking changes to bring Theia's workspace file schema into harmony with VSCode's)workspace-service.ts
for writing workspace files to accommodate fields other thanfolders
andsettings
(breaking changes for anyone calling those file-builder methods)WorkspaceSchemaUpdater
service to allow packages that contribute extensions to the schema of the workspace file (tasks, debug) to register their extensionsTaskConfigurationManager
How to test
tasks
field to a workspace file. ("tasks": {"tasks": [...]}
- has be double"tasks"
, one for the 'file' label (tasks.json
) and one for the tasks inside)Terminal
menu and selectRun Task...
tasks configure
,tasks run
) and confirm that they still work in each scope in single- and multi-root workspaces.Changes
This PR has grown to touch quite a number of files, so it might be useful for reviewers if I explain why each change was necessary:
preference-configurations.ts
: add anisAnyConfig
convenience method equivalent to checkingisSectionName() || isConfigName()
preference-contribution.ts
: change schema update logic to respect the superset / subset relationships among scopes. See Enable Workspace-Scoped Tasks #8917 (comment).preference-provider.ts
,preference-service.ts
: add section name argument togetConfigUri
andgetContainingConfigUri
to allow callers to find the URI of all preference files, not justsettings.json
. This is more systematic than the current implementation, and obviates workarounds usingPreferenceService.resolve
to find the URI for non-settings sections.debug-schema-updater.ts
: addlaunch
to the workspace file schema.folders-preference-provider.ts
,user-configs-preference-provider.ts
,workspace-preference-provider
: updategetConfigUri
andgetContainingConfigUri
to accommodate section name.section-preference-provider.ts
: minor cleanup to remove a non-null assertion.workspace-file-preference-provider.ts
: changes to handle sections other thansettings
, and to accommodate sections both inside thesettings
object and outside.quick-open-tasks.ts
: refactor theconfigure
method to accommodate workspace-scoped tasks.task-configuration-manager.ts
: rewrite logic to accommodate workspace-scoped tasks.task-configuration-model.ts
: make scope protected because in the case of workspace scope it becomes unreliable (sometimes pointing to folder, sometimes pointing to workspace file). It wasn't being accessed, and the only way to get access to a model is to know the scope you're interested in, so you shouldn't have to ask the model for that information.task-configurations.ts
: change to allow configure to work in workspace scope.task-preferences.ts
: better default value for preferences.task-schema-updater.ts
: make the basic schema for tasks publicly accessible (it is for preferences) so that it can be used for any defaulting. Also add tasks the workspace-file schema.workspace-commands.ts
,workspace-frontend-contribution.ts
: add a command to open the workspace file. It exists in VSCode and is parallel to commands to open e.g. the user-scopetasks.json
.workspace-schema-updater.ts
,workspace-frontend-module.ts
,workspace-service.ts
: add aWorkspaceSchemaUpdater
that can be accessed and updated by external packages (debug, tasks, preferences) to allow them to register their subschemas for the workspace file. Modify helper functions that assume that the schema will only ever includesettings
.Review checklist
Reminder for reviewers