Skip to content
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

tasks configs as preferences #5013

Closed
akosyakov opened this issue Apr 26, 2019 · 12 comments · Fixed by #6268
Closed

tasks configs as preferences #5013

akosyakov opened this issue Apr 26, 2019 · 12 comments · Fixed by #6268
Assignees
Labels
preferences issues related to preferences tasks issues related to the task system vscode issues related to VSCode compatibility

Comments

@akosyakov
Copy link
Member

Task extension should not access tasks.json directly but use preference for it.

In VS Code tasks can be defined in a separate file as well as part of setting.json. #4947 allows to contribute such preferences and convert launch configs to them. It's a follow-up issue to do the same for tasks in order to provide VS Code compatibility.

cc @marcdumais-work @elaihau

@akosyakov akosyakov added preferences issues related to preferences tasks issues related to the task system vscode issues related to VSCode compatibility labels Apr 26, 2019
@akosyakov akosyakov self-assigned this Aug 1, 2019
@akosyakov
Copy link
Member Author

I will try to take care about it. It is required to complete #3434 for input variables in tasks: https://code.visualstudio.com/docs/editor/variables-reference#_input-variables

@akosyakov
Copy link
Member Author

I will leave it out for now and open a follow-up issue. It is very involving and deserve a separate PR.

@elaihau
Copy link
Contributor

elaihau commented Aug 23, 2019

@akosyakov could you please advise where to find a tutorial / document that describes how "tasks from a separate file" is supported in vs code? I am interested. Thank you !

@akosyakov
Copy link
Member Author

@elaihau i don't think there is proper docs on it, we've learned by testing and analyzing VS Code repo. The heavy-lifting of mixing preferences from external file or from settings already done by a preference service. You only need to rewrite a task extension that it does not manage tasks.json file anymore, but use a preference service to read it, the same way how the debug extension was refactored.

There is a notice here:

Note: Workspace and Workspace Folder configurations contains launch and tasks settings. Their basename will be part of the section identifier. The following snippets shows how to retrieve all configurations from launch.json:

@elaihau elaihau self-assigned this Sep 2, 2019
@elaihau
Copy link
Contributor

elaihau commented Sep 6, 2019

we had some discussions in the office.

since it is a good amount of work to do all the refactoring, would it be possible to keep the task extension as is, and simply make some modifications to the preference service, so that we can use the preference service as a proxy to access the task config ?

in this way, the task configs wouldn't be stored with prefs or launch configs, but would be accessible from the preferences (or configurations, depending on how you want to call it). @akosyakov

@akosyakov
Copy link
Member Author

akosyakov commented Sep 6, 2019

@elaihau probably, yes, it could be enough to register tasks as preference configuration, i don’t think any modifications to preference service is required, it should not be aware of tasks - no, I don’t think you can avoid it, see #5013 (comment)

It could be error prone though since task extension and vscode extensions read the same state from different sources, not necessary consistent at any given moment of time. And not memory efficient since we have to keep the same data twice in memory.

@akosyakov
Copy link
Member Author

akosyakov commented Sep 6, 2019

Also you have to implement supporting tasks from settings.json and merging them with tasks from tasks.json and properly handling loading them from .theia or .vscode again for the task extension. I think it is easier to delete code from the task extension reading from a file and rely for it on the preferences service which already does everything.

@elaihau
Copy link
Contributor

elaihau commented Sep 6, 2019

Also you have to implement supporting tasks from settings.json and merging them with tasks from tasks.json

earlier today I also noticed that tasks from settings.json could be supported if preference is used to access task configs, however I am not sure how useful it would be:

1, defining tasks in settings.json is not (visibly) supported in vscode UI. also, key-in "tasks" the setting.json does not give me any hints of auto-completion. so I don't think it is a feature documented and exposed to the end user.

2, in theory, vscode extensions can add task configs to the pool through the preference API. but given all the flexibilities of contributing to the tasks pool directly through the tasks API, what is the advantage of going with the implicit way (i.e., using preferences) ?

could you please kindly share some insights in regards to how much we are going to gain from having this in Theia vesus the effort ? @akosyakov

@akosyakov
Copy link
Member Author

akosyakov commented Sep 7, 2019

earlier today I also noticed that tasks from settings.json could be supported if preference is used to access task configs,

What about handling different settings folders? like reading from .vscode instead of .theia? There is PreferenceConfiguration which allow end products to change them. Would we reimplement it as well?

I don't think partial reimplementation of the preference logic in the task extension is the right direction. Otherwise whenever we will need to change the preference logic we will need to update the task extension or think whether it is worth of doing.

I think the right direction is to hide this logic from the task extension in the preference service. It is correct even in corner cases, based on tested code (we have over 800 tests to make sure that preferences are merged in proper order from different folders and settings files) and with less memory consumption.

I don't know whether there are users or VS Code extensions doing it, but i could not exclude that, like one can have shared tasks between workspaces if one defines them in user settings or a vs code extension can modify tasks.json to add such task.

Also what is the effort? Register tasks as a preference contribution, replace watching with an event from PreferenceService and reading with a get call, or something else should be done?

@elaihau
Copy link
Contributor

elaihau commented Sep 9, 2019

Register tasks as a preference contribution, replace watching with an event from PreferenceService and reading with a get call, or something else should be done?

@akosyakov
i am fine with your proposal. please see below and kindly let me know whether i am correct or not - i just want to make sure i am not misunderstanding what you said:

  • task-service and task-configurations can be removed
  • all clients that retrieve task configs from task-service and task-configurations need change, as the task service will no longer be the place where the task configs are fetched from.
  • all task related APIs in the plugin-ext need to be reviewed and adjusted.

@akosyakov
Copy link
Member Author

task-service and task-configurations can be removed

Not remove them, but change to use PreferenceService instead of FileSystem. You can have a look how the debug extension does it: https://github.com/theia-ide/theia/blob/fd8f83efd512fd68fb2cd7323cfa80c2561d7062/packages/debug/src/browser/debug-configuration-manager.ts#L83-L113

all clients that retrieve task configs from task-service and task-configurations need change, as the task service will no longer be the place where the task configs are fetched from.
all task related APIs in the plugin-ext need to be reviewed and adjusted.

no, if you aim for the minimal change with perseverance of APIs as much as possible and only updating implementations of TaskService and TaskConfigurations

elaihau pushed a commit that referenced this issue Sep 26, 2019
- In current Theia, the task extension is responsible for reading,
writing, and watching `tasks.json` files. With this change, the task
extension does not access `tasks.json` files directly, and leaves the
work to the preference extension.
- resolves #5013
elaihau pushed a commit that referenced this issue Sep 26, 2019
- In current Theia, the task extension is responsible for reading,
writing, and watching `tasks.json` files. With this change, the task
extension does not access `tasks.json` files directly, and leaves the
work to the preference extension.
- resolves #5013

Signed-off-by: Liang Huang <liang.huang@ericsson.com>
elaihau pushed a commit that referenced this issue Sep 26, 2019
- In current Theia, the task extension is responsible for reading,
writing, and watching `tasks.json` files. With this change, the task
extension does not access `tasks.json` files directly, and leaves the
work to the preference extension.
- resolves #5013

Signed-off-by: Liang Huang <liang.huang@ericsson.com>
elaihau pushed a commit that referenced this issue Sep 26, 2019
- In current Theia, the task extension is responsible for reading,
writing, and watching `tasks.json` files. With this change, the task
extension does not access `tasks.json` files directly, and leaves the
work to the preference extension.
- resolves #5013

Signed-off-by: Liang Huang <liang.huang@ericsson.com>
elaihau pushed a commit to elaihau/theia that referenced this issue Sep 27, 2019
- In current Theia, the task extension is responsible for reading,
writing, and watching `tasks.json` files. With this change, the task
extension does not access `tasks.json` files directly, and leaves the
work to the preference extension.
- resolves eclipse-theia#5013

Signed-off-by: Liang Huang <liang.huang@ericsson.com>
elaihau pushed a commit that referenced this issue Sep 28, 2019
- In current Theia, the task extension is responsible for reading,
writing, and watching `tasks.json` files. With this change, the task
extension does not access `tasks.json` files directly, and leaves the
work to the preference extension.
- resolves #5013

Signed-off-by: Liang Huang <liang.huang@ericsson.com>
elaihau pushed a commit that referenced this issue Sep 29, 2019
- In current Theia, the task extension is responsible for reading,
writing, and watching `tasks.json` files. With this change, the task
extension does not access `tasks.json` files directly, and leaves the
work to the preference extension.
- resolves #5013

Signed-off-by: Liang Huang <liang.huang@ericsson.com>
akosyakov pushed a commit to akosyakov/theia that referenced this issue Feb 24, 2020
- In current Theia, the task extension is responsible for reading,
writing, and watching `tasks.json` files. With this change, the task
extension does not access `tasks.json` files directly, and leaves the
work to the preference extension.
- resolves eclipse-theia#5013

Signed-off-by: Liang Huang <liang.huang@ericsson.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences issues related to preferences tasks issues related to the task system vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants