From 9b6c4e046f61c39470d9d7ed295df03d55dc68df Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sat, 16 Sep 2017 10:57:49 -0700 Subject: [PATCH 1/5] Only refresh char atlas when char dimensions are valid See Microsoft/vscode#34493 --- src/renderer/BaseRenderLayer.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/renderer/BaseRenderLayer.ts b/src/renderer/BaseRenderLayer.ts index 8f655de5..4cc07472 100644 --- a/src/renderer/BaseRenderLayer.ts +++ b/src/renderer/BaseRenderLayer.ts @@ -57,6 +57,9 @@ export abstract class BaseRenderLayer implements IRenderLayer { * @param colorSet The color set to use for the char atlas. */ private _refreshCharAtlas(terminal: ITerminal, colorSet: IColorSet): void { + if (this._scaledCharWidth > 0 && this._scaledCharHeight > 0) { + return; + } this._charAtlas = null; const result = acquireCharAtlas(terminal, this._colors, this._scaledCharWidth, this._scaledCharHeight); if (result instanceof HTMLCanvasElement) { From c8153104184622a9a243a24110acf27d9be7665b Mon Sep 17 00:00:00 2001 From: Thomas Zilz Date: Sat, 16 Sep 2017 20:36:55 +0200 Subject: [PATCH 2/5] Fix inverted cells, make sure inverted cells are cleared correctly (#996) * Corretly inverse foreground and background. Be more conservative when clearing * add missing space * Re-enable optimisation with better checks * Fix insversed check --- src/renderer/TextRenderLayer.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/renderer/TextRenderLayer.ts b/src/renderer/TextRenderLayer.ts index ced4ceaa..74997866 100644 --- a/src/renderer/TextRenderLayer.ts +++ b/src/renderer/TextRenderLayer.ts @@ -83,7 +83,8 @@ export class TextRenderLayer extends BaseRenderLayer { } // Clear the old character was not a space with the default background - if (state && !(state[CHAR_DATA_CODE_INDEX] === 32 /*' '*/ && (state[CHAR_DATA_ATTR_INDEX] & 0x1ff) >= 256)) { + 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; @@ -94,7 +95,8 @@ export class TextRenderLayer extends BaseRenderLayer { // Skip rendering if the character is invisible const isDefaultBackground = bg >= 256; const isInvisible = flags & FLAGS.INVISIBLE; - if (!code || (code === 32 /*' '*/ && isDefaultBackground) || isInvisible) { + const isInverted = flags & FLAGS.INVERSE; + if (!code || (code === 32 /*' '*/ && isDefaultBackground && !isInverted) || isInvisible) { continue; } @@ -121,10 +123,10 @@ export class TextRenderLayer extends BaseRenderLayer { let fg = (attr >> 9) & 0x1ff; // If inverse flag is on, the foreground should become the background. - if (flags & FLAGS.INVERSE) { + if (isInverted) { const temp = bg; bg = fg; - fg = bg; + fg = temp; if (fg === 256) { fg = INVERTED_DEFAULT_COLOR; } From c4da8325ace6e4bde77822377bf994d389a7c75a Mon Sep 17 00:00:00 2001 From: Thomas Zilz Date: Sun, 17 Sep 2017 10:39:09 +0200 Subject: [PATCH 3/5] Allow all overflowing characters to extend to the next cell if followed by a space (Emojis etc) (#997) * Measure characters and thread them if they are overlapping * Fix whitespace * Use charData and ctx.save/restore, code cleanup * Fix trailing whitespace --- src/renderer/TextRenderLayer.ts | 79 +++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 24 deletions(-) diff --git a/src/renderer/TextRenderLayer.ts b/src/renderer/TextRenderLayer.ts index 74997866..e319efe7 100644 --- a/src/renderer/TextRenderLayer.ts +++ b/src/renderer/TextRenderLayer.ts @@ -16,10 +16,13 @@ import { BaseRenderLayer, INVERTED_DEFAULT_COLOR } from './BaseRenderLayer'; * when the character changes (a regular space ' ' character may not as it's * drawn state is a cleared cell). */ -const EMOJI_OWNED_CHAR_DATA: CharData = [null, '', 0, -1]; +const OVERLAP_OWNED_CHAR_DATA: CharData = [null, '', 0, -1]; export class TextRenderLayer extends BaseRenderLayer { private _state: GridCache; + private _characterWidth: number; + private _characterFont: string; + private _characterOverlapCache: { [key: string]: boolean } = {}; constructor(container: HTMLElement, zIndex: number, colors: IColorSet) { super(container, 'text', zIndex, false, colors); @@ -28,6 +31,14 @@ export class TextRenderLayer extends BaseRenderLayer { public resize(terminal: ITerminal, dim: IRenderDimensions, charSizeChanged: boolean): void { super.resize(terminal, dim, charSizeChanged); + + // Clear the character width cache if the font or width has changed + const terminalFont = `${terminal.options.fontSize * window.devicePixelRatio}px ${terminal.options.fontFamily}`; + if (this._characterWidth !== dim.scaledCharWidth || this._characterFont !== terminalFont) { + this._characterWidth = dim.scaledCharWidth; + this._characterFont = terminalFont; + this._characterOverlapCache = {}; + } // Resizing the canvas discards the contents of the canvas so clear state this._state.clear(); this._state.resize(terminal.cols, terminal.rows); @@ -63,12 +74,12 @@ export class TextRenderLayer extends BaseRenderLayer { } // If the character is a space and the character to the left is an - // emoji, skip the character and allow the emoji char to take full - // control over this character's cell. + // 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._isEmoji(previousChar[CHAR_DATA_CHAR_INDEX])) { + if (this._isOverlapping(previousChar)) { continue; } } @@ -100,23 +111,23 @@ export class TextRenderLayer extends BaseRenderLayer { continue; } - // If the character is an emoji and the character to the right is a + // 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 (this._isEmoji(char)) { - // If the character is an emoji, we want to force a re-render on every + if (width !== 0 && 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 - // emoji's `a` and `b` are adjacent, the cursor is moved to b and a + // overlaping chars `a` and `b` are adjacent, the cursor is moved to b and a // space is added. Without this, the first half of `b` would never // get removed, and `a` would not re-render because it thinks it's // already in the correct state. - this._state.cache[x][y] = EMOJI_OWNED_CHAR_DATA; + this._state.cache[x][y] = OVERLAP_OWNED_CHAR_DATA; if (x < line.length && line[x + 1][CHAR_DATA_CODE_INDEX] === 32 /*' '*/) { width = 2; this._clearChar(x + 1, y); - // The emoji owned char data will force a clear and render when the - // emoji is no longer to the left of the character and also when the - // space changes to another character. - this._state.cache[x + 1][y] = EMOJI_OWNED_CHAR_DATA; + // The overlapping char's char data will force a clear and render when the + // overlapping char is no longer to the left of the character and also when + // the space changes to another character. + this._state.cache[x + 1][y] = OVERLAP_OWNED_CHAR_DATA; } } @@ -171,18 +182,38 @@ export class TextRenderLayer extends BaseRenderLayer { } } - /** - * Whether the character is an emoji. - * @param char The character to search. - */ - private _isEmoji(char: string): boolean { - // TODO: We need a generic solution for handling characters like this - // Check special ambiguous width characters - if (char === '➜') { - return true; + /** + * Whether a character is overlapping to the + * next cell. + */ + private _isOverlapping(charData: CharData): boolean { + // We assume that any ascii character will not overlap + const code = charData[CHAR_DATA_CODE_INDEX]; + if (code < 256) { + return false; } - // Check emoji unicode range - return char.search(/([\uD800-\uDBFF][\uDC00-\uDFFF])/g) >= 0; + + // Deliver from cache if available + const char = charData[CHAR_DATA_CHAR_INDEX]; + if (this._characterOverlapCache.hasOwnProperty(char)) { + return this._characterOverlapCache[char]; + } + + // Setup the font + this._ctx.save(); + this._ctx.font = this._characterFont; + + // Measure the width of the character, but Math.floor it + // because that is what the renderer does when it calculates + // the character dimensions we are comparing against + const overlaps = Math.floor(this._ctx.measureText(char).width) > this._characterWidth; + + // Restore the original context + this._ctx.restore(); + + // Cache and return + this._characterOverlapCache[char] = overlaps; + return overlaps; } /** From 40fdd00b9e1334c94618a69e60bee6538ff7eb36 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 20 Sep 2017 09:02:21 +0900 Subject: [PATCH 4/5] Correct if statement, ensure charAtlas exists --- src/renderer/BaseRenderLayer.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/renderer/BaseRenderLayer.ts b/src/renderer/BaseRenderLayer.ts index 4cc07472..2bf49e68 100644 --- a/src/renderer/BaseRenderLayer.ts +++ b/src/renderer/BaseRenderLayer.ts @@ -57,7 +57,7 @@ export abstract class BaseRenderLayer implements IRenderLayer { * @param colorSet The color set to use for the char atlas. */ private _refreshCharAtlas(terminal: ITerminal, colorSet: IColorSet): void { - if (this._scaledCharWidth > 0 && this._scaledCharHeight > 0) { + if (this._scaledCharWidth <= 0 && this._scaledCharHeight <= 0) { return; } this._charAtlas = null; @@ -256,7 +256,7 @@ export abstract class BaseRenderLayer implements IRenderLayer { const isBasicColor = (colorIndex > 1 && fg < 16); const isDefaultColor = fg >= 256; const isDefaultBackground = bg >= 256; - if (isAscii && (isBasicColor || isDefaultColor) && isDefaultBackground) { + if (this._charAtlas && isAscii && (isBasicColor || isDefaultColor) && isDefaultBackground) { // ImageBitmap's draw about twice as fast as from a canvas const charAtlasCellWidth = this._scaledCharWidth + CHAR_ATLAS_CELL_SPACING; const charAtlasCellHeight = this._scaledCharHeight + CHAR_ATLAS_CELL_SPACING; From a2389d5c5b46d14797345ae75ba0bcc5b884f521 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sun, 24 Sep 2017 10:59:38 -0700 Subject: [PATCH 5/5] Fix validation callback to use the uri, not whole row --- src/Linkifier.test.ts | 11 +++++++++++ src/Linkifier.ts | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/Linkifier.test.ts b/src/Linkifier.test.ts index df9e459c..0878aafc 100644 --- a/src/Linkifier.test.ts +++ b/src/Linkifier.test.ts @@ -156,6 +156,17 @@ describe('Linkifier', () => { linkifier.linkifyRows(); }); + it('should validate the uri, not the row', done => { + addRow('abc test abc'); + linkifier.registerLinkMatcher(/test/, () => done(), { + validationCallback: (uri, cb) => { + assert.equal(uri, 'test'); + done(); + } + }); + linkifier.linkifyRows(); + }); + it('should disable link if false', done => { addRow('test'); linkifier.registerLinkMatcher(/test/, () => assert.fail(), { diff --git a/src/Linkifier.ts b/src/Linkifier.ts index fa47271c..42d20f9c 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -236,7 +236,7 @@ export class Linkifier extends EventEmitter implements ILinkifier { // Ensure the link is valid before registering if (matcher.validationCallback) { - matcher.validationCallback(text, isValid => { + matcher.validationCallback(uri, isValid => { // Discard link if the line has already changed if (this._rowsTimeoutId) { return;