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

Support "workspace folders" that are sub-folders of an existing open folder #45399

Closed
DanTup opened this issue Mar 9, 2018 · 22 comments
Closed
Labels
feature-request Request for new features or functionality workbench-multiroot Multi-root (multiple folders) issues

Comments

@DanTup
Copy link
Contributor

DanTup commented Mar 9, 2018

my_repo_folder/   <-- WorkspaceFolder
  |- bin/
      |- ...
  |- docs/
      |- ...
  |- examples/
      |- example_app_1/   <-- WorkspaceFolder
        |- .vscode/
        |- ...
      |- example_app_2/   <-- WorkspaceFolder
        |- .vscode/
        |- ...
  |- lib/
      |- ...
  |- tools/
      |- ...
  |- pubspec.yaml
  |- readme

Multiple projects open in Code has been an ongoing problem for Dart Code users. Users want to be able to open a folder that contains projects (eg. Flutter mobile apps) in sub-folders (or even example projects as sub-folders) however there are a bunch of complications, such as:

  1. Detection of which folders are "projects" means walking the whole tree detecting them (eg. which are projects, and of those which are Dart command line and which are Flutter) and it's incredibly complicated to then detect changes in them (I've recently had a go at doing this, but I was basically re-implementing multi-root workspaces in my own way, and it was still flawed in a few ways)
  2. launch.json/settings.json cannot be set for each "project"
  3. Debugger launching is complicated (we have the wrong root, can't detect which debugger entry point)

I tried to avoid fixing this for a long time telling my users to wait for and use multi-root workspaces when they arrive, however they're not very happy with this for a few reasons:

  1. It's clumsy to set up (they have to File -> Add Folder to Workspace for each folder - even though it's already in their explorer tree)
  2. It pulls those folders up to the top level so it's not the folder structure on disk/in source control that they're used to
  3. Those folders and files now exist multiple times in the explorer (which is both pointless and confusing)
  4. I don't well-believe it's defined which WorkspaceFolder we would get for a resource using getWorkspaceFolder if it exists in multiple roots

I've been trying to work around this and raised a couple of requests to try and handle this better (for #43902 and #45220, neither of which would be needed with this feature) but I think a better option is to support marking folders within a tree as workspace folders.

Here's how I think it could work:

  • A new context menu entry on folders in explorer - Mark folder as Workspace Folder
    This treats a folder as if it was added as a workspace folder but does not duplicate it at the root in the tree. It gets a slightly different icon to show it's a workspace folder (I think "project" is a better name, but I think Workspace folder is already somewhat stuck). This will fire onDidChangeWorkspaceFolders when the user sets/unsets it
  • Extensions can easily mark folders as workspace folders during activation (eg. they can find package.json/pubspec.yaml/app.config) - this should not cause a "dirty" workspace - it should quietly write into the workspace file (or settings file if only a single folder is open)
  • VS Code automatically marks any folder that contains a .vscode sub-folders as a WorkspaceFolder when opening a folder/workspace
  • These folders are treated as any other WorkspaceFolder - they appear in workspaceFolders, appear in the debug config drop-downs, are returned from getWorkspaceFolder, can have their own .vscode folder with settings and launch configs (and getting resource-level settings will cascade up the tree)

I think this would be a great change for code even outside of Dart/Flutter. It's really natural to have a repository with multiple "projects" in it and it's most natural to open the root. As far as I can tell there is nothing inherent in the WorkspaceFolder APIs that say they cannot appear within a tree, so I don't think is a big breaking change.

@vscodebot
Copy link

vscodebot bot commented Mar 9, 2018

(Experimental duplicate detection)
Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

@bpasero
Copy link
Member

bpasero commented Mar 9, 2018

@DanTup how is this issue different from #43902 ?

@DanTup
Copy link
Contributor Author

DanTup commented Mar 9, 2018

The idea in that one was, given the following structure:

MyRepoFolder/
  |- Sub Folder A/
  |- Sub Folder B/
  |- Sub Folder C/

By choosing to open "MyRepoFolder" would actually just open the three sub folders as workspace folders (so there is a 1-to-1 mapping of sub-folders to workspace folders, and the parent folder wouldn't actually appear in Code). This is pretty restrictive (you can't see the parent folder to access files in the root, it breaks the structure if your projects are not all at the same level, it requires you to open the project in a special way). What' I'd like to support is:

my_repo_folder/   <-- WorkspaceFolder
  |- bin/
      |- ...
  |- docs/
      |- ...
  |- examples/
      |- example_app_1/   <-- WorkspaceFolder
        |- .vscode/
        |- ...
      |- example_app_2/   <-- WorkspaceFolder
        |- .vscode/
        |- ...
  |- lib/
      |- ...
  |- tools/
      |- ...
  |- pubspec.yaml
  |- readme

The idea is you open up MyRepoFolder and the user or extension can mark some folders (at any level of the tree) as WorkspaceFolders. If Code finds a .vscode folder anywhere in the tree it could automatically mark them as a WorkspaceRoot (note: they do not get copied up to top level, they just stay in the tree with a special icon or something). A common pattern in Flutter/Dart is to have an example folder (or examples with sub-folders) that are full-fledged examples. They should be able to contain launch.jsons and settings.jsons.

This allows folders anywhere in the tree to get their own settings, launch configs, debugger launches etc.

Since raising this I found a bunch of issues (a few are shown above because I commented on them, though others were locked) where people had raised issues that could be handled by this (for ex. not detecting tasks from package.jsons in sub-folders, etc.).

It seems like the restriction of WorkspaceFolders having to be top-level in the tree shouldn't be required. Allowing them at any level within the tree makes things much more flexible.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 9, 2018

I've edited in a better example - using an examples sub-folder to show it could be at any level, though the most common scenario would be top-level folders. It feels wonky in my example to make the example apps roots, as they'll appear twice in the tree. In the common case (the project folders are top-level in the repo) it's still wonky, because if you open only the project folders you can't access the readme/gitignore/etc. (and if you open the repo root and then add each sub-folder as a project, you have loads of duplication in the trees).

@bpasero bpasero added the feature-request Request for new features or functionality label Mar 9, 2018
@bpasero bpasero removed their assignment Mar 9, 2018
@bpasero
Copy link
Member

bpasero commented Mar 9, 2018

@DanTup it seems to me all you want is that settings, launch and task configuration would be supported on any sub-folder location, is that true? Because if so, I would actually move this over to configuration land and make it independent from multi-root.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 9, 2018

@bpasero It's important to get the right "project folder" for many things, so being able to use getWorkspaceFolder(uri) and get that correct folder is important. If settings/launch/tasks/debug all start doing their own thing, it feels like they'll somewhat undermine WorkspaceFolders?

Ultimately, how it's achieved is less important to me than it works, but as things are I'm not I can do what I need without an equiv of getWorkspaceFolder that could get me the correct folder (eg. if the user has a file open and hits F5, how do I figure out the project type from its file path?).

I could achieve some of it by tracking an array of "project folders" myself, but I've tried doing that on a branch and it's turning out to be a lot of work (for ex., being able to track when a new folder becomes a project/gets deleted/etc.) and have hit issues with things like the debugging.

I thought this feature was a nice way to handle the whole lot and seemed to be useful to users - but if you propose another way, I will see if I can figure out whether it falls down anywhere for what I need. (it's likely there could be more smaller items like #45220 to get everything we need).

@bpasero
Copy link
Member

bpasero commented Mar 9, 2018

@DanTup so in #43902 you want each direct subfolder of an opened folder to become a workspace folder independent from .vscode folder within and here you want any subfolder with a .vscode folder to become a workspace folder?

I think this could be done from an extension with the new workspace API.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 9, 2018

so in #43902 you want each direct subfolder of an opened folder to become a workspace folder

#43902 was a bad attempt at trying to solve this problem, I'm happy for it to be rejected, I don't think it's a good solution anymore :)

here you want any subfolder with a .vscode folder to become a workspace folder?

I want to support any folder in the tree being capable of being a workspace folder (which means it can have its own .vscode folder, though it's not necessary - the same today you can add another workspace folder without having to put a .vscode folder in it - though it'll cause it to be shown at the top level).

I think this could be done from an extension with the new workspace API.

I think the problems with this are:

  • The child folders will be duplicated in the tree (where they normally are, and also again as a root), not presented as the user sees them on-disk of in source control
  • I don't think it's defined/documented what calls to getWorkspaceFolder for resources that are in two workspace folders (the top one, and the child one)
  • Extensions adding folders will result in a dirty workspace prompting the user to save every time they close the window
  • The user can't easily promote a folder to a root (there's no "Add folder to workspace" on the context menu within the explorer)
  • (Minor rant: the API for removing workspace folders is kinda wonky (I mentioned this in Add a command to add a folder to a workspace #43674) - for ex. adding/removing multiple folders that are not consecutive or re-ordering is a multi-step process and you have to wait for onDidChangeWorkspaceFolders to fire in the middle)

@bpasero
Copy link
Member

bpasero commented Mar 9, 2018

The child folders will be duplicated in the tree (where they normally are, and also again as a root), not presented as the user sees them on-disk of in source control

Possible with a files.exclude setting per folder, but a bit ugly.

I don't think it's defined/documented what calls to getWorkspaceFolder for resources that are in two workspace folders (the top one, and the child one)

It is. Always the closest one.

Extensions adding folders will result in a dirty workspace prompting the user to save every time they close the window

True, this would probably only make sense in a saved workspace.

The user can't easily promote a folder to a root (there's no "Add folder to workspace" on the context menu within the explorer)

I think we could have that independently.

(Minor rant: the API for removing workspace folders is kinda wonky (I mentioned this in #43674) - for ex. adding/removing multiple folders that are not consecutive or re-ordering is a multi-step process and you have to wait for onDidChangeWorkspaceFolders to fire in the middle)

Simple: you remove all folders and add all folders the way you want. We take care of the diff and do the right thing.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 9, 2018

Possible with a files.exclude setting per folder, but a bit ugly.

Yeah, and would only work the opposite way to the user wants (keeps the top-levels, hides the children)?

It is. Always the closest one.

Ah, that's good to know. Someone told me something didn't work that I assumed was because this was returning the wrong thing - I'll dig into it and raise a bug if it seems to do the wrong thing anywhere.

True, this would probably only make sense in a saved workspace.

Yeah, in this case I really want it to be transparent to the user. The user just wants to open a repo and have us do the right thing. Currently users keep asking why they can't debug their project in a sub-folder and it sounds kind ridiculous that I can't fix it (it needs #45220 - probably it didn't before I merged my debug types, but that was done for other good reasons).

Simple: you remove all folders and add all folders the way you want. We take care of the diff and do the right thing.

Ah cool, didn't realise I could remove and add the same folders in one operation. Will this cause a reload, or can it be done sneakily without prompting the user or affecting open files/etc.?

@bpasero
Copy link
Member

bpasero commented Mar 9, 2018

Yeah, and would only work the opposite way to the user wants (keeps the top-levels, hides the children)?

I am not sure I get that, but the real issue is that this setting is stored inside the folder but it only makes sense outside the folder (when opened as part of that workspace), so not a very good solution...

Ah, that's good to know. Someone told me something didn't work that I assumed was because this was returning the wrong thing - I'll dig into it and raise a bug if it seems to do the wrong thing anywhere.

I assume you refer to this method: https://github.com/Microsoft/vscode/blob/master/src/vs/vscode.d.ts#L5444. We might need to tweak the JSDoc to clarify this and/or fix bugs if there are any.

Yeah, in this case I really want it to be transparent to the user. The user just wants to open a repo and have us do the right thing. Currently users keep asking why they can't debug their project in a sub-folder and it sounds kind ridiculous that I can't fix it (it needs #45220 - probably it didn't before I merged my debug types, but that was done for other good reasons).

Have you thought about just checking in a code-workspace file into that repo that uses relative paths and is ready to go? Then tell users to just open that workspace file?

Ah cool, didn't realise I could remove and add the same folders in one operation. Will this cause a reload, or can it be done sneakily without prompting the user or affecting open files/etc.?

It will only cause a reload if the first folder changes or if you transition from 0/1-folder into a workspace, but that is independent from that method, that is just the way how we support backwards compatibility. The method will do a diff of the folders that changed and then do the minimal update and the minimal set of events.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 9, 2018

I am not sure I get that

Maybe I misunderstood, but I thought that your solution would hide the folders from inside the "main" (grandparent) root and than have it as its own root. This would mean it's displayed at the wrong place - the user wants to see it in its natural placing in the tree:

my_repo_folder/
  |- bin/
  |- docs/
  |- examples/
      |- example_app_1/   <-- WorkspaceFolder
        |- ...
      |- example_app_2/   <-- WorkspaceFolder
        |- ...

I assume you refer to this method

Yep, I do. I'll investigate and file bugs if I think it's not picking the closest one.

Have you thought about just checking in a code-workspace file into that repo

This isn't for a specific repo, it's for users own projects. It's more common than I expected for them to have Flutter projects in sub-folders and they're confused that it doesn't just work (on the surface it sounds simple, but when there are a bunch of commands - like package-restore - that need to be run at the right place, it gets complicated).

It will only cause a reload if the first folder changes or if you transition from 0/1-folder into a workspace

Ah, that would suck. User opens a folder - we find the pubspec.yamls and then try to add those folders as workspaces and they see an immediate reload right after they opened a folder.

That said, I think the "I don't want sub-folders of my tree appearing at the top-level" is the biggest issue.

@bpasero
Copy link
Member

bpasero commented Mar 9, 2018

Maybe I misunderstood

If I got it right you want to hide those folders from the grand-parent that appear as workspace folders. The more I think about it the more I think this should maybe be just a multi-root related setting and not something that should go into the individual folders. Because this setting should only be defined on the level of the workspace file and not within folders. Something to control if any workspace folder should appear in the parent folder if the parent is a workspace folder too.

This isn't for a specific repo, it's for users own projects.

I think depending on the scenario, some things should work even without multi-root workpace. E.g. we should scan for launch.json and task.json and maybe offer the user to run these even if none of the folders are workspace folders. Same goes for folder settings. What is the advantage of promoting these folders to be actual workspace folders? Is it so that the extension can properly deal with them?

Ah, that would suck. User opens a folder - we find the pubspec.yamls and then try to add those folders as workspaces and they see an immediate reload right after they opened a folder.

To be clear, this is a reload of the extension host, not the window. I do not think a user would notice.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 9, 2018

If I got it right you want to hide those folders from the grand-parent that appear as workspace folders.

I'm still confused, but I think I might want the opposite. I do not want them to appear as new top-level items in explorer. I want them to continue to appear where they would if they were not workspace folders (eg. deep in the tree). I want what explorer shows to match what's on disk/in git and not have some folders magically promoted up to the top because they are projects.

So in my previous "ascii drawings", the example apps should remain where they were there, but those folders would exist in workspace.workspaceFolders, be returned by getWorkspaceFolder(Uri) and be the workspace given to things like DebugConfigurationProvider.debugAdapterExecutable.

What is the advantage of promoting these folders to be actual workspace folders? Is it so that the extension can properly deal with them?

Yeah, there are two main reasons:

  1. There are lots of really useful APIs around workspace folders (getWorkspaceFolder(Uri), onDidChangeWorkspaceFolders, showWorkspaceFolderPick) and re-implementing them all and maintaining my own list of "project folders" has turned out to be complicated (and some I can't reproduce - like showQuickPick is too limited to replace showWorkspaceFolderPick - though I've raised another issue about that)
  2. There are some APIs that I just can't fix (such as the debugging one above - when I'm asked which debug adapter executable to use, I get given a WorkspaceFolder but that might actually have multiple different project types in it)

It's possible to do this another way - most likely it would involve some small tweaks/fixes around the place. I just thought the concept of a WorkspaceFolder extending to sub-folders was a good fit for it (because we could take advantage of a bunch of existing stuff - for ex. when a user wants to package restore, asking them which workspaceFolder would be a natural fit - if it was actually the project folders).

If the intention for multi-root is solely to support multiple folders from different locations rather than to introduce something that resembles "project folders" then fair enough, but I think the concept of "project folders" that can be anywhere in the tree and used as the root for various things (like debugging launching, executing commands that want a "project root" etc, having their own .vscode folders for settings/tasks/etc.) would be a good one.

@bpasero
Copy link
Member

bpasero commented Mar 9, 2018

I'm still confused, but I think I might want the opposite.

Ok got it. Yeah that could be yet another setting for the Explorer specifically to not promote workspace folders to be root folders. Though a bit weird to be honest... because this makes the UI somewhat inconsistent. We have many places where workspace folders show up as unit (e.g. search) and it seems you could want to hide all those places.

If the intention for multi-root is solely to support multiple folders from different locations rather than to introduce something that resembles "project folders" then fair enough, but I think the concept of "project folders" that can be anywhere in the tree and used as the root for various things (like debugging launching, executing commands that want a "project root" etc, having their own .vscode folders for settings/tasks/etc.) would be a good one.

I think the intention for multi-root is to allow to compose the workspace from many different folders that can have different roots. It is true that on top of that we have new APIs that make it easier to work with these folders. I also think that each workspace folder has more importance over any other folder, so you could argue that these folders become part of a higher concept (e.g. "projects").

I think your scenario can apply to any language and so I still think that only an extension for a specific language can understand the project nature of that language and that the extension should provide the functionality to convert the currently opened folder into a workspace.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 9, 2018

Yeah that could be yet another setting for the Explorer specifically to not promote workspace folders to be root folders. Though a bit weird to be honest... because this makes the UI somewhat inconsistent. We have many places where workspace folders show up as unit (e.g. search) and it seems you could want to hide all those places.

Actually, I think in other places treating them special is fine - if my_example_app_1 search results were shown separate from the main root, this wouldn't be a bad thing (as long as they're not also duped inside it), maybe even better - because it seems like their projects are being treated specially. It's only the explorer where I think it's bad, because the user expects that to mirror what's on their disk.

I think your scenario can apply to any language and so I still think that only an extension for a specific language can understand the project nature of that language and that the extension should provide the functionality to convert the currently opened folder into a workspace.

Yeah, I agree - I think the extension and/or the user should be able to set these. I'm not expecting Code to detect and promote things itself (I think if they have .vscode folders it might make sense, but it's not something that matters to me here).

So, let's say a setting is added that allows workspace roots to be hidden from the top-level of explorer if they are children of another workspace root (obviously if the parent root is removed, it should re-appear, etc.). I think the remaining concerns are:

  1. Having to reload the workspace when we set this up
    Not the end of world, I can prompt the user something like "Your workspace contains project in sub-folders, would you like to open as a multi-project workspace?" which could add all the folders and set the settings above at the workspace level (would need to make sure we can do both of these together)
  2. This results in a "dirty" workspace that would prompt the user on exit
    It'd be nice if we could auto-save this into the top-level .vscode folder for them so they don't get any prompt
  3. They'd have to repeat the prompt/reload process each time they opened this folder if they didn't do it from a .workspace file
    A possible fix for this would be that if there's a workspace saved in a .vscode folder then opening that folder automatically opens the workspace?

How does this sound?

(btw having #45220 and #45214 in the shorter term would at least allow me to fix my biggest outstanding issue where Flutter apps in sub-folders are just totally broken)

@bpasero
Copy link
Member

bpasero commented Mar 10, 2018

I have extracted #45470 for the explorer setting.

Having to reload the workspace when we set this up

Again, this is "just" an extension host reload, not a window reload, I do not think the user would notice.

This results in a "dirty" workspace that would prompt the user on exit

I think that is fine if the extension would have asked the user initially to convert into a multi-root workspace. The user can decide if he wants to stick with that workspace layout.

They'd have to repeat the prompt/reload process each time they opened this folder if they didn't do it from a .workspace file

We could offer API to save a workspace from an extension (possibly asking the user for the location). I think that would solve 2. and 3.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 10, 2018

I have extracted #45470 for the explorer setting.

SGTM! I've

Again, this is "just" an extension host reload, not a window reload, I do not think the user would notice.

Sorry, missed that in your previous comment, this should be great then :-)

I think that is fine if the extension would have asked the user initially to convert into a multi-root workspace. The user can decide if he wants to stick with that workspace layout.

Yeah, that's true. I think it'd be nice not to need the prompt, but we can get it working like this first and see what feedback we get.

We could offer API to save a workspace from an extension (possibly asking the user for the location). I think that would solve 2. and 3.

For 3, I was thinking more about people that go through this process (of converting to a workspace, and saving it into the .vscode folder) and then committing and pushing, and their teammates/whatever checking the repo out but just doing a File -> Open Folder. It'd be neat if just "opening the folder" actually opened the workspace.

I actually already have this niggle - I have some workspaces but often forget to open them and open the "main" folders instead - which means I'm missing some of the useful things I've normally added them.

Maybe it could be done with a setting and/or prompt? If you open a folder that has a single .code-workspace file inside its .vscode folder, it offers to open the workspace instead?

@bpasero
Copy link
Member

bpasero commented Mar 11, 2018

Maybe it could be done with a setting and/or prompt? If you open a folder that has a single .code-workspace file inside its .vscode folder, it offers to open the workspace instead?

Yeah maybe. This reminds me of the recommendation notification of extensions that can popup to guide you into installing useful extensions for a workspace. If there was a way to recommend workspaces within a folder to open, this could help.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 11, 2018

Yeah, exactly! "This folder contains a workspace, would you like to open that?". I think that'd be a neat feature (and if extensions could auto-save a workspace they set up into that folder, I think it addresses everything :-))

@DanTup
Copy link
Contributor Author

DanTup commented Mar 12, 2018

I don't think it's defined/documented what calls to getWorkspaceFolder for resources that are in two workspace folders (the top one, and the child one)

It is. Always the closest one.

Right, I found the issue I was having - it's not getWorkspaceFolder that's doing the wrong thing, but the debug configuration provider. I've opened #45580.

@DanTup
Copy link
Contributor Author

DanTup commented May 1, 2018

Closing this because I think the only significant thing left is covered by #45470. Our currently implementation works fine (with the noted exception that people don't like their projects being duplicated at the top level in the tree).

@DanTup DanTup closed this as completed May 1, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Jun 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality workbench-multiroot Multi-root (multiple folders) issues
Projects
None yet
Development

No branches or pull requests

2 participants