Merge pull request microsoft#203932 from microsoft/tyriar/203930

Don't recreate TerminalLinkManager after registered by ext

Author: Tyriar

src/vs/workbench/contrib/terminalContrib/links/browser/terminal.links.contribution.ts

@@ -55,29 +55,24 @@ class TerminalLinkContribution extends DisposableStore implements ITerminalContr
 
 		// Set widget manager
 		if (isTerminalProcessManager(this._processManager)) {
-			const disposable = this.add(Event.once(this._processManager.onProcessReady)(() => {
+			const disposable = linkManager.add(Event.once(this._processManager.onProcessReady)(() => {
 				linkManager.setWidgetManager(this._widgetManager);
 				this.delete(disposable);
 			}));
 		} else {
 			linkManager.setWidgetManager(this._widgetManager);
 		}
 
-		// Attach the link provider(s) to the instance and listen for changes
+		// Attach the external link provider to the instance and listen for changes
 		if (!isDetachedTerminalInstance(this._instance)) {
 			for (const linkProvider of this._terminalLinkProviderService.linkProviders) {
-				linkManager.registerExternalLinkProvider(linkProvider.provideLinks.bind(linkProvider, this._instance));
+				linkManager.externalProvideLinksCb = linkProvider.provideLinks.bind(linkProvider, this._instance);
 			}
 			linkManager.add(this._terminalLinkProviderService.onDidAddLinkProvider(e => {
-				linkManager.registerExternalLinkProvider(e.provideLinks.bind(e, this._instance as ITerminalInstance));
+				linkManager.externalProvideLinksCb = e.provideLinks.bind(e, this._instance as ITerminalInstance);
 			}));
 		}
-
-		// TODO: Currently only a single link provider is supported; the one registered by the ext host
-		linkManager.add(this._terminalLinkProviderService.onDidRemoveLinkProvider(e => {
-			linkManager.dispose();
-			this.xtermReady(xterm);
-		}));
+		linkManager.add(this._terminalLinkProviderService.onDidRemoveLinkProvider(() => linkManager.externalProvideLinksCb = undefined));
 	}
 
 	async showLinkQuickpick(extended?: boolean): Promise<void> {

src/vs/workbench/contrib/terminalContrib/links/browser/terminalLinkManager.ts

@@ -45,6 +45,8 @@ export class TerminalLinkManager extends DisposableStore {
 	private readonly _externalLinkProviders: IDisposable[] = [];
 	private readonly _openers: Map<TerminalLinkType, ITerminalLinkOpener> = new Map();
 
+	externalProvideLinksCb?: OmitFirstArg<ITerminalExternalLinkProvider['provideLinks']>;
+
 	constructor(
 		private readonly _xterm: Terminal,
 		private readonly _processInfo: ITerminalProcessInfo,
@@ -349,25 +351,30 @@ export class TerminalLinkManager extends DisposableStore {
 	}
 
 	private _registerStandardLinkProviders(): void {
+		// Forward any external link provider requests to the registered provider if it exists. This
+		// helps maintain the relative priority of the link providers as it's defined by the order
+		// in which they're registered in xterm.js.
+		//
+		/**
+		 * There's a bit going on here but here's another view:
+		 * - {@link externalProvideLinksCb} The external callback that gives the links (eg. from
+		 *   exthost)
+		 * - {@link proxyLinkProvider} A proxy that forwards the call over to
+		 *   {@link externalProvideLinksCb}
+		 * - {@link wrappedLinkProvider} Wraps the above in an `TerminalLinkDetectorAdapter`
+		 */
+		const proxyLinkProvider: OmitFirstArg<ITerminalExternalLinkProvider['provideLinks']> = async (bufferLineNumber) => {
+			return this.externalProvideLinksCb?.(bufferLineNumber);
+		};
+		const detectorId = `extension-${this._externalLinkProviders.length}`;
+		const wrappedLinkProvider = this._setupLinkDetector(detectorId, new TerminalExternalLinkDetector(detectorId, this._xterm, proxyLinkProvider), true);
+		this._linkProvidersDisposables.push(this._xterm.registerLinkProvider(wrappedLinkProvider));
+
 		for (const p of this._standardLinkProviders.values()) {
 			this._linkProvidersDisposables.push(this._xterm.registerLinkProvider(p));
 		}
 	}
 
-	registerExternalLinkProvider(provideLinks: OmitFirstArg<ITerminalExternalLinkProvider['provideLinks']>): void {
-		// Avoid any leaks in case this is already disposed
-		if (this.isDisposed) {
-			return;
-		}
-		// Clear and re-register the standard link providers so they are a lower priority than the new one
-		this._clearLinkProviders();
-		const detectorId = `extension-${this._externalLinkProviders.length}`;
-		const wrappedLinkProvider = this._setupLinkDetector(detectorId, new TerminalExternalLinkDetector(detectorId, this._xterm, provideLinks), true);
-		const newLinkProvider = this._xterm.registerLinkProvider(wrappedLinkProvider);
-		this._externalLinkProviders.push(newLinkProvider);
-		this._registerStandardLinkProviders();
-	}
-
 	protected _isLinkActivationModifierDown(event: MouseEvent): boolean {
 		const editorConf = this._configurationService.getValue<{ multiCursorModifier: 'ctrlCmd' | 'alt' }>('editor');
 		if (editorConf.multiCursorModifier === 'ctrlCmd') {

src/vs/workbench/contrib/terminalContrib/links/test/browser/terminalLinkManager.test.ts

@@ -101,9 +101,9 @@ suite('TerminalLinkManager', () => {
 
 	suite('registerExternalLinkProvider', () => {
 		test('should not leak disposables if the link manager is already disposed', () => {
-			linkManager.registerExternalLinkProvider(async () => undefined);
+			linkManager.externalProvideLinksCb = async () => undefined;
 			linkManager.dispose();
-			linkManager.registerExternalLinkProvider(async () => undefined);
+			linkManager.externalProvideLinksCb = async () => undefined;
 		});
 	});