Skip to content

Commit e32136a

Browse files
anthonykim1anthonykim1
committed
Switch over to executeCommand from sendText (microsoft#24078)
Resolves: microsoft#23929 TODO: (debt --> in separate PR) Have ensureTerminal return Promise<Terminal> instead of Promise<void> and saving this in the TerminalService class. Would avoid many uses of the !, and maybe even get to throw away the TerminalService class itself.
1 parent d1a7958 commit e32136a

17 files changed

+232
-429
lines changed

‎.github/actions/smoke-tests/action.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ runs:
4343

4444
# Bits from the VSIX are reused by smokeTest.ts to speed things up.
4545
- name: Download VSIX
46-
uses: actions/download-artifact@v2
46+
uses: actions/download-artifact@v3
4747
with:
4848
name: ${{ inputs.artifact_name }}
4949

‎package-lock.json

+8-8
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎package.json

+2-2
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@
4545
"theme": "dark"
4646
},
4747
"engines": {
48-
"vscode": "^1.91.0"
48+
"vscode": "^1.93.0"
4949
},
5050
"enableTelemetry": false,
5151
"keywords": [
@@ -1570,7 +1570,7 @@
15701570
"@types/sinon": "^17.0.3",
15711571
"@types/stack-trace": "0.0.29",
15721572
"@types/tmp": "^0.0.33",
1573-
"@types/vscode": "^1.81.0",
1573+
"@types/vscode": "^1.93.0",
15741574
"@types/which": "^2.0.1",
15751575
"@types/winreg": "^1.2.30",
15761576
"@types/xml2js": "^0.4.2",

‎src/client/common/application/terminalManager.ts

+16-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,16 @@
22
// Licensed under the MIT License.
33

44
import { injectable } from 'inversify';
5-
import { Event, EventEmitter, Terminal, TerminalOptions, window } from 'vscode';
5+
import {
6+
Disposable,
7+
Event,
8+
EventEmitter,
9+
Terminal,
10+
TerminalOptions,
11+
TerminalShellExecutionEndEvent,
12+
TerminalShellIntegrationChangeEvent,
13+
window,
14+
} from 'vscode';
615
import { traceLog } from '../../logging';
716
import { ITerminalManager } from './types';
817

@@ -23,6 +32,12 @@ export class TerminalManager implements ITerminalManager {
2332
public createTerminal(options: TerminalOptions): Terminal {
2433
return monkeyPatchTerminal(window.createTerminal(options));
2534
}
35+
public onDidChangeTerminalShellIntegration(handler: (e: TerminalShellIntegrationChangeEvent) => void): Disposable {
36+
return window.onDidChangeTerminalShellIntegration(handler);
37+
}
38+
public onDidEndTerminalShellExecution(handler: (e: TerminalShellExecutionEndEvent) => void): Disposable {
39+
return window.onDidEndTerminalShellExecution(handler);
40+
}
2641
}
2742

2843
/**

‎src/client/common/application/types.ts

+6
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ import {
4040
StatusBarItem,
4141
Terminal,
4242
TerminalOptions,
43+
TerminalShellExecutionEndEvent,
44+
TerminalShellIntegrationChangeEvent,
4345
TextDocument,
4446
TextDocumentChangeEvent,
4547
TextDocumentShowOptions,
@@ -936,6 +938,10 @@ export interface ITerminalManager {
936938
* @return A new Terminal.
937939
*/
938940
createTerminal(options: TerminalOptions): Terminal;
941+
942+
onDidChangeTerminalShellIntegration(handler: (e: TerminalShellIntegrationChangeEvent) => void): Disposable;
943+
944+
onDidEndTerminalShellExecution(handler: (e: TerminalShellExecutionEndEvent) => void): Disposable;
939945
}
940946

941947
export const IDebugService = Symbol('IDebugManager');

‎src/client/common/terminal/service.ts

+57-5
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
ITerminalService,
2121
TerminalCreationOptions,
2222
TerminalShellType,
23+
ITerminalExecutedCommand,
2324
} from './types';
2425

2526
@injectable()
@@ -32,6 +33,7 @@ export class TerminalService implements ITerminalService, Disposable {
3233
private terminalActivator: ITerminalActivator;
3334
private terminalAutoActivator: ITerminalAutoActivation;
3435
private readonly envVarScript = path.join(EXTENSION_ROOT_DIR, 'python_files', 'pythonrc.py');
36+
private readonly executeCommandListeners: Set<Disposable> = new Set();
3537
public get onDidCloseTerminal(): Event<void> {
3638
return this.terminalClosed.event.bind(this.terminalClosed);
3739
}
@@ -48,8 +50,12 @@ export class TerminalService implements ITerminalService, Disposable {
4850
this.terminalActivator = this.serviceContainer.get<ITerminalActivator>(ITerminalActivator);
4951
}
5052
public dispose() {
51-
if (this.terminal) {
52-
this.terminal.dispose();
53+
this.terminal?.dispose();
54+
55+
if (this.executeCommandListeners && this.executeCommandListeners.size > 0) {
56+
this.executeCommandListeners.forEach((d) => {
57+
d?.dispose();
58+
});
5359
}
5460
}
5561
public async sendCommand(command: string, args: string[], _?: CancellationToken): Promise<void> {
@@ -59,15 +65,60 @@ export class TerminalService implements ITerminalService, Disposable {
5965
this.terminal!.show(true);
6066
}
6167

62-
this.terminal!.sendText(text, true);
68+
await this.executeCommand(text);
6369
}
70+
/** @deprecated */
6471
public async sendText(text: string): Promise<void> {
6572
await this.ensureTerminal();
6673
if (!this.options?.hideFromUser) {
6774
this.terminal!.show(true);
6875
}
6976
this.terminal!.sendText(text);
7077
}
78+
public async executeCommand(commandLine: string): Promise<ITerminalExecutedCommand | undefined> {
79+
const terminal = this.terminal!;
80+
if (!this.options?.hideFromUser) {
81+
terminal.show(true);
82+
}
83+
84+
// If terminal was just launched, wait some time for shell integration to onDidChangeShellIntegration.
85+
if (!terminal.shellIntegration) {
86+
const promise = new Promise<boolean>((resolve) => {
87+
const shellIntegrationChangeEventListener = this.terminalManager.onDidChangeTerminalShellIntegration(
88+
() => {
89+
this.executeCommandListeners.delete(shellIntegrationChangeEventListener);
90+
resolve(true);
91+
},
92+
);
93+
const TIMEOUT_DURATION = 3000;
94+
setTimeout(() => {
95+
this.executeCommandListeners.add(shellIntegrationChangeEventListener);
96+
resolve(true);
97+
}, TIMEOUT_DURATION);
98+
});
99+
await promise;
100+
}
101+
102+
if (terminal.shellIntegration) {
103+
const execution = terminal.shellIntegration.executeCommand(commandLine);
104+
return await new Promise((resolve) => {
105+
const listener = this.terminalManager.onDidEndTerminalShellExecution((e) => {
106+
if (e.execution === execution) {
107+
this.executeCommandListeners.delete(listener);
108+
resolve({ execution, exitCode: e.exitCode });
109+
}
110+
});
111+
if (listener) {
112+
this.executeCommandListeners.add(listener);
113+
}
114+
});
115+
} else {
116+
terminal.sendText(commandLine);
117+
}
118+
119+
return undefined;
120+
}
121+
71122
public async show(preserveFocus: boolean = true): Promise<void> {
72123
await this.ensureTerminal(preserveFocus);
73124
if (!this.options?.hideFromUser) {
@@ -93,18 +144,19 @@ export class TerminalService implements ITerminalService, Disposable {
93144
// Sometimes the terminal takes some time to start up before it can start accepting input.
94145
await new Promise((resolve) => setTimeout(resolve, 100));
95146

96-
await this.terminalActivator.activateEnvironmentInTerminal(this.terminal!, {
147+
await this.terminalActivator.activateEnvironmentInTerminal(this.terminal, {
97148
resource: this.options?.resource,
98149
preserveFocus,
99150
interpreter: this.options?.interpreter,
100151
hideFromUser: this.options?.hideFromUser,
101152
});
102153

103154
if (!this.options?.hideFromUser) {
104-
this.terminal!.show(preserveFocus);
155+
this.terminal.show(preserveFocus);
105156
}
106157

107158
this.sendTelemetry().ignoreErrors();
159+
return;
108160
}
109161
private terminalCloseHandler(terminal: Terminal) {
110162
if (terminal === this.terminal) {

‎src/client/common/terminal/syncTerminalService.ts

+5-1
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import * as internalScripts from '../process/internal/scripts';
1414
import { createDeferred, Deferred } from '../utils/async';
1515
import { noop } from '../utils/misc';
1616
import { TerminalService } from './service';
17-
import { ITerminalService } from './types';
17+
import { ITerminalService, ITerminalExecutedCommand } from './types';
1818

1919
enum State {
2020
notStarted = 0,
@@ -146,9 +146,13 @@ export class SynchronousTerminalService implements ITerminalService, Disposable
146146
lockFile.dispose();
147147
}
148148
}
149+
/** @deprecated */
149150
public sendText(text: string): Promise<void> {
150151
return this.terminalService.sendText(text);
151152
}
153+
public executeCommand(commandLine: string): Promise<ITerminalExecutedCommand | undefined> {
154+
return this.terminalService.executeCommand(commandLine);
155+
}
152156
public show(preserveFocus?: boolean | undefined): Promise<void> {
153157
return this.terminalService.show(preserveFocus);
154158
}

‎src/client/common/terminal/types.ts

+8-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
'use strict';
55

6-
import { CancellationToken, Event, Terminal, Uri } from 'vscode';
6+
import { CancellationToken, Event, Terminal, Uri, TerminalShellExecution } from 'vscode';
77
import { PythonEnvironment } from '../../pythonEnvironments/info';
88
import { IEventNamePropertyMapping } from '../../telemetry/index';
99
import { IDisposable, Resource } from '../types';
@@ -52,10 +52,17 @@ export interface ITerminalService extends IDisposable {
5252
cancel?: CancellationToken,
5353
swallowExceptions?: boolean,
5454
): Promise<void>;
55+
/** @deprecated */
5556
sendText(text: string): Promise<void>;
57+
executeCommand(commandLine: string): Promise<ITerminalExecutedCommand | undefined>;
5658
show(preserveFocus?: boolean): Promise<void>;
5759
}
5860

61+
export interface ITerminalExecutedCommand {
62+
execution: TerminalShellExecution;
63+
exitCode: number | undefined;
64+
}
65+
5966
export const ITerminalServiceFactory = Symbol('ITerminalServiceFactory');
6067

6168
export type TerminalCreationOptions = {

‎src/client/terminals/codeExecution/terminalCodeExecution.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
5959
this.configurationService.updateSetting('REPL.enableREPLSmartSend', false, resource);
6060
}
6161
} else {
62-
await this.getTerminalService(resource).sendText(code);
62+
await this.getTerminalService(resource).executeCommand(code);
6363
}
6464
}
6565

‎src/test/common/terminals/service.unit.test.ts

+54-4
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,15 @@
44
import { expect } from 'chai';
55
import * as path from 'path';
66
import * as TypeMoq from 'typemoq';
7-
import { Disposable, Terminal as VSCodeTerminal, WorkspaceConfiguration } from 'vscode';
7+
import {
8+
Disposable,
9+
EventEmitter,
10+
TerminalShellExecution,
11+
TerminalShellExecutionEndEvent,
12+
TerminalShellIntegration,
13+
Terminal as VSCodeTerminal,
14+
WorkspaceConfiguration,
15+
} from 'vscode';
816
import { ITerminalManager, IWorkspaceService } from '../../../client/common/application/types';
917
import { EXTENSION_ROOT_DIR } from '../../../client/common/constants';
1018
import { IPlatformService } from '../../../client/common/platform/types';
@@ -26,9 +34,44 @@ suite('Terminal Service', () => {
2634
let disposables: Disposable[] = [];
2735
let mockServiceContainer: TypeMoq.IMock<IServiceContainer>;
2836
let terminalAutoActivator: TypeMoq.IMock<ITerminalAutoActivation>;
37+
let terminalShellIntegration: TypeMoq.IMock<TerminalShellIntegration>;
38+
let onDidEndTerminalShellExecutionEmitter: EventEmitter<TerminalShellExecutionEndEvent>;
39+
let event: TerminalShellExecutionEndEvent;
40+
2941
setup(() => {
3042
terminal = TypeMoq.Mock.ofType<VSCodeTerminal>();
43+
terminalShellIntegration = TypeMoq.Mock.ofType<TerminalShellIntegration>();
44+
terminal.setup((t) => t.shellIntegration).returns(() => terminalShellIntegration.object);
45+
46+
onDidEndTerminalShellExecutionEmitter = new EventEmitter<TerminalShellExecutionEndEvent>();
3147
terminalManager = TypeMoq.Mock.ofType<ITerminalManager>();
48+
const execution: TerminalShellExecution = {
49+
commandLine: {
50+
value: 'dummy text',
51+
isTrusted: true,
52+
confidence: 2,
53+
},
54+
cwd: undefined,
55+
read: function (): AsyncIterable<string> {
56+
throw new Error('Function not implemented.');
57+
},
58+
};
59+
60+
event = {
61+
execution,
62+
exitCode: 0,
63+
terminal: terminal.object,
64+
shellIntegration: terminalShellIntegration.object,
65+
};
66+
67+
terminalShellIntegration.setup((t) => t.executeCommand(TypeMoq.It.isAny())).returns(() => execution);
68+
69+
terminalManager
70+
.setup((t) => t.onDidEndTerminalShellExecution)
71+
.returns(() => {
72+
setTimeout(() => onDidEndTerminalShellExecutionEmitter.fire(event), 100);
73+
return onDidEndTerminalShellExecutionEmitter.event;
74+
});
3275
platformService = TypeMoq.Mock.ofType<IPlatformService>();
3376
workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
3477
terminalHelper = TypeMoq.Mock.ofType<ITerminalHelper>();
@@ -37,6 +80,7 @@ suite('Terminal Service', () => {
3780
disposables = [];
3881

3982
mockServiceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
83+
4084
mockServiceContainer.setup((c) => c.get(ITerminalManager)).returns(() => terminalManager.object);
4185
mockServiceContainer.setup((c) => c.get(ITerminalHelper)).returns(() => terminalHelper.object);
4286
mockServiceContainer.setup((c) => c.get(IPlatformService)).returns(() => platformService.object);
@@ -75,10 +119,16 @@ suite('Terminal Service', () => {
75119
.setup((h) => h.buildCommandForTerminal(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
76120
.returns(() => 'dummy text');
77121

122+
terminalManager
123+
.setup((t) => t.onDidEndTerminalShellExecution)
124+
.returns(() => {
125+
setTimeout(() => onDidEndTerminalShellExecutionEmitter.fire(event), 100);
126+
return onDidEndTerminalShellExecutionEmitter.event;
127+
});
78128
// Sending a command will cause the terminal to be created
79129
await service.sendCommand('', []);
80130

81-
terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.exactly(2));
131+
terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.atLeastOnce());
82132
service.dispose();
83133
terminal.verify((t) => t.dispose(), TypeMoq.Times.exactly(1));
84134
});
@@ -99,10 +149,10 @@ suite('Terminal Service', () => {
99149

100150
await service.sendCommand(commandToSend, args);
101151

102-
terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.exactly(2));
152+
terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.atLeastOnce());
103153
terminal.verify(
104154
(t) => t.sendText(TypeMoq.It.isValue(commandToExpect), TypeMoq.It.isValue(true)),
105-
TypeMoq.Times.exactly(1),
155+
TypeMoq.Times.never(),
106156
);
107157
});
108158

0 commit comments

Comments
 (0)