Fix debounce and add onData listener#31155

Lixire · 2017-07-20T19:59:32Z

Now listens to enter and onData (fires if the event has not been called for at least 100ms)

Refer to #30152 for details

Tyriar

Please link the issue on the PR, it has a description of the timer system we want for onData

Tyriar · 2017-07-20T21:23:08Z

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

+				debounceEvent(this._onCheckWindowsShell.event, (l, e) => e, 100, true)(() => {
+					this.checkWindowShell();
+				});
+				this.onData(() => this._onCheckWindowsShell.fire());


I think we need a new mechanism for onData. Currently if a command runs for 10 seconds, it will launch 10/0.1=100 processes during its lifetimes. Instead of debounce like on enter we need something that will go off when the output has finished coming in.

Doesn't debounce reset the timer whenever the function gets called again?

I'm 90% sure it does

You're right I got a little confused there. But doesn't that mean it will not fire at the beginning if output is returned within 100ms because onEnter is being debounced on the same event? We might actually want to throttle onEnter (fire immediate, but no more than every 100ms), and debounce onData (fire when output stops for 100ms).

It also occurs to me that onData will fire when you press any key while typing in the prompt which probably isn't what we want. Imagine typing "a" (wait 150ms) "b" (wait 150ms) "c": that would fire 3 processes because of the onData handler.

A more efficient approach could be to hook into when a new line is started in the terminal, this would require some new API in xterm.js though. You could probably test out the approach by monkey patching xterm.js temporarily to see if it does what we want. Something like this:

const originalLineFeed = (<any>this._xterm).inputHandler.lineFeed; (<any>this._xterm).inputHandler.lineFeed = function () { // ...fire event... originalLineFeed(); }

Bring in lineFeed event for #30152, #31155

roblourens · 2017-07-27T23:33:21Z

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

+				debounceEvent(this._onCheckWindowsShell.event, (l, e) => e, 200, true)(() => {
+					setTimeout(() => {
+						this.checkWindowShell();
+					}, 50);


I think you explained why there was a setTimeout inside the debounce, but I forget why, a comment would be nice

roblourens · 2017-07-28T00:58:07Z

Not sure whether the CI ❌ 's are from your changes? Can you try to get it back to ✅ ?

Lixire · 2017-07-28T02:33:39Z

Fixed the tests, @roblourens

Tyriar · 2017-07-28T15:38:32Z

The current failure looks unrelated.

Tyriar · 2017-07-28T16:04:33Z

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

 		this._createProcess(this._shellLaunchConfig);
 		this._createXterm();
+		if (this._shellLaunchConfig) {
+			this.setTitle(path.basename(this._shellLaunchConfig.executable), true);


If this._shellLaunchConfig.name is set, we do not want to do this.

Also this applies to Linux/macOS too, I don't think we use basename for non-Windows yet. This also doesn't strip .exe, we should rely on the basename/.exe strip code inside setTitle.

Tyriar · 2017-07-28T16:07:15Z

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

+					this.checkWindowShell();
+				});
+				this._xterm.on('lineFeed', () => this._onCheckWindowsShell.fire());
+				this._xterm.on('keypress', () => this._onCheckWindowsShell.fire());


If we're doing this here we still need the enter check and the this._messageTitleListener check.

Actually, I check this._messageTitleListener in the method. For the keypress, won't lineFeed suffice? I think for the enter spam, the debounced event will suffice?

Tyriar · 2017-07-28T16:10:42Z

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

+			this.setTitle(path.basename(this._shellLaunchConfig.executable), true);
+		}

 		if (platform.isWindows) {


This is getting very large, can we instead pass this._xterm to WindowsShellHelper and do everything in there? Encapsulating all this logic into WindowsShellHelper would be best as it's Windows specific and TerminalInstance is already quite large and complex.

This solves the blank flickering that appears on Mac as well as the undefined in Windows.

But if mac's executable is set to /bin/bash, it might be bash -> /bin/bash (some time in the future)?

But that's more for the comment above. This one is pointing at the if (platform.Windows) section being too large

Tyriar

Just a few more small comments and good to go!

Tyriar · 2017-07-28T19:39:20Z

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

 	private _onDataForApi: Emitter<{ instance: ITerminalInstance, data: string }>;
 	private _onProcessIdReady: Emitter<TerminalInstance>;
 	private _onTitleChanged: Emitter<string>;
+	private _onCheckWindowsShell: Emitter<TPromise<string>>;


Can be removed

Tyriar · 2017-07-28T19:39:22Z

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

 import * as platform from 'vs/base/common/platform';
 import * as dom from 'vs/base/browser/dom';
-import Event, { Emitter } from 'vs/base/common/event';
+import Event, { Emitter, debounceEvent } from 'vs/base/common/event';


Can be removed

Tyriar · 2017-07-28T19:42:32Z

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

 		this._createProcess(this._shellLaunchConfig);
 		this._createXterm();

+		if (this._shellLaunchConfig && !this._shellLaunchConfig.name) {


Let's move this into _createProcess, just before this._messageTitleListener is set. That way it's right next to the part that sets it for _shellLaunchConfig.name.

Tyriar · 2017-07-28T19:43:17Z

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

 	private _rootProcessId: number;
+	private _terminalInstance: ITerminalInstance;
+	private _onCheckShell: Emitter<TPromise<string>>;
+	private _xterm: any;


Pull in the type using import XTermTerminal = require('xterm');

Fix debounce and add onData listener

7529813

msftclas added the cla-not-required label Jul 20, 2017

Lixire requested a review from Tyriar July 20, 2017 19:59

Tyriar requested changes Jul 20, 2017

View reviewed changes

Tyriar added this to the July 2017 milestone Jul 20, 2017

Tyriar assigned Tyriar and Lixire Jul 20, 2017

Call debounce on enter

905ccbd

Tyriar mentioned this pull request Jul 21, 2017

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

Merged

Listens to newline or linefeed

55a97a9

Tyriar added a commit that referenced this pull request Jul 25, 2017

Uplevel xterm.js

bbfdd06

Bring in lineFeed event for #30152, #31155

Lixire added 2 commits July 25, 2017 11:37

Adjust debounce time

8ff7d38

Fix undefined title flicker

1929b35

Lixire force-pushed the amqi/update-window-shellname branch from e39e0a5 to 1929b35 Compare July 25, 2017 20:58

Lixire requested a review from roblourens July 27, 2017 22:56

roblourens reviewed Jul 27, 2017

View reviewed changes

Add comments and add keypress listener

5576f8a

Fix tests

ff65fed

Tyriar requested changes Jul 28, 2017

View reviewed changes

Address comments

84b531d

Tyriar requested changes Jul 28, 2017

View reviewed changes

Lixire added 2 commits July 28, 2017 14:06

Address more comments

ae91973

Fix title basename

9bd27af

Tyriar approved these changes Jul 28, 2017

View reviewed changes

Tyriar merged commit 2746e2f into master Jul 28, 2017

Tyriar deleted the amqi/update-window-shellname branch July 28, 2017 21:27

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

Conversation

Lixire commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tyriar left a comment

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!

roblourens commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lixire commented Jul 28, 2017

Uh oh!

Tyriar commented Jul 28, 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!

Lixire Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Tyriar left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Lixire commented Jul 20, 2017 •

edited

Loading

roblourens commented Jul 28, 2017 •

edited

Loading

Lixire Jul 28, 2017 •

edited

Loading