From 18fb1349105a3de4e5520d748c4a0258913b1204 Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Sun, 4 Mar 2018 20:55:42 -0800 Subject: [PATCH] Remove didCharSizeChange logic from renderer The only thing `didCharSizeChange` was used for was to figure out if we need to refresh the character atlas or not. However, `acquireCharAtlas` already inspects the atlas owned by this terminal, and if it matches, it avoids generating a new atlas. So if `didCharSizeChange` is true and it's not needed, it quickly bails out as a no-op. But if `didCharSizeChange` is false, and it was needed (possible given the complexity of testing all of these edge cases), it could introduce a bug. I think it makes sense to just get rid of `didCharSizeChange`, and just always call `acquireCharAtlas`. --- src/Terminal.ts | 8 +++----- src/renderer/BaseRenderLayer.ts | 6 ++---- src/renderer/CursorRenderLayer.ts | 4 ++-- src/renderer/LinkRenderLayer.ts | 4 ++-- src/renderer/Renderer.ts | 8 ++++---- src/renderer/SelectionRenderLayer.ts | 4 ++-- src/renderer/TextRenderLayer.ts | 4 ++-- src/renderer/Types.ts | 4 ++-- src/utils/TestUtils.test.ts | 2 +- 9 files changed, 20 insertions(+), 24 deletions(-) diff --git a/src/Terminal.ts b/src/Terminal.ts index 721996c967..fdf168304a 100644 --- a/src/Terminal.ts +++ b/src/Terminal.ts @@ -472,11 +472,9 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT case 'lineHeight': case 'fontWeight': case 'fontWeightBold': - const didCharSizeChange = (key === 'fontWeight' || key === 'fontWeightBold' || key === 'enableBold'); - // When the font changes the size of the cells may change which requires a renderer clear this.renderer.clear(); - this.renderer.onResize(this.cols, this.rows, didCharSizeChange); + this.renderer.onResize(this.cols, this.rows); this.refresh(0, this.rows - 1); case 'scrollback': this.buffers.resize(this.cols, this.rows); @@ -702,14 +700,14 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT this.viewport.onThemeChanged(this.renderer.colorManager.colors); this.on('cursormove', () => this.renderer.onCursorMove()); - this.on('resize', () => this.renderer.onResize(this.cols, this.rows, false)); + this.on('resize', () => this.renderer.onResize(this.cols, this.rows)); this.on('blur', () => this.renderer.onBlur()); this.on('focus', () => this.renderer.onFocus()); this.on('dprchange', () => this.renderer.onWindowResize(window.devicePixelRatio)); // dprchange should handle this case, we need this as well for browsers that don't support the // matchMedia query. window.addEventListener('resize', () => this.renderer.onWindowResize(window.devicePixelRatio)); - this.charMeasure.on('charsizechanged', () => this.renderer.onResize(this.cols, this.rows, true)); + this.charMeasure.on('charsizechanged', () => this.renderer.onResize(this.cols, this.rows)); this.renderer.on('resize', (dimensions) => this.viewport.syncScrollArea()); this.selectionManager = new SelectionManager(this, this.charMeasure); diff --git a/src/renderer/BaseRenderLayer.ts b/src/renderer/BaseRenderLayer.ts index 9423f65d39..f977d4c545 100644 --- a/src/renderer/BaseRenderLayer.ts +++ b/src/renderer/BaseRenderLayer.ts @@ -93,7 +93,7 @@ export abstract class BaseRenderLayer implements IRenderLayer { } } - public resize(terminal: ITerminal, dim: IRenderDimensions, charSizeChanged: boolean): void { + public resize(terminal: ITerminal, dim: IRenderDimensions): void { this._scaledCellWidth = dim.scaledCellWidth; this._scaledCellHeight = dim.scaledCellHeight; this._scaledCharWidth = dim.scaledCharWidth; @@ -110,9 +110,7 @@ export abstract class BaseRenderLayer implements IRenderLayer { this.clearAll(); } - if (charSizeChanged) { - this._refreshCharAtlas(terminal, this._colors); - } + this._refreshCharAtlas(terminal, this._colors); } public abstract reset(terminal: ITerminal): void; diff --git a/src/renderer/CursorRenderLayer.ts b/src/renderer/CursorRenderLayer.ts index d430b81d68..8db861d534 100644 --- a/src/renderer/CursorRenderLayer.ts +++ b/src/renderer/CursorRenderLayer.ts @@ -45,8 +45,8 @@ export class CursorRenderLayer extends BaseRenderLayer { // TODO: Consider initial options? Maybe onOptionsChanged should be called at the end of open? } - public resize(terminal: ITerminal, dim: IRenderDimensions, charSizeChanged: boolean): void { - super.resize(terminal, dim, charSizeChanged); + public resize(terminal: ITerminal, dim: IRenderDimensions): void { + super.resize(terminal, dim); // Resizing the canvas discards the contents of the canvas so clear state this._state = { x: null, diff --git a/src/renderer/LinkRenderLayer.ts b/src/renderer/LinkRenderLayer.ts index 61352f1540..4071523a08 100644 --- a/src/renderer/LinkRenderLayer.ts +++ b/src/renderer/LinkRenderLayer.ts @@ -18,8 +18,8 @@ export class LinkRenderLayer extends BaseRenderLayer { terminal.linkifier.on(LinkHoverEventTypes.LEAVE, (e: ILinkHoverEvent) => this._onLinkLeave(e)); } - public resize(terminal: ITerminal, dim: IRenderDimensions, charSizeChanged: boolean): void { - super.resize(terminal, dim, charSizeChanged); + public resize(terminal: ITerminal, dim: IRenderDimensions): void { + super.resize(terminal, dim); // Resizing the canvas discards the contents of the canvas so clear state this._state = null; } diff --git a/src/renderer/Renderer.ts b/src/renderer/Renderer.ts index fa1e34e628..bd0b688aa2 100644 --- a/src/renderer/Renderer.ts +++ b/src/renderer/Renderer.ts @@ -84,7 +84,7 @@ export class Renderer extends EventEmitter implements IRenderer { // and the terminal needs to refreshed if (this._devicePixelRatio !== devicePixelRatio) { this._devicePixelRatio = devicePixelRatio; - this.onResize(this._terminal.cols, this._terminal.rows, true); + this.onResize(this._terminal.cols, this._terminal.rows); } } @@ -106,12 +106,12 @@ export class Renderer extends EventEmitter implements IRenderer { return this.colorManager.colors; } - public onResize(cols: number, rows: number, didCharSizeChange: boolean): void { + public onResize(cols: number, rows: number): void { // Update character and canvas dimensions this._updateDimensions(); // Resize all render layers - this._renderLayers.forEach(l => l.resize(this._terminal, this.dimensions, didCharSizeChange)); + this._renderLayers.forEach(l => l.resize(this._terminal, this.dimensions)); // Force a refresh if (this._isPaused) { @@ -131,7 +131,7 @@ export class Renderer extends EventEmitter implements IRenderer { } public onCharSizeChanged(): void { - this.onResize(this._terminal.cols, this._terminal.rows, true); + this.onResize(this._terminal.cols, this._terminal.rows); } public onBlur(): void { diff --git a/src/renderer/SelectionRenderLayer.ts b/src/renderer/SelectionRenderLayer.ts index 53fc9b39d3..7a5557facc 100644 --- a/src/renderer/SelectionRenderLayer.ts +++ b/src/renderer/SelectionRenderLayer.ts @@ -20,8 +20,8 @@ export class SelectionRenderLayer extends BaseRenderLayer { }; } - public resize(terminal: ITerminal, dim: IRenderDimensions, charSizeChanged: boolean): void { - super.resize(terminal, dim, charSizeChanged); + public resize(terminal: ITerminal, dim: IRenderDimensions): void { + super.resize(terminal, dim); // Resizing the canvas discards the contents of the canvas so clear state this._state = { start: null, diff --git a/src/renderer/TextRenderLayer.ts b/src/renderer/TextRenderLayer.ts index a2ab9f19b8..3ab1fdf098 100644 --- a/src/renderer/TextRenderLayer.ts +++ b/src/renderer/TextRenderLayer.ts @@ -27,8 +27,8 @@ export class TextRenderLayer extends BaseRenderLayer { this._state = new GridCache(); } - public resize(terminal: ITerminal, dim: IRenderDimensions, charSizeChanged: boolean): void { - super.resize(terminal, dim, charSizeChanged); + public resize(terminal: ITerminal, dim: IRenderDimensions): void { + super.resize(terminal, dim); // Clear the character width cache if the font or width has changed const terminalFont = this._getFont(terminal, false); diff --git a/src/renderer/Types.ts b/src/renderer/Types.ts index 6f6e5f0ee3..1885d05914 100644 --- a/src/renderer/Types.ts +++ b/src/renderer/Types.ts @@ -24,7 +24,7 @@ export interface IRenderer extends IEventEmitter { setTheme(theme: ITheme): IColorSet; onWindowResize(devicePixelRatio: number): void; - onResize(cols: number, rows: number, didCharSizeChange: boolean): void; + onResize(cols: number, rows: number): void; onCharSizeChanged(): void; onBlur(): void; onFocus(): void; @@ -103,7 +103,7 @@ export interface IRenderLayer { /** * Resize the render layer. */ - resize(terminal: ITerminal, dim: IRenderDimensions, charSizeChanged: boolean): void; + resize(terminal: ITerminal, dim: IRenderDimensions): void; /** * Clear the state of the render layer. diff --git a/src/utils/TestUtils.test.ts b/src/utils/TestUtils.test.ts index 8ba1617951..0c53c9a077 100644 --- a/src/utils/TestUtils.test.ts +++ b/src/utils/TestUtils.test.ts @@ -310,7 +310,7 @@ export class MockRenderer implements IRenderer { } dimensions: IRenderDimensions; setTheme(theme: ITheme): IColorSet { return {}; } - onResize(cols: number, rows: number, didCharSizeChange: boolean): void {} + onResize(cols: number, rows: number): void {} onCharSizeChanged(): void {} onBlur(): void {} onFocus(): void {}