Skip to content

Support 'px' format in lineHeight option #5363

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 10 additions & 1 deletion addons/addon-webgl/src/CharAtlasUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ import { IColorSet, ReadonlyColorSet } from 'browser/Types';
import { NULL_COLOR } from 'common/Color';

export function generateConfig(deviceCellWidth: number, deviceCellHeight: number, deviceCharWidth: number, deviceCharHeight: number, options: Required<ITerminalOptions>, colors: ReadonlyColorSet, devicePixelRatio: number, deviceMaxTextureSize: number): ICharAtlasConfig {
// Convert lineHeight to a numeric multiplier for the char atlas
// If it's a px value, calculate the equivalent multiplier
let lineHeightMultiplier: number = typeof options.lineHeight === 'number' ? options.lineHeight : 1;
if (typeof options.lineHeight === 'string' && options.lineHeight.endsWith('px')) {
const pxValue = parseFloat(options.lineHeight.slice(0, -2));
// Calculate the multiplier based on font size
lineHeightMultiplier = pxValue / options.fontSize;
}

// null out some fields that don't matter
const clonedColors: IColorSet = {
foreground: colors.foreground,
Expand All @@ -36,7 +45,7 @@ export function generateConfig(deviceCellWidth: number, deviceCellHeight: number
devicePixelRatio,
deviceMaxTextureSize,
letterSpacing: options.letterSpacing,
lineHeight: options.lineHeight,
lineHeight: lineHeightMultiplier,
deviceCellWidth: deviceCellWidth,
deviceCellHeight: deviceCellHeight,
deviceCharWidth: deviceCharWidth,
Expand Down
23 changes: 18 additions & 5 deletions addons/addon-webgl/src/WebglRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@ import { addDisposableListener } from 'vs/base/browser/dom';
import { combinedDisposable, Disposable, MutableDisposable, toDisposable } from 'vs/base/common/lifecycle';
import { createRenderDimensions } from 'browser/renderer/shared/RendererUtils';

/**
* Calculates the line height in pixels based on the lineHeight option and character height.
*/
function calculateLineHeightInPixels(lineHeight: number | string, charHeight: number): number {
if (typeof lineHeight === 'string' && lineHeight.endsWith('px')) {
const pxValue = parseFloat(lineHeight.slice(0, -2));
// Ensure the pixel value is at least as large as the character height
return Math.max(pxValue, charHeight);
}
// Numeric lineHeight acts as a multiplier
return Math.floor(charHeight * (lineHeight as number));
}

export class WebglRenderer extends Disposable implements IRenderer {
private _renderLayers: IRenderLayer[];
private _cursorBlinkStateManager: MutableDisposable<CursorBlinkStateManager> = new MutableDisposable();
Expand Down Expand Up @@ -570,14 +583,14 @@ export class WebglRenderer extends Disposable implements IRenderer {
// cell.
this.dimensions.device.char.height = Math.ceil(this._charSizeService.height * this._devicePixelRatio);

// Calculate the device cell height, if lineHeight is _not_ 1, the resulting value will be
// floored since lineHeight can never be lower then 1, this guarentees the device cell height
// will always be larger than device char height.
this.dimensions.device.cell.height = Math.floor(this.dimensions.device.char.height * this._optionsService.rawOptions.lineHeight);
// Calculate the device cell height, supporting both numeric multipliers and 'px' values
this.dimensions.device.cell.height = calculateLineHeightInPixels(this._optionsService.rawOptions.lineHeight, this.dimensions.device.char.height);

// Calculate the y offset within a cell that glyph should draw at in order for it to be centered
// correctly within the cell.
this.dimensions.device.char.top = this._optionsService.rawOptions.lineHeight === 1 ? 0 : Math.round((this.dimensions.device.cell.height - this.dimensions.device.char.height) / 2);
const isDefaultLineHeight = (typeof this._optionsService.rawOptions.lineHeight === 'number' && this._optionsService.rawOptions.lineHeight === 1) ||
(typeof this._optionsService.rawOptions.lineHeight === 'string' && this.dimensions.device.cell.height === this.dimensions.device.char.height);
this.dimensions.device.char.top = isDefaultLineHeight ? 0 : Math.round((this.dimensions.device.cell.height - this.dimensions.device.char.height) / 2);

// Calculate the device cell width, taking the letterSpacing into account.
this.dimensions.device.cell.width = this.dimensions.device.char.width + Math.round(this._optionsService.rawOptions.letterSpacing);
Expand Down
78 changes: 78 additions & 0 deletions src/browser/renderer/LineHeight.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/**
* Copyright (c) 2023 The xterm.js authors. All rights reserved.
* @license MIT
*/

import { assert } from 'chai';

/**
* Calculates the line height in pixels based on the lineHeight option and character height.
* This is the same function used in both DomRenderer and WebglRenderer.
*/
function calculateLineHeightInPixels(lineHeight: number | string, charHeight: number): number {
if (typeof lineHeight === 'string' && lineHeight.endsWith('px')) {
const pxValue = parseFloat(lineHeight.slice(0, -2));
// Ensure the pixel value is at least as large as the character height
return Math.max(pxValue, charHeight);
}
// Numeric lineHeight acts as a multiplier
return Math.floor(charHeight * (lineHeight as number));
}

describe('calculateLineHeightInPixels', () => {
describe('numeric lineHeight', () => {
it('should calculate correctly with multiplier 1', () => {
assert.equal(calculateLineHeightInPixels(1, 20), 20);
});

it('should calculate correctly with multiplier 1.2', () => {
assert.equal(calculateLineHeightInPixels(1.2, 20), 24);
});

it('should calculate correctly with multiplier 1.5', () => {
assert.equal(calculateLineHeightInPixels(1.5, 20), 30);
});

it('should floor the result', () => {
assert.equal(calculateLineHeightInPixels(1.3, 20), 26); // Math.floor(26)
});
});

describe('string lineHeight with px format', () => {
it('should use px value directly when larger than char height', () => {
assert.equal(calculateLineHeightInPixels('25px', 20), 25);
});

it('should use char height when px value is smaller', () => {
assert.equal(calculateLineHeightInPixels('15px', 20), 20);
});

it('should handle decimal px values', () => {
assert.equal(calculateLineHeightInPixels('23.5px', 20), 23.5);
});

it('should handle edge case where px equals char height', () => {
assert.equal(calculateLineHeightInPixels('20px', 20), 20);
});

it('should handle very large px values', () => {
assert.equal(calculateLineHeightInPixels('100px', 20), 100);
});
});

describe('edge cases', () => {
it('should handle zero char height with numeric multiplier', () => {
assert.equal(calculateLineHeightInPixels(1.5, 0), 0);
});

it('should handle zero char height with px value', () => {
assert.equal(calculateLineHeightInPixels('25px', 0), 25);
});

it('should handle 1px with various char heights', () => {
assert.equal(calculateLineHeightInPixels('1px', 5), 5);
assert.equal(calculateLineHeightInPixels('1px', 20), 20);
assert.equal(calculateLineHeightInPixels('1px', 0), 1);
});
});
});
15 changes: 14 additions & 1 deletion src/browser/renderer/dom/DomRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,19 @@ const BG_CLASS_PREFIX = 'xterm-bg-';
const FOCUS_CLASS = 'xterm-focus';
const SELECTION_CLASS = 'xterm-selection';

/**
* Calculates the line height in pixels based on the lineHeight option and character height.
*/
function calculateLineHeightInPixels(lineHeight: number | string, charHeight: number): number {
if (typeof lineHeight === 'string' && lineHeight.endsWith('px')) {
const pxValue = parseFloat(lineHeight.slice(0, -2));
// Ensure the pixel value is at least as large as the character height
return Math.max(pxValue, charHeight);
}
// Numeric lineHeight acts as a multiplier
return Math.floor(charHeight * (lineHeight as number));
}

let nextTerminalId = 1;

/**
Expand Down Expand Up @@ -116,7 +129,7 @@ export class DomRenderer extends Disposable implements IRenderer {
this.dimensions.device.char.width = this._charSizeService.width * dpr;
this.dimensions.device.char.height = Math.ceil(this._charSizeService.height * dpr);
this.dimensions.device.cell.width = this.dimensions.device.char.width + Math.round(this._optionsService.rawOptions.letterSpacing);
this.dimensions.device.cell.height = Math.floor(this.dimensions.device.char.height * this._optionsService.rawOptions.lineHeight);
this.dimensions.device.cell.height = calculateLineHeightInPixels(this._optionsService.rawOptions.lineHeight, this.dimensions.device.char.height);
this.dimensions.device.char.left = 0;
this.dimensions.device.char.top = 0;
this.dimensions.device.canvas.width = this.dimensions.device.cell.width * this._bufferService.cols;
Expand Down
86 changes: 86 additions & 0 deletions src/common/services/OptionsService.lineHeight.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/**
* Copyright (c) 2023 The xterm.js authors. All rights reserved.
* @license MIT
*/

import { assert } from 'chai';
import { OptionsService } from 'common/services/OptionsService';

describe('OptionsService lineHeight', () => {
let optionsService: OptionsService;

beforeEach(() => {
optionsService = new OptionsService({});
});

describe('numeric lineHeight', () => {
it('should accept numeric values', () => {
optionsService.options.lineHeight = 1.5;
assert.equal(optionsService.rawOptions.lineHeight, 1.5);
});

it('should reject values less than 1', () => {
assert.throws(() => optionsService.options.lineHeight = 0.5);
});

it('should accept 1 as minimum value', () => {
optionsService.options.lineHeight = 1;
assert.equal(optionsService.rawOptions.lineHeight, 1);
});
});

describe('string lineHeight with px format', () => {
it('should accept px values', () => {
optionsService.options.lineHeight = '24px';
assert.equal(optionsService.rawOptions.lineHeight, '24px');
});

it('should accept decimal px values', () => {
optionsService.options.lineHeight = '23.5px';
assert.equal(optionsService.rawOptions.lineHeight, '23.5px');
});

it('should reject px values less than 1', () => {
assert.throws(() => optionsService.options.lineHeight = '0.5px');
});

it('should accept 1px as minimum value', () => {
optionsService.options.lineHeight = '1px';
assert.equal(optionsService.rawOptions.lineHeight, '1px');
});

it('should reject non-px string values', () => {
assert.throws(() => optionsService.options.lineHeight = '24' as any);
assert.throws(() => optionsService.options.lineHeight = '24em' as any);
assert.throws(() => optionsService.options.lineHeight = 'normal' as any);
});

it('should reject invalid px values', () => {
assert.throws(() => optionsService.options.lineHeight = 'invalidpx' as any);
assert.throws(() => optionsService.options.lineHeight = 'px' as any);
});
});

describe('invalid lineHeight types', () => {
it('should reject null', () => {
assert.throws(() => optionsService.options.lineHeight = null as any);
});

it('should reject undefined when explicitly set', () => {
assert.throws(() => optionsService.options.lineHeight = undefined as any);
});

it('should reject boolean values', () => {
assert.throws(() => optionsService.options.lineHeight = true as any);
assert.throws(() => optionsService.options.lineHeight = false as any);
});

it('should reject arrays', () => {
assert.throws(() => optionsService.options.lineHeight = [] as any);
});

it('should reject objects', () => {
assert.throws(() => optionsService.options.lineHeight = {} as any);
});
});
});
21 changes: 20 additions & 1 deletion src/common/services/OptionsService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,12 +172,31 @@ export class OptionsService extends Disposable implements IOptionsService {
case 'cursorWidth':
value = Math.floor(value);
// Fall through for bounds check
case 'lineHeight':
case 'tabStopWidth':
if (value < 1) {
throw new Error(`${key} cannot be less than 1, value: ${value}`);
}
break;
case 'lineHeight':
if (typeof value === 'string') {
// Handle 'px' format
if (value.endsWith('px')) {
const pxValue = parseFloat(value.slice(0, -2));
if (isNaN(pxValue) || pxValue < 1) {
throw new Error(`${key} with 'px' format must be a valid number >= 1, value: ${value}`);
}
// Keep the string value as-is for downstream processing
} else {
throw new Error(`${key} string value must end with 'px', value: ${value}`);
}
} else if (typeof value === 'number') {
if (value < 1) {
throw new Error(`${key} cannot be less than 1, value: ${value}`);
}
} else {
throw new Error(`${key} must be a number or string with 'px' suffix, value: ${value}`);
}
break;
case 'minimumContrastRatio':
value = Math.max(1, Math.min(21, Math.round(value * 10) / 10));
break;
Expand Down
2 changes: 1 addition & 1 deletion src/common/services/Services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ export interface ITerminalOptions {
fontWeightBold?: FontWeight;
ignoreBracketedPasteMode?: boolean;
letterSpacing?: number;
lineHeight?: number;
lineHeight?: number | string;
linkHandler?: ILinkHandler | null;
logLevel?: LogLevel;
logger?: ILogger | null;
Expand Down
6 changes: 4 additions & 2 deletions typings/xterm-headless.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,11 @@ declare module '@xterm/headless' {
letterSpacing?: number;

/**
* The line height used to render text.
* The line height used to render text. This can be specified as:
* - A number (e.g., 1.2) which acts as a multiplier of the font size
* - A string with 'px' suffix (e.g., '24px') which specifies an absolute pixel height
*/
lineHeight?: number;
lineHeight?: number | string;

/**
* What log level to use, this will log for all levels below and including
Expand Down
6 changes: 4 additions & 2 deletions typings/xterm.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,11 @@ declare module '@xterm/xterm' {
letterSpacing?: number;

/**
* The line height used to render text.
* The line height used to render text. This can be specified as:
* - A number (e.g., 1.2) which acts as a multiplier of the font size
* - A string with 'px' suffix (e.g., '24px') which specifies an absolute pixel height
*/
lineHeight?: number;
lineHeight?: number | string;

/**
* The handler for OSC 8 hyperlinks. Links will use the `confirm` browser
Expand Down