Adding support for TaskProvider.resolveTask#71027

joelday · 2019-03-23T21:29:19Z

First pass on #33523 for early review. Will be working on tests, possibly updating the example extension. Let me know if you have any suggestions for that.

Thanks @dbaeumer @alexr00!

joelday · 2019-03-24T16:54:28Z

Wasn't able to repro the macOS integration test failure.

alexr00 · 2019-03-25T13:20:34Z

It looks like you are going in a good direction! I only took a quick look today. I will give a more thorough reveiw later in the week.

src/vs/workbench/contrib/tasks/electron-browser/task.contribution.ts

joelday · 2019-03-29T19:07:55Z

@alexr00 Do you have any suggestions on which tests/test areas I should add coverage for this with? Thanks!

joelday · 2019-04-03T15:55:57Z

@alexr00 Just an assurance: I'm totally happy to rework this if the acceptance criteria needs to change. Also happy to do some general refactoring if this currently contributes to any debt.

alexr00 · 2019-04-08T12:38:42Z

@joelday, I'm sorry it took me so long to take a look at your PR! Most of it looks good. However, I think you are calling provider.resolveTask in the wrong place. resolveTask is intended to be used to increase task performance.
For example, users probably are most interested in the tasks defined in tasks.json. Calling provideTasks for every task provider can be slow. Instead of calling provideTasks for every provider, we can first call resolveTask for all the tasks in tasks.json and return those first to show in the quick pick.
Or, a simpler example: you can use a custom keybinding to run a specific task. Instead of getting all tasks by calling provideTasks on every provider then finding the task that the user wants, we can just call resolveTask and quickly get the one task they asked for.
So the pattern would be: do we already know which task the user wants? Call resolveTask. Otherwise, get all tasks by calling provideTasks for every provider then let the user pick the task.

alexr00

See my long comment about where resolveTask should be called.

joelday · 2019-04-08T18:26:38Z

@alexr00 I had a feeling that these requirements may have not been 100% right: #33523 (comment)

This should be an easy change and I don't think it'll conflict. I'll need to do some refactoring, largely along how things are asynchronously enumerated/transformed/joined. That function has a lot of complexity.

Thank you!

joelday · 2019-04-10T02:07:22Z

@alexr00 For the specific task run scenario: As I dig into the implications for the various current callers of getGroupedTasks, this gets pretty complex, bleeding into createRunnableTask and its resolution/disambiguation logic, which is currently written against already resolved tasks. Will have to think about it a bit, but I'm unsure when I'll be able to revisit it.

As for the Quick Pick scenario, I'm unsure of the performance benefit if provideTasks still has to be called on each provider either way. Additionally, the task providers don't have any way of knowing if a task previously provided via resolveTask should be excluded from the subsequent call to provideTasks. Right now, resolveTask is only called for tasks that weren't contributed, so there's no de-duplication problem. If resolveTask isn't used for Quick Pick at all, then only contributed tasks will show up, as is the current behavior.

alexr00 · 2019-04-12T08:37:09Z

I agree that this is not a trivial change. There's no rush to get it done though.

For cases like the VS Code repo the quick pick scenario could provide a significant improvement. 9.9 times out of 10 I want to run one of the tasks in the tasks.json. However, I have to wait up to a minute for all of our gulp and typescript tasks to enumerate (gulp in particular is slow to enumerate).

joelday · 2019-04-12T19:16:36Z

@alexr00 absolutely agreed on the impact of the performance for quick pick. Is it possible to present items in a quick pick incrementally? Otherwise I'm not sure how to reconcile presenting both the quickly resolved tasks without still having to wait for the slow contributed ones to finish first.

joelday · 2019-07-02T16:27:38Z

@alexr00 @dbaeumer Just pinging on this. My impression is that this was really a disconnect on the intended design of the Tasks feature entirely, so I'm happy to just decline this PR.

I do have some (hopefully useful) feedback on my experience contributing here, especially in contrast to having worked with @jrieken after I proposed and implemented the insertSnippetAPI.

Based on TaskProvider: resolveTask() never gets called #33523 (comment), I think what the OP was asking for and expected resolveTask to do was pretty clear, but it was definitely missed.
I sought and received confirmation of my implementation before proceeding: TaskProvider: resolveTask() never gets called #33523 (comment). That approach pretty clearly did not satisfy what is now expected.

Should resolveTask only ever be called to speed up the start time and validation of tasks? In my experience, the largest wait is when using quick pick, but this idea seems incompatible with that both from a technical and UX standpoint.

Thanks!

alexr00 · 2019-07-03T08:48:13Z

@joelday I'm sorry I've let this PR lapse. Looking at it again with a fresh set of eyes, I think that it resolveTask is done in the right place. The speed up from the quick pick wouldn't be from adding tasks to it slowly (as you pointed out this isn't an option right now), but from turning off task auto detection for a task type in settings (ex: "gulp.autoDetect": "off",) and allowing the extension to instead only provide tasks for for those that are customized in tasks.json. The advice you got in #33523 (comment) from the original API writer stands. The misunderstanding was mine.

As you say, what #33523 (comment) asks for isn't what resolveTask is intended to do. The issue for the kind of customization that OP wanted is #58836. I'll make some notes in the issue that make that clear.

Having an API that doesn't do anything (resolveTask) is confusing. I would like to get it implemented and properly documented. I'll take another thorough look at your PR today and see if I have any feedback.

alexr00 · 2019-07-03T10:07:18Z

src/vs/workbench/api/browser/mainThreadTask.ts

+			resolveTask: (task: ConfiguringTask) => {
+				const dto = TaskDTO.from(task);
+
+				// Only named tasks can be resolved. (That should be the case, right?)


As far as I can see, not having a name is fine. The important thing is that the task has a definition so that the task provider knows what to do with it.

alexr00 · 2019-07-03T10:17:36Z

src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts

+										try {
+											const resolvedTask = await provider.resolveTask(configuringTask);
+											if (resolvedTask) {
+												result.add(key, resolvedTask);


This adds the task as a contributed task in the quick pick. We need to add it as a configuring task.

dbaeumer · 2019-07-03T10:17:56Z

@joelday & @alexr00 thanks a lot for working on this. Highly appreciated.

alexr00 · 2019-07-03T13:29:18Z

@joelday this works very nicely! I made two very small changes in the areas I left comments while I was testing it. I also selfishly updated our builtin gulp extension to use resolveTask. I always disable gulp task auto detection in my settings, which means in the vscode repo I always get errors when I run tasks since they are in the tasks.json. No more errors!

joelday · 2019-07-03T15:16:32Z

Thanks! Regarding what #58836 asks for, this does allow an extension developer to "abuse" the API for that purpose, so that would be my only concern at this point.

joelday · 2019-07-03T15:18:09Z

Also, please don't take my follow-up to imply impatience. Having a PR to review doesn't change prioritization!

alexr00 · 2019-07-03T15:44:53Z

I am working on debt items this week, and an unimplemented API looked like debt to me. You had good timing :)

Yes, "abuse" is possible. It might be worth adding a check on the task after we get it back from resolveTask to make sure that it's essential/identifying properties are unchanged.

joelday · 2019-07-11T16:14:48Z

@alexr00 Thanks again!

GabeDeBacker · 2019-07-22T13:36:16Z

src/vs/workbench/api/node/extHostTask.ts

+		}
+
+		if (CustomExecutionDTO.is(resolvedTaskDTO.execution)) {
+			await this.addCustomExecution(taskDTO, <vscode.Task2>task);


@joelday & @alexr00 -> Joel, this is the wrong DTO and task. It should be resolvedTaskDTO. and resolvedTaskDTO

If we allow the provider to change the DTO properties, then we have a different task since those properties are the fingerprint of the task.

Adding support for TaskProvider.resolveTask.

5d057c7

joelday changed the title ~~Adding support for TaskProvider.resolveTask. #33523~~ Adding support for TaskProvider.resolveTask Mar 23, 2019

Merge. Resolve conflict with mainThreadTask.ts

e65f3e6

joelday mentioned this pull request Mar 23, 2019

Compiler integration with the Project Explorer (& Task Providers) joelday/papyrus-lang#23

Closed

dbaeumer assigned alexr00 Mar 25, 2019

alexr00 reviewed Mar 25, 2019

View reviewed changes

src/vs/workbench/contrib/tasks/electron-browser/task.contribution.ts Outdated Show resolved Hide resolved

icfjoeld added 2 commits March 26, 2019 08:47

Reorganize task grouping and resolution async code for readability.

ce1a668

Merge branch 'master' into task-provider-resolve-task

5ceabc6

alexr00 added this to the April 2019 milestone Apr 3, 2019

alexr00 requested changes Apr 8, 2019

View reviewed changes

alexr00 modified the milestones: April 2019, May 2019 May 9, 2019

alexr00 modified the milestones: May 2019, June 2019 May 31, 2019

alexr00 modified the milestones: June 2019, Backlog Jul 1, 2019

Merge master

b12ab13

alexr00 reviewed Jul 3, 2019

View reviewed changes

Made small changes and implmented resolveTask for gulp

fe7baeb

alexr00 approved these changes Jul 3, 2019

View reviewed changes

alexr00 merged commit b6089b1 into microsoft:master Jul 3, 2019

alexr00 modified the milestones: Backlog, July 2019 Jul 3, 2019

joelday deleted the task-provider-resolve-task branch July 6, 2019 18:26

GabeDeBacker reviewed Jul 22, 2019

View reviewed changes

GabeDeBacker mentioned this pull request Jul 22, 2019

extHostTasks.ts is using incorrect DTO and TASK in $resolveTask implementation (results in resolved tasks being ignored) #77757

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Conversation

joelday commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joelday commented Mar 24, 2019

Uh oh!

alexr00 commented Mar 25, 2019

Uh oh!

Uh oh!

joelday commented Mar 29, 2019

Uh oh!

joelday commented Apr 3, 2019

Uh oh!

alexr00 commented Apr 8, 2019

Uh oh!

alexr00 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelday commented Apr 8, 2019

Uh oh!

joelday commented Apr 10, 2019

Uh oh!

alexr00 commented Apr 12, 2019

Uh oh!

joelday commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joelday commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexr00 commented Jul 3, 2019

Uh oh!

alexr00 Jul 3, 2019

Choose a reason for hiding this comment

Uh oh!

alexr00 Jul 3, 2019

Choose a reason for hiding this comment

Uh oh!

dbaeumer commented Jul 3, 2019

Uh oh!

alexr00 commented Jul 3, 2019

Uh oh!

joelday commented Jul 3, 2019

Uh oh!

joelday commented Jul 3, 2019

Uh oh!

alexr00 commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joelday commented Jul 11, 2019

Uh oh!

GabeDeBacker Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

alexr00 Jul 29, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joelday commented Mar 23, 2019 •

edited

Loading

alexr00 left a comment •

edited

Loading

joelday commented Apr 12, 2019 •

edited

Loading

joelday commented Jul 2, 2019 •

edited

Loading

alexr00 commented Jul 3, 2019 •

edited

Loading