[WIP] Display initial loading message in terminal #6957

vitaliy-guliy · 2020-01-23T13:38:11Z

Signed-off-by: Vitaliy Gulyy vgulyy@redhat.com

What it does

It's a solution proposal.

On slow infrastructures after running a task user has to wait for several seconds until task terminal is appeared. On che.openshift.io where an extension of Theia is running, this delay is around 5 seconds.

The main idea is to open a terminal immediately when the user asked for running a task.
Terminal should have an initial message e.g. Task '${taskLabel}' - Connecting...

Solves issue eclipse-che/che#15277

The corresponding issue into Theia repository and demo videos demonstrating the problem will be created and recorded.

How to test

Run Theia on slow infrastructure
Go to che.openshift.io, create your workspace and test

Review checklist

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

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>

akosyakov · 2020-01-23T17:36:53Z

@vitaliy-guliy is there a way to reproduce with the vanilla Theia on master with system connection throttling? It would help to see the issue more clear and compare with the behavior proposed in the PR. Reproduction steps would help, thank you.

akosyakov · 2020-01-23T17:40:20Z

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

+export class TaskTerminal {
+
+    @inject(WidgetManager)
+    protected readonly widgetManager: WidgetManager;


Please use termialservice, it is responsible to create terminals and hides the logic of working with widhermanager

akosyakov · 2020-01-23T17:42:32Z

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

+
+@injectable()
+export class TaskTerminal {
+


TaskTerminal will mean that it provides an access to a terminal for a task. This class seems to be responsible to manage many tasks terminals. TakTerminalManager or TaskTerminalService would be the better name. Also it will be aligned with other wrappers around WidgetManager, e.g. EditorManager, TerminalService and so on

akosyakov · 2020-01-23T17:44:17Z

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

+    @inject(ApplicationShell)
+    protected readonly shell: ApplicationShell;
+
+    protected terminalWidgets: TerminalWidget[] = [];


There is a single place which keeps references to all widget. It’s WidgetManager, please use its methods to look up terminal widgets, don’t cache it. Caching is common cause of race conditions, memory leaks and so on

akosyakov · 2020-01-23T17:49:08Z

packages/terminal/src/browser/base/terminal-widget.ts

+    /**
+     * Initial message to be displayed while terminal is not started.
+     */
+    readonly loadingMessage?: string;


We should test reload page reload. What would you expect should happen? We try to preserve terminals. I assume terminal should appear immediately with loading message till connection to the task process is not recovered. We need ways to test it with the vanilla Theia.

akosyakov · 2020-01-23T17:52:38Z

packages/terminal/src/browser/terminal-widget-impl.ts

@@ -193,6 +194,24 @@ export class TerminalWidgetImpl extends TerminalWidget implements StatefulWidget
        for (const contribution of this.terminalContributionProvider.getContributions()) {
            contribution.onCreate(this);
        }
+
+        if (this.options.loadingMessage) {
+            this.loadingMessage = document.createElement('div');


please use css, you can find css guidance in our coding guidelines

akosyakov · 2020-01-23T17:55:12Z

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

+
+    protected terminalWidgets: TerminalWidget[] = [];
+
+    async openEmptyTerminal(taskLabel: string): Promise<void> {


How about returning a widget id here and passing it to attach later to avoid guessing?

akosyakov · 2020-01-23T17:58:17Z

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

@@ -454,6 +459,9 @@ export class TaskService implements TaskConfigurationClient {
     * It looks for configured and detected tasks.
     */
    async run(source: string, taskLabel: string, scope?: string): Promise<TaskInfo | undefined> {
+        // Open an empty terminal, display a message informing the user that terminal is starting
+        await this.taskTerminal.openEmptyTerminal(taskLabel);


I see that code below can exit without reaching attach ever what should happen with the terminal then?

@elaihau can a task be executed without the terminal? background tasks?

@vitaliy-guliy
Maybe it's not good place for creating a new terminal widget, for example here a task can be terminated or restarted, so how is a new terminal widget handled for this case?

@elaihau can a task be executed without the terminal? background tasks?

@akosyakov sorry for replying so late.
as far as I can tell, we create terminalProcess for all tasks.

@marechal-p please correct me if i am wrong

akosyakov · 2020-01-30T03:24:48Z

@vitaliy-guliy let us know if you need help to prepare a test case for the vanilla Theia

Signed-off-by: Vitaliy Guliy <vgulyy@redhat.com>

RomanNikitenko · 2020-02-08T17:51:24Z

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

@@ -454,6 +459,9 @@ export class TaskService implements TaskConfigurationClient {
     * It looks for configured and detected tasks.
     */
    async run(source: string, taskLabel: string, scope?: string): Promise<TaskInfo | undefined> {
+        // Open an empty terminal, display a message informing the user that terminal is starting
+        await this.taskTerminal.openEmptyTerminal(taskLabel);


@vitaliy-guliy
Maybe it's not good place for creating a new terminal widget, for example here a task can be terminated or restarted, so how is a new terminal widget handled for this case?

RomanNikitenko · 2020-02-08T18:01:45Z

packages/terminal/src/browser/terminal-widget-impl.ts

@@ -54,6 +54,7 @@ export class TerminalWidgetImpl extends TerminalWidget implements StatefulWidget
    protected waitForConnection: Deferred<MessageConnection> | undefined;
    protected hoverMessage: HTMLDivElement;
    protected lastTouchEnd: TouchEvent | undefined;
+    protected loadingMessage: HTMLDivElement;


Are you considering using some icon-loader for terminal tab till connection to the task process is not recovered instead of a loading message?
Like here, for example:

RomanNikitenko · 2020-02-08T18:16:22Z

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

+        if (taskInfo && taskInfo.config.label) {
+            for (let i = 0; i < this.terminalWidgets.length; i++) {
+                const w = this.terminalWidgets[i];
+                if (taskInfo.config.label === w.title.label) {


I think label usage to find the corresponding widget is not enough: user can have tasks with the same label, but different source or type, for example.

RomanNikitenko · 2020-02-08T18:59:37Z

@vitaliy-guliy
I would like to avoid usage of an additional storage for terminal widgets.
Could you take a look at #7108

Signed-off-by: Vitaliy Guliy <vgulyy@redhat.com>

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>

vitaliy-guliy · 2020-02-10T12:45:31Z

@vitaliy-guliy is there a way to reproduce with the vanilla Theia on master with system connection throttling? It would help to see the issue more clear and compare with the behavior proposed in the PR. Reproduction steps would help, thank you.

@akosyakov sorry for the delay. I cannot reproduce the problem with the vanilla Theia running on che.openshift.io and on the local machine. Network throttling does not affect on the delay. It looks like eclipse-che-theia extensions and eclipse-che plugins forces to appear the delay in running of tasks.
Let's leave this PR as draft as we need to investigate the issue more deep.

Display initial loading message in terminal

53d64f7

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>

akosyakov added proposal feature proposals (potential future features) tasks issues related to the task system labels Jan 23, 2020

akosyakov reviewed Jan 23, 2020

View reviewed changes

akosyakov requested review from elaihau and RomanNikitenko January 23, 2020 18:00

Code refactoring

d08d8c5

Signed-off-by: Vitaliy Guliy <vgulyy@redhat.com>

RomanNikitenko reviewed Feb 8, 2020

View reviewed changes

RomanNikitenko mentioned this pull request Feb 8, 2020

[DRAFT] Create terminal widget immediately at running task #7108

Closed

vitaliy-guliy added 2 commits February 10, 2020 10:10

Open terminal before running a task

9546acf

Signed-off-by: Vitaliy Guliy <vgulyy@redhat.com>

Merge branch 'master' into task-terminal

6fdf2f9

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>

azatsarynnyy mentioned this pull request Feb 13, 2020

Editors Sprint 179 eclipse-che/che#15902

Closed

20 tasks

vitaliy-guliy closed this Mar 19, 2020

vitaliy-guliy deleted the task-terminal branch March 31, 2020 09:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Display initial loading message in terminal #6957

[WIP] Display initial loading message in terminal #6957

vitaliy-guliy commented Jan 23, 2020

akosyakov commented Jan 23, 2020

akosyakov Jan 23, 2020

akosyakov Jan 23, 2020

akosyakov Jan 23, 2020 •

edited

Loading

akosyakov Jan 23, 2020

akosyakov Jan 23, 2020

akosyakov Jan 23, 2020

akosyakov Jan 23, 2020

akosyakov Jan 23, 2020

RomanNikitenko Feb 8, 2020

elaihau Feb 10, 2020

akosyakov commented Jan 30, 2020

RomanNikitenko Feb 8, 2020

RomanNikitenko Feb 8, 2020

RomanNikitenko Feb 8, 2020

RomanNikitenko commented Feb 8, 2020

vitaliy-guliy commented Feb 10, 2020


		protected terminalWidgets: TerminalWidget[] = [];

		async openEmptyTerminal(taskLabel: string): Promise<void> {

[WIP] Display initial loading message in terminal #6957

[WIP] Display initial loading message in terminal #6957

Conversation

vitaliy-guliy commented Jan 23, 2020

What it does

How to test

Review checklist

Reminder for reviewers

akosyakov commented Jan 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov commented Jan 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomanNikitenko commented Feb 8, 2020

vitaliy-guliy commented Feb 10, 2020

akosyakov Jan 23, 2020 •

edited

Loading