Provide additional workspace API to add/remove workspace folders (for #35407) #36820

bpasero · 2017-10-24T12:45:42Z

@jrieken here is the suggested API for adding/removing workspace folders, feedback welcome.

Note that I am still planning to tackle 2 outstanding issues independent of this PR to keep the discussion on the API shape only:

when removing a workspace folder from a single-folder workspace, we should probably turn the workspace into an empty one (same as File > Close Folder)
when adding a workspace folder while you are in empty/single-folder workspace we should transition into a workspace (same as we do today via File > Add Folder To Workspace...).

…35407)

jrieken · 2017-10-25T08:53:46Z

src/vs/vscode.d.ts

+		 *
+		 * @param folders a list of workspace folders to add.
+		 */
+		export function addWorkspaceFolders(folders: { uri: Uri, name?: string }[]): Thenable<void>;


Is adding many folders at once really a use-case? I'd expect this to be called for single folders in 99% of the cases. My proposal would be function addWorkspaceFolder(uri: Uri, name?: string)

Fixed via 158d152

jrieken · 2017-10-25T08:54:31Z

src/vs/vscode.d.ts

+		 *
+		 * @param folders a list of [workspace folders](#WorkspaceFolder) to remove.
+		 */
+		export function removeWorkspaceFolders(folders: WorkspaceFolder[]): Thenable<void>;


Same, N vs 1

Fixed via 158d152

jrieken · 2017-10-25T08:59:11Z

src/vs/workbench/api/node/extHostWorkspace.ts

@@ -78,6 +78,17 @@ export class ExtHostWorkspace implements ExtHostWorkspaceShape {
 		}
 	}

+	addWorkspaceFolders(folders: { uri: URI, name?: string }[]): Thenable<void> {
+		return this._proxy.$addFolders(folders);


Not yet decided if those changes should be reflected sync... We do that in some place, e.g the editor or Memento#update, where we assume optimistically that the change is good, then send to the main side which might cause another change... Needs some thinking.

@jrieken well in some of the cases this action will result in the extension host restarting, so in that case it does not really matter. For the other case of an already opened workspace, I think even with the current promise chaining there is a chance that the onDidChangeWorkspaceFolders has not yet fired when the method returns.

What is your suggestion?

jrieken · 2017-10-25T15:27:25Z

src/vs/workbench/api/node/extHostWorkspace.ts

+
+	removeWorkspaceFolder(folder: vscode.WorkspaceFolder): Thenable<void> {
+		if (this.getWorkspaceFolders().indexOf(folder) === -1) {
+			return Promise.resolve(undefined);


Maybe make this return a boolean to indicate success/failure. Might also be useful when having a user-confirmation in between. Then false could also mean blocked by the user

@jrieken yeah added via 1dc821b

bpasero · 2017-10-26T12:00:33Z

@jrieken the corner cases are now covered such as the API call will work in any workspace state (empty workspace, single-folder or multi-root workspace).

There is one remaining issue (related to your comment about running this sync or not): When the extension host needs to be restarted as a matter of the call (this can happen when adding a root folder while either no workspace is opened or when a single-folder is opened), the extension host goes down while executing the request and the result is an ugly error message:

The only way how to prevent this message is to run the workspace edit operation in a timeout and return to the extension directly without executing and waiting for the operation to be finished.

Undecided what to do here, I feel that executing the command sync and waiting for it is correct and we should maybe add code to not show this error message in that case.

jrieken · 2017-10-26T15:08:16Z

workspace edit operation in a timeout and return to the extension directly without executing and waiting for the operation to be finished.

Not nice... In the does-not-need-to-restart case we should ensure that the workspace change event happened in-between making the call and resolving the promise. Timeout-trick will likely break that. What of the call is causing the cancel-error? Is this us calling back to the extension host?

jrieken · 2017-10-26T15:08:26Z

src/vs/vscode.d.ts

@@ -5211,6 +5211,34 @@ declare module 'vscode' {
 		export const onDidChangeWorkspaceFolders: Event<WorkspaceFoldersChangeEvent>;

 		/**
+		 * Adds a workspac folder to the currently opened workspace.


via 95eccd6

bpasero · 2017-10-26T15:41:11Z

@jrieken the onDidChangeWorkspaceFolders is firing before the method returns with the current code. Agree that a timeout is just ugly.

The "Canceled" error seems to originate from rpcProtocol#dispose().

HutzlerGregg · 2017-10-26T16:16:15Z

Give as we doing and I'll.

jrieken · 2017-10-27T08:55:49Z

The "Canceled" error seems to originate from rpcProtocol#dispose().

Don't we handle those standard-cancellation-errors everywhere? And by "handle" I meaning "ignore"?

bpasero · 2017-10-30T07:00:40Z

@jrieken yeah looks like the commands list was not checking for that, I added it via 48c4511

Can we merge this in so that we have it for October or do you want more changes? I was still planning to look into the confirmation dialog thing but can do that outside of this PR.

bpasero · 2017-10-30T09:35:40Z

Added the confirm dialog via 42e6cd5 with a new setting workbench.confirmChangesToWorkspaceFromExtensions.

…35407) (#36820) * Provide additional workspace API to add/remove workspace folders (for #35407) * add/removeFolders => add/removeFolder * make add/remove folder return a boolean * use proper service for workspace editing * workspac => workspace * do not log promise canceled messages * show confirm dialog

Provide additional workspace API to add/remove workspace folders (for #…

4790e47

…35407)

bpasero added this to the October 2017 milestone Oct 24, 2017

bpasero assigned bpasero and jrieken Oct 24, 2017

bpasero requested a review from jrieken October 24, 2017 12:45

jrieken requested changes Oct 25, 2017

View reviewed changes

add/removeFolders => add/removeFolder

158d152

jrieken reviewed Oct 25, 2017

View reviewed changes

bpasero added 3 commits October 25, 2017 18:12

make add/remove folder return a boolean

1dc821b

Merge branch 'master' into ben/35407

9ca1653

use proper service for workspace editing

17fabe7

jrieken reviewed Oct 26, 2017

View reviewed changes

workspac => workspace

95eccd6

do not log promise canceled messages

48c4511

bpasero added 2 commits October 30, 2017 10:09

Merge branch 'master' into ben/35407

9ba2637

show confirm dialog

42e6cd5

bpasero merged commit 80ece09 into master Oct 30, 2017

bpasero deleted the ben/35407 branch October 30, 2017 13:49

github-actions bot locked and limited conversation to collaborators Mar 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide additional workspace API to add/remove workspace folders (for #35407) #36820

Provide additional workspace API to add/remove workspace folders (for #35407) #36820

bpasero commented Oct 24, 2017

jrieken Oct 25, 2017

bpasero Oct 25, 2017

jrieken Oct 25, 2017

bpasero Oct 25, 2017

jrieken Oct 25, 2017

bpasero Oct 25, 2017

jrieken Oct 25, 2017

bpasero Oct 25, 2017

bpasero commented Oct 26, 2017 •

edited

Loading

jrieken commented Oct 26, 2017

jrieken Oct 26, 2017

bpasero Oct 26, 2017

bpasero commented Oct 26, 2017

HutzlerGregg commented Oct 26, 2017

jrieken commented Oct 27, 2017 •

edited

Loading

bpasero commented Oct 30, 2017

bpasero commented Oct 30, 2017 •

edited

Loading

Provide additional workspace API to add/remove workspace folders (for #35407) #36820

Provide additional workspace API to add/remove workspace folders (for #35407) #36820

Conversation

bpasero commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero commented Oct 26, 2017 • edited Loading

jrieken commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero commented Oct 26, 2017

HutzlerGregg commented Oct 26, 2017

jrieken commented Oct 27, 2017 • edited Loading

bpasero commented Oct 30, 2017

bpasero commented Oct 30, 2017 • edited Loading

bpasero commented Oct 26, 2017 •

edited

Loading

jrieken commented Oct 27, 2017 •

edited

Loading

bpasero commented Oct 30, 2017 •

edited

Loading