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

Implement multiroot Open Folder command #10357

Merged
merged 6 commits into from
Jan 21, 2022

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Oct 29, 2021

What it does

Fixes #10334 by adding handling for multiselection to the Open Folder... command.
Fixes #10366 by implementing resolveToAbsolute for URI's.

How to test

  1. Use the 'Open Folder...' command.
  2. Select multiple folders.
  3. Observe that a new multi-root workspace is created and opened.
  4. It should work from either an existing workspace or a no-workspace state.

Review checklist

Reminder for reviewers

Signed-off-by: Colin Grant [email protected]

@colin-grant-work colin-grant-work force-pushed the feature/multiselect-open-workspace branch 2 times, most recently from 0811a1b to 4aecba6 Compare November 1, 2021 17:42
@colin-grant-work colin-grant-work marked this pull request as ready for review November 1, 2021 17:43
@kenneth-marut-work
Copy link
Contributor

Looks great, though I found a case where I'm getting some odd behavior

  • Start with an empty workspace (close your existing workspace if you need to)
  • Open Folder and select multiple folders (this will create the untitledWorkspace)
  • Don't save the workspace
  • Open Folder and select some new folders
  • Observe that a second window opens with a workspace consisting of the new folders, but the first window also changes its workspace to the second window's workspace

I believe that VSCode doesn't actually launch a second instance. You're first prompted to save your untitled workspace then it just swaps out the first workspace for the second

@colin-grant-work
Copy link
Contributor Author

Looks great, though I found a case where I'm getting some odd behavior

  • Start with an empty workspace (close your existing workspace if you need to)
  • Open Folder and select multiple folders (this will create the untitledWorkspace)
  • Don't save the workspace
  • Open Folder and select some new folders
  • Observe that a second window opens with a workspace consisting of the new folders, but the first window also changes its workspace to the second window's workspace

I believe that VSCode doesn't actually launch a second instance. You're first prompted to save your untitled workspace then it just swaps out the first workspace for the second

Thanks for checking it out. I have pushed a patch that should handle this better.

@colin-grant-work
Copy link
Contributor Author

@vince-fugnitto, @paul-marechal, to fix #10366 I've added an implementation of Path.resolve to our Path class, which seems in the spirit of that class: it's probably mostly fine, but it's a bit weird that we have it.

Alternatively, we could follow VSCode and just include the NodeJS Path package in our browser packages.

Let me know what you think.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking it out. I have pushed a patch that should handle this better.

I just retested it but was still able to reproduce the issue in another configuration:

  1. Open a multi-root workspace
  2. Open a new window
  3. Open another multi-root workspace using Open Folder in that new window
  4. The old window now uses the new multi-root workspace as well

@colin-grant-work
Copy link
Contributor Author

Thanks for checking it out. I have pushed a patch that should handle this better.

I just retested it but was still able to reproduce the issue in another configuration:

  1. Open a multi-root workspace
  2. Open a new window
  3. Open another multi-root workspace using Open Folder in that new window
  4. The old window now uses the new multi-root workspace as well

Fair enough. The problem is that we always generate the same untitled workspace. VSCode uses a time-stamped system so that every new untitled workspace is distinct, presumably with some cleanup when you leave the workspace behind. I will implement something similar.

@vince-fugnitto vince-fugnitto added the multi-root issues related to multi-root support label Nov 3, 2021
@colin-grant-work colin-grant-work force-pushed the feature/multiselect-open-workspace branch 3 times, most recently from 3e25ea9 to bea3dd0 Compare December 16, 2021 22:00
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me:

  • Enables selecting multiple folders when running Open Folder
  • Automatically creates an untitled workspace
  • Displays a dialog asking to save untitled workspaces

The untitled workspace always containing a suffix (in the file explorer) isn't optimal, but I don't see a better way around that right now to prevent overwriting other untitled workspaces. I do have one suggestion, but that doesn't prevent this from getting merged.

packages/workspace/src/browser/workspace-service.ts Outdated Show resolved Hide resolved
@colin-grant-work colin-grant-work force-pushed the feature/multiselect-open-workspace branch 2 times, most recently from 9d87e39 to cfe3ee9 Compare January 5, 2022 21:54
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colin-grant-work I noticed something:

  • if no workspace is present, and we perform the open folder... with multiple folders, the dialog appears (which occurs on unload), selecting don't save will close the dialog and restart the application without a workspace present. In vscode this same use-case will open the untitled workspace which was my expectation as a user.
no-workspace.mp4

@colin-grant-work colin-grant-work force-pushed the feature/multiselect-open-workspace branch from cfe3ee9 to 908aed9 Compare January 11, 2022 22:48
@colin-grant-work
Copy link
Contributor Author

@vince-fugnitto, thanks for the review and good catch. I've updated the code not to delete an untitled workspace file on exit, and instead added code to the backend WorkspaceServer that deletes untitled workspaces files that aren't in the top 10 most recent workspaces used. It's not foolproof: if you've kept an untitled workspace open while you've opened a bunch of other workspaces, and then your backend goes down and you have to restart, then the logic would delete that file even though it's still in use. If you have any suggestions for how to perform some cleanup without the risk of deleting a file in use, I'm happy to hear them.

@colin-grant-work colin-grant-work merged commit 96ff4ee into master Jan 21, 2022
@colin-grant-work colin-grant-work deleted the feature/multiselect-open-workspace branch January 21, 2022 16:14
@github-actions github-actions bot added this to the 1.22.0 milestone Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multi-root issues related to multi-root support
Projects
None yet
4 participants