From e356139a1095b92e0a89be34553aeedb24aa78c0 Mon Sep 17 00:00:00 2001
From: Alex Dima <alexdima@microsoft.com>
Date: Thu, 17 Feb 2022 17:40:04 +0100
Subject: [PATCH] Fixes #142155: Avoid flickering by handling composition
 typing synchronously. Increasing the textarea width while handling the
 `compositionupdate` event avoids flickers in the composition suggestion list.

---
 .../browser/controller/textAreaHandler.ts     |  7 +--
 .../commands/common/commandService.ts         | 51 +++++++++++--------
 .../test/common/commandService.test.ts        | 33 ++++++++++++
 .../common/abstractExtensionService.ts        | 11 ++++
 .../extensions/common/extensionHostManager.ts | 19 ++++++-
 .../services/extensions/common/extensions.ts  |  8 +++
 6 files changed, 105 insertions(+), 24 deletions(-)

diff --git a/src/vs/editor/browser/controller/textAreaHandler.ts b/src/vs/editor/browser/controller/textAreaHandler.ts
index 3738b00337f..782f1428d68 100644
--- a/src/vs/editor/browser/controller/textAreaHandler.ts
+++ b/src/vs/editor/browser/controller/textAreaHandler.ts
@@ -299,7 +299,8 @@ export class TextAreaHandler extends ViewPart {
 			// the selection.
 			//
 			// However, the text on the current line needs to be made visible because
-			// some IME methods allow to glyphs on the current line (by pressing arrow keys).
+			// some IME methods allow to move to other glyphs on the current line
+			// (by pressing arrow keys).
 			//
 			// (1) The textarea might contain only some parts of the current line,
 			// like the word before the selection. Also, the content inside the textarea
@@ -308,7 +309,7 @@ export class TextAreaHandler extends ViewPart {
 			//
 			// (2) Also, we should not make \t characters visible, because their rendering
 			// inside the <textarea> will not align nicely with our rendering. We therefore
-			// can hide some of the leading text on the current line.
+			// will hide (if necessary) some of the leading text on the current line.
 
 			const ta = this.textArea.domNode;
 			const modelSelection = this._modelSelections[0];
@@ -346,7 +347,7 @@ export class TextAreaHandler extends ViewPart {
 				return { distanceToModelLineEnd };
 			})();
 
-			// Scroll to reveal the location in the editor
+			// Scroll to reveal the location in the editor where composition occurs
 			this._context.viewModel.revealRange(
 				'keyboard',
 				true,
diff --git a/src/vs/workbench/services/commands/common/commandService.ts b/src/vs/workbench/services/commands/common/commandService.ts
index 0feccbe127b..85faaa2fb3d 100644
--- a/src/vs/workbench/services/commands/common/commandService.ts
+++ b/src/vs/workbench/services/commands/common/commandService.ts
@@ -46,32 +46,43 @@ export class CommandService extends Disposable implements ICommandService {
 		return this._starActivation;
 	}
 
-	executeCommand<T>(id: string, ...args: any[]): Promise<T> {
+	async executeCommand<T>(id: string, ...args: any[]): Promise<T> {
 		this._logService.trace('CommandService#executeCommand', id);
 
-		// we always send an activation event, but
-		// we don't wait for it when the extension
-		// host didn't yet start and the command is already registered
-
-		const activation: Promise<any> = this._extensionService.activateByEvent(`onCommand:${id}`);
+		const activationEvent = `onCommand:${id}`;
 		const commandIsRegistered = !!CommandsRegistry.getCommand(id);
 
-		if (!this._extensionHostIsReady && commandIsRegistered) {
-			return this._tryExecuteCommand(id, args);
-		} else {
-			let waitFor = activation;
-			if (!commandIsRegistered) {
-				waitFor = Promise.all([
-					activation,
-					Promise.race<any>([
-						// race * activation against command registration
-						this._activateStar(),
-						Event.toPromise(Event.filter(CommandsRegistry.onDidRegisterCommand, e => e === id))
-					]),
-				]);
+		if (commandIsRegistered) {
+
+			// if the activation event has already resolved (i.e. subsequent call),
+			// we will execute the registered command immediately
+			if (this._extensionService.activationEventIsDone(activationEvent)) {
+				return this._tryExecuteCommand(id, args);
 			}
-			return waitFor.then(_ => this._tryExecuteCommand(id, args));
+
+			// if the extension host didn't start yet, we will execute the registered
+			// command immediately and send an activation event, but not wait for it
+			if (!this._extensionHostIsReady) {
+				this._extensionService.activateByEvent(activationEvent); // intentionally not awaited
+				return this._tryExecuteCommand(id, args);
+			}
+
+			// we will wait for a simple activation event (e.g. in case an extension wants to overwrite it)
+			await this._extensionService.activateByEvent(activationEvent);
+			return this._tryExecuteCommand(id, args);
 		}
+
+		// finally, if the command is not registered we will send a simple activation event
+		// as well as a * activation event raced against registration and against 30s
+		await Promise.all([
+			this._extensionService.activateByEvent(activationEvent),
+			Promise.race<any>([
+				// race * activation against command registration
+				this._activateStar(),
+				Event.toPromise(Event.filter(CommandsRegistry.onDidRegisterCommand, e => e === id))
+			]),
+		]);
+		return this._tryExecuteCommand(id, args);
 	}
 
 	private _tryExecuteCommand(id: string, args: any[]): Promise<any> {
diff --git a/src/vs/workbench/services/commands/test/common/commandService.test.ts b/src/vs/workbench/services/commands/test/common/commandService.test.ts
index e4e13a410de..23110f4f75c 100644
--- a/src/vs/workbench/services/commands/test/common/commandService.test.ts
+++ b/src/vs/workbench/services/commands/test/common/commandService.test.ts
@@ -175,4 +175,37 @@ suite('CommandService', function () {
 			disposables.dispose();
 		});
 	});
+
+	test('issue #142155: execute commands synchronously if possible', async () => {
+		const actualOrder: string[] = [];
+
+		const disposables = new DisposableStore();
+		disposables.add(CommandsRegistry.registerCommand(`bizBaz`, () => {
+			actualOrder.push('executing command');
+		}));
+		const extensionService = new class extends NullExtensionService {
+			override activationEventIsDone(_activationEvent: string): boolean {
+				return true;
+			}
+		};
+		const service = new CommandService(new InstantiationService(), extensionService, new NullLogService());
+
+		await extensionService.whenInstalledExtensionsRegistered();
+
+		try {
+			actualOrder.push(`before call`);
+			const promise = service.executeCommand('bizBaz');
+			actualOrder.push(`after call`);
+			await promise;
+			actualOrder.push(`resolved`);
+			assert.deepStrictEqual(actualOrder, [
+				'before call',
+				'executing command',
+				'after call',
+				'resolved'
+			]);
+		} finally {
+			disposables.dispose();
+		}
+	});
 });
diff --git a/src/vs/workbench/services/extensions/common/abstractExtensionService.ts b/src/vs/workbench/services/extensions/common/abstractExtensionService.ts
index 7ec13d3ea79..6977fbe0006 100644
--- a/src/vs/workbench/services/extensions/common/abstractExtensionService.ts
+++ b/src/vs/workbench/services/extensions/common/abstractExtensionService.ts
@@ -719,6 +719,17 @@ export abstract class AbstractExtensionService extends Disposable implements IEx
 		return result;
 	}
 
+	public activationEventIsDone(activationEvent: string): boolean {
+		if (!this._installedExtensionsReady.isOpen()) {
+			return false;
+		}
+		if (!this._registry.containsActivationEvent(activationEvent)) {
+			// There is no extension that is interested in this activation event
+			return true;
+		}
+		return this._extensionHostManagers.every(manager => manager.activationEventIsDone(activationEvent));
+	}
+
 	public whenInstalledExtensionsRegistered(): Promise<boolean> {
 		return this._installedExtensionsReady.wait();
 	}
diff --git a/src/vs/workbench/services/extensions/common/extensionHostManager.ts b/src/vs/workbench/services/extensions/common/extensionHostManager.ts
index 07534f837cd..72400588d16 100644
--- a/src/vs/workbench/services/extensions/common/extensionHostManager.ts
+++ b/src/vs/workbench/services/extensions/common/extensionHostManager.ts
@@ -40,6 +40,7 @@ export interface IExtensionHostManager {
 	deltaExtensions(toAdd: IExtensionDescription[], toRemove: ExtensionIdentifier[]): Promise<void>;
 	activate(extension: ExtensionIdentifier, reason: ExtensionActivationReason): Promise<boolean>;
 	activateByEvent(activationEvent: string, activationKind: ActivationKind): Promise<void>;
+	activationEventIsDone(activationEvent: string): boolean;
 	getInspectPort(tryEnableInspector: boolean): Promise<number>;
 	resolveAuthority(remoteAuthority: string): Promise<ResolverResult>;
 	getCanonicalURI(remoteAuthority: string, uri: URI): Promise<URI>;
@@ -86,6 +87,7 @@ class ExtensionHostManager extends Disposable implements IExtensionHostManager {
 	 * A map of already requested activation events to speed things up if the same activation event is triggered multiple times.
 	 */
 	private readonly _cachedActivationEvents: Map<string, Promise<void>>;
+	private readonly _resolvedActivationEvents: Set<string>;
 	private _rpcProtocol: RPCProtocol | null;
 	private readonly _customers: IDisposable[];
 	private readonly _extensionHost: IExtensionHost;
@@ -104,6 +106,7 @@ class ExtensionHostManager extends Disposable implements IExtensionHostManager {
 	) {
 		super();
 		this._cachedActivationEvents = new Map<string, Promise<void>>();
+		this._resolvedActivationEvents = new Set<string>();
 		this._rpcProtocol = null;
 		this._customers = [];
 
@@ -325,6 +328,10 @@ class ExtensionHostManager extends Disposable implements IExtensionHostManager {
 		return this._cachedActivationEvents.get(activationEvent)!;
 	}
 
+	public activationEventIsDone(activationEvent: string): boolean {
+		return this._resolvedActivationEvents.has(activationEvent);
+	}
+
 	private async _activateByEvent(activationEvent: string, activationKind: ActivationKind): Promise<void> {
 		if (!this._proxy) {
 			return;
@@ -335,7 +342,8 @@ class ExtensionHostManager extends Disposable implements IExtensionHostManager {
 			// i.e. the extension host could not be started
 			return;
 		}
-		return proxy.activateByEvent(activationEvent, activationKind);
+		await proxy.activateByEvent(activationEvent, activationKind);
+		this._resolvedActivationEvents.add(activationEvent);
 	}
 
 	public async getInspectPort(tryEnableInspector: boolean): Promise<number> {
@@ -519,6 +527,15 @@ class LazyStartExtensionHostManager extends Disposable implements IExtensionHost
 			return this._actual.activateByEvent(activationEvent, activationKind);
 		}
 	}
+	public activationEventIsDone(activationEvent: string): boolean {
+		if (!this._startCalled.isOpen()) {
+			return false;
+		}
+		if (this._actual) {
+			return this._actual.activationEventIsDone(activationEvent);
+		}
+		return true;
+	}
 	public async getInspectPort(tryEnableInspector: boolean): Promise<number> {
 		await this._startCalled.wait();
 		if (this._actual) {
diff --git a/src/vs/workbench/services/extensions/common/extensions.ts b/src/vs/workbench/services/extensions/common/extensions.ts
index 7992aecd91c..6398b447a1a 100644
--- a/src/vs/workbench/services/extensions/common/extensions.ts
+++ b/src/vs/workbench/services/extensions/common/extensions.ts
@@ -241,6 +241,13 @@ export interface IExtensionService {
 	 */
 	activateByEvent(activationEvent: string, activationKind?: ActivationKind): Promise<void>;
 
+	/**
+	 * Determine if `activateByEvent(activationEvent)` has resolved already.
+	 *
+	 * i.e. the activation event is finished and all interested extensions are already active.
+	 */
+	activationEventIsDone(activationEvent: string): boolean;
+
 	/**
 	 * An promise that resolves when the installed extensions are registered after
 	 * their extension points got handled.
@@ -357,6 +364,7 @@ export class NullExtensionService implements IExtensionService {
 	onWillActivateByEvent: Event<IWillActivateEvent> = Event.None;
 	onDidChangeResponsiveChange: Event<IResponsiveStateChangeEvent> = Event.None;
 	activateByEvent(_activationEvent: string): Promise<void> { return Promise.resolve(undefined); }
+	activationEventIsDone(_activationEvent: string): boolean { return false; }
 	whenInstalledExtensionsRegistered(): Promise<boolean> { return Promise.resolve(true); }
 	getExtensions(): Promise<IExtensionDescription[]> { return Promise.resolve([]); }
 	getExtension() { return Promise.resolve(undefined); }