From 49f3723a3a1a1dcd0cf71b90f8f919cf7da2bd95 Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Sun, 8 Mar 2020 08:26:28 +0100 Subject: [PATCH] quick access - allow provide to return disposable and cancel token properly --- .../quickinput/browser/helpQuickAccess.ts | 8 ++- .../quickinput/browser/quickAccess.ts | 40 +++++++++----- .../platform/quickinput/common/quickAccess.ts | 12 +++-- .../browser/parts/quickopen/quickopen.ts | 12 ++--- .../test/browser/quickAccess.test.ts | 52 ++++++++++++++----- 5 files changed, 83 insertions(+), 41 deletions(-) diff --git a/src/vs/platform/quickinput/browser/helpQuickAccess.ts b/src/vs/platform/quickinput/browser/helpQuickAccess.ts index 0d4601edb17..d4a66d3556b 100644 --- a/src/vs/platform/quickinput/browser/helpQuickAccess.ts +++ b/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, token: CancellationToken): void { + provide(picker: IQuickPick, 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[] } { diff --git a/src/vs/platform/quickinput/browser/quickAccess.ts b/src/vs/platform/quickinput/browser/quickAccess.ts index f58d550fa5c..66f3ba390e8 100644 --- a/src/vs/platform/quickinput/browser/quickAccess.ts +++ b/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 */] { diff --git a/src/vs/platform/quickinput/common/quickAccess.ts b/src/vs/platform/quickinput/common/quickAccess.ts index ac45cc1c104..de79e148ff3 100644 --- a/src/vs/platform/quickinput/common/quickAccess.ts +++ b/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, token: CancellationToken): void; + provide(picker: IQuickPick, token: CancellationToken): IDisposable; } export interface IQuickAccessProviderHelp { diff --git a/src/vs/workbench/browser/parts/quickopen/quickopen.ts b/src/vs/workbench/browser/parts/quickopen/quickopen.ts index 6317058023a..d13c217727d 100644 --- a/src/vs/workbench/browser/parts/quickopen/quickopen.ts +++ b/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(WorkbenchExtensions.Workbench).registerWorkbenchContribution(LegacyQuickInputQuickOpenController, LifecyclePhase.Ready); class SampleQuickAccessProvider implements IQuickAccessProvider { - provide(picker: IQuickPick, token: CancellationToken): void { + provide(picker: IQuickPick, token: CancellationToken): IDisposable { picker.items = [ { label: 'Hello World' }, { label: 'Lorem Ipsum' }, { label: 'Something Else' } ]; - picker.show(); + return Disposable.None; } } -const quickAccessRegistry = Registry.as(QuickinputExtensions.Quickaccess); -['', '>', '@', ':', 'edt', 'edt active', 'edt mru', 'view'].forEach(prefix => { +const quickAccessRegistry = Registry.as(QuickInputExtensions.Quickaccess); +[''/*, '>', '@', ':', 'edt', 'edt active', 'edt mru', 'view'*/].forEach(prefix => { const provider = { ctor: SampleQuickAccessProvider, prefix, diff --git a/src/vs/workbench/test/browser/quickAccess.test.ts b/src/vs/workbench/test/browser/quickAccess.test.ts index da960ecb035..94ffc5ac658 100644 --- a/src/vs/workbench/test/browser/quickAccess.test.ts +++ b/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, token: CancellationToken) { + provide(picker: IQuickPick, 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, token: CancellationToken) { + provide(picker: IQuickPick, 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, token: CancellationToken) { + provide(picker: IQuickPick, 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, token: CancellationToken) { + provide(picker: IQuickPick, 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; }); -- GitLab