Properly format file path on when dragging and dropping a tab into the integrated terminal in Windows (version 2)#31234

rianadon · 2017-07-22T00:49:21Z

This is a rewrite of #30070 to make use of the new changes to the codebase since the previous PR.

mention-bot · 2017-07-22T00:49:23Z

@rianadon, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Tyriar and @dbaeumer to be potential reviewers.

Tyriar · 2017-07-22T01:14:16Z

src/vs/workbench/parts/terminal/electron-browser/terminalPanel.ts

+// List of regular expressions, for matching shells, functions to format a path, and booleans indicating whether the shell uses Windows-style paths
+const WIN_PATH_FORMATTERS: [RegExp, (path: string) => string, boolean][] = [
+	// WSL bash
+	[/^C:\\Windows\\(System32|sysnative)\\bash(.exe)?$/i, path => '/mnt/' + path[1] + path.substring(3), false],


Not sure if WSL bash is available in WoW64, might want to add |SysWoW64 too?

Tyriar · 2017-07-22T01:15:44Z

src/vs/workbench/parts/terminal/electron-browser/terminalPanel.ts

 import URI from 'vs/base/common/uri';

+// List of regular expressions, for matching shells, functions to format a path, and booleans indicating whether the shell uses Windows-style paths
+const WIN_PATH_FORMATTERS: [RegExp, (path: string) => string, boolean][] = [


Let's use an interface instead of an array so it's nicer to use:

interface IPathFormatter { regex: RegExp, format: (path: string) =>string, usesWindowsStylePaths: boolean } const WIN_PATH_FORMATTERS: IPathFormatter[] = [...]

An interface would be nicer but it does add some verbosity to WIN_PATH_FORMATTERS as the array elements would be switched from arrays to objects. Would doing something like:

type PathFormatter = [Regexp, (path: string) => string, boolean]; const WIN_PATH_FORMATTERS: PathFormatter[] = [...]

be any better? Or given I haven't really seen this used in VS Code would it be better to stick with interfaces?

Tyriar · 2017-07-22T01:21:13Z

src/vs/workbench/parts/terminal/electron-browser/terminalPanel.ts

+		if (platform.isWindows) {
+			const terminal = this._terminalService.getActiveInstance();
+			const shell = terminal.getShellName();
+			for (const [format, formatter, isWinPath] of WIN_PATH_FORMATTERS) {


Let's use WIN_PATH_FORMATTERS.forEach( to keep it consistent with the rest of the code.

Okay.

Is the following pattern used at all?

WIN_PATH_FORMATTERS.forEach(formatArray => { const [format, formatter, isWinPath] = formatArray; ... }

Or if I switched to interfaces I wouldn't have to worry about this.

Actually never mind. The regular expression for WSL bash is becoming quite long, so I may as well use an interface to split up the line.

I don't think using forEach will work because I need to break out of the loop. Either I would need to nest an if inside of it which would get ugly or use some other method (such as Array.prototype.find, but it seems the typescript target is set to es5 so that's not an option).

Tyriar · 2017-07-22T01:21:59Z

src/vs/workbench/parts/terminal/electron-browser/windowsShellHelper.ts

 			// We only go one level deep when checking for children of processes other then shells
 			if (SHELL_EXECUTABLES.indexOf(path.basename(result[0].executable)) === -1) {
-				return TPromise.as(result[0].executable);
+				return TPromise.as({ program: result[0].executable, shell: parent });


Perfect 👌

Tyriar · 2017-07-22T01:24:09Z

src/vs/workbench/parts/terminal/electron-browser/windowsShellHelper.ts


 	/**
-	 * Returns the innermost shell executable running in the terminal
+	 * Updates innermost shell executable and innermost shell running in the terminal


This comment is a little vague, something like this might be better:

Queries the OS and gets the inner-most shell running in the terminal as well as the program the shell is running if it exists.

Yeah that sounds better.

Tyriar · 2017-07-22T01:24:42Z

src/vs/workbench/parts/terminal/electron-browser/windowsShellHelper.ts

+	/**
+	 * Returns the innermost program executable running in the terminal
+	 */
+	public getProgramName(): string {


Let's make these getters:

public get programName(): string { return this._programName; }

You can then refer to them as if they were properties.

Oh yeah I forgot that was a thing! Is having this on a single line or multiple lines preferred?

Tyriar · 2017-07-22T01:25:55Z

src/vs/workbench/parts/terminal/electron-browser/windowsShellHelper.ts

-			this.refreshShellProcessTree(this._childProcessIdStack[this._childProcessIdStack.length - 1], null).then(result => {
-				resolve(result);
+		return new TPromise<void>((resolve) => {
+			this.refreshShellProcessTree(this._childProcessIdStack[this._childProcessIdStack.length - 1], this._shellName !== this._programName ? this._shellName : null).then(result => {


Let's pull this._shellName !== this._programName ? this._shellName : null into a variable so it's easier to read

rianadon · 2017-07-25T19:33:34Z

@Tyriar I'm not sure if you received any notifications from me responding to your comments or have seen them yet, but if you haven't I'm letting you know that I responded to them and had a few questions 😃.

rianadon · 2017-08-27T00:21:36Z

I've revised the PR do use the new native module. However the module only gives the executable name and not the full path so it can't differentiate between WSL bash and Git bash.

Tyriar · 2017-08-30T15:40:47Z

@rianadon sorry about the delay, I'll try get to this over the next couple of weeks 😃

rianadon · 2017-08-31T06:24:05Z

No problem. Since this is anyways going to have to be ported over to use node-pty when it gains support for listing processes on Windows, there isn't any rush to merge or review this.

Tyriar · 2018-05-19T15:18:35Z

Unfortunately this has become a bit too slate (my fault) so I'll have to close it. I am planning on improving WSL/Windows in general in the coming months so maybe this could be tackled then too.

Use WindowsShellHelper to format paths

c869807

msftclas added the cla-already-signed label Jul 22, 2017

rianadon mentioned this pull request Jul 22, 2017

Properly format file path on when dragging and dropping a tab into the integrated terminal in Windows #30070

Merged

Tyriar self-assigned this Jul 22, 2017

Tyriar requested review from Lixire and Tyriar July 22, 2017 01:12

Tyriar suggested changes Jul 22, 2017

View reviewed changes

Tyriar added this to the August 2017 milestone Jul 31, 2017

rianadon added 3 commits August 26, 2017 13:37

Merge with upstream

e8e3828

Update method names after merge

8505689

Make programName and shellName getters

a3a3f6d

rianadon mentioned this pull request Aug 27, 2017

Give full executable path for processes microsoft/vscode-windows-process-tree#7

Closed

rianadon added 4 commits August 26, 2017 21:44

Add other wsl paths

259acf1

Make comment clearer

f19aa53

Fix comments (again)

f637bfb

Fix whitespace issues

8567b98

Tyriar modified the milestones: August 2017, September 2017 Aug 28, 2017

msftgits removed the cla-already-signed label Sep 26, 2017

microsoft deleted a comment from msftclas Sep 27, 2017

Tyriar modified the milestones: September 2017, October 2017 Sep 29, 2017

Tyriar removed this from the October 2017 milestone Oct 30, 2017

Tyriar added this to the Backlog milestone Oct 30, 2017

Tyriar closed this May 19, 2018

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

Conversation

rianadon commented Jul 22, 2017

Uh oh!

mention-bot commented Jul 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rianadon commented Jul 25, 2017

Uh oh!

rianadon commented Aug 27, 2017

Uh oh!

Tyriar commented Aug 30, 2017

Uh oh!

rianadon commented Aug 31, 2017

Uh oh!

Tyriar commented May 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants