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.

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

Unnecessary delay when opening a terminal #29184

Closed
fabiospampinato opened this issue Jun 21, 2017 · 29 comments · Fixed by #30106
Closed

Unnecessary delay when opening a terminal #29184

fabiospampinato opened this issue Jun 21, 2017 · 29 comments · Fixed by #30106
Assignees
Labels
feature-request Request for new features or functionality help wanted Issues identified as good community contribution opportunities terminal General terminal issues that don't fall under another label
Milestone

Comments

@fabiospampinato
Copy link
Contributor

The Issue

I'm writing an AppleScript for opening a terminal and writing some stuff in it, the problem is that without an hard coded delay between the action "open the terminal" and the action "write this stuff in it" it doesn't work.

This is what, ideally, should be working (minus the useless delay call):

tell application "Visual Studio Code" to activate
tell application "System Events"
	keystroke "`" using control down
	delay 0
	keystroke "placeholder"
end tell

This is what I see in the terminal if I run the AppleScript with different delay values:

  • 0: fabiomac:project fabio$
  • 0.1: fabiomac:project fabio$
  • 0.15 placeholderfabiomac:project fabio$ placeholder ❌ 😮
  • 0.2 fabiomac:project fabio$ placeholder

IMHO this is broken because I shouldn't have to hard code environment-dependant delay calls in my AppleScripts, and consequentially I shouldn't have to wait those extra seconds.

My Setup

  • VSCode Version: Code 1.13.1 [Unsupported] (379d2ef, 2017-06-14T18:13:05.928Z)
  • OS Version: Darwin x64 16.7.0
  • Extensions:
Extension Author Version
EditorConfig EditorConfig 0.9.3
status-bar-tasks GuardRex 0.2.0
beautify HookyQR 1.1.1
crane HvyIndustries 0.3.6
vscode-docker PeterJausovec 0.0.16
code-settings-sync Shan 2.8.1
sort-lines Tyriar 1.3.0
vscode-color anseki 0.4.1
vscode-custom-css be5invis 2.3.2
gitignore codezombiech 0.5.0
vscode-svgviewer cssho 1.4.2
vscode-dash deerawan 1.5.0
python donjayamanne 0.6.5
applescript idleberg 0.6.0
indentation-level-movement kaiwood 1.1.1
graphql-for-vscode kumar-harsh 1.4.1
ecdc mitchdenny 0.10.3
vscode-duplicate mrmlnc 1.1.0
debugger-for-chrome msjsdiag 3.1.4
ava samverschueren 0.4.0
todotasks sandy081 0.5.0
php-ci small 0.2.0
Align steve8708 0.2.0
cordova-tools vsmobile 1.2.7
vscode-react-native vsmobile 0.3.2
change-case wmaurer 1.0.0
@Tyriar
Copy link
Member

Tyriar commented Jun 21, 2017

@fabiospampinato the problem is that the terminal takes more than 0ms to start up? It takes time to spin up the pty and attach everything, a little longer than other terminals because everything is going through V8 first.

@Tyriar Tyriar added terminal General terminal issues that don't fall under another label info-needed Issue requires more information from poster labels Jun 21, 2017
@fabiospampinato
Copy link
Contributor Author

fabiospampinato commented Jun 21, 2017

@Tyriar the problem is that I have to put workarounds in my AppleScripts in order for them to work.

Of course it won't be as fast as a native terminal, but perhaps instead of just throwing away whatever is typed during the time where the terminal panel is in focus but the new pty hasn't started yet we could store it and replay it back once the pty started. What do you think?

@Tyriar Tyriar added feature-request Request for new features or functionality and removed info-needed Issue requires more information from poster new release labels Jun 21, 2017
@Tyriar Tyriar added this to the Backlog milestone Jun 21, 2017
@Tyriar Tyriar added the help wanted Issues identified as good community contribution opportunities label Jun 21, 2017
@Tyriar
Copy link
Member

Tyriar commented Jun 21, 2017

Sure, I'll add it to the backlog and open it up for PRs. To anyone interested in implementing this you will need to listen to key strokes in TerminalInstance and add them to a queue. Once the pty is ready then push the events through the normal mechanism.

@fabiospampinato
Copy link
Contributor Author

Sounds nice, thanks!

@cleidigh
Copy link
Contributor

@fabiospampinato
@Tyriar
FTW - somewhere in the last month/1.5months I started to see a similar problem
under Windows using the run commands function on a text file I use for various DOS commands
building VS code running git commands, et cetera. before I could use run commands
in the terminal would open and run the command for 100% without fail
recently the terminal opens but the subsequent command is only executed seventy five percent
of the time and I have to redo the run commands
you can mark me down to do a PR on this

@cleidigh
Copy link
Contributor

@Tyriar
@fabiospampinato

I haven't looked at the code yet, but I'm assuming there could be some state awareness issues.
for instance , in order to track and queue the keystrokes the state in context of VS code has to transition
to the terminal context (presumably minimal time, but not zero as you stated above).
This means there will always be a window where keystrokes from an automation program
such as AppleScript will be missed. doing the queuing between the time the open terminal
command is executed and the PTY is ready only reduces the window but not to zero.

I also realized my situation with the VSC run commands is related but different since the run command is an internal command.

I'll look into what I think this takes

@Tyriar
Copy link
Member

Tyriar commented Jun 21, 2017

@cleidigh yes there are some async calls between toggling the terminal and the terminal actually coming up. It's possible this won't actually fix the issue of using a 0 delay, but it would definitely be beneficial to Windows in particular as I have keystrokes going missing all the time when I'm typing fast due to the increase time it takes to launch the shell.

@cleidigh
Copy link
Contributor

@Tyriar
okay I'll do some code inspection since I've never been in this part of the code, then some experiments.
probably ping you later with some questions before I do a PR

@cleidigh
Copy link
Contributor

@fabiospampinato
I'm not sure exactly what your use case is, in you may already be aware of
the fact that there are several extensions for running terminal commands
https://marketplace.visualstudio.com/items?itemName=formulahendry.code-runner
https://marketplace.visualstudio.com/items?itemName=mattn.Runner
these may not do exactly what you need, I don't know
what they will do is operate internally as opposed to trying to drive code from the outside
FWIW

@fabiospampinato
Copy link
Contributor Author

@cleidigh what I want to do is set-up a bunch of terminals, each of them might cd into a specific directory and type a command I might like to execute on it in the future, without actually executing it. So that in combination with Status Bar Tasks I can simply click a button on the statusbar and have all the terminals I needed ready for when I'll need them.

@Tyriar
Copy link
Member

Tyriar commented Jun 21, 2017

@fabiospampinato the typical way to do this is to create an extension which should give you enough to be able to create several terminals and if you want give them custom names/shells.

I still feel like this request adds value regardless of the direction you go though 😃

@Tyriar
Copy link
Member

Tyriar commented Jun 21, 2017

@fabiospampinato
Copy link
Contributor Author

fabiospampinato commented Jun 21, 2017

@Tyriar Well, I simply took the easy/fast route: having a program typing keystrokes for me. I'll check those links out, thanks!

@cleidigh
Copy link
Contributor

@Tyriar
since you still think this is relevant I will continue to look into it and touch base when I have some progress

@cleidigh
Copy link
Contributor

cleidigh commented Jun 27, 2017

@Tyriar
I've done quite a bit of code inspection and some debug tracing. I still cannot
follow the complete path for keyboard and context demultiplexing , from my basic understanding
I think there's a state issue to surmount.

  • currently until the terminal gets focus which ever other dom element has focus will continue to receive keyboard input
  • I can see it possible to collect keyboard input as soon as the terminal launch command is received
    (but before focus) but this is only half the problem
  • the problem is with the prior element still having focus and not ignoring keyboard input
  • I'm fairly certain layering and isolation prevent the terminal from changing any other context

am I missing some other mechanism that would effectively allow change of context and focus
without the focus really been changed yet?

@Tyriar
Copy link
Member

Tyriar commented Jun 27, 2017

@cleidigh I think xterm.js should get and process events before the process is ready. I was imagining queuing up the data from xterm.js from here instead of just discarding it https://github.com/Microsoft/vscode/blob/75082c7ecba8ea75a2bc8a75d41768eebcb04225/src/vs/workbench/parts/terminal/electron-browser/terminalInstance.ts#L213 and sending it through when _processReady is resolved?

@cleidigh
Copy link
Contributor

cleidigh commented Jun 27, 2017

@Tyriar
I agree and understand the concept of queuing up the input as soon as the terminal instance is invoked and up to the point the processReady fires
the problem I found is that I still think keyboard input will be passed to whatever has focus
eg the editor not just the pending terminal
are you thinking of hooking an event handler that will prevent propagation to the currently focused element?

@Tyriar
Copy link
Member

Tyriar commented Jun 27, 2017

@cleidigh the delay is due to the process warming up, xterm.js which handles the input events should be attached very quickly. I don't think it's worth pursuing this beyond holding onto what xterm.js wants to give us and making sure that the process is ready before sending it.

@cleidigh
Copy link
Contributor

cleidigh commented Jun 30, 2017

@Tyriar
Hey, just an update.

I have a solution working that is pretty much lossless.
I Q up the keyboard input until the process is ready
using sendText is straightforward and works however
this really doesn't cover keyboard input for anything other than
ASCII characters without modifiers.

I know the solution (Q the actual keyboard events and replay them when process ready)
so far I cannot figure out how to create a keyboard event emitter (created an event, used .fire, compiles post does not appear to be received by the xterm)
Also try to straight keyboard Event and document.dispatchEvent - no luck
I think this is based on the fact that you cannot create keyboard events artificially
on DOM elements for security reasons.
have not been able to directly push characters other than with sendText
The Xterm documentation only references using writeln (same as what sendText uses)
seems like I have to trigger the listener directly somehow. unless text only is okay?

@cleidigh
Copy link
Contributor

cleidigh commented Jul 4, 2017

@Tyriar
I Hope you had a great Fourth of July! (I used part of mine to "RTFC" vscode and xterm ;-)

After lots of experimentation and a better understanding the sequence and escape encoding location
I think I have a pretty clean solution, certainly fairly straightforward.

  • instead of the process object, I used the process ID to determine if we queue data
    in a string Q
  • when we get the process ID message, output the Q
  • keyboard events and escape encoding remain the same in xterm

I did a fair amount of testing. as you may remember I use Dragon voice recognition with my own Python extensions for everything (because of the ALS) This allows me to send characters
to code with a fair amount of Control . Usually it was from one hundred to three hundred milliseconds
between starting to launch and characters going through to the terminal.
With the data Q I can launch a terminal and send characters close to one millisecond apart
with no data loss.

I have posted a PR #30106

Hope that looks good.

Tyriar added a commit that referenced this issue Jul 5, 2017
Q data during pty launch delay Fixes: #29184
@Tyriar Tyriar modified the milestones: July 2017, Backlog Jul 5, 2017
@fabiospampinato
Copy link
Contributor Author

Sounds great, thanks a lot for your time!

@Tyriar
Copy link
Member

Tyriar commented Jul 5, 2017

@fabiospampinato thanks for the suggestion, this will make things feel much snappier 🙂

@cleidigh
Copy link
Contributor

cleidigh commented Jul 5, 2017

@fabiospampinato
@Tyriar
FYI I have limited means of doing work on OS X since I don't have Dragon voice Control for OS X
the only way I can do OS X work is a remote VNC connection which introduces its own set of problems
I finally got a chance to look at OS X unfortunately it does have different timing

using the exact same script as @fabiospampinato I can reproduce the problem as stated at the beginning of this issue. with the data to fix keystrokes to not appear lost but there is a double rendering
of "placeholder" as shown in @fabiospampinato 150mS delay new code with no delay
so the terminal is getting the correct characters but it duplication before the keystrokes
are captured by the terminal . as I possibly suspected there is a window where the keystrokes
after the launch terminal are consumed in two places.
bottom line is that I think I've improved things but there is still a vulnerability window
I can try some more experiments but this is difficult for me on OS X
I had thought that the Windows experiments were sufficiently similar - my error in this

@Tyriar
Copy link
Member

Tyriar commented Jul 5, 2017

@cleidigh the time to input is as small as it can get in a nice way imo, for the OP's case I'd say making an extension to drive the terminal would be a much more reliable way of scripting VS Code.

@cleidigh
Copy link
Contributor

cleidigh commented Jul 5, 2017

@Tyriar
I tend to agree, you can't change code state machine instantly without making a lot of other things
not work well.
I just feel badly this helps Windows more than OS X

@Tyriar
Copy link
Member

Tyriar commented Jul 5, 2017

@cleidigh well I can definitely notice the difference on macOS as well 👍

@cleidigh
Copy link
Contributor

cleidigh commented Jul 5, 2017

@Tyriar
I just had someone help me doing direct keyboard Control (on my OS X machine eliminating Dragon)
as far as I can tell there is duplicate rendering but the terminal only processes the correct keystrokes.
this may actually be related to what I mentioned about duplicate Windows terminal startup headers.
will try some more experiments

@fabiospampinato
Copy link
Contributor Author

@Tyriar I took your advice and built an extension for this use case. I think it came out pretty nice, I hope this will be useful for somebody else as well.

@Tyriar
Copy link
Member

Tyriar commented Jul 13, 2017

@fabiospampinato that's really cool! I'm going to share it with the team and in #20013

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality help wanted Issues identified as good community contribution opportunities terminal General terminal issues that don't fall under another label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants