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.

Sign up for GitHub

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

Jump to bottom

Add PS Core in select default shell on windows#72425 #72689

Merged
merged 14 commits into from
May 11, 2019
Merged

Add PS Core in select default shell on windows#72425 #72689

Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1c483b1
Created function due to get app path from registry
roottool Apr 18, 2019
edff6f4
Deleted unnecessary variable
roottool Apr 18, 2019
0f119a2
Chenged the argument of _getAppPathFromRegistry
roottool Apr 19, 2019
029f329
Added comment for _getAppPathFromRegistry function
roottool Apr 20, 2019
f96955a
Changed a part of name from 'app' to 'shell'
roottool Apr 20, 2019
8778906
Deleted debug data
roottool Apr 20, 2019
8e465d0
Update comment
roottool Apr 20, 2019
2ffd468
Deleted newline
roottool Apr 20, 2019
9f0d0bd
Moved Registry position and changed a return value
roottool Apr 23, 2019
f50f83d
Refactored the code
roottool Apr 23, 2019
60dc18d
Removed a part of comment
roottool Apr 23, 2019
4d96541
Changed logic to convert from undefined to string
roottool Apr 23, 2019
7c0151a
Fix: Added a logic to check undefined
roottool Apr 28, 2019
ffcb9e6
Merge remote-tracking branch 'origin/master' into pr/roottool/72689
Tyriar May 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 27 additions & 1 deletion src/vs/workbench/contrib/terminal/electron-browser/terminalService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,30 @@ export class TerminalService extends BrowserTerminalService implements ITerminal
});
}

private _detectWindowsShells(): Promise<IQuickPickItem[]> {
/**
* Get the executable file path of shell from registry.
* @param Registry The data of imported from `vscode-windows-registry`
* @param shellName The shell name to get the executable file path
* @returns The executable file path of shell or `'ShellNotFound'`
*/
private _getShellPathFromRegistry(Registry: typeof import('vscode-windows-registry'), shellName: string): string {
Copy link
Member

Choose a reason for hiding this comment

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

I think this function should be a little more targetted to just pwsh for now and then return either [] or [ 'path' ]

const shellNotFound = 'ShellNotFound';
let shellPath;

try {
shellPath = Registry.GetStringRegKey('HKEY_LOCAL_MACHINE', `SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\App Paths\\${shellName}.exe`, '');
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified:

try {
  return (grab from reg);
} catch () {
}
return [];

Copy link
Author

Choose a reason for hiding this comment

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

Registry.GetStringRegKey() function returns string or undefined.
https://github.com/Microsoft/vscode/blob/3a12b7ac2efd2f1f01a646ef5272313ad50bf618/src/typings/vscode-windows-registry.d.ts#L8
Therefore, this function needs a logic to convert from undefined to [].
But I referred your review when I fixed the code.

Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

I realized that using ! makes the logic unnecessary. So I deleted the logic.

sorry for the trouble!

} catch (e) {
shellPath = shellNotFound;
}

if (shellPath === undefined) {
shellPath = shellNotFound;
}

return shellPath;
}

private async _detectWindowsShells(): Promise<IQuickPickItem[]> {
// Determine the correct System32 path. We want to point to Sysnative
// when the 32-bit version of VS Code is running on a 64-bit machine.
// The reason for this is because PowerShell's important PSReadline
Expand All @@ -131,9 +154,12 @@ export class TerminalService extends BrowserTerminalService implements ITerminal
useWSLexe = true;
}

const Registry = await import('vscode-windows-registry');
Copy link
Member

Choose a reason for hiding this comment

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

Let's move the await into _getShellPathFromRegistry and make that function async as well


const expectedLocations = {
'Command Prompt': [`${system32Path}\\cmd.exe`],
PowerShell: [`${system32Path}\\WindowsPowerShell\\v1.0\\powershell.exe`],
'PowerShell Core': [this._getShellPathFromRegistry(Registry, 'pwsh')],
'WSL Bash': [`${system32Path}\\${useWSLexe ? 'wsl.exe' : 'bash.exe'}`],
'Git Bash': [
`${process.env['ProgramW6432']}\\Git\\bin\\bash.exe`,
Expand Down