From 26467162618d45ae5f0f24c004829008f22ed23a Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Mon, 13 Mar 2023 14:51:32 +0100 Subject: [PATCH 1/3] Remove args from `ProgressTask` This removes the `args` from the `ProgressTask` passed to `withProgress`. The `args` is only used by the `commandRunnerWithProgress` and can easily be replaced by an anonymous function that passes the `args` instead. This will simplify the `ProgressTask` interface and make it easier to use. --- extensions/ql-vscode/src/commandRunner.ts | 42 ++++++++++++++--------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/extensions/ql-vscode/src/commandRunner.ts b/extensions/ql-vscode/src/commandRunner.ts index 688d6fdb2..46680615b 100644 --- a/extensions/ql-vscode/src/commandRunner.ts +++ b/extensions/ql-vscode/src/commandRunner.ts @@ -42,22 +42,35 @@ export interface ProgressUpdate { export type ProgressCallback = (p: ProgressUpdate) => void; +/** + * A task that reports progress. + * + * @param progress a progress handler function. Call this + * function with a `ProgressUpdate` instance in order to + * denote some progress being achieved on this task. + * @param token a cancellation token + */ +export type ProgressTask = ( + progress: ProgressCallback, + token: CancellationToken, +) => Thenable; + /** * A task that handles command invocations from `commandRunner` * and includes a progress monitor. * * * Arguments passed to the command handler are passed along, - * untouched to this `ProgressTask` instance. + * untouched to this `ProgressTaskWithArgs` instance. * * @param progress a progress handler function. Call this * function with a `ProgressUpdate` instance in order to * denote some progress being achieved on this task. - * @param token a cencellation token + * @param token a cancellation token * @param args arguments passed to this task passed on from * `commands.registerCommand`. */ -export type ProgressTask = ( +export type ProgressTaskWithArgs = ( progress: ProgressCallback, token: CancellationToken, ...args: any[] @@ -92,20 +105,15 @@ type NoProgressTask = (...args: any[]) => Promise; export function withProgress( options: ProgressOptions, task: ProgressTask, - ...args: any[] ): Thenable { let progressAchieved = 0; return Window.withProgress(options, (progress, token) => { - return task( - (p) => { - const { message, step, maxStep } = p; - const increment = (100 * (step - progressAchieved)) / maxStep; - progressAchieved = step; - progress.report({ message, increment }); - }, - token, - ...args, - ); + return task((p) => { + const { message, step, maxStep } = p; + const increment = (100 * (step - progressAchieved)) / maxStep; + progressAchieved = step; + progress.report({ message, increment }); + }, token); }); } @@ -177,7 +185,7 @@ export function commandRunner( */ export function commandRunnerWithProgress( commandId: string, - task: ProgressTask, + task: ProgressTaskWithArgs, progressOptions: Partial, outputLogger = extLogger, ): Disposable { @@ -189,7 +197,9 @@ export function commandRunnerWithProgress( ...progressOptions, }; - return withProgress(progressOptionsWithDefaults, task, ...args); + return withProgress(progressOptionsWithDefaults, (progress, token) => + task(progress, token, ...args), + ); }, outputLogger, ); From 4969a085316d68a9d0e61a5d36e770021eb77f10 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Mon, 13 Mar 2023 15:13:55 +0100 Subject: [PATCH 2/3] Make progress options optional This will make the progress options passed to `withProgress` optional by moving it to be the second argument and setting a default value for the `location`. This will make it much easier to use from a variety of commands. --- extensions/ql-vscode/src/commandRunner.ts | 48 ++++++++++++------- .../src/contextual/templateProvider.ts | 20 ++++---- extensions/ql-vscode/src/extension.ts | 21 ++++---- extensions/ql-vscode/src/local-databases.ts | 6 +-- 4 files changed, 53 insertions(+), 42 deletions(-) diff --git a/extensions/ql-vscode/src/commandRunner.ts b/extensions/ql-vscode/src/commandRunner.ts index 46680615b..6f316be2b 100644 --- a/extensions/ql-vscode/src/commandRunner.ts +++ b/extensions/ql-vscode/src/commandRunner.ts @@ -1,6 +1,6 @@ import { CancellationToken, - ProgressOptions, + ProgressOptions as VSCodeProgressOptions, window as Window, commands, Disposable, @@ -42,6 +42,11 @@ export interface ProgressUpdate { export type ProgressCallback = (p: ProgressUpdate) => void; +// Make certain properties within a type optional +type Optional = Pick, K> & Omit; + +export type ProgressOptions = Optional; + /** * A task that reports progress. * @@ -103,18 +108,29 @@ type NoProgressTask = (...args: any[]) => Promise; * request). */ export function withProgress( - options: ProgressOptions, task: ProgressTask, + { + location = ProgressLocation.Notification, + title, + cancellable, + }: ProgressOptions = {}, ): Thenable { let progressAchieved = 0; - return Window.withProgress(options, (progress, token) => { - return task((p) => { - const { message, step, maxStep } = p; - const increment = (100 * (step - progressAchieved)) / maxStep; - progressAchieved = step; - progress.report({ message, increment }); - }, token); - }); + return Window.withProgress( + { + location, + title, + cancellable, + }, + (progress, token) => { + return task((p) => { + const { message, step, maxStep } = p; + const increment = (100 * (step - progressAchieved)) / maxStep; + progressAchieved = step; + progress.report({ message, increment }); + }, token); + }, + ); } /** @@ -186,19 +202,15 @@ export function commandRunner( export function commandRunnerWithProgress( commandId: string, task: ProgressTaskWithArgs, - progressOptions: Partial, + progressOptions: ProgressOptions, outputLogger = extLogger, ): Disposable { return commandRunner( commandId, async (...args: any[]) => { - const progressOptionsWithDefaults = { - location: ProgressLocation.Notification, - ...progressOptions, - }; - - return withProgress(progressOptionsWithDefaults, (progress, token) => - task(progress, token, ...args), + return withProgress( + (progress, token) => task(progress, token, ...args), + progressOptions, ); }, outputLogger, diff --git a/extensions/ql-vscode/src/contextual/templateProvider.ts b/extensions/ql-vscode/src/contextual/templateProvider.ts index 448d36732..1e2865d99 100644 --- a/extensions/ql-vscode/src/contextual/templateProvider.ts +++ b/extensions/ql-vscode/src/contextual/templateProvider.ts @@ -73,11 +73,6 @@ export class TemplateQueryDefinitionProvider implements DefinitionProvider { private async getDefinitions(uriString: string): Promise { return withProgress( - { - location: ProgressLocation.Notification, - cancellable: true, - title: "Finding definitions", - }, async (progress, token) => { return getLocationsForUriString( this.cli, @@ -91,6 +86,11 @@ export class TemplateQueryDefinitionProvider implements DefinitionProvider { (src, _dest) => src === uriString, ); }, + { + location: ProgressLocation.Notification, + cancellable: true, + title: "Finding definitions", + }, ); } } @@ -136,11 +136,6 @@ export class TemplateQueryReferenceProvider implements ReferenceProvider { private async getReferences(uriString: string): Promise { return withProgress( - { - location: ProgressLocation.Notification, - cancellable: true, - title: "Finding references", - }, async (progress, token) => { return getLocationsForUriString( this.cli, @@ -154,6 +149,11 @@ export class TemplateQueryReferenceProvider implements ReferenceProvider { (src, _dest) => src === uriString, ); }, + { + location: ProgressLocation.Notification, + cancellable: true, + title: "Finding references", + }, ); } } diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index e1ca5caef..eb9420b82 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -9,7 +9,6 @@ import { extensions, languages, ProgressLocation, - ProgressOptions, QuickPickItem, Range, Uri, @@ -322,16 +321,16 @@ export async function activate( await commands.executeCommand("workbench.action.reloadWindow"); } } else { - const progressOptions: ProgressOptions = { - title: progressTitle, - location: ProgressLocation.Notification, - }; - - await withProgress(progressOptions, (progress) => - distributionManager.installExtensionManagedDistributionRelease( - result.updatedRelease, - progress, - ), + await withProgress( + (progress) => + distributionManager.installExtensionManagedDistributionRelease( + result.updatedRelease, + progress, + ), + { + title: progressTitle, + location: ProgressLocation.Notification, + }, ); await ctx.globalState.update(shouldUpdateOnNextActivationKey, false); diff --git a/extensions/ql-vscode/src/local-databases.ts b/extensions/ql-vscode/src/local-databases.ts index 8ba756df7..c1d281474 100644 --- a/extensions/ql-vscode/src/local-databases.ts +++ b/extensions/ql-vscode/src/local-databases.ts @@ -795,9 +795,6 @@ export class DatabaseManager extends DisposableObject { public async loadPersistedState(): Promise { return withProgress( - { - location: vscode.ProgressLocation.Notification, - }, async (progress, token) => { const currentDatabaseUri = this.ctx.workspaceState.get(CURRENT_DB); @@ -861,6 +858,9 @@ export class DatabaseManager extends DisposableObject { void this.logger.log("Finished loading persisted databases."); }, + { + location: vscode.ProgressLocation.Notification, + }, ); } From 964640c757412993601f434388f5da434ce9cd29 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Tue, 14 Mar 2023 11:00:59 +0100 Subject: [PATCH 3/3] Remove default location values in withProgress calls --- .../src/contextual/templateProvider.ts | 3 - extensions/ql-vscode/src/extension.ts | 1 - extensions/ql-vscode/src/local-databases.ts | 116 ++++++++---------- 3 files changed, 54 insertions(+), 66 deletions(-) diff --git a/extensions/ql-vscode/src/contextual/templateProvider.ts b/extensions/ql-vscode/src/contextual/templateProvider.ts index 1e2865d99..4b53146d4 100644 --- a/extensions/ql-vscode/src/contextual/templateProvider.ts +++ b/extensions/ql-vscode/src/contextual/templateProvider.ts @@ -4,7 +4,6 @@ import { Location, LocationLink, Position, - ProgressLocation, ReferenceContext, ReferenceProvider, TextDocument, @@ -87,7 +86,6 @@ export class TemplateQueryDefinitionProvider implements DefinitionProvider { ); }, { - location: ProgressLocation.Notification, cancellable: true, title: "Finding definitions", }, @@ -150,7 +148,6 @@ export class TemplateQueryReferenceProvider implements ReferenceProvider { ); }, { - location: ProgressLocation.Notification, cancellable: true, title: "Finding references", }, diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index eb9420b82..5b4edf7cd 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -329,7 +329,6 @@ export async function activate( ), { title: progressTitle, - location: ProgressLocation.Notification, }, ); diff --git a/extensions/ql-vscode/src/local-databases.ts b/extensions/ql-vscode/src/local-databases.ts index c1d281474..9542ef767 100644 --- a/extensions/ql-vscode/src/local-databases.ts +++ b/extensions/ql-vscode/src/local-databases.ts @@ -794,74 +794,66 @@ export class DatabaseManager extends DisposableObject { } public async loadPersistedState(): Promise { - return withProgress( - async (progress, token) => { - const currentDatabaseUri = - this.ctx.workspaceState.get(CURRENT_DB); - const databases = this.ctx.workspaceState.get( - DB_LIST, - [], + return withProgress(async (progress, token) => { + const currentDatabaseUri = + this.ctx.workspaceState.get(CURRENT_DB); + const databases = this.ctx.workspaceState.get( + DB_LIST, + [], + ); + let step = 0; + progress({ + maxStep: databases.length, + message: "Loading persisted databases", + step, + }); + try { + void this.logger.log( + `Found ${databases.length} persisted databases: ${databases + .map((db) => db.uri) + .join(", ")}`, ); - let step = 0; - progress({ - maxStep: databases.length, - message: "Loading persisted databases", - step, - }); - try { - void this.logger.log( - `Found ${databases.length} persisted databases: ${databases - .map((db) => db.uri) - .join(", ")}`, - ); - for (const database of databases) { - progress({ - maxStep: databases.length, - message: `Loading ${ - database.options?.displayName || "databases" - }`, - step: ++step, - }); + for (const database of databases) { + progress({ + maxStep: databases.length, + message: `Loading ${database.options?.displayName || "databases"}`, + step: ++step, + }); - const databaseItem = - await this.createDatabaseItemFromPersistedState( - progress, - token, - database, - ); - try { - await databaseItem.refresh(); - await this.registerDatabase(progress, token, databaseItem); - if (currentDatabaseUri === database.uri) { - await this.setCurrentDatabaseItem(databaseItem, true); - } - void this.logger.log( - `Loaded database ${databaseItem.name} at URI ${database.uri}.`, - ); - } catch (e) { - // When loading from persisted state, leave invalid databases in the list. They will be - // marked as invalid, and cannot be set as the current database. - void this.logger.log( - `Error loading database ${database.uri}: ${e}.`, - ); + const databaseItem = await this.createDatabaseItemFromPersistedState( + progress, + token, + database, + ); + try { + await databaseItem.refresh(); + await this.registerDatabase(progress, token, databaseItem); + if (currentDatabaseUri === database.uri) { + await this.setCurrentDatabaseItem(databaseItem, true); } + void this.logger.log( + `Loaded database ${databaseItem.name} at URI ${database.uri}.`, + ); + } catch (e) { + // When loading from persisted state, leave invalid databases in the list. They will be + // marked as invalid, and cannot be set as the current database. + void this.logger.log( + `Error loading database ${database.uri}: ${e}.`, + ); } - await this.updatePersistedDatabaseList(); - } catch (e) { - // database list had an unexpected type - nothing to be done? - void showAndLogExceptionWithTelemetry( - redactableError( - asError(e), - )`Database list loading failed: ${getErrorMessage(e)}`, - ); } + await this.updatePersistedDatabaseList(); + } catch (e) { + // database list had an unexpected type - nothing to be done? + void showAndLogExceptionWithTelemetry( + redactableError( + asError(e), + )`Database list loading failed: ${getErrorMessage(e)}`, + ); + } - void this.logger.log("Finished loading persisted databases."); - }, - { - location: vscode.ProgressLocation.Notification, - }, - ); + void this.logger.log("Finished loading persisted databases."); + }); } public get databaseItems(): readonly DatabaseItem[] {