From b853aa492914bbaef91a73eeae0c904b621bfd9e Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Wed, 14 Mar 2018 10:05:29 -0700 Subject: [PATCH 1/3] Separate foreground & background rendering passes I'm primarily interested in doing this because it'll allow me to optimize the background rendering in later diffs, but this may also make it possible to fix some minor rendering issues. Right now, if we draw a single-width character that overflows its cell's bounds, and the character to the right of it has a background, that background may cover up the first character's foreground. By drawing the background in a separate pass, we can avoid those cases. There's still some issues with how dirty regions are computed that makes rendering stuff like that flaky, but this at least gets us closer to "correct" rendering. Other terminal emulators (e.g. alacritty) render the foreground and background in separate passes: https://github.com/jwilm/alacritty/blob/1b7ffea/src/renderer/mod.rs#L766 --- src/renderer/TextRenderLayer.ts | 155 +++++++++++++++----------------- 1 file changed, 72 insertions(+), 83 deletions(-) diff --git a/src/renderer/TextRenderLayer.ts b/src/renderer/TextRenderLayer.ts index 2487a294..8b061511 100644 --- a/src/renderer/TextRenderLayer.ts +++ b/src/renderer/TextRenderLayer.ts @@ -48,21 +48,24 @@ export class TextRenderLayer extends BaseRenderLayer { this.clearAll(); } - public onGridChanged(terminal: ITerminal, startRow: number, endRow: number): void { - // Resize has not been called yet - if (this._state.cache.length === 0) { - return; - } - + private _forEachCell( + terminal: ITerminal, + startRow: number, + endRow: number, + callback: ( + code: number, + char: string, + width: number, + x: number, + y: number, + fg: number, + bg: number, + flags: number + ) => void + ): void { for (let y = startRow; y <= endRow; y++) { const row = y + terminal.buffer.ydisp; const line = terminal.buffer.lines.get(row); - - this.clearCells(0, y, terminal.cols, 1); - // for (let x = 0; x < terminal.cols; x++) { - // this._state.cache[x][y] = null; - // } - for (let x = 0; x < terminal.cols; x++) { const charData = line[x]; const code: number = charData[CHAR_DATA_CODE_INDEX]; @@ -73,51 +76,12 @@ export class TextRenderLayer extends BaseRenderLayer { // The character to the left is a wide character, drawing is owned by // the char at x-1 if (width === 0) { - // this._state.cache[x][y] = null; - continue; - } - - // If the character is a space and the character to the left is an - // overlapping character, skip the character and allow the overlapping - // char to take full control over this character's cell. - if (code === 32 /*' '*/) { - if (x > 0) { - const previousChar: CharData = line[x - 1]; - if (this._isOverlapping(previousChar)) { - continue; - } - } - } - - // Skip rendering if the character is identical - // const state = this._state.cache[x][y]; - // if (state && state[CHAR_DATA_CHAR_INDEX] === char && state[CHAR_DATA_ATTR_INDEX] === attr) { - // // Skip render, contents are identical - // this._state.cache[x][y] = charData; - // continue; - // } - - // Clear the old character was not a space with the default background - // const wasInverted = !!(state && state[CHAR_DATA_ATTR_INDEX] && state[CHAR_DATA_ATTR_INDEX] >> 18 & FLAGS.INVERSE); - // if (state && !(state[CHAR_DATA_CODE_INDEX] === 32 /*' '*/ && (state[CHAR_DATA_ATTR_INDEX] & 0x1ff) >= 256 && !wasInverted)) { - // this._clearChar(x, y); - // } - // this._state.cache[x][y] = charData; - - const flags = attr >> 18; - let bg = attr & 0x1ff; - - // Skip rendering if the character is invisible - const isDefaultBackground = bg >= 256; - const isInvisible = flags & FLAGS.INVISIBLE; - const isInverted = flags & FLAGS.INVERSE; - if (!code || (code === 32 /*' '*/ && isDefaultBackground && !isInverted) || isInvisible) { continue; } // If the character is an overlapping char and the character to the right is a // space, take ownership of the cell to the right. - if (width !== 0 && this._isOverlapping(charData)) { + if (this._isOverlapping(charData)) { // If the character is overlapping, we want to force a re-render on every // frame. This is specifically to work around the case where two // overlaping chars `a` and `b` are adjacent, the cursor is moved to b and a @@ -135,10 +99,12 @@ export class TextRenderLayer extends BaseRenderLayer { } } + const flags = attr >> 18; + let bg = attr & 0x1ff; let fg = (attr >> 9) & 0x1ff; // If inverse flag is on, the foreground should become the background. - if (isInverted) { + if (flags & FLAGS.INVERSE) { const temp = bg; bg = fg; fg = temp; @@ -150,47 +116,70 @@ export class TextRenderLayer extends BaseRenderLayer { } } - // Clear the cell next to this character if it's wide - if (width === 2) { - // this.clearCells(x + 1, y, 1, 1); - } - - // Draw background - if (bg < 256) { - this._ctx.save(); - this._ctx.fillStyle = (bg === INVERTED_DEFAULT_COLOR ? this._colors.foreground.css : this._colors.ansi[bg].css); - this.fillCells(x, y, width, 1); - this._ctx.restore(); - } - - this._ctx.save(); if (flags & FLAGS.BOLD) { - this._ctx.font = this._getFont(terminal, true); // Convert the FG color to the bold variant if (fg < 8) { fg += 8; } } - if (flags & FLAGS.UNDERLINE) { - if (fg === INVERTED_DEFAULT_COLOR) { - this._ctx.fillStyle = this._colors.background.css; - } else if (fg < 256) { - // 256 color support - this._ctx.fillStyle = this._colors.ansi[fg].css; - } else { - this._ctx.fillStyle = this._colors.foreground.css; - } - this.fillBottomLineAtCells(x, y); - } - - this.drawChar(terminal, char, code, width, x, y, fg, bg, !!(flags & FLAGS.BOLD), !!(flags & FLAGS.DIM)); - - this._ctx.restore(); + callback(code, char, width, x, y, fg, bg, flags); } } } + private _drawBackground(terminal: ITerminal, startRow: number, endRow: number): void { + this._forEachCell(terminal, startRow, endRow, (code, char, width, x, y, fg, bg, flags) => { + // libvte and xterm both draw the background (but not foreground) of invisible characters, + // so we should too. + const isDefaultBackground = bg >= 256; + if (!isDefaultBackground) { + this._ctx.save(); + this._ctx.fillStyle = (bg === INVERTED_DEFAULT_COLOR ? this._colors.foreground.css : this._colors.ansi[bg].css); + this.fillCells(x, y, width, 1); + this._ctx.restore(); + } + }); + } + + private _drawForeground(terminal: ITerminal, startRow: number, endRow: number): void { + this._forEachCell(terminal, startRow, endRow, (code, char, width, x, y, fg, bg, flags) => { + if (flags & FLAGS.INVISIBLE) { + return; + } + if (flags & FLAGS.UNDERLINE) { + this._ctx.save(); + if (fg === INVERTED_DEFAULT_COLOR) { + this._ctx.fillStyle = this._colors.background.css; + } else if (fg < 256) { + // 256 color support + this._ctx.fillStyle = this._colors.ansi[fg].css; + } else { + this._ctx.fillStyle = this._colors.foreground.css; + } + this.fillBottomLineAtCells(x, y); + this._ctx.restore(); + } + this.drawChar( + terminal, char, code, + width, x, y, + fg, bg, + !!(flags & FLAGS.BOLD), !!(flags & FLAGS.DIM) + ); + }); + } + + public onGridChanged(terminal: ITerminal, startRow: number, endRow: number): void { + // Resize has not been called yet + if (this._state.cache.length === 0) { + return; + } + + this.clearCells(0, startRow, terminal.cols, endRow - startRow + 1); // endRow is inclusive + this._drawBackground(terminal, startRow, endRow); + this._drawForeground(terminal, startRow, endRow); + } + public onOptionsChanged(terminal: ITerminal): void { this.setTransparency(terminal, terminal.options.allowTransparency); } From 0fe59df2e91e766f5bcec2e7e2479cb32a32bd3b Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Wed, 18 Apr 2018 20:22:55 -0700 Subject: [PATCH 2/3] Rename startRow/endRow in TextRenderLayer Renaming these to firstRow/lastRow makes the inclusivity of the range clearer. Addresses this comment: https://github.com/xtermjs/xterm.js/pull/1393#discussion_r182471582 --- src/renderer/TextRenderLayer.ts | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/renderer/TextRenderLayer.ts b/src/renderer/TextRenderLayer.ts index 8b061511..58d0d790 100644 --- a/src/renderer/TextRenderLayer.ts +++ b/src/renderer/TextRenderLayer.ts @@ -50,8 +50,8 @@ export class TextRenderLayer extends BaseRenderLayer { private _forEachCell( terminal: ITerminal, - startRow: number, - endRow: number, + firstRow: number, + lastRow: number, callback: ( code: number, char: string, @@ -63,7 +63,7 @@ export class TextRenderLayer extends BaseRenderLayer { flags: number ) => void ): void { - for (let y = startRow; y <= endRow; y++) { + for (let y = firstRow; y <= lastRow; y++) { const row = y + terminal.buffer.ydisp; const line = terminal.buffer.lines.get(row); for (let x = 0; x < terminal.cols; x++) { @@ -128,8 +128,8 @@ export class TextRenderLayer extends BaseRenderLayer { } } - private _drawBackground(terminal: ITerminal, startRow: number, endRow: number): void { - this._forEachCell(terminal, startRow, endRow, (code, char, width, x, y, fg, bg, flags) => { + private _drawBackground(terminal: ITerminal, firstRow: number, lastRow: number): void { + this._forEachCell(terminal, firstRow, lastRow, (code, char, width, x, y, fg, bg, flags) => { // libvte and xterm both draw the background (but not foreground) of invisible characters, // so we should too. const isDefaultBackground = bg >= 256; @@ -142,8 +142,8 @@ export class TextRenderLayer extends BaseRenderLayer { }); } - private _drawForeground(terminal: ITerminal, startRow: number, endRow: number): void { - this._forEachCell(terminal, startRow, endRow, (code, char, width, x, y, fg, bg, flags) => { + private _drawForeground(terminal: ITerminal, firstRow: number, lastRow: number): void { + this._forEachCell(terminal, firstRow, lastRow, (code, char, width, x, y, fg, bg, flags) => { if (flags & FLAGS.INVISIBLE) { return; } @@ -169,15 +169,15 @@ export class TextRenderLayer extends BaseRenderLayer { }); } - public onGridChanged(terminal: ITerminal, startRow: number, endRow: number): void { + public onGridChanged(terminal: ITerminal, firstRow: number, lastRow: number): void { // Resize has not been called yet if (this._state.cache.length === 0) { return; } - this.clearCells(0, startRow, terminal.cols, endRow - startRow + 1); // endRow is inclusive - this._drawBackground(terminal, startRow, endRow); - this._drawForeground(terminal, startRow, endRow); + this.clearCells(0, firstRow, terminal.cols, lastRow - firstRow + 1); + this._drawBackground(terminal, firstRow, lastRow); + this._drawForeground(terminal, firstRow, lastRow); } public onOptionsChanged(terminal: ITerminal): void { From 9e863d037742346e6dbe201ab60a10e819c5fd31 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Mon, 23 Apr 2018 11:52:42 -0700 Subject: [PATCH 3/3] Add no else return tslint rule --- package.json | 1 + src/CompositionHelper.ts | 7 +++---- src/handlers/AltClickHandler.ts | 15 ++++++--------- tslint.json | 9 ++++++++- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/package.json b/package.json index 49ccc387..d4a06f82 100644 --- a/package.json +++ b/package.json @@ -67,6 +67,7 @@ "npm-run-all": "^4.1.2", "sorcery": "^0.10.0", "tslint": "^5.9.1", + "tslint-consistent-codestyle": "^1.13.0", "typescript": "~2.7.1", "vinyl-buffer": "^1.0.0", "vinyl-source-stream": "^1.1.0", diff --git a/src/CompositionHelper.ts b/src/CompositionHelper.ts index 389cb782..b721b7f1 100644 --- a/src/CompositionHelper.ts +++ b/src/CompositionHelper.ts @@ -92,11 +92,10 @@ export class CompositionHelper { } else if (ev.keyCode === 16 || ev.keyCode === 17 || ev.keyCode === 18) { // Continue composing if the keyCode is a modifier key return false; - } else { - // Finish composition immediately. This is mainly here for the case where enter is - // pressed and the handler needs to be triggered before the command is executed. - this._finalizeComposition(false); } + // Finish composition immediately. This is mainly here for the case where enter is + // pressed and the handler needs to be triggered before the command is executed. + this._finalizeComposition(false); } if (ev.keyCode === 229) { diff --git a/src/handlers/AltClickHandler.ts b/src/handlers/AltClickHandler.ts index f77637ea..221a993f 100644 --- a/src/handlers/AltClickHandler.ts +++ b/src/handlers/AltClickHandler.ts @@ -73,12 +73,11 @@ export class AltClickHandler { private _resetStartingRow(): string { if (this._moveToRequestedRow().length === 0) { return ''; - } else { - return repeat(this._bufferLine( - this._startCol, this._startRow, this._startCol, - this._startRow - this._wrappedRowsForRow(this._startRow), false - ).length, this._sequence(Direction.Left)); } + return repeat(this._bufferLine( + this._startCol, this._startRow, this._startCol, + this._startRow - this._wrappedRowsForRow(this._startRow), false + ).length, this._sequence(Direction.Left)); } /** @@ -180,9 +179,8 @@ export class AltClickHandler { (this._startCol >= this._endCol && startRow < this._endRow)) { // down/left or same y/left return Direction.Right; - } else { - return Direction.Left; } + return Direction.Left; } /** @@ -191,9 +189,8 @@ export class AltClickHandler { private _verticalDirection(): Direction { if (this._startRow > this._endRow) { return Direction.Up; - } else { - return Direction.Down; } + return Direction.Down; } /** diff --git a/tslint.json b/tslint.json index d42fda71..5cd44ecf 100644 --- a/tslint.json +++ b/tslint.json @@ -1,4 +1,7 @@ { + "rulesDirectory": [ + "tslint-consistent-codestyle" + ], "rules": { "array-type": [ true, @@ -86,6 +89,10 @@ "check-type", "check-type-operator", "check-preblock" - ] + ], + + "no-else-after-return": { + "options": "allow-else-if" + } } }