-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Display initial loading message in terminal #6957
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
/******************************************************************************** | ||
* Copyright (C) 2020 Red Hat, Inc. and others. | ||
* | ||
* This program and the accompanying materials are made available under the | ||
* terms of the Eclipse Public License v. 2.0 which is available at | ||
* http://www.eclipse.org/legal/epl-2.0. | ||
* | ||
* This Source Code may also be made available under the following Secondary | ||
* Licenses when the conditions for such availability set forth in the Eclipse | ||
* Public License v. 2.0 are satisfied: GNU General Public License, version 2 | ||
* with the GNU Classpath Exception which is available at | ||
* https://www.gnu.org/software/classpath/license.html. | ||
* | ||
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 | ||
********************************************************************************/ | ||
|
||
import { inject, injectable } from 'inversify'; | ||
import { TerminalWidgetFactoryOptions, TERMINAL_WIDGET_FACTORY_ID } from '@theia/terminal/lib/browser/terminal-widget-impl'; | ||
import { TerminalWidget } from '@theia/terminal/lib/browser/base/terminal-widget'; | ||
import { ApplicationShell, WidgetManager } from '@theia/core/lib/browser'; | ||
import { TaskInfo, RevealKind } from '../common'; | ||
|
||
@injectable() | ||
export class TaskTerminal { | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
@inject(WidgetManager) | ||
protected readonly widgetManager: WidgetManager; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use termialservice, it is responsible to create terminals and hides the logic of working with widhermanager |
||
|
||
@inject(ApplicationShell) | ||
protected readonly shell: ApplicationShell; | ||
|
||
protected terminalWidgets: TerminalWidget[] = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
async openEmptyTerminal(taskLabel: string): Promise<void> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about returning a widget id here and passing it to attach later to avoid guessing? |
||
const id = TERMINAL_WIDGET_FACTORY_ID + '-connecting'; | ||
|
||
const widget = <TerminalWidget>await this.widgetManager.getOrCreateWidget( | ||
TERMINAL_WIDGET_FACTORY_ID, | ||
<TerminalWidgetFactoryOptions>{ | ||
created: new Date().toString(), | ||
id: id, | ||
title: taskLabel, | ||
destroyTermOnClose: true, | ||
loadingMessage: `Task '${taskLabel}' - Connecting...` | ||
} | ||
); | ||
|
||
this.shell.addWidget(widget, { area: 'bottom' }); | ||
this.shell.revealWidget(widget.id); | ||
|
||
this.terminalWidgets.push(widget); | ||
} | ||
|
||
private findWidget(taskInfo: TaskInfo | undefined): TerminalWidget | undefined { | ||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
this.terminalWidgets.splice(i, 1); | ||
return w; | ||
} | ||
} | ||
} | ||
|
||
return undefined; | ||
} | ||
|
||
async attach(processId: number, taskId: number, taskInfo: TaskInfo | undefined, terminalWidgetId: string): Promise<void> { | ||
let widget: TerminalWidget | undefined = this.findWidget(taskInfo); | ||
if (widget) { | ||
widget.id = terminalWidgetId; | ||
} else { | ||
widget = <TerminalWidget>await this.widgetManager.getOrCreateWidget( | ||
TERMINAL_WIDGET_FACTORY_ID, | ||
<TerminalWidgetFactoryOptions>{ | ||
created: new Date().toString(), | ||
id: terminalWidgetId, | ||
title: taskInfo | ||
? `Task: ${taskInfo.config.label}` | ||
: `Task: #${taskId}`, | ||
destroyTermOnClose: true | ||
} | ||
); | ||
|
||
this.shell.addWidget(widget, { area: 'bottom' }); | ||
} | ||
|
||
if (taskInfo && taskInfo.config.presentation && taskInfo.config.presentation.reveal === RevealKind.Always) { | ||
if (taskInfo.config.presentation.focus) { // assign focus to the terminal if presentation.focus is true | ||
this.shell.activateWidget(widget.id); | ||
} else { // show the terminal but not assign focus | ||
this.shell.revealWidget(widget.id); | ||
} | ||
} | ||
|
||
widget.start(processId); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,4 +106,9 @@ export interface TerminalWidgetOptions { | |
* Terminal attributes. Can be useful to apply some implementation specific information. | ||
*/ | ||
readonly attributes?: { [key: string]: string | null }; | ||
|
||
/** | ||
* Initial message to be displayed while terminal is not started. | ||
*/ | ||
readonly loadingMessage?: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
@inject(WorkspaceService) protected readonly workspaceService: WorkspaceService; | ||
@inject(WebSocketConnectionProvider) protected readonly webSocketConnectionProvider: WebSocketConnectionProvider; | ||
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use css, you can find css guidance in our coding guidelines |
||
this.loadingMessage.style.zIndex = '20'; | ||
this.loadingMessage.style.display = 'block'; | ||
this.loadingMessage.style.position = 'absolute'; | ||
this.loadingMessage.style.left = '0px'; | ||
this.loadingMessage.style.right = '0px'; | ||
this.loadingMessage.style.top = '30%'; | ||
this.loadingMessage.style.textAlign = 'center'; | ||
this.loadingMessage.style.color = 'var(--theia-editorWidget-foreground)'; | ||
|
||
const text = document.createElement('pre'); | ||
text.textContent = this.options.loadingMessage; | ||
this.loadingMessage.appendChild(text); | ||
|
||
this.node.appendChild(this.loadingMessage); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -280,6 +299,12 @@ export class TerminalWidgetImpl extends TerminalWidget implements StatefulWidget | |
this.terminalId = typeof id !== 'number' ? await this.createTerminal() : await this.attachTerminal(id); | ||
this.resizeTerminalProcess(); | ||
this.connectTerminalProcess(); | ||
|
||
// Hide loading message after starting the terminal. | ||
if (this.loadingMessage) { | ||
this.loadingMessage.style.display = 'none'; | ||
} | ||
|
||
if (IBaseTerminalServer.validateId(this.terminalId)) { | ||
this.onDidOpenEmitter.fire(undefined); | ||
return this.terminalId; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that code below can exit without reaching attach ever what should happen with the terminal then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elaihau can a task be executed without the terminal? background tasks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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