Skip to content

Commit

Permalink
Unexpected events for single updateWorkspaceFolders operation (fixes #…
Browse files Browse the repository at this point in the history 
…42639)
  • Loading branch information
@bpasero
bpasero committed Feb 5, 2018
1 parent 73e6193 commit e9650bd
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 65 deletions.
126 changes: 65 additions & 61 deletions src/vs/workbench/services/configuration/node/configurationService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 {
Expand All @@ -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);
}

Expand Down Expand Up @@ -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);
Expand All @@ -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));
}

Expand Down Expand Up @@ -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;
Expand Down
49 changes: 49 additions & 0 deletions src/vs/workbench/services/configuration/test/node/configurationService.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
21 changes: 17 additions & 4 deletions src/vs/workbench/services/workspace/node/workspaceEditingService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down

0 comments on commit e9650bd

Please sign in to comment.