integrated terminal #5985
integrated terminal #5985
Conversation
Part of #143
The foreground color was not set so the editor's color was used
This is temporary until multiple terminals are available.
|
so, on windows I see this... |
|
pty.js doesn't seem to build on at least my machine... |
| }; | ||
| } | ||
|
|
||
| export interface ITerminalService { |
There was a problem hiding this comment.
it doesn't seem like this service isn't buying us anything
There was a problem hiding this comment.
I figured this could be useful later on when things like multiple terminals and switching between them are added.
| private createTerminal(): void { | ||
| this.parentDomElement.innerHTML = ''; | ||
| this.ptyProcess = fork(this.getShell(), [], { | ||
| name: fs.existsSync('/usr/share/terminfo/x/xterm-256color') ? 'xterm-256color' : 'xterm', |
There was a problem hiding this comment.
⚡ evil sync io inside the main thread. use callbacks/promises esp given your outer caller (create) allows you to do that
| @@ -27,8 +27,10 @@ | |||
| "iconv-lite": "0.4.13", | |||
| "minimist": "^1.2.0", | |||
| "native-keymap": "0.1.2", | |||
| "pty.js": "git+https://github.com/jeremyramin/pty.js.git#28f2667", | |||
| "sax": "1.1.2", | |||
There was a problem hiding this comment.
@bpasero You raised concerns about NAN versions but those packages are prebuilt so it shouldn't be a problem, right?
There was a problem hiding this comment.
@jrieken when I did try out this branch, our changes with NPM 3 were not present, so I ended up getting an older version of NAN for the terminal. Since you merged, I would assume, this terminal picks up the latest NAN we provide and it should be fine. As long as you can run the terminal successfully with latest, it should be good.
There was a problem hiding this comment.
#6353 and the "Cannot find module '../build/Release/pty.node'" warning is related to this. The prebuilt packages are used because the build fails so the warning shows up. Eventually we will probably want to fork pty.js ourselves to sort out any dependency problems.
|
Postponing until @Tyriar is back |
|
Yeah, it's not ready to be merged yet, the dependency issue is the main blocker. Thanks for the comments, I'll check them out when I'm back. |
|
|
||
| // Register Output Panel | ||
| (<panel.PanelRegistry>Registry.as(panel.Extensions.Panels)).registerPanel(new panel.PanelDescriptor( | ||
| 'vs/workbench/parts/terminal/electron-browser/terminalPanel', |
There was a problem hiding this comment.
@Tyriar if you have an AMD entry point like this, it needs to be declared in our buildfile.js (see https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/buildfile.js#L34)
There was a problem hiding this comment.
Yes looking into this now, it looks like there is a problem with the loader as well assuming modules don't have . chars in their name (pty.js, term.js). Chatting with @alexandrudima
Starting PR such that we can try this out soon in alpha builds