From d0bcba4cc04073b4d319d611fd75d1e20b3d45be Mon Sep 17 00:00:00 2001 From: Benjin Dubishar Date: Wed, 11 Aug 2021 09:11:00 -0700 Subject: [PATCH] Corrects workspace project tree refresh behavior for adding new projects to the workspace (#16650) * bugfix and updates * PR feedback * Deferred promise for project disk scan * fix casing * fixing race condition on extension activation, test failure --- extensions/data-workspace/package.json | 6 +- .../src/common/dataWorkspaceExtension.ts | 4 +- .../data-workspace/src/common/interfaces.ts | 4 +- .../data-workspace/src/common/promise.ts | 25 ++++ .../src/common/workspaceTreeDataProvider.ts | 5 +- .../data-workspace/src/dataworkspace.d.ts | 3 +- extensions/data-workspace/src/main.ts | 8 +- .../src/services/workspaceService.ts | 109 ++++++++++++------ .../src/test/workspaceService.test.ts | 6 +- .../test/workspaceTreeDataProvider.test.ts | 25 ++-- .../addDatabaseReferenceDialog.test.ts | 2 +- .../src/test/projectController.test.ts | 2 +- 12 files changed, 134 insertions(+), 65 deletions(-) create mode 100644 extensions/data-workspace/src/common/promise.ts diff --git a/extensions/data-workspace/package.json b/extensions/data-workspace/package.json index a24e30876f..fdfd61d604 100644 --- a/extensions/data-workspace/package.json +++ b/extensions/data-workspace/package.json @@ -59,7 +59,7 @@ { "command": "dataworkspace.refresh", "title": "%refresh-workspace-command%", - "category": "", + "category": "%data-workspace-view-container-name%", "icon": "$(refresh)" }, { @@ -78,12 +78,12 @@ { "command": "dataworkspace.refresh", "when": "view == dataworkspace.views.main", - "group": "1_currentWorkspace" + "group": "navigation" }, { "command": "dataworkspace.close", "when": "view == dataworkspace.views.main && workbenchState == workspace", - "group": "2_commands" + "group": "1_commands" }, { "command": "projects.new", diff --git a/extensions/data-workspace/src/common/dataWorkspaceExtension.ts b/extensions/data-workspace/src/common/dataWorkspaceExtension.ts index a0b672cc81..ba9e0af343 100644 --- a/extensions/data-workspace/src/common/dataWorkspaceExtension.ts +++ b/extensions/data-workspace/src/common/dataWorkspaceExtension.ts @@ -13,8 +13,8 @@ export class DataWorkspaceExtension implements IExtension { constructor(private workspaceService: WorkspaceService) { } - getProjectsInWorkspace(ext?: string): Promise { - return this.workspaceService.getProjectsInWorkspace(ext); + getProjectsInWorkspace(ext?: string, refreshFromDisk?: boolean): Promise { + return this.workspaceService.getProjectsInWorkspace(ext, refreshFromDisk); } addProjectsToWorkspace(projectFiles: vscode.Uri[]): Promise { diff --git a/extensions/data-workspace/src/common/interfaces.ts b/extensions/data-workspace/src/common/interfaces.ts index 98b6e7fbb1..e074e22e9d 100644 --- a/extensions/data-workspace/src/common/interfaces.ts +++ b/extensions/data-workspace/src/common/interfaces.ts @@ -50,8 +50,10 @@ export interface IWorkspaceService { /** * Gets the project files in current workspace + * @param ext project extension to filter on. If this is passed in, this will only return projects with this file extension + * @param refreshFromDisk whether to rescan the folder for project files, or return the cached version. Defaults to false. */ - getProjectsInWorkspace(): Promise; + getProjectsInWorkspace(ext?: string, refreshFromDisk?: boolean): Promise; /** * Gets the project provider by project file diff --git a/extensions/data-workspace/src/common/promise.ts b/extensions/data-workspace/src/common/promise.ts new file mode 100644 index 0000000000..c8d40d7a67 --- /dev/null +++ b/extensions/data-workspace/src/common/promise.ts @@ -0,0 +1,25 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the Source EULA. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +/** + * Deferred promise + */ +export class Deferred { + promise: Promise; + resolve!: (value: T | PromiseLike) => void; + reject!: (reason?: any) => void; + constructor() { + this.promise = new Promise((resolve, reject) => { + this.resolve = resolve; + this.reject = reject; + }); + } + + then(onfulfilled?: (value: T) => TResult | Thenable, onrejected?: (reason: any) => TResult | Thenable): Thenable; + then(onfulfilled?: (value: T) => TResult | Thenable, onrejected?: (reason: any) => void): Thenable; + then(onfulfilled?: (value: T) => TResult | Thenable, onrejected?: (reason: any) => TResult | Thenable): Thenable { + return this.promise.then(onfulfilled, onrejected); + } +} diff --git a/extensions/data-workspace/src/common/workspaceTreeDataProvider.ts b/extensions/data-workspace/src/common/workspaceTreeDataProvider.ts index ef43adbc6d..7d3f09930a 100644 --- a/extensions/data-workspace/src/common/workspaceTreeDataProvider.ts +++ b/extensions/data-workspace/src/common/workspaceTreeDataProvider.ts @@ -23,7 +23,8 @@ export class WorkspaceTreeDataProvider implements vscode.TreeDataProvider | undefined = new vscode.EventEmitter(); readonly onDidChangeTreeData?: vscode.Event | undefined = this._onDidChangeTreeData?.event; - refresh(): void { + async refresh(): Promise { + await this._workspaceService.getProjectsInWorkspace(undefined, true); this._onDidChangeTreeData?.fire(); } @@ -38,7 +39,7 @@ export class WorkspaceTreeDataProvider implements vscode.TreeDataProvider; + getProjectsInWorkspace(ext?: string, refreshFromDisk?: boolean): Promise; /** * Add projects to the workspace diff --git a/extensions/data-workspace/src/main.ts b/extensions/data-workspace/src/main.ts index ed9e52d782..9ffed062e3 100644 --- a/extensions/data-workspace/src/main.ts +++ b/extensions/data-workspace/src/main.ts @@ -22,8 +22,8 @@ export async function activate(context: vscode.ExtensionContext): Promise { - workspaceTreeDataProvider.refresh(); + context.subscriptions.push(vscode.workspace.onDidChangeWorkspaceFolders(async () => { + await workspaceTreeDataProvider.refresh(); })); const dataWorkspaceExtension = new DataWorkspaceExtension(workspaceService); context.subscriptions.push(vscode.window.registerTreeDataProvider('dataworkspace.views.main', workspaceTreeDataProvider)); @@ -54,8 +54,8 @@ export async function activate(context: vscode.ExtensionContext): Promise { - workspaceTreeDataProvider.refresh(); + context.subscriptions.push(vscode.commands.registerCommand('dataworkspace.refresh', async () => { + await workspaceTreeDataProvider.refresh(); })); context.subscriptions.push(vscode.commands.registerCommand('dataworkspace.close', () => { diff --git a/extensions/data-workspace/src/services/workspaceService.ts b/extensions/data-workspace/src/services/workspaceService.ts index 13d67e16d2..9fc9d626f7 100644 --- a/extensions/data-workspace/src/services/workspaceService.ts +++ b/extensions/data-workspace/src/services/workspaceService.ts @@ -13,13 +13,18 @@ import { IWorkspaceService } from '../common/interfaces'; import { ProjectProviderRegistry } from '../common/projectProviderRegistry'; import Logger from '../common/logger'; import { TelemetryReporter, TelemetryViews, TelemetryActions } from '../common/telemetry'; +import { Deferred } from '../common/promise'; import { getAzdataApi } from '../common/utils'; export class WorkspaceService implements IWorkspaceService { private _onDidWorkspaceProjectsChange: vscode.EventEmitter = new vscode.EventEmitter(); readonly onDidWorkspaceProjectsChange: vscode.Event = this._onDidWorkspaceProjectsChange?.event; - constructor() { } + private openedProjects: vscode.Uri[] = []; + + constructor() { + this.getProjectsInWorkspace(undefined, true); + } get isProjectProviderAvailable(): boolean { for (const extension of vscode.extensions.all) { @@ -49,46 +54,53 @@ export class WorkspaceService implements IWorkspaceService { } } - async addProjectsToWorkspace(projectFiles: vscode.Uri[]): Promise { - if (!projectFiles || projectFiles.length === 0) { - return; - } + public async addProjectsToWorkspace(projectFiles: vscode.Uri[]): Promise { + // 1. Include new workspace folders if any of the new projects' locations aren't already included - const currentProjects: vscode.Uri[] = await this.getProjectsInWorkspace(); const newWorkspaceFolders: string[] = []; - let newProjectFileAdded = false; + for (const projectFile of projectFiles) { - if (currentProjects.findIndex((p: vscode.Uri) => p.fsPath === projectFile.fsPath) === -1) { - currentProjects.push(projectFile); - newProjectFileAdded = true; + const relativePath = vscode.workspace.asRelativePath(projectFile, false); - TelemetryReporter.createActionEvent(TelemetryViews.WorkspaceTreePane, TelemetryActions.ProjectAddedToWorkspace) - .withAdditionalProperties({ - projectType: path.extname(projectFile.fsPath) - }).send(); - - // if the relativePath and the original path is the same, that means the project file is not under - // any workspace folders, we should add the parent folder of the project file to the workspace - const relativePath = vscode.workspace.asRelativePath(projectFile, false); - if (vscode.Uri.file(relativePath).fsPath === projectFile.fsPath) { - newWorkspaceFolders.push(path.dirname(projectFile.path)); - } - } else { - vscode.window.showInformationMessage(constants.ProjectAlreadyOpened(projectFile.fsPath)); + if (relativePath === undefined || vscode.Uri.file(relativePath).fsPath === projectFile.fsPath) { + newWorkspaceFolders.push(path.dirname(projectFile.path)); } } - if (newProjectFileAdded) { - this._onDidWorkspaceProjectsChange.fire(); - } - if (newWorkspaceFolders.length > 0) { // Add to the end of the workspace folders to avoid a restart of the extension host if we can vscode.workspace.updateWorkspaceFolders(vscode.workspace.workspaceFolders?.length || 0, undefined, ...(newWorkspaceFolders.map(folder => ({ uri: vscode.Uri.file(folder) })))); } + + // 2. Re-detect projects from the updated set of workspace folders + + const previousProjects: string[] = await (await this.getProjectsInWorkspace(undefined, true)).map(p => p.path); + let newProjectAdded: boolean = false; + const projectsAlreadyOpen: string[] = []; + + for (const projectFile of projectFiles) { + if (previousProjects.includes(projectFile.path)) { + projectsAlreadyOpen.push(projectFile.fsPath); + vscode.window.showInformationMessage(constants.ProjectAlreadyOpened(projectFile.fsPath)); + } + else { + newProjectAdded = true; + + TelemetryReporter.createActionEvent(TelemetryViews.WorkspaceTreePane, TelemetryActions.ProjectAddedToWorkspace) + .withAdditionalProperties({ + projectType: path.extname(projectFile.fsPath) + }).send(); + } + } + + // 3. If any new projects are detected, fire event to refresh projects tree + + if (newProjectAdded) { + this._onDidWorkspaceProjectsChange.fire(); + } } - async getAllProjectTypes(): Promise { + public async getAllProjectTypes(): Promise { await this.ensureProviderExtensionLoaded(); const projectTypes: dataworkspace.IProjectType[] = []; ProjectProviderRegistry.providers.forEach(provider => { @@ -97,19 +109,46 @@ export class WorkspaceService implements IWorkspaceService { return projectTypes; } - async getProjectsInWorkspace(ext?: string): Promise { - const projectPromises = vscode.workspace.workspaceFolders?.map(f => this.getAllProjectsInFolder(f.uri)); - if (!projectPromises) { - return []; + private getProjectsPromise: Deferred | undefined = undefined; + + /** + * Returns all the projects in the workspace + * @param ext project extension to filter on. If this is passed in, this will only return projects with this file extension + * @param refreshFromDisk whether to rescan the folder for project files, or return the cached version. Defaults to false. + * @returns array of file URIs for projects + */ + public async getProjectsInWorkspace(ext?: string, refreshFromDisk: boolean = false): Promise { + + if (refreshFromDisk || this.openedProjects.length === 0) { // always check if nothing cached + await this.refreshProjectsFromDisk(); } - let projects = (await Promise.all(projectPromises)).reduce((prev, curr) => prev.concat(curr), []); // filter by specified extension if (ext) { - projects = projects.filter(p => p.fsPath.toLowerCase().endsWith(ext.toLowerCase())); + return this.openedProjects.filter(p => p.fsPath.toLowerCase().endsWith(ext.toLowerCase())); + } else { + return this.openedProjects; + } + } + + private async refreshProjectsFromDisk(): Promise { + // Only allow one disk scan to be happening at a time + if (this.getProjectsPromise) { + return this.getProjectsPromise.promise; } - return projects; + this.getProjectsPromise = new Deferred(); + + try { + const projectPromises = vscode.workspace.workspaceFolders?.map(f => this.getAllProjectsInFolder(f.uri)) ?? []; + this.openedProjects = (await Promise.all(projectPromises)).reduce((prev, curr) => prev.concat(curr), []); + this.getProjectsPromise.resolve(); + } catch (err) { + this.getProjectsPromise.reject(err); + throw err; + } finally { + this.getProjectsPromise = undefined; + } } /** diff --git a/extensions/data-workspace/src/test/workspaceService.test.ts b/extensions/data-workspace/src/test/workspaceService.test.ts index 0c15ea222d..267cbedbbd 100644 --- a/extensions/data-workspace/src/test/workspaceService.test.ts +++ b/extensions/data-workspace/src/test/workspaceService.test.ts @@ -44,19 +44,19 @@ suite('WorkspaceService', function (): void { test('getProjectsInWorkspace', async () => { // No workspace is loaded - let projects = await service.getProjectsInWorkspace(); + let projects = await service.getProjectsInWorkspace(undefined, true); should.strictEqual(projects.length, 0, 'no projects should be returned when no workspace is loaded'); // No projects are present in the workspace file const workspaceFoldersStub = sinon.stub(vscode.workspace, 'workspaceFolders').value([]); - projects = await service.getProjectsInWorkspace(); + projects = await service.getProjectsInWorkspace(undefined, true); should.strictEqual(projects.length, 0, 'no projects should be returned when projects are present in the workspace file'); workspaceFoldersStub.restore(); // Projects are present sinon.stub(vscode.workspace, 'workspaceFolders').value([{ uri: vscode.Uri.file('')}]); sinon.stub(service, 'getAllProjectsInFolder').resolves([vscode.Uri.file('/test/folder/abc.sqlproj'), vscode.Uri.file('/test/folder/folder1/abc1.sqlproj'), vscode.Uri.file('/test/folder/folder2/abc2.sqlproj')]); - projects = await service.getProjectsInWorkspace(); + projects = await service.getProjectsInWorkspace(undefined, true); should.strictEqual(projects.length, 3, 'there should be 3 projects'); const project1 = vscode.Uri.file('/test/folder/abc.sqlproj'); const project2 = vscode.Uri.file('/test/folder/folder1/abc1.sqlproj'); diff --git a/extensions/data-workspace/src/test/workspaceTreeDataProvider.test.ts b/extensions/data-workspace/src/test/workspaceTreeDataProvider.test.ts index 018c38b74d..c3aa39445c 100644 --- a/extensions/data-workspace/src/test/workspaceTreeDataProvider.test.ts +++ b/extensions/data-workspace/src/test/workspaceTreeDataProvider.test.ts @@ -20,12 +20,12 @@ suite('workspaceTreeDataProvider Tests', function (): void { sinon.restore(); }); - test('test refresh()', () => { + test('test refresh()', async () => { const treeDataChangeHandler = sinon.stub(); treeProvider.onDidChangeTreeData!((e) => { treeDataChangeHandler(e); }); - treeProvider.refresh(); + await treeProvider.refresh(); should.strictEqual(treeDataChangeHandler.calledOnce, true); }); @@ -100,16 +100,17 @@ suite('workspaceTreeDataProvider Tests', function (): void { }], getDashboardComponents: (projectFile: string): IDashboardTable[] => { return [{ - name: 'Deployments', - columns: [{ displayName: 'c1', width: 75, type: 'string' }], - data: [['d1']] - }, - { - name: 'Builds', - columns: [{ displayName: 'c1', width: 75, type: 'string' }], - data: [['d1']] - }]; - }}; + name: 'Deployments', + columns: [{ displayName: 'c1', width: 75, type: 'string' }], + data: [['d1']] + }, + { + name: 'Builds', + columns: [{ displayName: 'c1', width: 75, type: 'string' }], + data: [['d1']] + }]; + } + }; const getProjectProviderStub = sinon.stub(workspaceService, 'getProjectProvider'); getProjectProviderStub.onFirstCall().resolves(undefined); getProjectProviderStub.onSecondCall().resolves(projectProvider); diff --git a/extensions/sql-database-projects/src/test/dialogs/addDatabaseReferenceDialog.test.ts b/extensions/sql-database-projects/src/test/dialogs/addDatabaseReferenceDialog.test.ts index 82336f8acb..f6fdc6c7e9 100644 --- a/extensions/sql-database-projects/src/test/dialogs/addDatabaseReferenceDialog.test.ts +++ b/extensions/sql-database-projects/src/test/dialogs/addDatabaseReferenceDialog.test.ts @@ -23,7 +23,7 @@ describe('Add Database Reference Dialog', () => { beforeEach(function (): void { const dataWorkspaceMock = TypeMoq.Mock.ofType(); - dataWorkspaceMock.setup(x => x.getProjectsInWorkspace(TypeMoq.It.isAny())).returns(() => Promise.resolve([])); + dataWorkspaceMock.setup(x => x.getProjectsInWorkspace(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve([])); sinon.stub(vscode.extensions, 'getExtension').returns({ exports: dataWorkspaceMock.object }); }); diff --git a/extensions/sql-database-projects/src/test/projectController.test.ts b/extensions/sql-database-projects/src/test/projectController.test.ts index 41efaf0615..93a81eeb45 100644 --- a/extensions/sql-database-projects/src/test/projectController.test.ts +++ b/extensions/sql-database-projects/src/test/projectController.test.ts @@ -597,7 +597,7 @@ describe('ProjectsController', function (): void { const project2 = await Project.openProject(vscode.Uri.file(projPath2).fsPath); const showErrorMessageSpy = sinon.spy(vscode.window, 'showErrorMessage'); const dataWorkspaceMock = TypeMoq.Mock.ofType(); - dataWorkspaceMock.setup(x => x.getProjectsInWorkspace(TypeMoq.It.isAny())).returns(() => Promise.resolve([vscode.Uri.file(project1.projectFilePath), vscode.Uri.file(project2.projectFilePath)])); + dataWorkspaceMock.setup(x => x.getProjectsInWorkspace(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve([vscode.Uri.file(project1.projectFilePath), vscode.Uri.file(project2.projectFilePath)])); sinon.stub(vscode.extensions, 'getExtension').returns({ exports: dataWorkspaceMock.object }); // add project reference from project1 to project2