use task definition as a reference to compare tasks #5975

elaihau · 2019-08-18T04:51:09Z

Although task users have much flexibility to define tasks through
contributing to the Task definitions, TaskConfiguration.equals() and
ContributedTaskConfiguration.equals() compare certain properties of task
configs, and thus are not reliable enough. With changes in this pull
request, Theia uses task definitions as a reference to decide which
properties in task configs should be considered in comparison.
fixed Incorrect displaying of configured task after refreshing page #5878

Signed-off-by: Liang Huang liang.huang@ericsson.com

How to test

Use npm extension
Configure one or more npm tasks, and refresh page
Go to Terminal -> Run Task menu to check if the configured task(s) are displayed correctly
Check if the configured task(s) can be run

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

elaihau · 2019-08-21T02:31:05Z

@theia-ide/task-extension @theia-ide/plugin-system
This pull request is ready for review.

RomanNikitenko

Hello!
I tested the changes and can not reproduce the issue #5878
thank you!

but I faced with another problem, please see video
before changes: https://youtu.be/VCCnIkjE9vQ
after changes: https://youtu.be/RFB3bkZn-tk

So:

displaying of a typescript task is different
I can not get output for a typescript task

Could you check it?
thanks in advance!

elaihau · 2019-08-22T16:27:54Z

Thank you for your time! I will check what I did wrong.

elaihau · 2019-08-23T00:47:26Z

Hello!
I tested the changes and can not reproduce the issue #5878
thank you!

but I faced with another problem, please see video
before changes: https://youtu.be/VCCnIkjE9vQ
after changes: https://youtu.be/RFB3bkZn-tk

So:

displaying of a typescript task is different

I can not get output for a typescript task

Could you check it?
thanks in advance!

@RomanNikitenko I checked the latest master 06346e5 and found it populated the same error when running the tsc.
The error came from terminal extension, and looked related to #3791

I am curious which version of Theia you used to make the first video ? Could you please help me check?

RomanNikitenko · 2019-08-23T08:33:11Z

@elaihau

@RomanNikitenko I checked the latest master 06346e5 and found it populated the same error when running the tsc.

For detected tsc task - yes, but not for the configured tsc task.
I have no answer why configured tsc task is working , but detected tsc task is not working for master branch. I need to investigate what's difference. But for your branch configured tsc task is not working for me.
Could you try to configure a tsc task and try to run for master branch and for you branch.

I am curious which version of Theia you used to make the first video ?

I guess you tried to run detected task, please try to run configured task.
I can run configured task and get output for the latest master aa00fe7
Please see video and let me know if you can not reproduce it: https://youtu.be/pbEQJtVALIQ

thank you!

elaihau · 2019-08-25T04:23:45Z

@RomanNikitenko thank you for all the details ! yes i can finally reproduce the problem.

The latest version of my code in this pull request should have fixed it. Not only the "configured tsc" but also the "detected tsc" can be started from Theia.

Please take a look when you get the chance, and kindly let me know if you have any suggestions.

Thanks !

RomanNikitenko

@elaihau
Hello!
I tested the PR changes after update and can confirm that:

the bug Incorrect displaying of configured task after refreshing page #5878 is fixed
I can run and get output for detected tsc task as well as for configured one

Do we have some reasons to change displaying of typescript tasks?
I mean that typescript tasks for Terminal -> Run Task menu are displayed:

For Configure Tasks:

For VS Code:

So the behavior for Configure Tasks is closer to VS Code behavior than for Run Task.
I guess it's related to changes for TaskRunQuickOpenItem.
For TaskConfigureQuickOpenItem as label still this way is used.

It works well for me, thank you, just to know if we have some reasons to change way of displaying items...

elaihau · 2019-08-27T11:19:15Z

@RomanNikitenko
the task labels are displayed correctly, however the tasks are not sorted properly

is this what you suggested in the comment?

I quickly looked at the code and found we don't have extra sorting logic for the displayed tasks in the quick open. the reason I changed the task label is that, the "task type" could possibly be overwritten after task resolution, and "tsc: run" would be displayed as "shell: run" after the task is run once.

I could create a separate issue in github and address it in a separete pull request. are you OK with that? After all, the sorting might be a little beyond the scope the #5878

RomanNikitenko · 2019-08-27T11:28:45Z

@elaihau
I mean that labels for typescript tasks are displayed:

typescript:build - tsconfig.json for Run Task
tsc:build - tsconfig.json for Configure Tasks
tsc:build - tsconfig.json for VS Code

So the question is - why do we display it like typescript: build for Run Task instead of tsc:build

elaihau · 2019-08-27T12:06:46Z

@RomanNikitenko wow thank you for finding it out and pinpoint the root cause.

I can revert my changes to getLabel() to make sure task labels are consistent.

I will create a separate GH issue for the problem I described.

RomanNikitenko · 2019-08-27T13:50:30Z

@elaihau
The displaying of tsc tasks are fixed after update.

Could you try the following case:

Configure a npm task
Run the configured task.
Go to Run Task :

You can see that recently used section is absent and the task is in the configured section, not in the recently used section.

Refresh page and rerun the same task
Go to Run Task :

You can see that the task in the recently used section, but looks like:

Please see the video: https://youtu.be/DixaBLnl1M8

Can you reproduce it? Should I create the separate issue for the case?

elaihau · 2019-08-27T14:03:49Z

Yes I am aware of this problem. And that is why I changed getLabel() function in the quick-open-task.ts.

The problem comes from the vscode extension typescript-language-features, where the type of tasks is “tsc” in the Task instantiation, while the type in the task definition is “typescript”.

Therefore, if I want the tsc tasks to be added to the “recently run”, we have inconsistent labels. And if we want consistent labels in the display, they are not added to the “recently run”. It’s really annoying.

Luckily enough, this only happens to detected tasks from “typescript-language-features”. The ones from npm and grunt are not impacted.

Maybe I should take a deeper look into this problem. If you have time please create an issue in GitHub and I will tackle it after the “fetchTask()” pull request.

**With all the given information, do you prefer “tsc build” or “typescript build” in the quick open ? **

“tsc build” is consistent with vscode with the bug you described, whereas “typescript build” is inconsistent with vscode.

Your comments are really helpful. Thank you!

elaihau · 2019-08-27T14:17:22Z

Let’s keep this pull request open. I prefer to fix it in this one instead of leaving it to the future :)

RomanNikitenko · 2019-08-27T14:54:48Z

**With all the given information, do you prefer “tsc build” or “typescript build” in the quick open ? **

Do you mean - we don't have the problem described here if we have your changes for getLabel()?
If so - personally I think it's better to have inconsistent labels for one extension to avoid the bug.

In general, when I do review I don't have the goal to block some PR - my goal is to identify the potential problems before merge some changes. Then the author, reviewers, code owners can assess and discuss - should it be fixed within the current PR or we should create an issue for detected problem and fix it in separate PR.
So - I don't mind to have inconsistent labels for one extension to avoid the bug, but up to you!

- Although task users have much flexibility to define tasks through contributing to the Task definitions, TaskConfiguration.equals() and ContributedTaskConfiguration.equals() compare certain properties of task configs, and thus are not reliable enough. With changes in this pull request, Theia uses task definitions as a reference to decide which properties in task configs should be considered in comparison. - fixed #5878 Signed-off-by: Liang Huang <liang.huang@ericsson.com>

elaihau · 2019-08-28T03:37:53Z

In general, when I do review I don't have the goal to block some PR - my goal is to identify the potential problems before merge some changes. Then the author, reviewers, code owners can assess and discuss - should it be fixed within the current PR or we should create an issue for detected problem and fix it in separate PR.

there is no misunderstanding here :) i think you have made all the comments in good faith, and i sincerely appreciate it.

I made more changes to my code, and tested with npm and typescript-language-feature. i believe the display problem has been fixed with my latest change. and "recently run tasks" should also be functional.

@RomanNikitenko
Thank you !

RomanNikitenko

I tested the changes using npm and tsc tasks after update:

I can not reproduce the bug Incorrect displaying of configured task after refreshing page #5878
I can run a tsc task as detected as well as configured - thank you very much for the fix for detected tsc task!
a task after running is placed in recently used section
after refreshing page label for a task looks correctly
labels for tsc tasks are consistent

@elaihau thank you for fix!

elaihau · 2019-08-28T11:07:08Z

Thank you Roman 👍

elaihau requested review from akosyakov, RomanNikitenko and vince-fugnitto August 18, 2019 04:51

elaihau requested a review from a team as a code owner August 18, 2019 04:51

elaihau force-pushed the Liang/new_task_compare branch from 548d6da to 0f8e3c6 Compare August 18, 2019 04:52

elaihau mentioned this pull request Aug 18, 2019

Incorrect displaying of configured task after refreshing page #5878

Closed

akosyakov added the tasks issues related to the task system label Aug 18, 2019

akosyakov requested a review from a team August 18, 2019 07:06

elaihau force-pushed the Liang/new_task_compare branch from 0f8e3c6 to 528ded1 Compare August 21, 2019 02:30

elaihau changed the title ~~[DRAFT] use task definition as a reference to compare tasks~~ use task definition as a reference to compare tasks Aug 21, 2019

RomanNikitenko reviewed Aug 22, 2019

View reviewed changes

elaihau force-pushed the Liang/new_task_compare branch 2 times, most recently from 0ce0e93 to 56c44db Compare August 25, 2019 04:21

elaihau force-pushed the Liang/new_task_compare branch from 56c44db to 8fcc050 Compare August 26, 2019 11:39

elaihau mentioned this pull request Aug 27, 2019

Allow not persistent changes for task before run #5312

Closed

RomanNikitenko reviewed Aug 27, 2019

View reviewed changes

RomanNikitenko mentioned this pull request Aug 27, 2019

[DRAFT] fetch tasks by type and schema version #5348

Closed

elaihau force-pushed the Liang/new_task_compare branch from 8fcc050 to f235bda Compare August 27, 2019 12:08

elaihau force-pushed the Liang/new_task_compare branch from f235bda to 8555761 Compare August 28, 2019 01:45

elaihau force-pushed the Liang/new_task_compare branch from 8555761 to 8eec728 Compare August 28, 2019 03:36

RomanNikitenko approved these changes Aug 28, 2019

View reviewed changes

elaihau merged commit fce7783 into master Aug 28, 2019

elaihau deleted the Liang/new_task_compare branch August 28, 2019 11:07

akosyakov mentioned this pull request Apr 28, 2020

defer plugin contributions (preferences and tasks) #7676

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use task definition as a reference to compare tasks #5975

use task definition as a reference to compare tasks #5975

elaihau commented Aug 18, 2019 •

edited

Loading

elaihau commented Aug 21, 2019

RomanNikitenko left a comment •

edited

Loading

elaihau commented Aug 22, 2019

elaihau commented Aug 23, 2019 •

edited

Loading

RomanNikitenko commented Aug 23, 2019

elaihau commented Aug 25, 2019 •

edited

Loading

RomanNikitenko left a comment •

edited

Loading

elaihau commented Aug 27, 2019 •

edited

Loading

RomanNikitenko commented Aug 27, 2019

elaihau commented Aug 27, 2019

RomanNikitenko commented Aug 27, 2019

elaihau commented Aug 27, 2019 •

edited

Loading

elaihau commented Aug 27, 2019

RomanNikitenko commented Aug 27, 2019

elaihau commented Aug 28, 2019 •

edited

Loading

RomanNikitenko left a comment

elaihau commented Aug 28, 2019

use task definition as a reference to compare tasks #5975

use task definition as a reference to compare tasks #5975

Conversation

elaihau commented Aug 18, 2019 • edited Loading

How to test

Review checklist

elaihau commented Aug 21, 2019

RomanNikitenko left a comment • edited Loading

Choose a reason for hiding this comment

elaihau commented Aug 22, 2019

elaihau commented Aug 23, 2019 • edited Loading

RomanNikitenko commented Aug 23, 2019

elaihau commented Aug 25, 2019 • edited Loading

RomanNikitenko left a comment • edited Loading

Choose a reason for hiding this comment

elaihau commented Aug 27, 2019 • edited Loading

RomanNikitenko commented Aug 27, 2019

elaihau commented Aug 27, 2019

RomanNikitenko commented Aug 27, 2019

elaihau commented Aug 27, 2019 • edited Loading

elaihau commented Aug 27, 2019

RomanNikitenko commented Aug 27, 2019

elaihau commented Aug 28, 2019 • edited Loading

RomanNikitenko left a comment

Choose a reason for hiding this comment

elaihau commented Aug 28, 2019

elaihau commented Aug 18, 2019 •

edited

Loading

RomanNikitenko left a comment •

edited

Loading

elaihau commented Aug 23, 2019 •

edited

Loading

elaihau commented Aug 25, 2019 •

edited

Loading

RomanNikitenko left a comment •

edited

Loading

elaihau commented Aug 27, 2019 •

edited

Loading

elaihau commented Aug 27, 2019 •

edited

Loading

elaihau commented Aug 28, 2019 •

edited

Loading