Merge pull request #41204 from Microsoft/41110_terminal_config_cleanup

Clean up terminal config access

src/vs/workbench/parts/terminal/common/terminal.ts

@@ -47,6 +47,8 @@ export const TerminalCursorStyle = {
 	UNDERLINE: 'underline'
 };
 
+export const TERMINAL_CONFIG_SECTION = 'terminal.integrated';
+
 export interface ITerminalConfiguration {
 	shell: {
 		linux: string;
@@ -170,7 +172,6 @@ export interface ITerminalService {
 	showPreviousFindTermFindWidget(): void;
 
 	setContainers(panelContainer: HTMLElement, terminalContainer: HTMLElement): void;
-	updateConfig(): void;
 	selectDefaultWindowsShell(): TPromise<string>;
 	setWorkspaceShellAllowed(isAllowed: boolean): void;
 }

src/vs/workbench/parts/terminal/common/terminalService.ts

@@ -9,10 +9,8 @@ import { IContextKeyService, IContextKey } from 'vs/platform/contextkey/common/c
 import { ILifecycleService } from 'vs/platform/lifecycle/common/lifecycle';
 import { IPanelService } from 'vs/workbench/services/panel/common/panelService';
 import { IPartService } from 'vs/workbench/services/part/common/partService';
-import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
 import { ITerminalService, ITerminalInstance, IShellLaunchConfig, ITerminalConfigHelper, KEYBINDING_CONTEXT_TERMINAL_FOCUS, KEYBINDING_CONTEXT_TERMINAL_FIND_WIDGET_VISIBLE, TERMINAL_PANEL_ID } from 'vs/workbench/parts/terminal/common/terminal';
 import { TPromise } from 'vs/base/common/winjs.base';
-import { IEditorOptions } from 'vs/editor/common/config/editorOptions';
 
 export abstract class TerminalService implements ITerminalService {
 	public _serviceBrand: any;
@@ -44,7 +42,6 @@ export abstract class TerminalService implements ITerminalService {
 
 	constructor(
 		@IContextKeyService private _contextKeyService: IContextKeyService,
-		@IConfigurationService protected _configurationService: IConfigurationService,
 		@IPanelService protected _panelService: IPanelService,
 		@IPartService private _partService: IPartService,
 		@ILifecycleService lifecycleService: ILifecycleService
@@ -60,14 +57,6 @@ export abstract class TerminalService implements ITerminalService {
 		this._onInstanceTitleChanged = new Emitter<string>();
 		this._onInstancesChanged = new Emitter<string>();
 
-		this._configurationService.onDidChangeConfiguration(e => {
-			if (e.affectsConfiguration('terminal.integrated')) {
-				this.updateConfig();
-			}
-			if (e.affectsConfiguration('editor.accessibilitySupport')) {
-				this.updateAccessibilitySupport();
-			}
-		});
 		lifecycleService.onWillShutdown(event => event.veto(this._onWillShutdown()));
 		lifecycleService.onShutdown(() => this._onShutdown());
 		this._terminalFocusContextKey = KEYBINDING_CONTEXT_TERMINAL_FOCUS.bindTo(this._contextKeyService);
@@ -240,15 +229,6 @@ export abstract class TerminalService implements ITerminalService {
 		return terminalIndex;
 	}
 
-	public updateConfig(): void {
-		this.terminalInstances.forEach(instance => instance.updateConfig());
-	}
-
-	public updateAccessibilitySupport(): void {
-		const isEnabled = this._configurationService.getValue<IEditorOptions>('editor').accessibilitySupport === 'on';
-		this.terminalInstances.forEach(instance => instance.updateAccessibilitySupport(isEnabled));
-	}
-
 	public setWorkspaceShellAllowed(isAllowed: boolean): void {
 		this.configHelper.setWorkspaceShellAllowed(isAllowed);
 	}

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

@@ -11,21 +11,10 @@ import { IConfigurationService } from 'vs/platform/configuration/common/configur
 import { IWorkspaceConfigurationService } from 'vs/workbench/services/configuration/common/configuration';
 import { IChoiceService } from 'vs/platform/message/common/message';
 import { IStorageService, StorageScope } from 'vs/platform/storage/common/storage';
-import { ITerminalConfiguration, ITerminalConfigHelper, ITerminalFont, IShellLaunchConfig, IS_WORKSPACE_SHELL_ALLOWED_STORAGE_KEY } from 'vs/workbench/parts/terminal/common/terminal';
+import { ITerminalConfiguration, ITerminalConfigHelper, ITerminalFont, IShellLaunchConfig, IS_WORKSPACE_SHELL_ALLOWED_STORAGE_KEY, TERMINAL_CONFIG_SECTION } from 'vs/workbench/parts/terminal/common/terminal';
 import { TPromise } from 'vs/base/common/winjs.base';
 import Severity from 'vs/base/common/severity';
 import { isFedora } from 'vs/workbench/parts/terminal/electron-browser/terminal';
-import { deepClone } from 'vs/base/common/objects';
-
-interface IEditorConfiguration {
-	editor: IEditorOptions;
-}
-
-interface IFullTerminalConfiguration {
-	terminal: {
-		integrated: ITerminalConfiguration;
-	};
-}
 
 const DEFAULT_LINE_HEIGHT = 1.0;
 
@@ -41,16 +30,24 @@ export class TerminalConfigHelper implements ITerminalConfigHelper {
 
 	private _charMeasureElement: HTMLElement;
 	private _lastFontMeasurement: ITerminalFont;
+	public config: ITerminalConfiguration;
 
 	public constructor(
 		@IConfigurationService private _configurationService: IConfigurationService,
 		@IWorkspaceConfigurationService private _workspaceConfigurationService: IWorkspaceConfigurationService,
 		@IChoiceService private _choiceService: IChoiceService,
-		@IStorageService private _storageService: IStorageService) {
+		@IStorageService private _storageService: IStorageService
+	) {
+		this._updateConfig();
+		this._configurationService.onDidChangeConfiguration(e => {
+			if (e.affectsConfiguration(TERMINAL_CONFIG_SECTION)) {
+				this._updateConfig();
+			}
+		});
 	}
 
-	public get config(): ITerminalConfiguration {
-		return deepClone(this._configurationService.getValue<IFullTerminalConfiguration>().terminal.integrated);
+	private _updateConfig(): void {
+		this.config = this._configurationService.getValue<ITerminalConfiguration>(TERMINAL_CONFIG_SECTION);
 	}
 
 	private _measureFont(fontFamily: string, fontSize: number, lineHeight: number): ITerminalFont {
@@ -89,21 +86,19 @@ export class TerminalConfigHelper implements ITerminalConfigHelper {
 	 * terminal.integrated.fontSize, terminal.integrated.lineHeight configuration properties
 	 */
 	public getFont(excludeDimensions?: boolean): ITerminalFont {
-		const config = this._configurationService.getValue();
-		const editorConfig = (<IEditorConfiguration>config).editor;
-		const terminalConfig = this.config;
+		const editorConfig = this._configurationService.getValue<IEditorOptions>('editor');
 
-		let fontFamily = terminalConfig.fontFamily || editorConfig.fontFamily;
+		let fontFamily = this.config.fontFamily || editorConfig.fontFamily;
 
 		// Work around bad font on Fedora
-		if (!terminalConfig.fontFamily) {
+		if (!this.config.fontFamily) {
 			if (isFedora) {
 				fontFamily = '\'DejaVu Sans Mono\'';
 			}
 		}
 
-		let fontSize = this._toInteger(terminalConfig.fontSize, MINIMUM_FONT_SIZE, MAXIMUM_FONT_SIZE, EDITOR_FONT_DEFAULTS.fontSize);
-		const lineHeight = terminalConfig.lineHeight ? Math.max(terminalConfig.lineHeight, 1) : DEFAULT_LINE_HEIGHT;
+		let fontSize = this._toInteger(this.config.fontSize, MINIMUM_FONT_SIZE, MAXIMUM_FONT_SIZE, EDITOR_FONT_DEFAULTS.fontSize);
+		const lineHeight = this.config.lineHeight ? Math.max(this.config.lineHeight, 1) : DEFAULT_LINE_HEIGHT;
 
 		if (excludeDimensions) {
 			return {

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

@@ -171,6 +171,15 @@ export class TerminalInstance implements ITerminalInstance {
 				this.attachToElement(_container);
 			}
 		});
+
+		this._configurationService.onDidChangeConfiguration(e => {
+			if (e.affectsConfiguration('terminal.integrated')) {
+				this.updateConfig();
+			}
+			if (e.affectsConfiguration('editor.accessibilitySupport')) {
+				this.updateAccessibilitySupport();
+			}
+		});
 	}
 
 	public addDisposable(disposable: lifecycle.IDisposable): void {
@@ -967,8 +976,9 @@ export class TerminalInstance implements ITerminalInstance {
 		this._setEnableBell(this._configHelper.config.enableBell);
 	}
 
-	public updateAccessibilitySupport(isEnabled: boolean): void {
-		this._xterm.setOption('screenReaderMode', isEnabled);
+	public updateAccessibilitySupport(): void {
+		const value = this._configurationService.getValue('editor.accessibilitySupport');
+		this._xterm.setOption('screenReaderMode', value === 'on');
 	}
 
 	private _setCursorBlink(blink: boolean): void {

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

@@ -30,17 +30,17 @@ export class TerminalService extends AbstractTerminalService implements ITermina
 
 	constructor(
 		@IContextKeyService _contextKeyService: IContextKeyService,
-		@IConfigurationService _configurationService: IConfigurationService,
 		@IPanelService _panelService: IPanelService,
 		@IPartService _partService: IPartService,
 		@ILifecycleService _lifecycleService: ILifecycleService,
+		@IConfigurationService private _configurationService: IConfigurationService,
 		@IInstantiationService private _instantiationService: IInstantiationService,
 		@IQuickOpenService private _quickOpenService: IQuickOpenService,
 		@IChoiceService private _choiceService: IChoiceService,
 		@IStorageService private _storageService: IStorageService,
 		@IMessageService private _messageService: IMessageService
 	) {
-		super(_contextKeyService, _configurationService, _panelService, _partService, _lifecycleService);
+		super(_contextKeyService, _panelService, _partService, _lifecycleService);
 
 		this._configHelper = this._instantiationService.createInstance(TerminalConfigHelper);
 	}

src/vs/workbench/parts/terminal/test/electron-browser/terminalConfigHelper.test.ts

@@ -6,24 +6,10 @@
 'use strict';
 
 import * as assert from 'assert';
-import { IConfigurationService, getConfigurationValue, IConfigurationOverrides } from 'vs/platform/configuration/common/configuration';
-import { TPromise } from 'vs/base/common/winjs.base';
 import { TerminalConfigHelper } from 'vs/workbench/parts/terminal/electron-browser/terminalConfigHelper';
 import { EDITOR_FONT_DEFAULTS } from 'vs/editor/common/config/editorOptions';
 import { isFedora } from 'vs/workbench/parts/terminal/electron-browser/terminal';
-
-class MockConfigurationService implements IConfigurationService {
-	public _serviceBrand: any;
-	public serviceId = IConfigurationService;
-	public constructor(private configuration: any = {}) { }
-	public inspect<T>(key: string, overrides?: IConfigurationOverrides): any { return { value: getConfigurationValue<T>(this.getValue(), key), default: getConfigurationValue<T>(this.getValue(), key), user: getConfigurationValue<T>(this.getValue(), key), workspace: void 0, workspaceFolder: void 0 }; }
-	public keys() { return { default: [] as string[], user: [] as string[], workspace: [] as string[], workspaceFolder: [] as string[] }; }
-	public getValue(): any { return this.configuration; }
-	public updateValue(): TPromise<void> { return null; }
-	public getConfigurationData(): any { return null; }
-	public onDidChangeConfiguration() { return { dispose() { } }; }
-	public reloadConfiguration(): TPromise<void> { return null; }
-}
+import { TestConfigurationService } from 'vs/platform/configuration/test/common/testConfigurationService';
 
 suite('Workbench - TerminalConfigHelper', () => {
 	let fixture: HTMLElement;
@@ -33,33 +19,17 @@ suite('Workbench - TerminalConfigHelper', () => {
 	});
 
 	test('TerminalConfigHelper - getFont fontFamily', function () {
-		let configurationService: IConfigurationService;
-		let configHelper: TerminalConfigHelper;
-
-		configurationService = new MockConfigurationService({
-			editor: {
-				fontFamily: 'foo'
-			},
-			terminal: {
-				integrated: {
-					fontFamily: 'bar'
-				}
-			}
-		});
-		configHelper = new TerminalConfigHelper(configurationService, null, null, null);
+		const configurationService = new TestConfigurationService();
+		configurationService.setUserConfiguration('editor', { fontFamily: 'foo' });
+		configurationService.setUserConfiguration('terminal', { integrated: { fontFamily: 'bar' } });
+
+		let configHelper = new TerminalConfigHelper(configurationService, null, null, null);
 		configHelper.panelContainer = fixture;
 		assert.equal(configHelper.getFont().fontFamily, 'bar', 'terminal.integrated.fontFamily should be selected over editor.fontFamily');
 
-		configurationService = new MockConfigurationService({
-			editor: {
-				fontFamily: 'foo'
-			},
-			terminal: {
-				integrated: {
-					fontFamily: 0
-				}
-			}
-		});
+		configurationService.setUserConfiguration('terminal', { integrated: { fontFamily: null } });
+
+		// Recreate config helper as onDidChangeConfiguration isn't implemented in TestConfigurationService
 		configHelper = new TerminalConfigHelper(configurationService, null, null, null);
 		configHelper.panelContainer = fixture;
 		if (isFedora) {
@@ -70,64 +40,55 @@ suite('Workbench - TerminalConfigHelper', () => {
 	});
 
 	test('TerminalConfigHelper - getFont fontSize', function () {
-		let configurationService: IConfigurationService;
-		let configHelper: TerminalConfigHelper;
-
-		configurationService = new MockConfigurationService({
-			editor: {
-				fontFamily: 'foo',
-				fontSize: 9
-			},
-			terminal: {
-				integrated: {
-					fontFamily: 'bar',
-					fontSize: 10
-				}
+		const configurationService = new TestConfigurationService();
+
+		configurationService.setUserConfiguration('editor', {
+			fontFamily: 'foo',
+			fontSize: 9
+		});
+		configurationService.setUserConfiguration('terminal', {
+			integrated: {
+				fontFamily: 'bar',
+				fontSize: 10
 			}
 		});
-		configHelper = new TerminalConfigHelper(configurationService, null, null, null);
+		let configHelper = new TerminalConfigHelper(configurationService, null, null, null);
 		configHelper.panelContainer = fixture;
 		assert.equal(configHelper.getFont().fontSize, 10, 'terminal.integrated.fontSize should be selected over editor.fontSize');
 
-		configurationService = new MockConfigurationService({
-			editor: {
-				fontFamily: 'foo'
-			},
-			terminal: {
-				integrated: {
-					fontFamily: 0,
-					fontSize: 0
-				}
+		configurationService.setUserConfiguration('editor', {
+			fontFamily: 'foo'
+		});
+		configurationService.setUserConfiguration('terminal', {
+			integrated: {
+				fontFamily: null,
+				fontSize: 0
 			}
 		});
 		configHelper = new TerminalConfigHelper(configurationService, null, null, null);
 		configHelper.panelContainer = fixture;
 		assert.equal(configHelper.getFont().fontSize, 6, 'The minimum terminal font size should be used when terminal.integrated.fontSize less than it');
 
-		configurationService = new MockConfigurationService({
-			editor: {
-				fontFamily: 'foo'
-			},
-			terminal: {
-				integrated: {
-					fontFamily: 0,
-					fontSize: 1500
-				}
+		configurationService.setUserConfiguration('editor', {
+			fontFamily: 'foo'
+		});
+		configurationService.setUserConfiguration('terminal', {
+			integrated: {
+				fontFamily: 0,
+				fontSize: 1500
 			}
 		});
 		configHelper = new TerminalConfigHelper(configurationService, null, null, null);
 		configHelper.panelContainer = fixture;
 		assert.equal(configHelper.getFont().fontSize, 25, 'The maximum terminal font size should be used when terminal.integrated.fontSize more than it');
 
-		configurationService = new MockConfigurationService({
-			editor: {
-				fontFamily: 'foo',
-			},
-			terminal: {
-				integrated: {
-					fontFamily: 0,
-					fontSize: null
-				}
+		configurationService.setUserConfiguration('editor', {
+			fontFamily: 'foo'
+		});
+		configurationService.setUserConfiguration('terminal', {
+			integrated: {
+				fontFamily: 0,
+				fontSize: null
 			}
 		});
 		configHelper = new TerminalConfigHelper(configurationService, null, null, null);
@@ -136,35 +97,30 @@ suite('Workbench - TerminalConfigHelper', () => {
 	});
 
 	test('TerminalConfigHelper - getFont lineHeight', function () {
-		let configurationService: IConfigurationService;
-		let configHelper: TerminalConfigHelper;
-
-		configurationService = new MockConfigurationService({
-			editor: {
-				fontFamily: 'foo',
-				lineHeight: 1
-			},
-			terminal: {
-				integrated: {
-					fontFamily: 0,
-					lineHeight: 2
-				}
+		const configurationService = new TestConfigurationService();
+
+		configurationService.setUserConfiguration('editor', {
+			fontFamily: 'foo',
+			lineHeight: 1
+		});
+		configurationService.setUserConfiguration('terminal', {
+			integrated: {
+				fontFamily: 0,
+				lineHeight: 2
 			}
 		});
-		configHelper = new TerminalConfigHelper(configurationService, null, null, null);
+		let configHelper = new TerminalConfigHelper(configurationService, null, null, null);
 		configHelper.panelContainer = fixture;
 		assert.equal(configHelper.getFont().lineHeight, 2, 'terminal.integrated.lineHeight should be selected over editor.lineHeight');
 
-		configurationService = new MockConfigurationService({
-			editor: {
-				fontFamily: 'foo',
-				lineHeight: 1
-			},
-			terminal: {
-				integrated: {
-					fontFamily: 0,
-					lineHeight: 0
-				}
+		configurationService.setUserConfiguration('editor', {
+			fontFamily: 'foo',
+			lineHeight: 1
+		});
+		configurationService.setUserConfiguration('terminal', {
+			integrated: {
+				fontFamily: 0,
+				lineHeight: 0
 			}
 		});
 		configHelper = new TerminalConfigHelper(configurationService, null, null, null);

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

@@ -21,8 +21,11 @@ import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
 import { IContextKeyService } from 'vs/platform/contextkey/common/contextkey';
 import { IHistoryService } from 'vs/workbench/services/history/common/history';
 import { TPromise } from 'vs/base/common/winjs.base';
+import { TestConfigurationService } from 'vs/platform/configuration/test/common/testConfigurationService';
+import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
 
 class TestTerminalInstance extends TerminalInstance {
+
 	public _getCwd(shell: IShellLaunchConfig, root: Uri): string {
 		return super._getCwd(shell, root);
 	}
@@ -152,6 +155,7 @@ suite('Workbench - TerminalInstance', () => {
 			let keybindingService = new MockKeybindingService();
 			let terminalFocusContextKey = contextKeyService.createKey('test', false);
 			instantiationService = new TestInstantiationService();
+			instantiationService.stub(IConfigurationService, new TestConfigurationService());
 			instantiationService.stub(IMessageService, new TestMessageService());
 			instantiationService.stub(IWorkspaceContextService, new TestContextService());
 			instantiationService.stub(IKeybindingService, keybindingService);