-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
Rename terminal instance on enter in Windows #30732
Conversation
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
* Copyright (c) Microsoft Corporation. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why there are diffs on this line? You might have committed the wrong line endings or something
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
|
||
'use strict'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this adds anything in a file compiled by strict TS
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
* Copyright (c) Microsoft Corporation. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also not have a diff
this._pidStack.push(this._processId); | ||
} | ||
return new TPromise<string>((resolve) => { | ||
// wait 100ms before running getChildProcesses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should explain why the wait is needed here
@@ -846,6 +867,57 @@ export class TerminalInstance implements ITerminalInstance { | |||
this._messageTitleListener = null; | |||
} | |||
} | |||
|
|||
private static executeWMIC(pid: number): TPromise<{ executable: string, pid: number }[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more general name for what the function is actually doing here, is this accurate? getFirstWindowsChildProcess
}); | ||
} | ||
|
||
private getChildProcesses(pid: number): TPromise<string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better name might be refreshWindowsShellProcessTree
?
@@ -135,6 +139,18 @@ export class TerminalInstance implements ITerminalInstance { | |||
this._createProcess(this._shellLaunchConfig); | |||
this._createXterm(); | |||
|
|||
this._pidStack = []; | |||
this._checkWindowShell = new Emitter<string>(); | |||
debounceEvent(this._checkWindowShell.event, (l, e) => e, 100, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only be called on Windows, otherwise we're still doing a lot of work on enter for nothing on macOS/Linux.
@@ -261,6 +277,11 @@ export class TerminalInstance implements ITerminalInstance { | |||
if (TabFocus.getTabFocusMode() && event.keyCode === 9) { | |||
return false; | |||
} | |||
|
|||
if (platform.isWindows && event.keyCode === 13 /* ENTER */) { | |||
this._checkWindowShell.fire(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we actually want to use this instead of bringing in the overhead of an event/emitter https://github.com/Microsoft/vscode/blob/e49ae72a37b22a7e7f2ed3a6bf052076e9a283d9/extensions/git/src/contentProvider.ts#L42
@@ -135,6 +139,18 @@ export class TerminalInstance implements ITerminalInstance { | |||
this._createProcess(this._shellLaunchConfig); | |||
this._createXterm(); | |||
|
|||
this._pidStack = []; | |||
this._checkWindowShell = new Emitter<string>(); | |||
debounceEvent(this._checkWindowShell.event, (l, e) => e, 100, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should pull all this code out into another file so we don't clutter TerminalInstance? This would also simplify the function names as we can have Windows
in the new class name.
} | ||
resolve(this._shellLaunchConfig.executable); | ||
}, error => { return error; }); | ||
}, 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move this to a named constant (especially because the comment explaining is so far from this line.
@@ -81,6 +82,8 @@ export class TerminalInstance implements ITerminalInstance { | |||
private _messageTitleListener: (message: { type: string, content: string }) => void; | |||
private _preLaunchInputQueue: string; | |||
private _initialCwd: string; | |||
private _windowsShellService: WindowsShellService; | |||
private _checkWindowShell: Emitter<string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this _onCheckWindowsShell
to be consistent with other Emitter
s.
@@ -846,6 +862,15 @@ export class TerminalInstance implements ITerminalInstance { | |||
this._messageTitleListener = null; | |||
} | |||
} | |||
|
|||
public eventuallyGetShellName(): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's return the promise here. Also a better name would be updateShellName
(or updateWindowsShellName
if it's in TerminalInstance
. Since TerminalInstance.setTitle
is on the interface let's move this whole function into WindowsShellService
.
/** The amount of time to wait before getting the shell process name */ | ||
const WAIT_FOR_SHELL_UPDATE = 100; | ||
|
||
export class WindowsShellService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this WindowsShellHelper
because it is not a "registered service" like TerminalService
is.
|
||
export class WindowsShellService { | ||
private _pidStack: number[]; | ||
private _processId: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_rootProcessId
to differentiate from _pidStack
export class WindowsShellService { | ||
private _pidStack: number[]; | ||
private _processId: number; | ||
private _shellLaunchConfig: IShellLaunchConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's pass in the executable instead (_rootProcessExecutable
) as that's all this class cares about.
this._shellLaunchConfig = shell; | ||
} | ||
|
||
private static getFirstWindowsChildProcess(pid: number): TPromise<{ executable: string, pid: number }[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove Windows from the name here since we're in a class containing "Windows" now. Also doesn't need to be static.
}); | ||
} | ||
|
||
private refreshWindowsShellProcessTree(pid: number, flag: boolean): TPromise<string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, remove "Windows"
private refreshWindowsShellProcessTree(pid: number, flag: boolean): TPromise<string> { | ||
return WindowsShellService.getFirstWindowsChildProcess(pid).then(result => { | ||
if (result.length === 0) { | ||
if (flag) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what flag does?
*/ | ||
public getShellName(): TPromise<string> { | ||
if (platform.platform !== platform.Platform.Windows) { | ||
throw null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this to the constructor.
throw new Error(`WindowsShellHelper cannot be instantiated on ${platform.platform}`);
import { TPromise } from 'vs/base/common/winjs.base'; | ||
|
||
/** The amount of time to wait before getting the shell process name */ | ||
const WAIT_FOR_SHELL_UPDATE = 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WAIT_AFTER_ENTER_TIME
is a little more descriptive.
0c1d2e9
to
a737109
Compare
a737109
to
a2a61b1
Compare
Woo! 🎉 |
Fix part of #30152
Noted flaws:
npm install
is run and finishes