-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
Implement integrated terminal extension API #10635
Changes from 11 commits
d464fdc
ab96b19
57375e9
0d90238
9d16f3c
d0f8347
cce4650
734f91b
ff87ab3
4d14ca0
2c95aed
d3dd3c6
2fb2d9a
6588805
4f44e9c
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 |
---|---|---|
|
@@ -2971,6 +2971,37 @@ declare namespace vscode { | |
dispose(): void; | ||
} | ||
|
||
/** | ||
* An individual terminal instance within the integrated terminal. | ||
*/ | ||
export interface Terminal { | ||
|
||
/** | ||
* Send text to the terminal. | ||
* | ||
* @param addNewLine Whether to add a new line to the text being sent, this is normally | ||
* required to run a command in the terminal. This defaults to `true`. | ||
*/ | ||
sendText(text: string, addNewLine?: boolean); | ||
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. Is this always the same sequence for new line or should 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. missing return type, comment for |
||
|
||
/** | ||
* Reveal this channel in the UI. | ||
* | ||
* @param preserveFocus When `true` the channel will not take focus. | ||
*/ | ||
show(preservceFocus?: boolean): void; | ||
|
||
/** | ||
* Hide this channel from the UI. | ||
*/ | ||
hide(): void; | ||
|
||
/** | ||
* Dispose and free associated resources. | ||
*/ | ||
dispose(): void; | ||
} | ||
|
||
/** | ||
* Represents an extension. | ||
* | ||
|
@@ -3429,6 +3460,15 @@ declare namespace vscode { | |
* @return A new status bar item. | ||
*/ | ||
export function createStatusBarItem(alignment?: StatusBarAlignment, priority?: number): StatusBarItem; | ||
|
||
/** | ||
* Creates a [Terminal](#Terminal). | ||
* | ||
* @param name The optional name of a terminal, this will override the label used in the | ||
* terminal dropdown. | ||
* @return A new Terminal. | ||
*/ | ||
export function createTerminal(name?: string): Thenable<vscode.Terminal>; | ||
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 do not return promises from there functions but already working objects (see 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 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 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. To clarify - making the create-call sync doesn't mean the logic behind it must be all sync. Since all functions of the terminal are async under the hood anyways (talking to the main thread) you can just make them all wait on the initial creation (promise). 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. The ID of the terminal is currently the process ID of the terminal process which must be async, would you suggest I use some other ID to refer to the from the API then? 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. Sure - reusing that PID just seems like an implementation detail. Create an extension host side id which maps to Promise and you have it |
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
/*--------------------------------------------------------------------------------------------- | ||
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
'use strict'; | ||
|
||
import {TPromise} from 'vs/base/common/winjs.base'; | ||
import {IThreadService} from 'vs/workbench/services/thread/common/threadService'; | ||
import vscode = require('vscode'); | ||
import {MainContext, MainThreadTerminalServiceShape} from './extHost.protocol'; | ||
|
||
export class ExtHostTerminal implements vscode.Terminal { | ||
|
||
private _id: number; | ||
private _proxy: MainThreadTerminalServiceShape; | ||
private _disposed: boolean; | ||
|
||
constructor(proxy: MainThreadTerminalServiceShape, id: number) { | ||
this._id = id; | ||
this._proxy = proxy; | ||
} | ||
|
||
public sendText(text: string, addNewLine: boolean = true) { | ||
this._proxy.$sendText(this._id, text, addNewLine); | ||
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. throw if disposed? |
||
} | ||
|
||
public show(preserveFocus: boolean): void { | ||
this._proxy.$show(this._id, preserveFocus); | ||
} | ||
|
||
public hide(): void { | ||
this._proxy.$hide(this._id); | ||
} | ||
|
||
public dispose(): void { | ||
if (!this._disposed) { | ||
this._disposed = true; | ||
this._proxy.$dispose(this._id); | ||
} | ||
} | ||
} | ||
|
||
export class ExtHostTerminalService { | ||
|
||
private _proxy: MainThreadTerminalServiceShape; | ||
|
||
constructor(threadService: IThreadService) { | ||
this._proxy = threadService.get(MainContext.MainThreadTerminalService); | ||
} | ||
|
||
public createTerminal(name?: string): TPromise<vscode.Terminal> { | ||
return this._proxy.$createTerminal(name).then((terminalId) => { | ||
return new ExtHostTerminal(this._proxy, terminalId); | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
/*--------------------------------------------------------------------------------------------- | ||
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
'use strict'; | ||
|
||
import {TPromise} from 'vs/base/common/winjs.base'; | ||
import {ITerminalService} from 'vs/workbench/parts/terminal/electron-browser/terminal'; | ||
import {MainThreadTerminalServiceShape} from './extHost.protocol'; | ||
|
||
export class MainThreadTerminalService extends MainThreadTerminalServiceShape { | ||
|
||
private _terminalService: ITerminalService; | ||
|
||
constructor( | ||
@ITerminalService terminalService: ITerminalService | ||
) { | ||
super(); | ||
this._terminalService = terminalService; | ||
} | ||
|
||
public $createTerminal(name?: string): TPromise<number> { | ||
return this._terminalService.createNew(name); | ||
} | ||
|
||
public $show(terminalId: number, preserveFocus: boolean): void { | ||
this._terminalService.show(!preserveFocus).then((terminalPanel) => { | ||
terminalPanel.setActiveTerminalById(terminalId); | ||
}); | ||
} | ||
|
||
public $hide(terminalId: number): void { | ||
this._terminalService.hide(); | ||
} | ||
|
||
public $dispose(terminalId: number): void { | ||
// TODO: This could be improved by not first showing the terminal to be disposed | ||
var self = this; | ||
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 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. Oh, didn't know TS worked like that. 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. It's even JS these days ;-) |
||
this._terminalService.show(false).then((terminalPanel) => { | ||
terminalPanel.setActiveTerminalById(terminalId); | ||
self._terminalService.close(); | ||
});; | ||
} | ||
|
||
public $sendText(terminalId: number, text: string, addNewLine: boolean): void { | ||
this._terminalService.show(false).then((terminalPanel) => { | ||
terminalPanel.setActiveTerminalById(terminalId); | ||
terminalPanel.sendTextToActiveTerminal(text, addNewLine); | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,15 +65,15 @@ export interface ITerminalService { | |
|
||
close(): TPromise<any>; | ||
copySelection(): TPromise<any>; | ||
createNew(): TPromise<any>; | ||
focus(): TPromise<any>; | ||
createNew(name?: string): TPromise<number>; | ||
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. As a recommendation I'd return the 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. Deferring this to #10721 |
||
focusNext(): TPromise<any>; | ||
focusPrevious(): TPromise<any>; | ||
hide(): TPromise<any>; | ||
paste(): TPromise<any>; | ||
runSelectedText(): TPromise<any>; | ||
scrollDown(): TPromise<any>; | ||
scrollUp(): TPromise<any>; | ||
show(focus: boolean): TPromise<ITerminalPanel>; | ||
setActiveTerminal(index: number): TPromise<any>; | ||
toggle(): TPromise<any>; | ||
|
||
|
@@ -82,3 +82,8 @@ export interface ITerminalService { | |
initConfigHelper(panelContainer: Builder): void; | ||
killTerminalProcess(terminalProcess: ITerminalProcess): void; | ||
} | ||
|
||
export interface ITerminalPanel { | ||
sendTextToActiveTerminal(text: string, addNewLine: boolean): void; | ||
setActiveTerminalById(terminalId: number): void; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,8 @@ import {TabFocus} from 'vs/editor/common/config/commonEditorConfig'; | |
|
||
export class TerminalInstance { | ||
|
||
public id: number; | ||
|
||
private static eolRegex = /\r?\n/g; | ||
|
||
private isExiting: boolean = false; | ||
|
@@ -53,6 +55,7 @@ export class TerminalInstance { | |
this.terminalDomElement = document.createElement('div'); | ||
this.xterm = xterm(); | ||
|
||
this.id = this.terminalProcess.process.pid; | ||
this.terminalProcess.process.on('message', (message) => { | ||
if (message.type === 'data') { | ||
this.xterm.write(message.content); | ||
|
@@ -63,6 +66,8 @@ export class TerminalInstance { | |
event: 'input', | ||
data: this.sanitizeInput(data) | ||
}); | ||
|
||
console.log('this.terminalProcess.process.pid=' + this.terminalProcess.process.pid); | ||
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. 💥 |
||
return false; | ||
}); | ||
this.xterm.attachCustomKeydownHandler(function (event: KeyboardEvent) { | ||
|
@@ -176,6 +181,16 @@ export class TerminalInstance { | |
}); | ||
} | ||
|
||
public sendText(text: string, addNewLine: boolean): void {; | ||
if (addNewLine && text.substr(text.length - os.EOL.length) !== os.EOL) { | ||
text += os.EOL; | ||
} | ||
this.terminalProcess.process.send({ | ||
event: 'input', | ||
data: text | ||
}); | ||
} | ||
|
||
public focus(force?: boolean): void { | ||
if (!this.xterm) { | ||
return; | ||
|
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.
add
name
as readonly so people can access that data they have passed in the create callThere 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 added that and then reverted it, it never changes currently so they should be able to keep their old reference? Figured this would enable more flexibility later on if name becomes more dynamic (
setName
,getName
or something.)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.
For properties we use stuff like
name: string
and add a@readonly
tag, like so: https://github.com/Microsoft/vscode/blob/master/src/vs/vscode.d.ts#L2843 or https://github.com/Microsoft/vscode/blob/master/src/vs/vscode.d.ts#L2751 (actually misses@readonly
)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.
In the interest of keeping the API slim, I don't think it's worth keeping this there since the
name
never changes after passed intocreateTerminal
.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 put consistency against that