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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Apr 18, 2016
Merged

Adds custom terminal launch settings #3495

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2695f19
adds terminal command configuration for windows and linux
Feb 26, 2016
abe8d12
makes gnome-terminal default when running on gnome desktop
Mar 21, 2016
aa17b93
adds debian check for default terminal
Mar 21, 2016
aa3f8d7
adds TerminalService tests
Mar 23, 2016
c84a715
Response to ALL_CAPS_NAMING and code org.
Apr 18, 2016
ef72b65
Response to typo
Apr 18, 2016
5d46df9
Response to configuration property name change
Apr 18, 2016
58317c2
Response to move test to electron-browser folder
Apr 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/vs/workbench/parts/execution/electron-browser/terminal.contribution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {SyncActionDescriptor} from 'vs/platform/actions/common/actions';
import {IInstantiationService} from 'vs/platform/instantiation/common/instantiation';
import {KeyMod, KeyCode} from 'vs/base/common/keyCodes';
import {Extensions, IConfigurationRegistry} from 'vs/platform/configuration/common/configurationRegistry';
import {defaultWindowsTerm, defaultLinuxTerm} from 'vs/workbench/parts/execution/common/terminal';
import {DEFAILT_WINDOWS_TERM, DEFAULT_LINUX_TERM} from 'vs/workbench/parts/execution/electron-browser/terminal';

let configurationRegistry = <IConfigurationRegistry>Registry.as(Extensions.Configuration);
configurationRegistry.registerConfiguration({
Expand All @@ -36,7 +36,7 @@ configurationRegistry.registerConfiguration({
'exec': {
'type': 'string',
'description': nls.localize('terminal.windows.exec', "Customizes which terminal to run."),
'default': defaultWindowsTerm
'default': DEFAILT_WINDOWS_TERM
}
}
},
Expand All @@ -47,7 +47,7 @@ configurationRegistry.registerConfiguration({
'exec': {
Copy link
Member

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... and terminal.integrated... or something?

Copy link
Author

@pflannery pflannery Apr 18, 2016 •
edited
Loading

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.

Copy link
Member

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:

terminal.external.windowsExec
terminal.external.linuxExec

I'll see what @joaomoreno thinks of the naming later

'type': 'string',
'description': nls.localize('terminal.linux.exec', "Customizes which terminal to run."),
'default': defaultLinuxTerm
'default': DEFAULT_LINUX_TERM
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions ...kbench/parts/execution/common/terminal.ts → ...ts/execution/electron-browser/terminal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@
import fs = require('fs');
import env = require('vs/base/common/platform');

export let defaultLinuxTerm = 'x-terminal-emulator';
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') {
defaultLinuxTerm = 'gnome-terminal';
DEFAULT_LINUX_TERM = 'gnome-terminal';
}

export const defaultWindowsTerm = 'cmd';
export const DEFAILT_WINDOWS_TERM = 'cmd';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo DEFAULT

6 changes: 3 additions & 3 deletions src/vs/workbench/parts/execution/electron-browser/terminalService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {defaultWindowsTerm, defaultLinuxTerm} from 'vs/workbench/parts/execution/common/terminal';
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');
Expand Down Expand Up @@ -42,7 +42,7 @@ export class WinTerminalService implements ITerminalService {

private spawnTerminal(spawner, configuration, command: string, path: string, onExit, onError) {
let terminalConfig = configuration.terminal;
let exec = terminalConfig.windows.exec || defaultWindowsTerm;
let exec = terminalConfig.windows.exec || DEFAILT_WINDOWS_TERM;
let cmdArgs = ['/c', 'start', '/wait', exec];

let child = spawner.spawn(command, cmdArgs, { cwd: path });
Expand Down Expand Up @@ -103,7 +103,7 @@ export class LinuxTerminalService implements ITerminalService {

private spawnTerminal(spawner, configuration, path: string, onExit, onError) {
let terminalConfig = configuration.terminal;
let exec = terminalConfig.linux.exec || defaultLinuxTerm;
let exec = terminalConfig.linux.exec || DEFAULT_LINUX_TERM;
const child = spawner.spawn(exec, [], { cwd: path });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does just setting the cwd work with gnome-terminal? You may need to pass in --working-directory=path for that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node runs gnome-terminal at cwd specified in the options. I tested on Fedora 23 and it does go to the cwd specified. I think it's just a useful flag when someone wants to run from a different path other than the cwd gnome-terminal was executed from.

child.on('error', onError);
child.on('exit', onExit);
Expand Down
6 changes: 3 additions & 3 deletions src/vs/workbench/parts/execution/test/browser/terminalService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import {equal} from 'assert';
import {WinTerminalService, LinuxTerminalService} from 'vs/workbench/parts/execution/electron-browser/terminalService';
import {defaultWindowsTerm, defaultLinuxTerm} from 'vs/workbench/parts/execution/common/terminal';
import {DEFAILT_WINDOWS_TERM, DEFAULT_LINUX_TERM} from 'vs/workbench/parts/execution/electron-browser/terminal';

suite('Execution - TerminalService', () => {
let mockOnExit;
Expand Down Expand Up @@ -61,7 +61,7 @@ suite('Execution - TerminalService', () => {
let mockSpawner = {
spawn: (command, args, opts) => {
// assert
equal(args[args.length - 1], defaultWindowsTerm, 'terminal should equal expected')
equal(args[args.length - 1], DEFAILT_WINDOWS_TERM, 'terminal should equal expected')
done();
return {
on: (evt) => evt
Expand Down Expand Up @@ -108,7 +108,7 @@ suite('Execution - TerminalService', () => {
let mockSpawner = {
spawn: (command, args, opts) => {
// assert
equal(command, defaultLinuxTerm, 'terminal should equal expected')
equal(command, DEFAULT_LINUX_TERM, 'terminal should equal expected')
done();
return {
on: (evt) => evt
Expand Down