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.

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

@@ -176,7 +176,6 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape
 
 	public $startLinkProvider(): void {
 		this._linkProvider?.dispose();
-		// TODO: Verify sharing a link provider works fine with removal of intersecting links
 		this._linkProvider = this._terminalService.registerLinkProvider(new ExtensionTerminalLinkProvider(this._proxy));
 	}
 
@@ -432,10 +431,7 @@ class ExtensionTerminalLinkProvider implements ITerminalExternalLinkProvider {
 			startIndex: dto.startIndex,
 			length: dto.length,
 			label: dto.label,
-			activate(text: string) {
-				console.log('Activated! ' + text);
-				proxy.$activateLink(instance.id, dto.id);
-			}
+			activate: () => proxy.$activateLink(instance.id, dto.id)
 		}));
 	}
 }

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

@@ -627,8 +627,6 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ
 		// when new links are provided.
 		this._terminalLinkCache.delete(terminalId);
 
-		// TODO: Store link activate callback
-		// TODO: Discard of links when appropriate
 		const result: ITerminalLinkDto[] = [];
 		const context: vscode.TerminalLinkContext = { terminal, line };
 		const promises: vscode.ProviderResult<{ provider: vscode.TerminalLinkProvider, links: vscode.TerminalLink[] }>[] = [];

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

@@ -50,7 +50,6 @@ export class TerminalExternalLinkProviderAdapter extends TerminalBaseLinkProvide
 			return [];
 		}
 
-		// TODO: Add handling default handling of links via the target property on the ext host
 		return externalLinks.map(link => {
 			const bufferRange = convertLinkRangeToBuffer(lines, this._xterm.cols, {
 				startColumn: link.startIndex + 1,

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

@@ -257,6 +257,7 @@ export interface ITerminalInstance {
 
 	onFocused: Event<ITerminalInstance>;
 	onProcessIdReady: Event<ITerminalInstance>;
+	onLinksReady: Event<ITerminalInstance>;
 	onRequestExtHostProcess: Event<ITerminalInstance>;
 	onDimensionsChanged: Event<void>;
 	onMaximumDimensionsChanged: Event<void>;
@@ -295,12 +296,11 @@ export interface ITerminalInstance {
 
 	readonly exitCode: number | undefined;
 
+	readonly areLinksReady: boolean;
+
 	/** A promise that resolves when the terminal's pty/process have been created. */
 	processReady: Promise<void>;
 
-	/** Whether xterm.js has been created. */
-	isXtermReady: boolean;
-
 	/** A promise that resolves when xterm.js has been created. */
 	xtermReady: Promise<void>;
 

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

@@ -95,6 +95,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
 	private _xtermReadyPromise: Promise<XTermTerminal>;
 	private _titleReadyPromise: Promise<string>;
 	private _titleReadyComplete: ((title: string) => any) | undefined;
+	private _areLinksReady: boolean = false;
 
 	private _messageTitleDisposable: IDisposable | undefined;
 
@@ -127,7 +128,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
 	// TODO: How does this work with detached processes?
 	// TODO: Should this be an event as it can fire twice?
 	public get processReady(): Promise<void> { return this._processManager.ptyProcessReady; }
-	public get isXtermReady(): boolean { return !!this._xterm; }
+	public get areLinksReady(): boolean { return this._areLinksReady; }
 	public get xtermReady(): Promise<void> { return this._xtermReadyPromise.then(() => { }); }
 	public get exitCode(): number | undefined { return this._exitCode; }
 	public get title(): string { return this._title; }
@@ -146,6 +147,8 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
 	public get onFocused(): Event<ITerminalInstance> { return this._onFocused.event; }
 	private readonly _onProcessIdReady = new Emitter<ITerminalInstance>();
 	public get onProcessIdReady(): Event<ITerminalInstance> { return this._onProcessIdReady.event; }
+	private readonly _onLinksReady = new Emitter<ITerminalInstance>();
+	public get onLinksReady(): Event<ITerminalInstance> { return this._onLinksReady.event; }
 	private readonly _onTitleChanged = new Emitter<ITerminalInstance>();
 	public get onTitleChanged(): Event<ITerminalInstance> { return this._onTitleChanged.event; }
 	private readonly _onData = new Emitter<string>();
@@ -203,7 +206,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
 		this._logService.trace(`terminalInstance#ctor (id: ${this.id})`, this._shellLaunchConfig);
 
 		this._initDimensions();
-		this._createProcess();
+		this._createProcessManager();
 
 		this._xtermReadyPromise = this._createXterm();
 		this._xtermReadyPromise.then(() => {
@@ -211,6 +214,12 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
 			if (_container) {
 				this._attachToElement(_container);
 			}
+
+			this._processManager.createProcess(this._shellLaunchConfig, this._cols, this._rows, this._accessibilityService.isScreenReaderOptimized()).then(error => {
+				if (error) {
+					this._onProcessExit(error);
+				}
+			});
 		});
 
 		this.addDisposable(this._configurationService.onDidChangeConfiguration(e => {
@@ -418,6 +427,8 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
 				e.terminal = this;
 				this._onBeforeHandleLink.fire(e);
 			});
+			this._areLinksReady = true;
+			this._onLinksReady.fire(this);
 		});
 
 		this._commandTrackerAddon = new CommandTrackerAddon();
@@ -870,9 +881,12 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
 		this._terminalHasTextContextKey.set(isActive && this.hasSelection());
 	}
 
-	protected _createProcess(): void {
+	protected _createProcessManager(): void {
 		this._processManager = this._instantiationService.createInstance(TerminalProcessManager, this._id, this._configHelper);
-		this._processManager.onProcessReady(() => this._onProcessIdReady.fire(this));
+		this._processManager.onProcessReady(() => {
+			console.log('_processManager.onProcessReady');
+			this._onProcessIdReady.fire(this);
+		});
 		this._processManager.onProcessExit(exitCode => this._onProcessExit(exitCode));
 		this._processManager.onProcessData(data => this._onData.fire(data));
 		this._processManager.onProcessOverrideDimensions(e => this.setDimensions(e));
@@ -916,14 +930,6 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
 				});
 			});
 		}
-
-		// Create the process asynchronously to allow the terminal's container to be created so
-		// dimensions are accurate
-		this._processManager.createProcess(this._shellLaunchConfig, this._cols, this._rows, this._accessibilityService.isScreenReaderOptimized()).then(error => {
-			if (error) {
-				this._onProcessExit(error);
-			}
-		});
 	}
 
 	private getShellType(executable: string): TerminalShellType {
@@ -1110,7 +1116,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
 		// Launch the process unless this is only a renderer.
 		// In the renderer only cases, we still need to set the title correctly.
 		const oldTitle = this._title;
-		this._createProcess();
+		this._createProcessManager();
 
 		if (oldTitle !== this._title) {
 			this.setTitle(this._title, TitleEventSource.Process);
@@ -1497,7 +1503,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
 
 	public registerLinkProvider(provider: ITerminalExternalLinkProvider): IDisposable {
 		if (!this._linkManager) {
-			throw new Error('TerminalInstance.registerLinkProvider before xterm was created');
+			throw new Error('TerminalInstance.registerLinkProvider before link manager was ready');
 		}
 		return this._linkManager.registerExternalLinkProvider(this, provider);
 	}

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

@@ -55,6 +55,7 @@ export class TerminalService implements ITerminalService {
 	private _activeTabIndex: number;
 	private _linkHandlers: { [key: string]: TerminalLinkHandlerCallback } = {};
 	private _linkProviders: Set<ITerminalExternalLinkProvider> = new Set();
+	private _linkProviderDisposables: Map<ITerminalExternalLinkProvider, IDisposable[]> = new Map();
 
 	public get activeTabIndex(): number { return this._activeTabIndex; }
 	public get terminalInstances(): ITerminalInstance[] { return this._terminalInstances; }
@@ -73,6 +74,8 @@ export class TerminalService implements ITerminalService {
 	public get onInstanceDisposed(): Event<ITerminalInstance> { return this._onInstanceDisposed.event; }
 	private readonly _onInstanceProcessIdReady = new Emitter<ITerminalInstance>();
 	public get onInstanceProcessIdReady(): Event<ITerminalInstance> { return this._onInstanceProcessIdReady.event; }
+	private readonly _onInstanceLinksReady = new Emitter<ITerminalInstance>();
+	public get onInstanceLinksReady(): Event<ITerminalInstance> { return this._onInstanceLinksReady.event; }
 	private readonly _onInstanceRequestSpawnExtHostProcess = new Emitter<ISpawnExtHostProcessRequest>();
 	public get onInstanceRequestSpawnExtHostProcess(): Event<ISpawnExtHostProcessRequest> { return this._onInstanceRequestSpawnExtHostProcess.event; }
 	private readonly _onInstanceRequestStartExtensionTerminal = new Emitter<IStartExtensionTerminalRequest>();
@@ -131,7 +134,7 @@ export class TerminalService implements ITerminalService {
 			const instance = this.getActiveInstance();
 			this._onActiveInstanceChanged.fire(instance ? instance : undefined);
 		});
-		this.onInstanceXtermReady(instance => this._setInstanceLinkProviders(instance));
+		this.onInstanceLinksReady(instance => this._setInstanceLinkProviders(instance));
 
 		this._handleContextKeys();
 	}
@@ -433,6 +436,7 @@ export class TerminalService implements ITerminalService {
 		instance.addDisposable(instance.onDisposed(this._onInstanceDisposed.fire, this._onInstanceDisposed));
 		instance.addDisposable(instance.onTitleChanged(this._onInstanceTitleChanged.fire, this._onInstanceTitleChanged));
 		instance.addDisposable(instance.onProcessIdReady(this._onInstanceProcessIdReady.fire, this._onInstanceProcessIdReady));
+		instance.addDisposable(instance.onLinksReady(this._onInstanceLinksReady.fire, this._onInstanceLinksReady));
 		instance.addDisposable(instance.onDimensionsChanged(() => this._onInstanceDimensionsChanged.fire(instance)));
 		instance.addDisposable(instance.onMaximumDimensionsChanged(() => this._onInstanceMaximumDimensionsChanged.fire(instance)));
 		instance.addDisposable(instance.onFocus(this._onActiveInstanceChanged.fire, this._onActiveInstanceChanged));
@@ -483,20 +487,18 @@ export class TerminalService implements ITerminalService {
 	}
 
 	public registerLinkProvider(linkProvider: ITerminalExternalLinkProvider): IDisposable {
-		// TODO: Register it from the main thread class
 		const disposables: IDisposable[] = [];
 		this._linkProviders.add(linkProvider);
 		for (const instance of this.terminalInstances) {
 			// Only register immediately when xterm is ready
-			if (instance.isXtermReady) {
+			if (instance.areLinksReady) {
 				disposables.push(instance.registerLinkProvider(linkProvider));
 			}
 		}
-		console.log('registerLinkProvider register');
+		this._linkProviderDisposables.set(linkProvider, disposables);
 		return {
 			dispose: () => {
-				console.log('registerLinkProvider dispose');
-				// TODO: Remove from xterm instances
+				const disposables = this._linkProviderDisposables.get(linkProvider) || [];
 				for (const disposable of disposables) {
 					disposable.dispose();
 				}
@@ -507,7 +509,9 @@ export class TerminalService implements ITerminalService {
 
 	private _setInstanceLinkProviders(instance: ITerminalInstance): void {
 		for (const linkProvider of this._linkProviders) {
-			instance.registerLinkProvider(linkProvider);
+			const disposables = this._linkProviderDisposables.get(linkProvider);
+			const provider = instance.registerLinkProvider(linkProvider);
+			disposables?.push(provider);
 		}
 	}