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.

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

Allow all overflowing characters to extend to the next cell if followed by a space (Emojis etc) #997

Merged
merged 4 commits into from
Sep 17, 2017
Merged
Changes from 2 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
82 changes: 55 additions & 27 deletions src/renderer/TextRenderLayer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<CharData>;
private _characterWidth: number;
private _characterFont: string;
private _characterOverlapCache: { [key: string]: boolean } = {};

constructor(container: HTMLElement, zIndex: number, colors: IColorSet) {
super(container, 'text', zIndex, false, colors);
Expand All @@ -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);
Expand Down Expand Up @@ -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[CHAR_DATA_CHAR_INDEX])) {
continue;
}
}
Expand Down Expand Up @@ -98,23 +109,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(char)) {
// 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;
}
}

Expand Down Expand Up @@ -168,20 +179,37 @@ 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;
}
// Check emoji unicode range
return char.search(/([\uD800-\uDBFF][\uDC00-\uDFFF])/g) >= 0;
}

/**
* Whether a character is overlapping to the
* next cell.
*/
private _isOverlapping(char: string): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Let's also pass in char code, maybe the CharData object would be easier since we need 2 of its props?


Copy link
Member

Choose a reason for hiding this comment

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

The common case:

if (code < 256) {
  return false;
}

It should be safe to assume this.

// Deliver from cache if available
if (this._characterOverlapCache.hasOwnProperty(char)) {
return this._characterOverlapCache[char];
}

// Set the correct character font for the current context
let font;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing this use this._ctx.save() before you change .font and .restore() after. This allows you to temporarily change the style and then revert the changes after restore.

if (this._ctx.font !== this._characterFont) {
font = this._ctx.font;
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;
Copy link
Member

Choose a reason for hiding this comment

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

This function seems a lot faster than I was expecting 😄


// Restore the original font to the context
if (font) this._ctx.font = font;

// Cache and return
return this._characterOverlapCache[char] = overlaps;
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this 2 lines, I always have to have a second look as doing this behaves very differently in different languages.


}

/**
* Clear the charcater at the cell specified.
Expand Down