quick access - allow provide to return disposable and cancel token properly

src/vs/platform/quickinput/browser/helpQuickAccess.ts

@@ -8,8 +8,7 @@ import { IQuickAccessProvider, IQuickAccessRegistry, Extensions } from 'vs/platf
 import { Registry } from 'vs/platform/registry/common/platform';
 import { CancellationToken } from 'vs/base/common/cancellation';
 import { localize } from 'vs/nls';
-import { DisposableStore } from 'vs/base/common/lifecycle';
-import { once } from 'vs/base/common/functional';
+import { DisposableStore, IDisposable } from 'vs/base/common/lifecycle';
 
 interface IHelpQuickAccessPickItem extends IQuickPickItem {
 	prefix: string;
@@ -21,9 +20,8 @@ export class HelpQuickAccessProvider implements IQuickAccessProvider {
 
 	constructor(@IQuickInputService private readonly quickInputService: IQuickInputService) { }
 
-	provide(picker: IQuickPick<IHelpQuickAccessPickItem>, token: CancellationToken): void {
+	provide(picker: IQuickPick<IHelpQuickAccessPickItem>, token: CancellationToken): IDisposable {
 		const disposables = new DisposableStore();
-		once(token.onCancellationRequested)(() => disposables.dispose());
 
 		// Open a picker with the selected value if picked
 		disposables.add(picker.onDidAccept(() => {
@@ -50,7 +48,7 @@ export class HelpQuickAccessProvider implements IQuickAccessProvider {
 				...editorProviders
 			];
 
-		picker.show();
+		return disposables;
 	}
 
 	private getQuickAccessProviders(): { editorProviders: IHelpQuickAccessPickItem[], globalProviders: IHelpQuickAccessPickItem[] } {

src/vs/platform/quickinput/browser/quickAccess.ts

@@ -10,7 +10,6 @@ import { Registry } from 'vs/platform/registry/common/platform';
 import { CancellationTokenSource } from 'vs/base/common/cancellation';
 import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
 import { once } from 'vs/base/common/functional';
-import { Event } from 'vs/base/common/event';
 
 export class QuickAccessController extends Disposable implements IQuickAccessController {
 
@@ -27,23 +26,40 @@ export class QuickAccessController extends Disposable implements IQuickAccessCon
 	}
 
 	show(value = ''): void {
+		const disposables = new DisposableStore();
 
 		// Hide any previous picker if any
-		this.lastActivePicker?.dispose();
+		this.lastActivePicker?.hide();
 
 		// Find provider for the value to show
 		const [provider, prefix] = this.getOrInstantiateProvider(value);
 
 		// Create a picker for the provider to use with the initial value
 		// and adjust the filtering to exclude the prefix from filtering
-		const picker = this.lastActivePicker = this.quickInputService.createQuickPick();
+		const picker = disposables.add(this.quickInputService.createQuickPick());
 		picker.value = value;
 		picker.valueSelection = [value.length, value.length];
 		picker.filterValue = (value: string) => value.substring(prefix.length);
 
-		// Cleanup when picker hides or gets disposed
-		const disposables = new DisposableStore();
-		once(Event.any(picker.onDidHide, picker.onDispose))(() => disposables.dispose());
+		// Remember as last active picker and clean up once picker get's disposed
+		this.lastActivePicker = picker;
+		disposables.add(toDisposable(() => {
+			if (picker === this.lastActivePicker) {
+				this.lastActivePicker = undefined;
+			}
+		}));
+
+		// Create a cancellation token source that is valid as long as the
+		// picker has not been closed without picking an item
+		const cts = disposables.add(new CancellationTokenSource());
+		once(picker.onDidHide)(() => {
+			if (picker.selectedItems.length === 0) {
+				cts.cancel();
+			}
+
+			// Start to dispose once picker hides
+			disposables.dispose();
+		});
 
 		// Whenever the value changes, check if the provider has
 		// changed and if so - re-create the picker from the beginning
@@ -54,13 +70,13 @@ export class QuickAccessController extends Disposable implements IQuickAccessCon
 			}
 		}));
 
-		// Create a cancellation token source that is valid
-		// as long as the picker has not been closed
-		const cts = new CancellationTokenSource();
-		disposables.add(toDisposable(() => cts.dispose(true)));
+		// Ask provider to fill the picker as needed
+		disposables.add(provider.provide(picker, cts.token));
 
-		// Finally ask provider to fill the picker as needed
-		provider.provide(picker, cts.token);
+		// Finally, show the picker. This is important because a provider
+		// may not call this and then our disposables would leak that rely
+		// on the onDidHide event.
+		picker.show();
 	}
 
 	private getOrInstantiateProvider(value: string): [IQuickAccessProvider, string /* prefix */] {

src/vs/platform/quickinput/common/quickAccess.ts

@@ -24,12 +24,16 @@ export interface IQuickAccessProvider {
 	/**
 	 * Called whenever a prefix was typed into quick pick that matches the provider.
 	 *
-	 * @param picker the picker to use for showing provider results.
+	 * @param picker the picker to use for showing provider results. The picker is
+	 * automatically shown after the method returns, no need to call `show()`.
 	 * @param token providers have to check the cancellation token everytime after
-	 * a long running operation because it could be that the picker has been closed
-	 * or changed meanwhile.
+	 * a long running operation or from event handlers because it could be that the
+	 * picker has been closed or changed meanwhile. The token can be used to find out
+	 * that the picker was closed without picking an entry (e.g. was canceled by the user).
+	 * @return a disposable that will automatically be disposed when the picker
+	 * closes or is replaced by another picker.
 	 */
-	provide(picker: IQuickPick<IQuickPickItem>, token: CancellationToken): void;
+	provide(picker: IQuickPick<IQuickPickItem>, token: CancellationToken): IDisposable;
 }
 
 export interface IQuickAccessProviderHelp {

src/vs/workbench/browser/parts/quickopen/quickopen.ts

@@ -11,11 +11,11 @@ import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
 import { ContextKeyExpr, RawContextKey, IContextKeyService } from 'vs/platform/contextkey/common/contextkey';
 import { ICommandHandler, CommandsRegistry } from 'vs/platform/commands/common/commands';
 import { ServicesAccessor } from 'vs/platform/instantiation/common/instantiation';
-import { Disposable } from 'vs/base/common/lifecycle';
+import { Disposable, IDisposable } from 'vs/base/common/lifecycle';
 import { IWorkbenchContributionsRegistry, Extensions as WorkbenchExtensions } from 'vs/workbench/common/contributions';
 import { LifecyclePhase } from 'vs/platform/lifecycle/common/lifecycle';
 import { Registry } from 'vs/platform/registry/common/platform';
-import { IQuickAccessRegistry, Extensions as QuickinputExtensions, IQuickAccessProvider } from 'vs/platform/quickinput/common/quickAccess';
+import { IQuickAccessProvider, IQuickAccessRegistry, Extensions as QuickInputExtensions } from 'vs/platform/quickinput/common/quickAccess';
 import { CancellationToken } from 'vs/base/common/cancellation';
 
 const inQuickOpenKey = 'inQuickOpen';
@@ -209,19 +209,19 @@ export class LegacyQuickInputQuickOpenController extends Disposable {
 Registry.as<IWorkbenchContributionsRegistry>(WorkbenchExtensions.Workbench).registerWorkbenchContribution(LegacyQuickInputQuickOpenController, LifecyclePhase.Ready);
 
 class SampleQuickAccessProvider implements IQuickAccessProvider {
-	provide(picker: IQuickPick<IQuickPickItem>, token: CancellationToken): void {
+	provide(picker: IQuickPick<IQuickPickItem>, token: CancellationToken): IDisposable {
 		picker.items = [
 			{ label: 'Hello World' },
 			{ label: 'Lorem Ipsum' },
 			{ label: 'Something Else' }
 		];
 
-		picker.show();
+		return Disposable.None;
 	}
 }
 
-const quickAccessRegistry = Registry.as<IQuickAccessRegistry>(QuickinputExtensions.Quickaccess);
-['', '>', '@', ':', 'edt', 'edt active', 'edt mru', 'view'].forEach(prefix => {
+const quickAccessRegistry = Registry.as<IQuickAccessRegistry>(QuickInputExtensions.Quickaccess);
+[''/*, '>', '@', ':', 'edt', 'edt active', 'edt mru', 'view'*/].forEach(prefix => {
 	const provider = {
 		ctor: SampleQuickAccessProvider,
 		prefix,

src/vs/workbench/test/browser/quickAccess.test.ts

@@ -10,7 +10,7 @@ import { IQuickPick, IQuickPickItem, IQuickInputService } from 'vs/platform/quic
 import { CancellationToken } from 'vs/base/common/cancellation';
 import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
 import { TestServiceAccessor, workbenchInstantiationService } from 'vs/workbench/test/browser/workbenchTestServices';
-import { DisposableStore } from 'vs/base/common/lifecycle';
+import { DisposableStore, toDisposable, IDisposable } from 'vs/base/common/lifecycle';
 import { timeout } from 'vs/base/common/async';
 
 suite('QuickAccess', () => {
@@ -20,61 +20,66 @@ suite('QuickAccess', () => {
 
 	let provider1Called = false;
 	let provider1Canceled = false;
+	let provider1Disposed = false;
 
 	let provider2Called = false;
 	let provider2Canceled = false;
+	let provider2Disposed = false;
 
 	let provider3Called = false;
 	let provider3Canceled = false;
+	let provider3Disposed = false;
 
 	let provider4Called = false;
 	let provider4Canceled = false;
+	let provider4Disposed = false;
 
 	class TestProvider1 implements IQuickAccessProvider {
-		provide(picker: IQuickPick<IQuickPickItem>, token: CancellationToken) {
+		provide(picker: IQuickPick<IQuickPickItem>, token: CancellationToken): IDisposable {
 			assert.ok(picker);
 			provider1Called = true;
 			token.onCancellationRequested(() => provider1Canceled = true);
 
-			picker.show();
+			return toDisposable(() => provider1Disposed = true);
 		}
 	}
 
 	class TestProvider2 implements IQuickAccessProvider {
-		provide(picker: IQuickPick<IQuickPickItem>, token: CancellationToken) {
+		provide(picker: IQuickPick<IQuickPickItem>, token: CancellationToken): IDisposable {
 			assert.ok(picker);
 			provider2Called = true;
 			token.onCancellationRequested(() => provider2Canceled = true);
 
-			// Not calling picker show explicitly to test bad provider
-			// picker.show();
+			return toDisposable(() => provider2Disposed = true);
 		}
 	}
 
 	class TestProvider3 implements IQuickAccessProvider {
 
-		constructor(@IQuickInputService private readonly quickInputService: IQuickInputService) { }
+		constructor(@IQuickInputService private readonly quickInputService: IQuickInputService, disposables: DisposableStore) { }
 
-		async provide(picker: IQuickPick<IQuickPickItem>, token: CancellationToken) {
+		provide(picker: IQuickPick<IQuickPickItem>, token: CancellationToken): IDisposable {
 			assert.ok(picker);
 			provider3Called = true;
 			token.onCancellationRequested(() => provider3Canceled = true);
 
-			picker.show();
-
 			// bring up provider #4
-			await timeout(0);
-			this.quickInputService.quickAccess.show(providerDescriptor4.prefix);
+			setTimeout(() => this.quickInputService.quickAccess.show(providerDescriptor4.prefix));
+
+			return toDisposable(() => provider3Disposed = true);
 		}
 	}
 
 	class TestProvider4 implements IQuickAccessProvider {
-		provide(picker: IQuickPick<IQuickPickItem>, token: CancellationToken) {
+		provide(picker: IQuickPick<IQuickPickItem>, token: CancellationToken): IDisposable {
 			assert.ok(picker);
 			provider4Called = true;
 			token.onCancellationRequested(() => provider4Canceled = true);
 
-			picker.show();
+			// hide without picking
+			setTimeout(() => picker.hide());
+
+			return toDisposable(() => provider4Disposed = true);
 		}
 	}
 
@@ -124,6 +129,10 @@ suite('QuickAccess', () => {
 		assert.equal(provider2Canceled, false);
 		assert.equal(provider3Canceled, false);
 		assert.equal(provider4Canceled, false);
+		assert.equal(provider1Disposed, false);
+		assert.equal(provider2Disposed, false);
+		assert.equal(provider3Disposed, false);
+		assert.equal(provider4Disposed, false);
 		provider1Called = false;
 
 		accessor.quickInputService.quickAccess.show('test something');
@@ -135,8 +144,13 @@ suite('QuickAccess', () => {
 		assert.equal(provider2Canceled, false);
 		assert.equal(provider3Canceled, false);
 		assert.equal(provider4Canceled, false);
+		assert.equal(provider1Disposed, true);
+		assert.equal(provider2Disposed, false);
+		assert.equal(provider3Disposed, false);
+		assert.equal(provider4Disposed, false);
 		provider2Called = false;
 		provider1Canceled = false;
+		provider1Disposed = false;
 
 		accessor.quickInputService.quickAccess.show('usedefault');
 		assert.equal(provider1Called, false);
@@ -147,12 +161,22 @@ suite('QuickAccess', () => {
 		assert.equal(provider2Canceled, true);
 		assert.equal(provider3Canceled, false);
 		assert.equal(provider4Canceled, false);
+		assert.equal(provider1Disposed, false);
+		assert.equal(provider2Disposed, true);
+		assert.equal(provider3Disposed, false);
+		assert.equal(provider4Disposed, false);
 
 		await timeout(1);
 
 		assert.equal(provider3Canceled, true);
+		assert.equal(provider3Disposed, true);
 		assert.equal(provider4Called, true);
 
+		await timeout(1);
+
+		assert.equal(provider4Canceled, true);
+		assert.equal(provider4Disposed, true);
+
 		disposables.dispose();
 		registry.defaultProvider = defaultProvider;
 	});