Split terminal widget(browser) logic #4518

AndrienkoAleksandr · 2019-03-10T22:30:52Z

Referenced issue:

#4523
eclipse-che/che#12609

Split terminal widget(browser) logic

Requested changes:

Split terminal widget logic into two components: TerminalWidget and TerminalClient:

TerminalWidget - contains logic to configure and create new terminal widget and methods to manipulate with this widget.
TerminalClient - create connection with terminal backend and manage terminal widget; encapsulate theia backend connection specific logic(to have ability simplier rebind terminal widget to non default back-end).

Remove using in the extensions depracated method activateTerminal.
Resolve an issue : task widget should display title with label task, not terminal!!!
Simplify imports for @theia/terminal and @theia/xterm.js browser package.

Breaking stuff:

Depracate terminalWidget.start() method. This method has too tricky behavior. I think this methods does too much operations with too much cases. I propose to split this method onto two separated:

createAndAttach()
attach()

Benefits(or why do we need this...)

Split code base.
It's useful for terminal plugin api. We could provide in the plugin api method to create new terminal widget without connection to the Theia specific backend. And user could define own connection logic(to the own backend) in the Theia plugin.

Notice for future: this pull request it's a first step: split logic. Second step(next pull request): introduce ability to create terminal widget with help plugin api.

Signed-off-by: Oleksandr Andriienko <[email protected]>

…tTerminal

Signed-off-by: Oleksandr Andriienko <[email protected]>

…tTerminal

Signed-off-by: Oleksandr Andriienko <[email protected]>

akosyakov · 2019-03-11T06:11:42Z

Don't you use xterm anymore? Why a new extension is created for it? It seems this PR is attempt to abstract away connection logic, not a concrete terminal implementation.

I've never liked a separation between TerminalWidget and TerminalWidgetImpl. This PR takes separation one step farther without any reasons. It could be as simple like TerminalWidget (one class with interface and implementation parts) -> Terminal (interface) if we wanted to abstract a concrete terminal.

cc @svenefftinge

akosyakov · 2019-03-11T06:25:02Z

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

+     * @param cols - amount displayed columns in the output frame.
+     * @param rows - amount displayed rows in the output frame.
+     */
+    resize(size: TerminalSize): Promise<void>;


it is never used in TerminalWidget, why is it exposed?

akosyakov · 2019-03-11T06:25:18Z

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

+    /**
+     * Kill backend process.
+     */
+    kill(): Promise<void>;


it's never used in TerminalWidget. Why is it exposed?

akosyakov · 2019-03-11T06:25:39Z

packages/terminal/src/browser/default-terminal-client.ts

+            }
+        }));
+
+       this.toDispose.push(this.widget.onTerminalResize(size => this.resize(size)));


Should not this code always be in TerminalWidget?

Could You elaborate more information, please? You against dispose of resize subscription? Or Do you want that terminal widget call TerminalClient#resize?

I meant that https://github.com/theia-ide/theia/blob/7cea837f5cf9607c86cc2f65a6799e0742f4ba3b/packages/terminal/src/browser/default-terminal-client.ts#L80-L93 could stay in TerminalWidgetImpl.

akosyakov · 2019-03-11T06:30:40Z

We could provide in the plugin api method to create new terminal widget without connection to the Theia specific backend.

Could you elaborate on it? Do you want to connect to an alternative backend in Eclipse Che?

benoitf · 2019-03-11T06:46:45Z

For now in Eclipse Che there is a fork of the Theia terminal extension. (To have multi remote terminal access). The idea is to move some logic to be re-used so it can work with different endpoints like endpoint = host running theia or a different endpoint (all containers in Eclipse Che).

AndrienkoAleksandr · 2019-03-11T07:11:18Z

Don't you use xterm anymore? Why a new extension is created for it? It seems this PR is attempt to abstract away connection logic, not a concrete terminal implementation.
I've never liked a separation between TerminalWidget and TerminalWidgetImpl. This PR takes separation one step farther without any reasons. It could be as simple like TerminalWidget (one class with interface and implementation parts) -> Terminal (interface) if we wanted to abstract a concrete terminal.
cc @svenefftinge

If you against this changes, I could revert it. But for me it's not bad if somebody with minimal changes could integrate some another terminal widget #1204 .

AndrienkoAleksandr · 2019-03-11T07:20:09Z

It seems this PR is attempt to abstract away connection logic, not a concrete terminal implementation.

Yes, it's really "attempt" to split connection logic. And I didn't intergrate all wanted changes for better split, because for that I need change terminal restore logic, it's possible, but it could be buggy. So for this step I did minimal changes for split logic + moving xterm.js to the separated extension.

AndrienkoAleksandr · 2019-03-11T07:26:06Z

Don't you use xterm anymore?

We still want to use xterm.js. It's nice widget, and it become better and better.

akosyakov · 2019-03-11T09:37:15Z

If you against this changes, I could revert it. But for me it's not bad if somebody with minimal changes could integrate some another terminal widget #1204 .

The point is that it should be done differently. TerminalWidget should be the same, there should be a new Terminal interface used by TerminalWidget. xterm/domtermn extensions should only implement Terminal interface. It is how it is done for editor. Not like in this PR.

Yes, it's really "attempt" to split connection logic.

please make this PR about it and revert all other unnecessary changes

Signed-off-by: Oleksandr Andriienko <[email protected]>

AndrienkoAleksandr · 2019-03-11T11:53:44Z

please make this PR about it and revert all other unnecessary changes

Reverted xterm.js extension. Should I revert something else?

#4518 (comment) sorry, apply more information, please, I didn't understood what did you mean.

akosyakov · 2019-03-12T06:25:39Z

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

+
+    private terminalClient: TerminalClient;
+
+    protected readonly _onTerminalDidClose = new Emitter<TerminalWidget>();


Remove, renaming protected properties is a breaking change, please keep names there it is possible.

akosyakov · 2019-03-12T06:28:21Z

packages/terminal/src/browser/default-terminal-client.ts

+    protected init(): void {
+        this.toDispose.push(this.terminalWatcher.onTerminalError(({ terminalId, error }) => {
+            if (terminalId === this.terminalId) {
+                this.dispose();


before it will always fire close event, the same for onTerminalExit

akosyakov · 2019-03-12T06:29:10Z

Reverted xterm.js extension. Should I revert something else?

the change should be minimal as possible with minimal breaking changes. terminal is quite central thing to break it.

akosyakov · 2019-03-12T06:30:12Z

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

+ * - resize backend output frame(in case if backend support it);
+ */
+export const TerminalClient = Symbol('TerminalClient');
+export interface TerminalClient extends Disposable {


not sure about naming, TerminalClient is something which provide an access to a terminal, it is TerminalConnection to me, or TerminalServer

akosyakov · 2019-03-12T06:30:43Z

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

+    /**
+     * Terminal widget which is under TerminalClient control.
+     */
+    readonly widget: TerminalWidget;


that's very weird that there is a cycle dependency between TerminalClient and TerminalWidget

I would try to see whether one can get rid of a dependency to a terminal widget from the terminal client by changing the interface and moving code from a client to a widget.

akosyakov · 2019-03-12T06:30:56Z

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

+    /**
+     * Create new process and attach terminal widget to this process.
+     */
+    createAndAttach(): Promise<number>;


i would call it launch, similar to debugging

akosyakov · 2019-03-12T06:31:17Z

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

+     * @param createNewTerminalOnFaill - spawn new process in case if
+     * target process was not found by terminalId or it's gone. NOTICE: False by default.
+     */
+    attach(terminalId: number, createNewTerminalOnFail?: boolean): Promise<number>;


i would use an object as a second arg to provide extensible options

akosyakov · 2019-03-12T06:33:09Z

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

+    /*
+     * Send event on terminal resize.
+     */
+    abstract onTerminalResize: Event<TerminalSize>;


on[Did/Will]Verb(Noun)? is how events should be named

akosyakov · 2019-03-12T06:38:17Z

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

+    private terminalClient: TerminalClient;
+
+    protected readonly _onTerminalDidClose = new Emitter<TerminalWidget>();
+    protected readonly _onUserInput = new Emitter<string | undefined>();


also please study naming conventions, we use _ only in some cases

AndrienkoAleksandr · 2019-11-20T11:17:52Z

Close like to much outdated and some time ago it was canceled due priority. And VSCode already has such thing like we wanted https://github.com/microsoft/vscode/blob/master/src/vs/vscode.d.ts#L7518

AndrienkoAleksandr added 30 commits February 20, 2019 12:54

Move terminal widget logic to the separed extension.

9a3dec1

Signed-off-by: Oleksandr Andriienko <[email protected]>

Merge branch 'master' of https://github.com/theia-ide/theia into spli…

3a5d2b8

…tTerminal

Add missing change after merge with master.

db10c0d

Signed-off-by: Oleksandr Andriienko <[email protected]>

Move back connection definition to the terminal package

a850dee

Signed-off-by: Oleksandr Andriienko <[email protected]>

Move back frontend contribution to the terminal extension.

bd17554

Signed-off-by: Oleksandr Andriienko <[email protected]>

Inject connection logic to the terminal client.

2119ed4

Signed-off-by: Oleksandr Andriienko <[email protected]>

Use terminal client.

d01a5fa

Signed-off-by: Oleksandr Andriienko <[email protected]>

Send user input to the terminal client

e9306a2

Signed-off-by: Oleksandr Andriienko <[email protected]>

Add resize, kill to the terminal client.

6f1498e

Signed-off-by: Oleksandr Andriienko <[email protected]>

Integreate terminal watcher to terminal client

0951bc1

Signed-off-by: Oleksandr Andriienko <[email protected]>

Fix up.

e357704

Signed-off-by: Oleksandr Andriienko <[email protected]>

Work on restore...

c45b105

Signed-off-by: Oleksandr Andriienko <[email protected]>

Restore works.

53f37ea

Signed-off-by: Oleksandr Andriienko <[email protected]>

Use attach on restore.

0c94766

Signed-off-by: Oleksandr Andriienko <[email protected]>

Reconnection finally works!

fcdb6d8

Signed-off-by: Oleksandr Andriienko <[email protected]>

Inject terminal widget to the constructor terminal client.

b6b3d7e

Signed-off-by: Oleksandr Andriienko <[email protected]>

Work on clientOptions.

14ef448

Signed-off-by: Oleksandr Andriienko <[email protected]>

Fix tasks.

4b0b4bd

Signed-off-by: Oleksandr Andriienko <[email protected]>

Merge branch 'master' of https://github.com/theia-ide/theia into spli…

46af854

…tTerminal

Improve code.

7ed6450

Signed-off-by: Oleksandr Andriienko <[email protected]>

Revert some bad changes. Debugger with terminal works.

68e1575

Signed-off-by: Oleksandr Andriienko <[email protected]>

Show terminal console.

76504de

Signed-off-by: Oleksandr Andriienko <[email protected]>

Fix some plugin api.

44d905d

Signed-off-by: Oleksandr Andriienko <[email protected]>

Bring back start method...

20ec26a

Signed-off-by: Oleksandr Andriienko <[email protected]>

Fix restore, terminal plugin api, remove redudant code.

ed88b96

Signed-off-by: Oleksandr Andriienko <[email protected]>

Remove redudant domId option

be2ccb1

Signed-off-by: Oleksandr Andriienko <[email protected]>

Reuse shell options.

efeb82e

Signed-off-by: Oleksandr Andriienko <[email protected]>

Remove redudunt changes. Code clean up.

9415294

Signed-off-by: Oleksandr Andriienko <[email protected]>

Fix terminal recreation after stop theia. Apply more docs

3a21c1c

Signed-off-by: Oleksandr Andriienko <[email protected]>

Rename TerminalWidget#create to createAndAttach. Improve docs.

a805d54

Signed-off-by: Oleksandr Andriienko <[email protected]>

AndrienkoAleksandr requested review from benoitf, evidolob, akosyakov, akurinnoy and marcdumais-work March 10, 2019 22:30

akosyakov requested a review from svenefftinge March 11, 2019 06:00

akosyakov reviewed Mar 11, 2019

View reviewed changes

AndrienkoAleksandr added 2 commits March 11, 2019 13:46

Address changes. Remove xterm.js extension.

444cb3d

Signed-off-by: Oleksandr Andriienko <[email protected]>

Fix up.

7cea837

Signed-off-by: Oleksandr Andriienko <[email protected]>

akosyakov reviewed Mar 12, 2019

View reviewed changes

AndrienkoAleksandr closed this Nov 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split terminal widget(browser) logic #4518

Split terminal widget(browser) logic #4518

AndrienkoAleksandr commented Mar 10, 2019 •

edited

Loading

akosyakov commented Mar 11, 2019 •

edited

Loading

akosyakov Mar 11, 2019

akosyakov Mar 11, 2019

akosyakov Mar 11, 2019

AndrienkoAleksandr Mar 11, 2019

akosyakov Mar 12, 2019

akosyakov commented Mar 11, 2019

benoitf commented Mar 11, 2019

AndrienkoAleksandr commented Mar 11, 2019 •

edited

Loading

AndrienkoAleksandr commented Mar 11, 2019

AndrienkoAleksandr commented Mar 11, 2019

akosyakov commented Mar 11, 2019 •

edited

Loading

AndrienkoAleksandr commented Mar 11, 2019

akosyakov Mar 12, 2019

akosyakov Mar 12, 2019

akosyakov commented Mar 12, 2019

akosyakov Mar 12, 2019 •

edited

Loading

akosyakov Mar 12, 2019 •

edited

Loading

akosyakov Mar 12, 2019

akosyakov Mar 12, 2019

akosyakov Mar 12, 2019

akosyakov Mar 12, 2019

AndrienkoAleksandr commented Nov 20, 2019 •

edited

Loading


		private terminalClient: TerminalClient;

		protected readonly _onTerminalDidClose = new Emitter<TerminalWidget>();

Split terminal widget(browser) logic #4518

Split terminal widget(browser) logic #4518

Conversation

AndrienkoAleksandr commented Mar 10, 2019 • edited Loading

Referenced issue:

Requested changes:

Breaking stuff:

Benefits(or why do we need this...)

akosyakov commented Mar 11, 2019 • 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

akosyakov commented Mar 11, 2019

benoitf commented Mar 11, 2019

AndrienkoAleksandr commented Mar 11, 2019 • edited Loading

AndrienkoAleksandr commented Mar 11, 2019

AndrienkoAleksandr commented Mar 11, 2019

akosyakov commented Mar 11, 2019 • edited Loading

AndrienkoAleksandr commented Mar 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov commented Mar 12, 2019

akosyakov Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

akosyakov Mar 12, 2019 • 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

AndrienkoAleksandr commented Nov 20, 2019 • edited Loading

AndrienkoAleksandr commented Mar 10, 2019 •

edited

Loading

akosyakov commented Mar 11, 2019 •

edited

Loading

AndrienkoAleksandr commented Mar 11, 2019 •

edited

Loading

akosyakov commented Mar 11, 2019 •

edited

Loading

akosyakov Mar 12, 2019 •

edited

Loading

akosyakov Mar 12, 2019 •

edited

Loading

AndrienkoAleksandr commented Nov 20, 2019 •

edited

Loading