ask user to terminate or restart a task if it is active #6668

elaihau · 2019-12-01T01:25:43Z

With changes in this pull request, Theia offers users the flexibility of terminating or restarting a task, if the user tries to run a task that is actively running.
This pull request resolves #6618 (comment), and resolves #5269

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

How to test

Start a long running task, or a background task
Before the task that is started in step 1 finishes, run it for one more time
- Expected: Users should see a prompt. From the prompt they are offered 2 options: terminate the task (started in step 1), or restart the task.

Review checklist

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

akosyakov · 2019-12-02T09:42:57Z

@elaihau Is it aligned with VS Code UX? i.e. if task is finished and then started again the same terminal will be used

To resolve #5269 we should print to the terminal like in #5269 (comment). Would it happen with this PR?

RomanNikitenko

@elaihau
I tested your changes and noticed that the difference with VS Code behavior is:

VS Code hides the bottom panel when user selects Terminate Task
another terminal widget is used if a task is completed and then started again (the current PR behavior)

Is it under the scope of the current PR?

It's not critical for me - just let you know the difference.

RomanNikitenko · 2019-12-02T10:48:44Z

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

+            }
+            this.messageService.info(`The task \'${taskName}\' is already active`, 'Terminate Task', 'Restart Task').then(actionTask => {
+                if (actionTask) {
+                    this.taskServer.kill(taskId);


Maybe it makes sense to use the corresponding method for terminating a task, this one handles errors

elaihau · 2019-12-02T12:12:57Z

@elaihau Is it aligned with VS Code UX? i.e. if task is finished and then started again the same terminal will be used

To resolve #5269 we should print to the terminal like in #5269 (comment). Would it happen with this PR?

No what you described here is not implemented in this PR.

We have a separated GH issue for it #5332

And the features of

reusing the terminal
printing the message(s) in the terminal

are controled by the presentation config (https://code.visualstudio.com/docs/editor/tasks#_output-behavior) which is a larger problem. I will resolve it bit by bit.

@akosyakov

elaihau · 2019-12-02T12:23:28Z

@elaihau
I tested your changes and noticed that the difference with VS Code behavior is:

VS Code hides the bottom panel when user selects Terminate Task

@RomanNikitenko thank you for the review ! I noticed that too :)
It is not implemented in the initial version of this patch because I am wondering if it is truly user friendly. For myself, as a developer, the terminal output is interesting even if the task is terminated. I wish the terminal stays visible after hitting the Terminate Task button, rather than seeing it varnish all of sudden.

But that's just my opinion ... I should follow the vscode behavior. And if users are unhappy someone can file a GH issue :)

another terminal widget is used if a task is completed and then started again (the current PR behavior)

Yes I can change my PR to follow this vscode behaviors.

RomanNikitenko · 2019-12-02T12:56:57Z

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

+        }
+    }
+
+    private async doRunTask(task: TaskConfiguration, option?: RunTaskOption): Promise<TaskInfo | undefined> {


Can we have protected modifier here?
I think it will be more flexible for cases when some class extends TaskService
From this point of view also would nice to have at least protected restartTask method.
For example, at the moment attach method is public and it's possible to override this logic, provide own logic.
restartTask logic is relative to it.
wdyt?

lmcbout · 2019-12-02T15:01:50Z

Comparing VSCode and Theia
I like in VSCode the task command being display at the beginning of the execution in the terminal.
When stopping the task, VSCode remove the terminal while Theia keeps it, prefer the Theia version.

Suggestion: Would it be nice to put the message "... terminated by signal SIGHUP" at the end of the trace in the terminal and/or popping up a dialog to show the warning. In this case, the end-user would see in the terminal he/she stops the task while keeping the terminal view available!

lmcbout · 2019-12-02T15:21:37Z

I just noticed something. when you select the "Terminate task", the terminal stays OK
But if you resize the terminal, look at the server side console, you receive a lot of the following: "root ERROR Couldn't resize terminal 41, because it doesn't exist."

vince-fugnitto · 2019-12-02T15:31:14Z

I just noticed something. when you select the "Terminate task", the terminal stays OK
But if you resize the terminal, look at the server side console, you receive a lot of the following: "root ERROR Couldn't resize terminal 41, because it doesn't exist."

I don't see the warning following your procedure.
In any case, I don't think it is harmful, the issue is not caused by this PR: #4229

vince-fugnitto · 2019-12-02T15:33:24Z

Comparing VSCode and Theia
I like in VSCode the task command being display at the beginning of the execution in the terminal.
When stopping the task, VSCode remove the terminal while Theia keeps it, prefer the Theia version.

Suggestion: Would it be nice to put the message "... terminated by signal SIGHUP" at the end of the trace in the terminal and/or popping up a dialog to show the warning. In this case, the end-user would see in the terminal he/she stops the task while keeping the terminal view available!

@lmcbout I'd argue it's an enhancement to the current behavior. When we add support to display information regarding the task in the terminal #5332 we can also support displaying the exit message.

elaihau · 2019-12-03T03:46:12Z

Comparing VSCode and Theia
I like in VSCode the task command being display at the beginning of the execution in the terminal.
When stopping the task, VSCode remove the terminal while Theia keeps it, prefer the Theia version.

Suggestion: Would it be nice to put the message "... terminated by signal SIGHUP" at the end of the trace in the terminal and/or popping up a dialog to show the warning. In this case, the end-user would see in the terminal he/she stops the task while keeping the terminal view available!

@lmcbout Thank you for the feedback
Yes I love seeing that message too :) And the feature (i.e., adding messages before and after running the task) you described here is controled by the presentation config (https://code.visualstudio.com/docs/editor/tasks#_output-behavior) which is a larger problem. I will resolve it bit by bit.

elaihau · 2019-12-03T03:47:07Z

@RomanNikitenko I resolved your comments and patched the code.

akosyakov · 2019-12-03T05:07:34Z

@elaihau thanks for the explanation!

are controled by the presentation config (https://code.visualstudio.com/docs/editor/tasks#_output-behavior) which is a larger problem. I will resolve it bit by bit.

Please make sure to create issues for all missing features to make progress visible for others.

RomanNikitenko · 2019-12-03T08:16:51Z

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

+                        const parent = terminal.parent;
+                        terminal.close();
+                        if (actionTask === 'Terminate Task' && parent && parent.id === BOTTOM_AREA_ID) {
+                            parent.hide();


The behavior can be improved for case when user has a few running tasks.
So if user terminates one task, but another task is running - output for second task should be displayed, we shouldn't hide the bottom area.
I don't mind to improve the behavior in another PR, up to you.

RomanNikitenko · 2019-12-03T08:20:31Z

@RomanNikitenko I resolved your comments and patched the code.

@elaihau
thank you very much!
wdyt about separate method restartTask as I mentioned here

lmcbout · 2019-12-03T12:17:15Z

Just tested the latest commit dc75955
If I open a terminal at the bottom, then run a long running task + terminate the task, when terminating the task, the terminal window is also removed from the bottom view area with the terminated task terminal view.

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

elaihau · 2019-12-03T21:36:44Z

@RomanNikitenko I refactored the code to add restartTask and terminateTask.

Also I fixed the bug @lmcbout and @RomanNikitenko found. With the most recent change, the bottom panel wouldn't be hidden if there are other terminals open.

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

lmcbout · 2019-12-04T12:20:58Z

There are still an error with the bottom panel.
How to reproduce:

Start a long running process
Open the Problem View
Terminate the long running task
--> The bottom panel will go away closing the problem view
Using commit 4d8dfd7

lmcbout · 2019-12-04T12:25:22Z

Question:
When the long running task is moved to another panel, let say the right| left panel, and you restart the long running task, where should it be shown on he restart: Right, left or bottom panel?
Currently, it re-open on the bottom panel.

vince-fugnitto · 2019-12-04T15:02:45Z

Question:
When the long running task is moved to another panel, let say the right| left panel, and you restart the long running task, where should it be shown on he restart: Right, left or bottom panel?
Currently, it re-open on the bottom panel.

In order to support such a feature, we'd have to extend the TerminalWidgetOptions to specify where to spawn the terminal widget. At the moment, terminals are always created in the bottom area:

theia/packages/core/src/browser/shell/application-shell.ts

Lines 1605 to 1625 in d5c8110

    
               /** 
        
                * Options for adding a widget to the application shell. 
        
                */ 
        
               export interface WidgetOptions extends SidePanel.WidgetOptions { 
        
                   /** 
        
                    * The area of the application shell where the widget will reside. 
        
                    */ 
        
                   area?: Area; 
        
                   /** 
        
                    * The insertion mode for adding the widget. 
        
                    * 
        
                    * The default is `'tab-after'`. 
        
                    */ 
        
                   mode?: DockLayout.InsertMode | OpenToSideMode 
        
                   /** 
        
                    * The reference widget for the insert location. 
        
                    * 
        
                    * The default is `undefined`. 
        
                    */ 
        
                   ref?: Widget; 
        
               }

elaihau · 2019-12-04T19:00:43Z

Terminate the long running task
--> The bottom panel will go away closing the problem view
Using commit 4d8dfd7

@lmcbout Yes and vs code works this way too. I dislike it but my plan is complying with the vs code behavior - if anyone hates it s/he could create a ticket.

elaihau · 2019-12-04T19:02:49Z

Question:
When the long running task is moved to another panel, let say the right| left panel, and you restart the long running task, where should it be shown on he restart: Right, left or bottom panel?
Currently, it re-open on the bottom panel.

Vincent's correct. Theia always spawn the terminal at the bottom panel. And we will need change the interface to include an option that controls the area that terminal is expected to be spawned.

akosyakov · 2019-12-05T07:54:51Z

Question:
When the long running task is moved to another panel, let say the right| left panel, and you restart the long running task, where should it be shown on he restart: Right, left or bottom panel?
Currently, it re-open on the bottom panel.

I think it should be another issue. We can ignore it for now. Don't think a user will move it.

elaihau · 2019-12-06T13:16:13Z

In the latest patch I removed the code that closes the terminal & hides the panel.

Also, Anton's review comments are addressed.

Thank you all the for the feedback !

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

akosyakov

code-wise it looks good, there are couple minor comments about control flow, please have a look

I've not had time to try it out, please rely for testing on other reviewers.

vince-fugnitto

I tested the functionality and it worked as intended 👍

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

elaihau · 2019-12-06T15:58:24Z

Thank you for the review Anton and Vincent ! Comments are resolved.

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

With changes in this pull request, Theia offers users the flexibility of terminating or restarting a task, if the user tries to run a task that is actively running. This pull request resolves eclipse-theia#6618 (comment) Signed-off-by: Liang Huang <liang.huang@ericsson.com>

vince-fugnitto

I'm happy with the latest changes :)

RomanNikitenko · 2019-12-06T22:47:22Z

I tested the changes after last update and I can see the following behavior for the long running task:

For Terminate Task action:

the task is terminated
the terminal widget is open after terminating the task

For Restart Task action:

the task is terminated
the terminal widget is open after terminating the task
the task is started
the new terminal is used for the output

May be it makes sense to replace the gif in the PR description by gif which reflects the actual behavior.

Also I tested for detected tsc tasks and it works well for me except for the one case:
https://youtu.be/Ka0NCENTqw4

So I run detected task for che-theia project, but the notification and the recently used section tell me that task for theia project is running.
The attempt to run the task for che-theia project leads to displaying the prompt terminate or restart the task.

I believe that the problem is not related to the current PR changes, something wrong with running detected tasks in multi-root workspaces case. So it can be reproduced for very specific case:

multi-root workspace
the tasks have the similar configuration and have only different scope (I guess, we need investigate it)

@elaihau
could you confirm that you can reproduce it and I'll create the separate issue for this bug.

elaihau · 2019-12-07T02:13:43Z

@RomanNikitenko Yes I can reproduce the bug you described with NPM extension.

There is something going wrong with the way that we search tasks from the cached data.

Please feel free to file a GH issue. If you don't already have a PR for it, I can take a look at it tomorrow.

elaihau · 2019-12-08T02:32:17Z

@RomanNikitenko I created #6715 and will merge this one. Thanks !

elaihau changed the title ~~ask user to terminate or restart a task if it is actve~~ ask user to terminate or restart a task if it is active Dec 1, 2019

elaihau force-pushed the Liang/run_active_task branch from 5969bed to 4c5e5c6 Compare December 1, 2019 01:27

elaihau requested review from vince-fugnitto and RomanNikitenko December 1, 2019 02:45

akosyakov added tasks issues related to the task system vscode issues related to VSCode compatibility labels Dec 2, 2019

RomanNikitenko reviewed Dec 2, 2019

View reviewed changes

vince-fugnitto requested a review from lmcbout December 2, 2019 12:32

RomanNikitenko reviewed Dec 2, 2019

View reviewed changes

elaihau force-pushed the Liang/run_active_task branch from 4c5e5c6 to dc75955 Compare December 3, 2019 03:43

RomanNikitenko reviewed Dec 3, 2019

View reviewed changes

akosyakov reviewed Dec 3, 2019

View reviewed changes

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

akosyakov reviewed Dec 3, 2019

View reviewed changes

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

elaihau force-pushed the Liang/run_active_task branch from dc75955 to 4d8dfd7 Compare December 3, 2019 21:34

akosyakov reviewed Dec 4, 2019

View reviewed changes

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

akosyakov reviewed Dec 4, 2019

View reviewed changes

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

akosyakov reviewed Dec 4, 2019

View reviewed changes

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

elaihau force-pushed the Liang/run_active_task branch from 4d8dfd7 to 52063cb Compare December 6, 2019 13:14

akosyakov reviewed Dec 6, 2019

View reviewed changes

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

akosyakov reviewed Dec 6, 2019

View reviewed changes

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

akosyakov reviewed Dec 6, 2019

View reviewed changes

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

akosyakov approved these changes Dec 6, 2019

View reviewed changes

vince-fugnitto approved these changes Dec 6, 2019

View reviewed changes

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

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

elaihau force-pushed the Liang/run_active_task branch from 52063cb to cd8a4c6 Compare December 6, 2019 15:57

elaihau force-pushed the Liang/run_active_task branch 2 times, most recently from 1bad617 to 8ff506f Compare December 6, 2019 16:45

vince-fugnitto reviewed Dec 6, 2019

View reviewed changes

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

elaihau force-pushed the Liang/run_active_task branch from 8ff506f to dde9bac Compare December 6, 2019 18:12

vince-fugnitto approved these changes Dec 6, 2019

View reviewed changes

elaihau mentioned this pull request Dec 8, 2019

Theia could possibly start wrong detected task in a multi root workspace #6715

Closed

elaihau merged commit 4d3a125 into eclipse-theia:master Dec 8, 2019

elaihau deleted the Liang/run_active_task branch December 8, 2019 02:32

akosyakov mentioned this pull request Dec 16, 2019

@theia/task Doesn't run task, creates new terminal #5825

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ask user to terminate or restart a task if it is active #6668

ask user to terminate or restart a task if it is active #6668

elaihau commented Dec 1, 2019 •

edited

Loading

akosyakov commented Dec 2, 2019 •

edited

Loading

RomanNikitenko left a comment •

edited

Loading

RomanNikitenko Dec 2, 2019

elaihau commented Dec 2, 2019 •

edited

Loading

elaihau commented Dec 2, 2019

RomanNikitenko Dec 2, 2019

lmcbout commented Dec 2, 2019

lmcbout commented Dec 2, 2019

vince-fugnitto commented Dec 2, 2019

vince-fugnitto commented Dec 2, 2019

elaihau commented Dec 3, 2019

elaihau commented Dec 3, 2019

akosyakov commented Dec 3, 2019

RomanNikitenko Dec 3, 2019

RomanNikitenko commented Dec 3, 2019

lmcbout commented Dec 3, 2019

elaihau commented Dec 3, 2019

lmcbout commented Dec 4, 2019

lmcbout commented Dec 4, 2019

vince-fugnitto commented Dec 4, 2019

elaihau commented Dec 4, 2019 •

edited

Loading

elaihau commented Dec 4, 2019

akosyakov commented Dec 5, 2019

elaihau commented Dec 6, 2019

akosyakov left a comment •

edited

Loading

vince-fugnitto left a comment

elaihau commented Dec 6, 2019

vince-fugnitto left a comment

RomanNikitenko commented Dec 6, 2019

elaihau commented Dec 7, 2019 •

edited

Loading

elaihau commented Dec 8, 2019

ask user to terminate or restart a task if it is active #6668

ask user to terminate or restart a task if it is active #6668

Conversation

elaihau commented Dec 1, 2019 • edited Loading

How to test

Review checklist

akosyakov commented Dec 2, 2019 • edited Loading

RomanNikitenko left a comment • edited Loading

Choose a reason for hiding this comment

RomanNikitenko Dec 2, 2019

Choose a reason for hiding this comment

elaihau commented Dec 2, 2019 • edited Loading

elaihau commented Dec 2, 2019

RomanNikitenko Dec 2, 2019

Choose a reason for hiding this comment

lmcbout commented Dec 2, 2019

lmcbout commented Dec 2, 2019

vince-fugnitto commented Dec 2, 2019

vince-fugnitto commented Dec 2, 2019

elaihau commented Dec 3, 2019

elaihau commented Dec 3, 2019

akosyakov commented Dec 3, 2019

RomanNikitenko Dec 3, 2019

Choose a reason for hiding this comment

RomanNikitenko commented Dec 3, 2019

lmcbout commented Dec 3, 2019

elaihau commented Dec 3, 2019

lmcbout commented Dec 4, 2019

lmcbout commented Dec 4, 2019

vince-fugnitto commented Dec 4, 2019

elaihau commented Dec 4, 2019 • edited Loading

elaihau commented Dec 4, 2019

akosyakov commented Dec 5, 2019

elaihau commented Dec 6, 2019

akosyakov left a comment • edited Loading

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

elaihau commented Dec 6, 2019

vince-fugnitto left a comment

Choose a reason for hiding this comment

RomanNikitenko commented Dec 6, 2019

elaihau commented Dec 7, 2019 • edited Loading

elaihau commented Dec 8, 2019

elaihau commented Dec 1, 2019 •

edited

Loading

akosyakov commented Dec 2, 2019 •

edited

Loading

RomanNikitenko left a comment •

edited

Loading

elaihau commented Dec 2, 2019 •

edited

Loading

elaihau commented Dec 4, 2019 •

edited

Loading

akosyakov left a comment •

edited

Loading

elaihau commented Dec 7, 2019 •

edited

Loading