-
Notifications
You must be signed in to change notification settings - Fork 29.4k
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
Adds custom terminal launch settings #3495
Changes from 5 commits
2695f19
abe8d12
aa17b93
aa3f8d7
c84a715
ef72b65
5d46df9
58317c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/*--------------------------------------------------------------------------------------------- | ||
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
import fs = require('fs'); | ||
import env = require('vs/base/common/platform'); | ||
|
||
export let DEFAULT_LINUX_TERM = 'x-terminal-emulator'; | ||
|
||
// if we're not on debian and using gnome then | ||
// set default to gnome-terminal | ||
if (env.isLinux | ||
&& fs.existsSync('/etc/debian_version') === false | ||
&& process.env.DESKTOP_SESSION === 'gnome') { | ||
DEFAULT_LINUX_TERM = 'gnome-terminal'; | ||
} | ||
|
||
export const DEFAILT_WINDOWS_TERM = 'cmd'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo DEFAULT |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ import uri from 'vs/base/common/uri'; | |
import {TPromise} from 'vs/base/common/winjs.base'; | ||
import {ITerminalService} from 'vs/workbench/parts/execution/common/execution'; | ||
import {IConfigurationService} from 'vs/platform/configuration/common/configuration'; | ||
import {IMessageService} from 'vs/platform/message/common/message'; | ||
import {DEFAILT_WINDOWS_TERM, DEFAULT_LINUX_TERM} from 'vs/workbench/parts/execution/electron-browser/terminal'; | ||
|
||
import cp = require('child_process'); | ||
import processes = require('vs/base/node/processes'); | ||
|
@@ -18,13 +18,36 @@ export class WinTerminalService implements ITerminalService { | |
public serviceId = ITerminalService; | ||
|
||
constructor( | ||
@IConfigurationService private _configurationService: IConfigurationService, | ||
@IMessageService private _messageService: IMessageService | ||
@IConfigurationService private _configurationService: IConfigurationService | ||
) { | ||
} | ||
|
||
public openTerminal(path: string): void { | ||
cp.spawn(processes.getWindowsShell(), ['/c', 'start', '/wait'], { cwd: path }); | ||
this._configurationService.loadConfiguration().done(configuration => { | ||
return new Promise((success, failed) => { | ||
this.spawnTerminal( | ||
cp, | ||
configuration, | ||
processes.getWindowsShell(), | ||
path, | ||
success, | ||
err => { | ||
errors.onUnexpectedError(err); | ||
failed(err); | ||
} | ||
); | ||
}); | ||
}, errors.onUnexpectedError); | ||
} | ||
|
||
private spawnTerminal(spawner, configuration, command: string, path: string, onExit, onError) { | ||
let terminalConfig = configuration.terminal; | ||
let exec = terminalConfig.windows.exec || DEFAILT_WINDOWS_TERM; | ||
let cmdArgs = ['/c', 'start', '/wait', exec]; | ||
|
||
let child = spawner.spawn(command, cmdArgs, { cwd: path }); | ||
child.on('error', onError); | ||
child.on('exit', onExit); | ||
} | ||
} | ||
|
||
|
@@ -49,14 +72,41 @@ export class MacTerminalService implements ITerminalService { | |
child.on('exit', (code: number) => { | ||
c(code === 0 ? 'iterm.scpt' : 'terminal.scpt'); | ||
}); | ||
}).then(name => uri.parse(require.toUrl(`vs/workbench/parts/execution/electron-browser/${ name }`)).fsPath); | ||
}).then(name => uri.parse(require.toUrl(`vs/workbench/parts/execution/electron-browser/${name}`)).fsPath); | ||
} | ||
} | ||
|
||
export class LinuxTerminalService implements ITerminalService { | ||
public serviceId = ITerminalService; | ||
|
||
constructor( | ||
@IConfigurationService private _configurationService: IConfigurationService | ||
) { } | ||
|
||
|
||
public openTerminal(path: string): void { | ||
cp.spawn('x-terminal-emulator', [], { cwd: path }); | ||
this._configurationService.loadConfiguration().done(configuration => { | ||
return new Promise((success, failed) => { | ||
this.spawnTerminal( | ||
cp, | ||
configuration, | ||
path, | ||
success, | ||
err => { | ||
errors.onUnexpectedError(err); | ||
failed(err); | ||
} | ||
); | ||
}); | ||
}, errors.onUnexpectedError); | ||
} | ||
|
||
private spawnTerminal(spawner, configuration, path: string, onExit, onError) { | ||
let terminalConfig = configuration.terminal; | ||
let exec = terminalConfig.linux.exec || DEFAULT_LINUX_TERM; | ||
const child = spawner.spawn(exec, [], { cwd: path }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does just setting the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. node runs |
||
child.on('error', onError); | ||
child.on('exit', onExit); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,129 @@ | ||
/*--------------------------------------------------------------------------------------------- | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should also live in |
||
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
|
||
'use strict'; | ||
|
||
import {equal} from 'assert'; | ||
import {WinTerminalService, LinuxTerminalService} from 'vs/workbench/parts/execution/electron-browser/terminalService'; | ||
import {DEFAILT_WINDOWS_TERM, DEFAULT_LINUX_TERM} from 'vs/workbench/parts/execution/electron-browser/terminal'; | ||
|
||
suite('Execution - TerminalService', () => { | ||
let mockOnExit; | ||
let mockOnError; | ||
let mockConfig; | ||
|
||
setup(() => { | ||
mockConfig = { | ||
terminal: { | ||
windows: { | ||
exec: 'testWindowsShell' | ||
}, | ||
linux: { | ||
exec: 'testLinuxShell' | ||
} | ||
} | ||
}; | ||
mockOnExit = s => s; | ||
mockOnError = e => e; | ||
}); | ||
|
||
test("WinTerminalService - uses terminal from configuration", done => { | ||
let testShell = 'cmd'; | ||
let testCwd = 'path/to/workspace'; | ||
let mockSpawner = { | ||
spawn: (command, args, opts) => { | ||
// assert | ||
equal(command, testShell, 'shell should equal expected'); | ||
equal(args[args.length - 1], mockConfig.terminal.windows.exec, 'terminal should equal expected') | ||
equal(opts.cwd, testCwd, 'opts.cwd should equal expected'); | ||
done(); | ||
return { | ||
on: (evt) => evt | ||
} | ||
} | ||
}; | ||
let testService = new WinTerminalService(mockConfig); | ||
(<any>testService).spawnTerminal( | ||
mockSpawner, | ||
mockConfig, | ||
testShell, | ||
testCwd, | ||
mockOnExit, | ||
mockOnError | ||
); | ||
}); | ||
|
||
test("WinTerminalService - uses default terminal when configuration.terminal.windows.exec is undefined", done => { | ||
let testShell = 'cmd'; | ||
let testCwd = 'path/to/workspace'; | ||
let mockSpawner = { | ||
spawn: (command, args, opts) => { | ||
// assert | ||
equal(args[args.length - 1], DEFAILT_WINDOWS_TERM, 'terminal should equal expected') | ||
done(); | ||
return { | ||
on: (evt) => evt | ||
} | ||
} | ||
}; | ||
mockConfig.terminal.windows.exec = undefined; | ||
let testService = new WinTerminalService(mockConfig); | ||
(<any>testService).spawnTerminal( | ||
mockSpawner, | ||
mockConfig, | ||
testShell, | ||
testCwd, | ||
mockOnExit, | ||
mockOnError | ||
); | ||
}); | ||
|
||
test("LinuxTerminalService - uses terminal from configuration", done => { | ||
let testCwd = 'path/to/workspace'; | ||
let mockSpawner = { | ||
spawn: (command, args, opts) => { | ||
// assert | ||
equal(command, mockConfig.terminal.linux.exec, 'terminal should equal expected'); | ||
equal(opts.cwd, testCwd, 'opts.cwd should equal expected'); | ||
done(); | ||
return { | ||
on: (evt) => evt | ||
} | ||
} | ||
}; | ||
let testService = new LinuxTerminalService(mockConfig); | ||
(<any>testService).spawnTerminal( | ||
mockSpawner, | ||
mockConfig, | ||
testCwd, | ||
mockOnExit, | ||
mockOnError | ||
); | ||
}); | ||
|
||
test("LinuxTerminalService - uses default terminal when configuration.terminal.linux.exec is undefined", done => { | ||
let testCwd = 'path/to/workspace'; | ||
let mockSpawner = { | ||
spawn: (command, args, opts) => { | ||
// assert | ||
equal(command, DEFAULT_LINUX_TERM, 'terminal should equal expected') | ||
done(); | ||
return { | ||
on: (evt) => evt | ||
} | ||
} | ||
}; | ||
mockConfig.terminal.linux.exec = undefined; | ||
let testService = new LinuxTerminalService(mockConfig); | ||
(<any>testService).spawnTerminal( | ||
mockSpawner, | ||
mockConfig, | ||
testCwd, | ||
mockOnExit, | ||
mockOnError | ||
); | ||
}); | ||
|
||
}); |
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.
Have you thought about how the settings should be named given that the eventual integrated terminal #143 will likely have a set of settings as well? Maybe something like
terminal.external...
andterminal.integrated...
or something?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.
@Tyriar This wasn't related to #143. This just solved the hard coded terminal issue for windows and Linux.
Imo I would of thought #143 would be better as extension.
I can change the name if you like.
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.
Just forward thinking, I'm currently experimenting with the integrated terminal so I want to make sure the settings have good names to prevent changes/inconsistencies in the future. I'm thinking lets rename them to this for now:
I'll see what @joaomoreno thinks of the naming later