Skip to content

🐛 Fix repeated CWD entries when creating new terminal in multi-root workspace #153204

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

Merged

Conversation

babakks
Copy link
Contributor

@babakks babakks commented Jun 25, 2022

This PR fixes #153125

Tests to verify the correct dropping of repeated CWDs were added.

Screenshot from 2022-06-25 17-31-18

In the screenshot:

  • a multi-root directory workspace with four roots is activated. You can clone the test workspace from babakks/vscode-test-multi-root-workspace.
  • Overridden in dir-a/.vscode/settings.json, the CWD for dir-a is set to ../dir-b. Due to this, dir-a was dropped from the picker list.
  • Overridden in dir-d/.vscode/settings.json, the CWD for dir-d is set to .. (parent directory). Since just showing dir-d and its path in the picker makes no sense to the user, it is shown with an "(Overridden)" indicator followed by the actual CWD to clarify the user.

Since the PICK_WORKSPACE_FOLDER_COMMAND is a general-purpose functionality which is used in different places, I opted to locally implement the picker within the terminal module.

@Tyriar Although the fix seems to work, there are two other instances in the codebase where the CWD picker does not respect the terminal.integrated.cwd parameter. Please check these and tell me whether to fix them.

terminalActions.ts#L62:

case 'workspaceRoot':
	if (folders !== undefined && commandService !== undefined) {
		if (folders.length === 1) {
			return folders[0].uri;
		} else if (folders.length > 1) {
			// Only choose a path when there's more than 1 folder
			const options: IPickOptions<IQuickPickItem> = {
				placeHolder: localize('workbench.action.terminal.newWorkspacePlaceholder', "Select current working directory for new terminal")
			};
			const workspace = await commandService.executeCommand(PICK_WORKSPACE_FOLDER_COMMAND_ID, [options]);
			if (!workspace) {
				// Don't split the instance if the workspace picker was canceled
				return undefined;
			}
			return Promise.resolve(workspace.uri);
		}
	}
	return '';

terminalActions.ts#L2453:

const folders = workspaceContextService.getWorkspace().folders;
if (folders.length > 1) {
	// multi-root workspace, create root picker
	const options: IPickOptions<IQuickPickItem> = {
		placeHolder: localize('workbench.action.terminal.newWorkspacePlaceholder', "Select current working directory for new terminal")
	};
	const workspace = await commandService.executeCommand(PICK_WORKSPACE_FOLDER_COMMAND_ID, [options]);
	if (!workspace) {
		// Don't create the instance if the workspace picker was canceled
		return;
	}
	cwd = workspace.uri;
}

babakks added 4 commits June 25, 2022 13:03
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
…er picker command)

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks babakks marked this pull request as ready for review June 25, 2022 13:37
@Tyriar Tyriar added this to the July 2022 milestone Jun 27, 2022
@Tyriar Tyriar modified the milestones: July 2022, August 2022 Jul 19, 2022
meganrogge
meganrogge previously approved these changes Sep 9, 2022
@meganrogge meganrogge requested a review from Tyriar September 9, 2022 21:28
@meganrogge meganrogge removed their assignment Oct 6, 2022
@Tyriar Tyriar modified the milestones: October 2022, November 2022 Oct 24, 2022
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Works well 👍 sorry about the delay getting to this

@Tyriar Tyriar enabled auto-merge November 8, 2022 19:09
@Tyriar Tyriar merged commit 2cd1bf6 into microsoft:main Nov 8, 2022
@babakks babakks deleted the fix-repeated-cwd-for-new-terminal-picker branch November 9, 2022 09:12
@github-actions github-actions bot locked and limited conversation to collaborators Dec 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vscode multi-root workspace: open terminal asks me to choose cwd even if both have been set to same place
3 participants