Skip to content

Commit cb5ddcf

Browse files
TyriarTyriar
committed
Resolve todos, move process creation after xterm
Having _processManager.createProcess called before _createXterm was causing some event listeners like onProcessReady to fire before they were registered within _createXterm.
1 parent 34b20f9 commit cb5ddcf

6 files changed

Lines changed: 35 additions & 32 deletions

File tree

‎src/vs/workbench/api/browser/mainThreadTerminalService.ts‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,6 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape
176176

177177
public $startLinkProvider(): void {
178178
this._linkProvider?.dispose();
179-
// TODO: Verify sharing a link provider works fine with removal of intersecting links
180179
this._linkProvider = this._terminalService.registerLinkProvider(new ExtensionTerminalLinkProvider(this._proxy));
181180
}
182181

@@ -432,10 +431,7 @@ class ExtensionTerminalLinkProvider implements ITerminalExternalLinkProvider {
432431
startIndex: dto.startIndex,
433432
length: dto.length,
434433
label: dto.label,
435-
activate(text: string) {
436-
console.log('Activated! ' + text);
437-
proxy.$activateLink(instance.id, dto.id);
438-
}
434+
activate: () => proxy.$activateLink(instance.id, dto.id)
439435
}));
440436
}
441437
}

‎src/vs/workbench/api/common/extHostTerminalService.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -627,8 +627,6 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ
627627
// when new links are provided.
628628
this._terminalLinkCache.delete(terminalId);
629629

630-
// TODO: Store link activate callback
631-
// TODO: Discard of links when appropriate
632630
const result: ITerminalLinkDto[] = [];
633631
const context: vscode.TerminalLinkContext = { terminal, line };
634632
const promises: vscode.ProviderResult<{ provider: vscode.TerminalLinkProvider, links: vscode.TerminalLink[] }>[] = [];

‎src/vs/workbench/contrib/terminal/browser/links/terminalExternalLinkProviderAdapter.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ export class TerminalExternalLinkProviderAdapter extends TerminalBaseLinkProvide
5050
return [];
5151
}
5252

53-
// TODO: Add handling default handling of links via the target property on the ext host
5453
return externalLinks.map(link => {
5554
const bufferRange = convertLinkRangeToBuffer(lines, this._xterm.cols, {
5655
startColumn: link.startIndex + 1,

‎src/vs/workbench/contrib/terminal/browser/terminal.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,7 @@ export interface ITerminalInstance {
257257

258258
onFocused: Event<ITerminalInstance>;
259259
onProcessIdReady: Event<ITerminalInstance>;
260+
onLinksReady: Event<ITerminalInstance>;
260261
onRequestExtHostProcess: Event<ITerminalInstance>;
261262
onDimensionsChanged: Event<void>;
262263
onMaximumDimensionsChanged: Event<void>;
@@ -295,12 +296,11 @@ export interface ITerminalInstance {
295296

296297
readonly exitCode: number | undefined;
297298

299+
readonly areLinksReady: boolean;
300+
298301
/** A promise that resolves when the terminal's pty/process have been created. */
299302
processReady: Promise<void>;
300303

301-
/** Whether xterm.js has been created. */
302-
isXtermReady: boolean;
303-
304304
/** A promise that resolves when xterm.js has been created. */
305305
xtermReady: Promise<void>;
306306

‎src/vs/workbench/contrib/terminal/browser/terminalInstance.ts‎

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
9595
private _xtermReadyPromise: Promise<XTermTerminal>;
9696
private _titleReadyPromise: Promise<string>;
9797
private _titleReadyComplete: ((title: string) => any) | undefined;
98+
private _areLinksReady: boolean = false;
9899

99100
private _messageTitleDisposable: IDisposable | undefined;
100101

@@ -127,7 +128,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
127128
// TODO: How does this work with detached processes?
128129
// TODO: Should this be an event as it can fire twice?
129130
public get processReady(): Promise<void> { return this._processManager.ptyProcessReady; }
130-
public get isXtermReady(): boolean { return !!this._xterm; }
131+
public get areLinksReady(): boolean { return this._areLinksReady; }
131132
public get xtermReady(): Promise<void> { return this._xtermReadyPromise.then(() => { }); }
132133
public get exitCode(): number | undefined { return this._exitCode; }
133134
public get title(): string { return this._title; }
@@ -146,6 +147,8 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
146147
public get onFocused(): Event<ITerminalInstance> { return this._onFocused.event; }
147148
private readonly _onProcessIdReady = new Emitter<ITerminalInstance>();
148149
public get onProcessIdReady(): Event<ITerminalInstance> { return this._onProcessIdReady.event; }
150+
private readonly _onLinksReady = new Emitter<ITerminalInstance>();
151+
public get onLinksReady(): Event<ITerminalInstance> { return this._onLinksReady.event; }
149152
private readonly _onTitleChanged = new Emitter<ITerminalInstance>();
150153
public get onTitleChanged(): Event<ITerminalInstance> { return this._onTitleChanged.event; }
151154
private readonly _onData = new Emitter<string>();
@@ -203,14 +206,20 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
203206
this._logService.trace(`terminalInstance#ctor (id: ${this.id})`, this._shellLaunchConfig);
204207

205208
this._initDimensions();
206-
this._createProcess();
209+
this._createProcessManager();
207210

208211
this._xtermReadyPromise = this._createXterm();
209212
this._xtermReadyPromise.then(() => {
210213
// Only attach xterm.js to the DOM if the terminal panel has been opened before.
211214
if (_container) {
212215
this._attachToElement(_container);
213216
}
217+
218+
this._processManager.createProcess(this._shellLaunchConfig, this._cols, this._rows, this._accessibilityService.isScreenReaderOptimized()).then(error => {
219+
if (error) {
220+
this._onProcessExit(error);
221+
}
222+
});
214223
});
215224

216225
this.addDisposable(this._configurationService.onDidChangeConfiguration(e => {
@@ -418,6 +427,8 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
418427
e.terminal = this;
419428
this._onBeforeHandleLink.fire(e);
420429
});
430+
this._areLinksReady = true;
431+
this._onLinksReady.fire(this);
421432
});
422433

423434
this._commandTrackerAddon = new CommandTrackerAddon();
@@ -870,9 +881,12 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
870881
this._terminalHasTextContextKey.set(isActive && this.hasSelection());
871882
}
872883

873-
protected _createProcess(): void {
884+
protected _createProcessManager(): void {
874885
this._processManager = this._instantiationService.createInstance(TerminalProcessManager, this._id, this._configHelper);
875-
this._processManager.onProcessReady(() => this._onProcessIdReady.fire(this));
886+
this._processManager.onProcessReady(() => {
887+
console.log('_processManager.onProcessReady');
888+
this._onProcessIdReady.fire(this);
889+
});
876890
this._processManager.onProcessExit(exitCode => this._onProcessExit(exitCode));
877891
this._processManager.onProcessData(data => this._onData.fire(data));
878892
this._processManager.onProcessOverrideDimensions(e => this.setDimensions(e));
@@ -916,14 +930,6 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
916930
});
917931
});
918932
}
919-
920-
// Create the process asynchronously to allow the terminal's container to be created so
921-
// dimensions are accurate
922-
this._processManager.createProcess(this._shellLaunchConfig, this._cols, this._rows, this._accessibilityService.isScreenReaderOptimized()).then(error => {
923-
if (error) {
924-
this._onProcessExit(error);
925-
}
926-
});
927933
}
928934

929935
private getShellType(executable: string): TerminalShellType {
@@ -1110,7 +1116,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
11101116
// Launch the process unless this is only a renderer.
11111117
// In the renderer only cases, we still need to set the title correctly.
11121118
const oldTitle = this._title;
1113-
this._createProcess();
1119+
this._createProcessManager();
11141120

11151121
if (oldTitle !== this._title) {
11161122
this.setTitle(this._title, TitleEventSource.Process);
@@ -1497,7 +1503,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
14971503

14981504
public registerLinkProvider(provider: ITerminalExternalLinkProvider): IDisposable {
14991505
if (!this._linkManager) {
1500-
throw new Error('TerminalInstance.registerLinkProvider before xterm was created');
1506+
throw new Error('TerminalInstance.registerLinkProvider before link manager was ready');
15011507
}
15021508
return this._linkManager.registerExternalLinkProvider(this, provider);
15031509
}

‎src/vs/workbench/contrib/terminal/browser/terminalService.ts‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ export class TerminalService implements ITerminalService {
5555
private _activeTabIndex: number;
5656
private _linkHandlers: { [key: string]: TerminalLinkHandlerCallback } = {};
5757
private _linkProviders: Set<ITerminalExternalLinkProvider> = new Set();
58+
private _linkProviderDisposables: Map<ITerminalExternalLinkProvider, IDisposable[]> = new Map();
5859

5960
public get activeTabIndex(): number { return this._activeTabIndex; }
6061
public get terminalInstances(): ITerminalInstance[] { return this._terminalInstances; }
@@ -73,6 +74,8 @@ export class TerminalService implements ITerminalService {
7374
public get onInstanceDisposed(): Event<ITerminalInstance> { return this._onInstanceDisposed.event; }
7475
private readonly _onInstanceProcessIdReady = new Emitter<ITerminalInstance>();
7576
public get onInstanceProcessIdReady(): Event<ITerminalInstance> { return this._onInstanceProcessIdReady.event; }
77+
private readonly _onInstanceLinksReady = new Emitter<ITerminalInstance>();
78+
public get onInstanceLinksReady(): Event<ITerminalInstance> { return this._onInstanceLinksReady.event; }
7679
private readonly _onInstanceRequestSpawnExtHostProcess = new Emitter<ISpawnExtHostProcessRequest>();
7780
public get onInstanceRequestSpawnExtHostProcess(): Event<ISpawnExtHostProcessRequest> { return this._onInstanceRequestSpawnExtHostProcess.event; }
7881
private readonly _onInstanceRequestStartExtensionTerminal = new Emitter<IStartExtensionTerminalRequest>();
@@ -131,7 +134,7 @@ export class TerminalService implements ITerminalService {
131134
const instance = this.getActiveInstance();
132135
this._onActiveInstanceChanged.fire(instance ? instance : undefined);
133136
});
134-
this.onInstanceXtermReady(instance => this._setInstanceLinkProviders(instance));
137+
this.onInstanceLinksReady(instance => this._setInstanceLinkProviders(instance));
135138

136139
this._handleContextKeys();
137140
}
@@ -433,6 +436,7 @@ export class TerminalService implements ITerminalService {
433436
instance.addDisposable(instance.onDisposed(this._onInstanceDisposed.fire, this._onInstanceDisposed));
434437
instance.addDisposable(instance.onTitleChanged(this._onInstanceTitleChanged.fire, this._onInstanceTitleChanged));
435438
instance.addDisposable(instance.onProcessIdReady(this._onInstanceProcessIdReady.fire, this._onInstanceProcessIdReady));
439+
instance.addDisposable(instance.onLinksReady(this._onInstanceLinksReady.fire, this._onInstanceLinksReady));
436440
instance.addDisposable(instance.onDimensionsChanged(() => this._onInstanceDimensionsChanged.fire(instance)));
437441
instance.addDisposable(instance.onMaximumDimensionsChanged(() => this._onInstanceMaximumDimensionsChanged.fire(instance)));
438442
instance.addDisposable(instance.onFocus(this._onActiveInstanceChanged.fire, this._onActiveInstanceChanged));
@@ -483,20 +487,18 @@ export class TerminalService implements ITerminalService {
483487
}
484488

485489
public registerLinkProvider(linkProvider: ITerminalExternalLinkProvider): IDisposable {
486-
// TODO: Register it from the main thread class
487490
const disposables: IDisposable[] = [];
488491
this._linkProviders.add(linkProvider);
489492
for (const instance of this.terminalInstances) {
490493
// Only register immediately when xterm is ready
491-
if (instance.isXtermReady) {
494+
if (instance.areLinksReady) {
492495
disposables.push(instance.registerLinkProvider(linkProvider));
493496
}
494497
}
495-
console.log('registerLinkProvider register');
498+
this._linkProviderDisposables.set(linkProvider, disposables);
496499
return {
497500
dispose: () => {
498-
console.log('registerLinkProvider dispose');
499-
// TODO: Remove from xterm instances
501+
const disposables = this._linkProviderDisposables.get(linkProvider) || [];
500502
for (const disposable of disposables) {
501503
disposable.dispose();
502504
}
@@ -507,7 +509,9 @@ export class TerminalService implements ITerminalService {
507509

508510
private _setInstanceLinkProviders(instance: ITerminalInstance): void {
509511
for (const linkProvider of this._linkProviders) {
510-
instance.registerLinkProvider(linkProvider);
512+
const disposables = this._linkProviderDisposables.get(linkProvider);
513+
const provider = instance.registerLinkProvider(linkProvider);
514+
disposables?.push(provider);
511515
}
512516
}
513517

0 commit comments

Comments
 (0)