Unexpected events for single updateWorkspaceFolders operation (fixes #42639)

src/vs/workbench/services/configuration/node/configurationService.ts

@@ -5,9 +5,8 @@
 'use strict';
 
 import URI from 'vs/base/common/uri';
-import * as paths from 'vs/base/common/paths';
 import { TPromise } from 'vs/base/common/winjs.base';
-import { dirname } from 'path';
+import { dirname, basename } from 'path';
 import * as assert from 'vs/base/common/assert';
 import Event, { Emitter } from 'vs/base/common/event';
 import { StrictResourceMap } from 'vs/base/common/map';
@@ -111,13 +110,16 @@ export class WorkspaceService extends Disposable implements IWorkspaceConfigurat
 	}
 
 	public addFolders(foldersToAdd: IWorkspaceFolderCreationData[], index?: number): TPromise<void> {
-		assert.ok(this.jsonEditingService, 'Workbench is not initialized yet');
-		return this.workspaceEditingQueue.queue(() => this.doAddFolders(foldersToAdd, index));
+		return this.updateFolders(foldersToAdd, [], index);
 	}
 
 	public removeFolders(foldersToRemove: URI[]): TPromise<void> {
+		return this.updateFolders([], foldersToRemove);
+	}
+
+	public updateFolders(foldersToAdd: IWorkspaceFolderCreationData[], foldersToRemove: URI[], index?: number): TPromise<void> {
 		assert.ok(this.jsonEditingService, 'Workbench is not initialized yet');
-		return this.workspaceEditingQueue.queue(() => this.doRemoveFolders(foldersToRemove));
+		return this.workspaceEditingQueue.queue(() => this.doUpdateFolders(foldersToAdd, foldersToRemove, index));
 	}
 
 	public isInsideWorkspace(resource: URI): boolean {
@@ -134,80 +136,82 @@ export class WorkspaceService extends Disposable implements IWorkspaceConfigurat
 		return false;
 	}
 
-	private doAddFolders(foldersToAdd: IWorkspaceFolderCreationData[], index?: number): TPromise<void> {
+	private doUpdateFolders(foldersToAdd: IWorkspaceFolderCreationData[], foldersToRemove: URI[], index?: number): TPromise<void> {
 		if (this.getWorkbenchState() !== WorkbenchState.WORKSPACE) {
 			return TPromise.as(void 0); // we need a workspace to begin with
 		}
 
-		const currentWorkspaceFolders = this.getWorkspace().folders;
-		const currentWorkspaceFolderUris = currentWorkspaceFolders.map(folder => folder.uri);
-		const currentStoredFolders = currentWorkspaceFolders.map(folder => folder.raw);
-
-		const storedFoldersToAdd: IStoredWorkspaceFolder[] = [];
+		if (foldersToAdd.length + foldersToRemove.length === 0) {
+			return TPromise.as(void 0); // nothing to do
+		}
 
-		const workspaceConfigFolder = dirname(this.getWorkspace().configuration.fsPath);
+		let foldersHaveChanged = false;
 
-		foldersToAdd.forEach(folderToAdd => {
-			if (this.contains(currentWorkspaceFolderUris, folderToAdd.uri)) {
-				return; // already existing
+		// Remove first (if any)
+		let currentWorkspaceFolders = this.getWorkspace().folders;
+		let newStoredFolders: IStoredWorkspaceFolder[] = currentWorkspaceFolders.map(f => f.raw).filter((folder, index) => {
+			if (!isStoredWorkspaceFolder(folder)) {
+				return true; // keep entries which are unrelated
 			}
 
-			let storedFolder: IStoredWorkspaceFolder;
+			return !this.contains(foldersToRemove, currentWorkspaceFolders[index].uri); // keep entries which are unrelated
+		});
 
-			// File resource: use "path" property
-			if (folderToAdd.uri.scheme === Schemas.file) {
-				storedFolder = {
-					path: massageFolderPathForWorkspace(folderToAdd.uri.fsPath, workspaceConfigFolder, currentStoredFolders)
-				};
-			}
+		foldersHaveChanged = currentWorkspaceFolders.length !== newStoredFolders.length;
 
-			// Any other resource: use "uri" property
-			else {
-				storedFolder = {
-					uri: folderToAdd.uri.toString(true)
-				};
-			}
+		// Add afterwards (if any)
+		if (foldersToAdd.length) {
 
-			if (folderToAdd.name) {
-				storedFolder.name = folderToAdd.name;
-			}
+			// Recompute current workspace folders if we have folders to add
+			const workspaceConfigFolder = dirname(this.getWorkspace().configuration.fsPath);
+			currentWorkspaceFolders = toWorkspaceFolders(newStoredFolders, URI.file(workspaceConfigFolder));
+			const currentWorkspaceFolderUris = currentWorkspaceFolders.map(folder => folder.uri);
 
-			storedFoldersToAdd.push(storedFolder);
-		});
+			const storedFoldersToAdd: IStoredWorkspaceFolder[] = [];
 
-		if (storedFoldersToAdd.length > 0) {
-			let newStoredWorkspaceFolders: IStoredWorkspaceFolder[] = [];
+			foldersToAdd.forEach(folderToAdd => {
+				if (this.contains(currentWorkspaceFolderUris, folderToAdd.uri)) {
+					return; // already existing
+				}
 
-			if (typeof index === 'number' && index >= 0 && index < currentStoredFolders.length) {
-				newStoredWorkspaceFolders = currentStoredFolders.slice(0);
-				newStoredWorkspaceFolders.splice(index, 0, ...storedFoldersToAdd);
-			} else {
-				newStoredWorkspaceFolders = [...currentStoredFolders, ...storedFoldersToAdd];
-			}
+				let storedFolder: IStoredWorkspaceFolder;
 
-			return this.setFolders(newStoredWorkspaceFolders);
-		}
+				// File resource: use "path" property
+				if (folderToAdd.uri.scheme === Schemas.file) {
+					storedFolder = {
+						path: massageFolderPathForWorkspace(folderToAdd.uri.fsPath, workspaceConfigFolder, newStoredFolders)
+					};
+				}
 
-		return TPromise.as(void 0);
-	}
+				// Any other resource: use "uri" property
+				else {
+					storedFolder = {
+						uri: folderToAdd.uri.toString(true)
+					};
+				}
 
-	private doRemoveFolders(foldersToRemove: URI[]): TPromise<void> {
-		if (this.getWorkbenchState() !== WorkbenchState.WORKSPACE) {
-			return TPromise.as(void 0); // we need a workspace to begin with
-		}
+				if (folderToAdd.name) {
+					storedFolder.name = folderToAdd.name;
+				}
 
-		const currentWorkspaceFolders = this.getWorkspace().folders;
-		const currentStoredFolders = currentWorkspaceFolders.map(folder => folder.raw);
+				storedFoldersToAdd.push(storedFolder);
+			});
 
-		const newStoredFolders: IStoredWorkspaceFolder[] = currentStoredFolders.filter((folder, index) => {
-			if (!isStoredWorkspaceFolder(folder)) {
-				return true; // keep entries which are unrelated
-			}
+			// Apply to array of newStoredFolders
+			if (storedFoldersToAdd.length > 0) {
+				foldersHaveChanged = true;
 
-			return !this.contains(foldersToRemove, currentWorkspaceFolders[index].uri); // keep entries which are unrelated
-		});
+				if (typeof index === 'number' && index >= 0 && index < newStoredFolders.length) {
+					newStoredFolders = newStoredFolders.slice(0);
+					newStoredFolders.splice(index, 0, ...storedFoldersToAdd);
+				} else {
+					newStoredFolders = [...newStoredFolders, ...storedFoldersToAdd];
+				}
+			}
+		}
 
-		if (newStoredFolders.length !== currentStoredFolders.length) {
+		// Set folders if we recorded a change
+		if (foldersHaveChanged) {
 			return this.setFolders(newStoredFolders);
 		}
 
@@ -331,7 +335,7 @@ export class WorkspaceService extends Disposable implements IWorkspaceConfigurat
 		const workspaceConfigPath = URI.file(workspaceIdentifier.configPath);
 		return this.workspaceConfiguration.load(workspaceConfigPath)
 			.then(() => {
-				const workspaceFolders = toWorkspaceFolders(this.workspaceConfiguration.getFolders(), URI.file(paths.dirname(workspaceConfigPath.fsPath)));
+				const workspaceFolders = toWorkspaceFolders(this.workspaceConfiguration.getFolders(), URI.file(dirname(workspaceConfigPath.fsPath)));
 				const workspaceId = workspaceIdentifier.id;
 				const workspaceName = getWorkspaceLabel({ id: workspaceId, configPath: workspaceConfigPath.fsPath }, this.environmentService);
 				return new Workspace(workspaceId, workspaceName, workspaceFolders, workspaceConfigPath);
@@ -350,7 +354,7 @@ export class WorkspaceService extends Disposable implements IWorkspaceConfigurat
 	}
 
 	private createEmptyWorkspace(configuration: IWindowConfiguration): TPromise<Workspace> {
-		let id = configuration.backupPath ? URI.from({ path: paths.basename(configuration.backupPath), scheme: 'empty' }).toString() : '';
+		let id = configuration.backupPath ? URI.from({ path: basename(configuration.backupPath), scheme: 'empty' }).toString() : '';
 		return TPromise.as(new Workspace(id));
 	}
 
@@ -506,7 +510,7 @@ export class WorkspaceService extends Disposable implements IWorkspaceConfigurat
 	private onWorkspaceConfigurationChanged(): TPromise<void> {
 		if (this.workspace && this.workspace.configuration && this._configuration) {
 			const workspaceConfigurationChangeEvent = this._configuration.compareAndUpdateWorkspaceConfiguration(this.workspaceConfiguration.getConfiguration());
-			let configuredFolders = toWorkspaceFolders(this.workspaceConfiguration.getFolders(), URI.file(paths.dirname(this.workspace.configuration.fsPath)));
+			let configuredFolders = toWorkspaceFolders(this.workspaceConfiguration.getFolders(), URI.file(dirname(this.workspace.configuration.fsPath)));
 			const changes = this.compareFolders(this.workspace.folders, configuredFolders);
 			if (changes.added.length || changes.removed.length || changes.changed.length) {
 				this.workspace.folders = configuredFolders;

src/vs/workbench/services/configuration/test/node/configurationService.test.ts

@@ -274,6 +274,55 @@ suite('WorkspaceContextService - Workspace', () => {
 			});
 	});
 
+	test('update folders (remove last and add to end)', () => {
+		const target = sinon.spy();
+		testObject.onDidChangeWorkspaceFolders(target);
+		const workspaceDir = path.dirname(testObject.getWorkspace().folders[0].uri.fsPath);
+		const addedFolders = [{ uri: URI.file(path.join(workspaceDir, 'd')) }, { uri: URI.file(path.join(workspaceDir, 'c')) }];
+		const removedFolders = [testObject.getWorkspace().folders[1]].map(f => f.uri);
+		return testObject.updateFolders(addedFolders, removedFolders)
+			.then(() => {
+				assert.ok(target.calledOnce);
+				const actual = <IWorkspaceFoldersChangeEvent>target.args[0][0];
+				assert.deepEqual(actual.added.map(r => r.uri.toString()), addedFolders.map(a => a.uri.toString()));
+				assert.deepEqual(actual.removed.map(r => r.uri.toString()), removedFolders.map(a => a.toString()));
+				assert.deepEqual(actual.changed, []);
+			});
+	});
+
+	test('update folders (rename first via add and remove)', () => {
+		const target = sinon.spy();
+		testObject.onDidChangeWorkspaceFolders(target);
+		const workspaceDir = path.dirname(testObject.getWorkspace().folders[0].uri.fsPath);
+		const addedFolders = [{ uri: URI.file(path.join(workspaceDir, 'a')), name: 'The Folder' }];
+		const removedFolders = [testObject.getWorkspace().folders[0]].map(f => f.uri);
+		return testObject.updateFolders(addedFolders, removedFolders, 0)
+			.then(() => {
+				assert.ok(target.calledOnce);
+				const actual = <IWorkspaceFoldersChangeEvent>target.args[0][0];
+				assert.deepEqual(actual.added, []);
+				assert.deepEqual(actual.removed, []);
+				assert.deepEqual(actual.changed.map(r => r.uri.toString()), removedFolders.map(a => a.toString()));
+			});
+	});
+
+	test('update folders (remove first and add to end)', () => {
+		const target = sinon.spy();
+		testObject.onDidChangeWorkspaceFolders(target);
+		const workspaceDir = path.dirname(testObject.getWorkspace().folders[0].uri.fsPath);
+		const addedFolders = [{ uri: URI.file(path.join(workspaceDir, 'd')) }, { uri: URI.file(path.join(workspaceDir, 'c')) }];
+		const removedFolders = [testObject.getWorkspace().folders[0]].map(f => f.uri);
+		const changedFolders = [testObject.getWorkspace().folders[1]].map(f => f.uri);
+		return testObject.updateFolders(addedFolders, removedFolders)
+			.then(() => {
+				assert.ok(target.calledOnce);
+				const actual = <IWorkspaceFoldersChangeEvent>target.args[0][0];
+				assert.deepEqual(actual.added.map(r => r.uri.toString()), addedFolders.map(a => a.uri.toString()));
+				assert.deepEqual(actual.removed.map(r => r.uri.toString()), removedFolders.map(a => a.toString()));
+				assert.deepEqual(actual.changed.map(r => r.uri.toString()), changedFolders.map(a => a.toString()));
+			});
+	});
+
 	test('reorder folders trigger change event', () => {
 		const target = sinon.spy();
 		testObject.onDidChangeWorkspaceFolders(target);

src/vs/workbench/services/workspace/node/workspaceEditingService.ts

@@ -73,15 +73,28 @@ export class WorkspaceEditingService implements IWorkspaceEditingService {
 		}
 
 		// Add & Delete Folders
-		if (this.includesSingleFolderWorkspace(foldersToDelete)) {
+		else {
+
 			// if we are in single-folder state and the folder is replaced with
 			// other folders, we handle this specially and just enter workspace
 			// mode with the folders that are being added.
-			return this.createAndEnterWorkspace(foldersToAdd);
+			if (this.includesSingleFolderWorkspace(foldersToDelete)) {
+				return this.createAndEnterWorkspace(foldersToAdd);
+			}
+
+			// if we are not in workspace-state, we just add the folders
+			if (this.contextService.getWorkbenchState() !== WorkbenchState.WORKSPACE) {
+				return this.doAddFolders(foldersToAdd, index, donotNotifyError);
+			}
+
+			// finally, update folders within the workspace
+			return this.doUpdateFolders(foldersToAdd, foldersToDelete, index, donotNotifyError);
 		}
+	}
 
-		// Make sure to first remove folders and then add them to account for folders being updated
-		return this.removeFolders(foldersToDelete).then(() => this.doAddFolders(foldersToAdd, index, donotNotifyError));
+	private doUpdateFolders(foldersToAdd: IWorkspaceFolderCreationData[], foldersToDelete: URI[], index?: number, donotNotifyError: boolean = false): TPromise<void> {
+		return this.contextService.updateFolders(foldersToAdd, foldersToDelete, index)
+			.then(() => null, error => donotNotifyError ? TPromise.wrapError(error) : this.handleWorkspaceConfigurationEditingError(error));
 	}
 
 	public addFolders(foldersToAdd: IWorkspaceFolderCreationData[], donotNotifyError: boolean = false): TPromise<void> {