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

Add setting for fontSize and lineHeight to the integrated terminal #6998

Merged
merged 1 commit into from
Jun 3, 2016
Merged

Add setting for fontSize and lineHeight to the integrated terminal #6998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions src/vs/workbench/parts/terminal/electron-browser/terminal.contribution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ configurationRegistry.registerConfiguration({
'terminal.integrated.fontFamily': {
'description': nls.localize('terminal.integrated.fontFamily', "The font family used by the terminal (CSS font-family format), this defaults to editor.fontFamily's value."),
'type': 'string'
},
'terminal.integrated.fontSize': {
Copy link
Member

Choose a reason for hiding this comment

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

Font size should have a default here, otherwise auto-complete does this.

image

'description': nls.localize('terminal.integrated.fontSize', "The font size used by the terminal (in pixels), this defaults to editor.fontSize's value."),
Copy link
Member

Choose a reason for hiding this comment

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

Sorry but let's go for consistency with the editor settings for this in commonEditorConfig.ts:

"Controls the font family of the terminal."
"Controls the font size of the terminal."
"Controls the line height of the terminal."

'type': 'number'
},
'terminal.integrated.lineHeight': {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, it needs the default set.

'description': nls.localize('terminal.integrated.lineHeight', "The line height used by the terminal (in pixels), this defaults to editor.lineHeight's value."),
'type': 'number'
}
}
});
Expand Down
4 changes: 3 additions & 1 deletion src/vs/workbench/parts/terminal/electron-browser/terminal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ export interface ITerminalConfiguration {
osx: string,
windows: string
},
fontFamily: string
fontFamily: string,
fontSize: number,
lineHeight: number
}
};
}
Expand Down
72 changes: 62 additions & 10 deletions src/vs/workbench/parts/terminal/electron-browser/terminalConfigHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {IConfiguration} from 'vs/editor/common/config/defaultConfig';
import {IConfigurationService} from 'vs/platform/configuration/common/configuration';
import {ITerminalConfiguration} from 'vs/workbench/parts/terminal/electron-browser/terminal';
import {IThemeService} from 'vs/workbench/services/themes/common/themeService';
import {GOLDEN_LINE_HEIGHT_RATIO} from 'vs/editor/common/config/defaultConfig';
import {Builder} from 'vs/base/browser/builder';

const DEFAULT_ANSI_COLORS = {
'hc-black': [
Expand Down Expand Up @@ -67,34 +69,73 @@ const DEFAULT_ANSI_COLORS = {
]
};

export interface ITerminalFont {
fontFamily: string;
fontSize: number;
lineHeight: number;
charWidth: number;
charHeight: number;
}

/**
* Encapsulates terminal configuration logic, the primary purpose of this file is so that platform
* specific test cases can be written.
*/
export class TerminalConfigHelper {
private charMeasureElement: HTMLElement;

public constructor(
private platform: Platform,
private configurationService: IConfigurationService,
private themeService: IThemeService) {
private themeService: IThemeService,
private parentDomElement: HTMLElement) {
}

public getTheme(): string[] {
let baseThemeId = getBaseThemeId(this.themeService.getTheme());
return DEFAULT_ANSI_COLORS[baseThemeId];
}

private neasureFont(fontFamily: string, fontSize: number, lineHeight: number): ITerminalFont {
if (!this.charMeasureElement) {
this.charMeasureElement = new Builder(this.parentDomElement, true).div().build().getHTMLElement();
}
let style = this.charMeasureElement.style;
style.display = 'inline';
Copy link
Member

Choose a reason for hiding this comment

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

Will this flicker the element on the screen? Should it be positioned absolutely off screen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check, but this triggers a synchronous layout and then immediatelly removes it I think it is not visible.

style.fontFamily = fontFamily;
style.fontSize = fontSize + 'px';
style.lineHeight = lineHeight + 'px';
this.charMeasureElement.innerText = 'X';
let rect = this.charMeasureElement.getBoundingClientRect();
style.display = 'none';
let charWidth = Math.ceil(rect.width);
let charHeight = Math.ceil(rect.height);
return {
fontFamily,
fontSize,
lineHeight,
charWidth,
charHeight
};
}

/**
* Set the terminal font to `terminal.integrated.fontFamily` if it is set, otherwise fallback to
* `editor.fontFamily`.
* Gets the font information based on the terminal.integrated.fontFamily,
* terminal.integrated.fontSize, terminal.integrated.lineHeight configuration properties
*/
public getFontFamily(): string {
let terminalConfig = this.configurationService.getConfiguration<ITerminalConfiguration>();
let fontFamily = terminalConfig.terminal.integrated.fontFamily;
if (!fontFamily) {
let editorConfig = this.configurationService.getConfiguration<IConfiguration>();
fontFamily = editorConfig.editor.fontFamily;
public getFont(): ITerminalFont {
let terminalConfig = this.configurationService.getConfiguration<ITerminalConfiguration>().terminal.integrated;
let editorConfig = this.configurationService.getConfiguration<IConfiguration>();

let fontFamily = terminalConfig.fontFamily || editorConfig.editor.fontFamily;
let fontSize = this.toInteger(terminalConfig.fontSize, 0) || editorConfig.editor.fontSize;
let lineHeight = this.toInteger(terminalConfig.lineHeight, 0) || editorConfig.editor.lineHeight;

if (lineHeight === 0) {
lineHeight = Math.round(GOLDEN_LINE_HEIGHT_RATIO * fontSize);
Copy link
Member

Choose a reason for hiding this comment

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

We basically want consistency with how editor.lineHeight works here, is this how that falls back?

}
return fontFamily;

return this.neasureFont(fontFamily, fontSize, lineHeight);
}

public getShell(): string {
Expand All @@ -107,4 +148,15 @@ export class TerminalConfigHelper {
}
return config.terminal.integrated.shell.linux;
}

private toInteger(source: any, minimum?: number): number {
let r = parseInt(source, 10);
if (isNaN(r)) {
r = 0;
}
if (typeof minimum === 'number') {
r = Math.max(minimum, r);
}
return r;
}
}
49 changes: 25 additions & 24 deletions src/vs/workbench/parts/terminal/electron-browser/terminalPanel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,7 @@ import {ITerminalService, TERMINAL_PANEL_ID} from 'vs/workbench/parts/terminal/e
import {Panel} from 'vs/workbench/browser/panel';
import {DomScrollableElement} from 'vs/base/browser/ui/scrollbar/scrollableElement';
import {ScrollbarVisibility} from 'vs/base/browser/ui/scrollbar/scrollableElementOptions';
import {TerminalConfigHelper} from 'vs/workbench/parts/terminal/electron-browser/terminalConfigHelper';

const TERMINAL_CHAR_WIDTH = 8;
const TERMINAL_CHAR_HEIGHT = 18;
import {TerminalConfigHelper, ITerminalFont} from 'vs/workbench/parts/terminal/electron-browser/terminalConfigHelper';

export class TerminalPanel extends Panel {

Expand All @@ -35,6 +32,7 @@ export class TerminalPanel extends Panel {
private terminal;
private terminalDomElement: HTMLDivElement;
private configurationHelper: TerminalConfigHelper;
private font: ITerminalFont;

constructor(
@IConfigurationService private configurationService: IConfigurationService,
Expand All @@ -44,22 +42,23 @@ export class TerminalPanel extends Panel {
@IThemeService private themeService: IThemeService
) {
super(TERMINAL_PANEL_ID, telemetryService);
this.configurationHelper = new TerminalConfigHelper(platform.platform, this.configurationService, this.themeService);
this.toDispose = [];
}

public layout(dimension: Dimension): void {
let cols = Math.floor(this.parentDomElement.offsetWidth / TERMINAL_CHAR_WIDTH);
let rows = Math.floor(this.parentDomElement.offsetHeight / TERMINAL_CHAR_HEIGHT);
if (this.terminal) {
this.terminal.resize(cols, rows);
}
if (this.ptyProcess.connected) {
this.ptyProcess.send({
event: 'resize',
cols: cols,
rows: rows
});
if (this.font.charWidth && this.font.charHeight) {
Copy link
Member

Choose a reason for hiding this comment

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

Prefer early exits to keep indentation low:

if (!this.font.charWidth || !this.font.charHeight) {
    return;
}

Also should this attempt to measure at this point or would that just be rework?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a matter of taste actually 😄 I prefer one entry point - one exit point in methods.
But to keep to the conventions I'll change it 😄

Copy link
Member

Choose a reason for hiding this comment

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

@kisstkondoros I used to be in that camp but I came around 😛

let cols = Math.floor(this.parentDomElement.offsetWidth / this.font.charWidth);
let rows = Math.floor(this.parentDomElement.offsetHeight / this.font.charHeight);
if (this.terminal) {
this.terminal.resize(cols, rows);
}
if (this.ptyProcess.connected) {
this.ptyProcess.send({
event: 'resize',
cols: cols,
rows: rows
});
}
}
}

Expand Down Expand Up @@ -98,6 +97,7 @@ export class TerminalPanel extends Panel {

private createTerminal(): TPromise<void> {
return new TPromise<void>(resolve => {
this.configurationHelper = new TerminalConfigHelper(platform.platform, this.configurationService, this.themeService, this.parentDomElement);
this.parentDomElement.innerHTML = '';
this.ptyProcess = this.createTerminalProcess();
this.terminalDomElement = document.createElement('div');
Expand Down Expand Up @@ -157,13 +157,13 @@ export class TerminalPanel extends Panel {
this.setTerminalTheme();
}));
this.toDispose.push(this.configurationService.onDidUpdateConfiguration((e) => {
this.setTerminalFont();
this.updateFont();
}));

this.terminal.open(this.terminalDomElement);
this.parentDomElement.appendChild(terminalScrollbar.getDomNode());

this.setTerminalFont();
this.updateFont();
Copy link
Member

Choose a reason for hiding this comment

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

Why not measure the characters at this point?

this.setTerminalTheme();
resolve(void 0);
});
Expand All @@ -177,14 +177,15 @@ export class TerminalPanel extends Panel {
this.terminal.refresh(0, this.terminal.rows);
}

/**
* Set the terminal font to `terminal.integrated.fontFamily` if it is set, otherwise fallback to
* `editor.fontFamily`.
*/
private setTerminalFont(): void {
this.terminalDomElement.style.fontFamily = this.configurationHelper.getFontFamily();
private updateFont(): void {
this.font = this.configurationHelper.getFont();
Copy link
Member

Choose a reason for hiding this comment

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

Fix indentation

this.terminalDomElement.style.fontFamily = this.font.fontFamily;
this.terminalDomElement.style.lineHeight = this.font.lineHeight + 'px';
this.terminalDomElement.style.fontSize = this.font.fontSize + 'px';
this.layout(new Dimension(this.parentDomElement.offsetWidth, this.parentDomElement.offsetHeight));
}


public focus(): void {
this.focusTerminal(true);
}
Expand Down
94 changes: 83 additions & 11 deletions src/vs/workbench/parts/terminal/test/terminalConfigHelper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,19 @@ class MockThemeService implements IThemeService {
}

suite('Workbench - TerminalConfigHelper', () => {
test('TerminalConfigHelper - getFontFamily', function () {
let fixture: HTMLElement;
let fixtureId = 'terminal-config-helper-fixture';
setup(() => {
fixture = document.createElement('div');
fixture.id = fixtureId;
document.body.appendChild(fixture);
});

teardown(() => {
document.body.removeChild(fixture);
});

test('TerminalConfigHelper - getFont', function () {
let themeService: IThemeService = new MockThemeService();
let configurationService: IConfigurationService;
let configHelper: TerminalConfigHelper;
Expand All @@ -51,8 +63,8 @@ suite('Workbench - TerminalConfigHelper', () => {
}
}
});
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService);
assert.equal(configHelper.getFontFamily(), 'bar', 'terminal.integrated.fontFamily should be selected over editor.fontFamily');
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService, fixture);
assert.equal(configHelper.getFont().fontFamily, 'bar', 'terminal.integrated.fontFamily should be selected over editor.fontFamily');

configurationService = new MockConfigurationService({
editor: {
Expand All @@ -64,8 +76,68 @@ suite('Workbench - TerminalConfigHelper', () => {
}
}
});
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService);
assert.equal(configHelper.getFontFamily(), 'foo', 'editor.fontFamily should be the fallback when terminal.integrated.fontFamily not set');
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService, fixture);
assert.equal(configHelper.getFont().fontFamily, 'foo', 'editor.fontFamily should be the fallback when terminal.integrated.fontFamily not set');

configurationService = new MockConfigurationService({
editor: {
fontFamily: 'foo',
fontSize: 1
},
terminal: {
integrated: {
fontFamily: 'bar',
fontSize: 2
}
}
});
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService, fixture);
assert.equal(configHelper.getFont().fontSize, 2, 'terminal.integrated.fontSize should be selected over editor.fontSize');

configurationService = new MockConfigurationService({
editor: {
fontFamily: 'foo',
fontSize: 1
},
terminal: {
integrated: {
fontFamily: undefined,
fontSize: undefined
}
}
});
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService, fixture);
assert.equal(configHelper.getFont().fontSize, 1, 'editor.fontSize should be the fallback when terminal.integrated.fontSize not set');

configurationService = new MockConfigurationService({
editor: {
fontFamily: 'foo',
lineHeight: 1
},
terminal: {
integrated: {
fontFamily: undefined,
lineHeight: 2
}
}
});
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService, 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: undefined,
lineHeight: undefined
}
}
});
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService, fixture);
assert.equal(configHelper.getFont().lineHeight, 1, 'editor.lineHeight should be the fallback when terminal.integrated.lineHeight not set');
});

test('TerminalConfigHelper - getShell', function () {
Expand All @@ -82,7 +154,7 @@ suite('Workbench - TerminalConfigHelper', () => {
}
}
});
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService);
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService, fixture);
assert.equal(configHelper.getShell(), 'foo', 'terminal.integrated.shell.linux should be selected on Linux');

configurationService = new MockConfigurationService({
Expand All @@ -94,7 +166,7 @@ suite('Workbench - TerminalConfigHelper', () => {
}
}
});
configHelper = new TerminalConfigHelper(Platform.Mac, configurationService, themeService);
configHelper = new TerminalConfigHelper(Platform.Mac, configurationService, themeService, fixture);
assert.equal(configHelper.getShell(), 'foo', 'terminal.integrated.shell.osx should be selected on OS X');

configurationService = new MockConfigurationService({
Expand All @@ -106,7 +178,7 @@ suite('Workbench - TerminalConfigHelper', () => {
}
}
});
configHelper = new TerminalConfigHelper(Platform.Windows, configurationService, themeService);
configHelper = new TerminalConfigHelper(Platform.Windows, configurationService, themeService, fixture);
assert.equal(configHelper.getShell(), 'foo', 'terminal.integrated.shell.windows should be selected on Windows');
});

Expand All @@ -116,7 +188,7 @@ suite('Workbench - TerminalConfigHelper', () => {
let configHelper: TerminalConfigHelper;

themeService = new MockThemeService('hc-black foo');
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService);
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService, fixture);
assert.deepEqual(configHelper.getTheme(), [
'#000000',
'#cd0000',
Expand All @@ -137,7 +209,7 @@ suite('Workbench - TerminalConfigHelper', () => {
], 'The high contrast terminal theme should be selected when the hc-black theme is active');

themeService = new MockThemeService('vs foo');
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService);
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService, fixture);
assert.deepEqual(configHelper.getTheme(), [
'#000000',
'#cd3131',
Expand All @@ -158,7 +230,7 @@ suite('Workbench - TerminalConfigHelper', () => {
], 'The light terminal theme should be selected when a vs theme is active');

themeService = new MockThemeService('vs-dark foo');
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService);
configHelper = new TerminalConfigHelper(Platform.Linux, configurationService, themeService, fixture);
assert.deepEqual(configHelper.getTheme(), [
'#000000',
'#cd3131',
Expand Down