Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

addWorkspaceFolder / removeWorkspaceFolder are not effective API #37301

Closed
alexdima opened this issue Oct 31, 2017 · 5 comments
Closed

addWorkspaceFolder / removeWorkspaceFolder are not effective API #37301

alexdima opened this issue Oct 31, 2017 · 5 comments
Assignees
Labels
api *duplicate Issue identified as a duplicate of another issue(s) feature-request Request for new features or functionality under-discussion Issue is under discussion for relevance, priority, approach workbench-multiroot Multi-root (multiple folders) issues
Milestone

Comments

@alexdima
Copy link
Member

Testing #37133

fyi @jrieken .


  • observe that a WorkspaceFolder has three fields: uri, name and index.
  • observe the reading API: export let workspaceFolders: WorkspaceFolder[] | undefined;

IMHO, the editing API seems out of place:

  • removeWorkspaceFolder takes as argument a single WorkspaceFolder; why a single one ?
  • addWorkspaceFolder also takes in a single WorkspaceFolder; why ? It also ignores the index property; how to add something at the beginning of the array ?

e.g. of difficulty:

  • how to implement replace folder at i with another folder. i.e. workspaceFolders[i] = newWorkspaceFolder:
    • remove N folders where folder.index >= i, i.e. call removeWorkspaceFolder N times
    • add N folders back again i.e. call addWorkspaceFolder N times

IMHO a much simpler API would be a setWorkspaceFolders(newWorkspaceFolders: WorkspaceFolder[])

@bpasero bpasero added this to the October 2017 milestone Oct 31, 2017
@bpasero
Copy link
Member

bpasero commented Oct 31, 2017

@egamma one idea is to put our new API to proposed API first to better understand the requirements from other extensions. Would that be ok?

@bpasero bpasero added the under-discussion Issue is under discussion for relevance, priority, approach label Oct 31, 2017
@bpasero
Copy link
Member

bpasero commented Nov 1, 2017

@jrieken @alexandrudima without having this coded up yet, what about this:

export function setWorkspaceFolders(folders: (WorkspaceFolder | { uri: Uri; name?: string; })[]): Thenable<boolean>;

Where a folder can either be an existing WorkspaceFolder or a new one.

It avoids having to introduce a separate createWorkspaceFolder method with all the associated issues and it is still long running as before. The downside I see is that we need to tackle the corner cases, e.g. someone could pass in the URI of an existing WorkspaceFolder but with a different name so we should update the name of that workspace folder I guess.

Another downside is that the simple case of adding or removing a single WorkspaceFolder is a lot more work for an extension author...

One interesting (new) challenge with this approach is that it would then be possible to go from 1 single-folder workspace to a multi-root workspace where the first folder is different, which is a transition that we do not support today without window reload because some extensions could be disabled in the single-folder case and no longer for the new first root folder. In that case we would still need to do a window reload.

@bpasero bpasero added api feature-request Request for new features or functionality workbench-multiroot Multi-root (multiple folders) issues labels Nov 1, 2017
@bpasero bpasero modified the milestones: October 2017, On Deck Nov 1, 2017
@jrieken
Copy link
Member

jrieken commented Nov 1, 2017

If full power is needed we should go for something splice'ish. Maybe updateWorkspaceFolders(index, deleteCount, add:{uri, name}[])

@bpasero
Copy link
Member

bpasero commented Nov 1, 2017

@jrieken yeah I like that for the reason of not having the ugly or-type between WorkspaceFolder and {uri, name}

@bpasero bpasero modified the milestones: On Deck, November 2017 Nov 2, 2017
@bpasero
Copy link
Member

bpasero commented Nov 3, 2017

Continues via #35407

@bpasero bpasero closed this as completed Nov 3, 2017
@bpasero bpasero added the *duplicate Issue identified as a duplicate of another issue(s) label Nov 3, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api *duplicate Issue identified as a duplicate of another issue(s) feature-request Request for new features or functionality under-discussion Issue is under discussion for relevance, priority, approach workbench-multiroot Multi-root (multiple folders) issues
Projects
None yet
Development

No branches or pull requests

3 participants