Skip to content

Allow inverse colors to be stored in glyph keys, fix inverted colors in DOM renderer #1739

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Nov 24, 2018
Merged

Allow inverse colors to be stored in glyph keys, fix inverted colors in DOM renderer #1739

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
791c020
Allow inverse colors to be stored in glyph keys
Tyriar Oct 9, 2018
1e95647
Use constants for default colors
Tyriar Oct 9, 2018
bc16a8b
Handle inverted default colors properly in DOM renderer
Tyriar Oct 9, 2018
91d04f2
Merge branch 'master' into 1737_inverse_dynamic_cache
Tyriar Oct 9, 2018
18035e2
Fix tests
Tyriar Oct 9, 2018
2e88985
Merge branch 'master' into 1737_inverse_dynamic_cache
Tyriar Nov 23, 2018
5fca73a
Merge branch 'master' into 1737_inverse_dynamic_cache
Tyriar Nov 23, 2018
c6f1de7
Introduce is256Color helper
Tyriar Nov 24, 2018
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
3 changes: 2 additions & 1 deletion src/Buffer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import { CharData, ITerminal, IBuffer, IBufferLine, BufferIndex, IBufferStringIt
import { EventEmitter } from './common/EventEmitter';
import { IMarker } from 'xterm';
import { BufferLine, BufferLineTypedArray } from './BufferLine';
import { DEFAULT_COLOR } from './renderer/atlas/Types';

export const DEFAULT_ATTR = (0 << 18) | (257 << 9) | (256 << 0);
export const DEFAULT_ATTR = (0 << 18) | (DEFAULT_COLOR << 9) | (256 << 0);
export const CHAR_DATA_ATTR_INDEX = 0;
export const CHAR_DATA_CHAR_INDEX = 1;
export const CHAR_DATA_WIDTH_INDEX = 2;
Expand Down
4 changes: 2 additions & 2 deletions src/renderer/BaseRenderLayer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import { IRenderLayer, IColorSet, IRenderDimensions } from './Types';
import { CharData, ITerminal } from '../Types';
import { DIM_OPACITY, INVERTED_DEFAULT_COLOR, IGlyphIdentifier } from './atlas/Types';
import { DIM_OPACITY, INVERTED_DEFAULT_COLOR, IGlyphIdentifier, DEFAULT_COLOR } from './atlas/Types';
import BaseCharAtlas from './atlas/BaseCharAtlas';
import { acquireCharAtlas } from './atlas/CharAtlasCache';
import { CHAR_DATA_CHAR_INDEX } from '../Buffer';
Expand Down Expand Up @@ -298,7 +298,7 @@ export abstract class BaseRenderLayer implements IRenderLayer {

if (fg === INVERTED_DEFAULT_COLOR) {
this._ctx.fillStyle = this._colors.background.css;
} else if (fg < 256) {
} else if (fg < DEFAULT_COLOR) {
Copy link
Member

@bgw bgw Nov 23, 2018

Choose a reason for hiding this comment

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

Doing a range check against what is essentially an enum value is a little confusing. Since we're doing this in a few places, maybe we should have a isNormalColor helper function?

Copy link
Member Author

@Tyriar Tyriar Nov 23, 2018

Choose a reason for hiding this comment

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

Sounds good 😃

// 256 color support
this._ctx.fillStyle = this._colors.ansi[fg].css;
} else {
Expand Down
4 changes: 2 additions & 2 deletions src/renderer/LinkRenderLayer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { ILinkHoverEvent, ITerminal, ILinkifierAccessor, LinkHoverEventTypes } from '../Types';
import { IColorSet, IRenderDimensions } from './Types';
import { BaseRenderLayer } from './BaseRenderLayer';
import { INVERTED_DEFAULT_COLOR } from './atlas/Types';
import { INVERTED_DEFAULT_COLOR, DEFAULT_COLOR } from './atlas/Types';

export class LinkRenderLayer extends BaseRenderLayer {
private _state: ILinkHoverEvent = null;
Expand Down Expand Up @@ -42,7 +42,7 @@ export class LinkRenderLayer extends BaseRenderLayer {
private _onLinkHover(e: ILinkHoverEvent): void {
if (e.fg === INVERTED_DEFAULT_COLOR) {
this._ctx.fillStyle = this._colors.background.css;
} else if (e.fg < 256) {
} else if (e.fg < DEFAULT_COLOR) {
// 256 color support
this._ctx.fillStyle = this._colors.ansi[e.fg].css;
} else {
Expand Down
10 changes: 5 additions & 5 deletions src/renderer/TextRenderLayer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { CHAR_DATA_ATTR_INDEX, CHAR_DATA_CODE_INDEX, CHAR_DATA_CHAR_INDEX, CHAR_DATA_WIDTH_INDEX, NULL_CELL_CODE } from '../Buffer';
import { FLAGS, IColorSet, IRenderDimensions, ICharacterJoinerRegistry } from './Types';
import { CharData, ITerminal } from '../Types';
import { INVERTED_DEFAULT_COLOR } from './atlas/Types';
import { INVERTED_DEFAULT_COLOR, DEFAULT_COLOR } from './atlas/Types';
import { GridCache } from './GridCache';
import { BaseRenderLayer } from './BaseRenderLayer';

Expand Down Expand Up @@ -143,10 +143,10 @@ export class TextRenderLayer extends BaseRenderLayer {
const temp = bg;
bg = fg;
fg = temp;
if (fg === 256) {
if (fg === DEFAULT_COLOR) {
fg = INVERTED_DEFAULT_COLOR;
}
if (bg === 257) {
if (bg === DEFAULT_COLOR) {
bg = INVERTED_DEFAULT_COLOR;
}
}
Expand Down Expand Up @@ -186,7 +186,7 @@ export class TextRenderLayer extends BaseRenderLayer {
let nextFillStyle = null; // null represents default background color
if (bg === INVERTED_DEFAULT_COLOR) {
nextFillStyle = this._colors.foreground.css;
} else if (bg < 256) {
} else if (bg < DEFAULT_COLOR) {
nextFillStyle = this._colors.ansi[bg].css;
}

Expand Down Expand Up @@ -230,7 +230,7 @@ export class TextRenderLayer extends BaseRenderLayer {
this._ctx.save();
if (fg === INVERTED_DEFAULT_COLOR) {
this._ctx.fillStyle = this._colors.background.css;
} else if (fg < 256) {
} else if (fg < DEFAULT_COLOR) {
// 256 color support
this._ctx.fillStyle = this._colors.ansi[fg].css;
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/atlas/DynamicCharAtlas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ interface IGlyphCacheValue {
inBitmap: boolean;
}

function getGlyphCacheKey(glyph: IGlyphIdentifier): number {
export function getGlyphCacheKey(glyph: IGlyphIdentifier): number {
// Note that this only returns a valid key when code < 256
// Layout:
// 0b00000000000000000000000000000001: italic (1)
Expand Down
10 changes: 5 additions & 5 deletions src/renderer/atlas/StaticCharAtlas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* @license MIT
*/

import { DIM_OPACITY, IGlyphIdentifier } from './Types';
import { DIM_OPACITY, IGlyphIdentifier, DEFAULT_COLOR } from './Types';
import { CHAR_ATLAS_CELL_SPACING, ICharAtlasConfig } from '../../shared/atlas/Types';
import { generateStaticCharAtlasTexture } from '../../shared/atlas/CharAtlasGenerator';
import BaseCharAtlas from './BaseCharAtlas';
Expand Down Expand Up @@ -41,8 +41,8 @@ export default class StaticCharAtlas extends BaseCharAtlas {
const isAscii = glyph.code < 256;
// A color is basic if it is one of the 4 bit ANSI colors.
const isBasicColor = glyph.fg < 16;
const isDefaultColor = glyph.fg >= 256;
const isDefaultBackground = glyph.bg >= 256;
const isDefaultColor = glyph.fg === DEFAULT_COLOR;
const isDefaultBackground = glyph.bg === DEFAULT_COLOR;
return isAscii && (isBasicColor || isDefaultColor) && isDefaultBackground && !glyph.italic;
}

Expand All @@ -58,9 +58,9 @@ export default class StaticCharAtlas extends BaseCharAtlas {
}

let colorIndex = 0;
if (glyph.fg < 256) {
if (glyph.fg < DEFAULT_COLOR) {
colorIndex = 2 + glyph.fg + (glyph.bold ? 16 : 0);
} else {
} else if (glyph.fg === DEFAULT_COLOR) {
// If default color and bold
if (glyph.bold) {
colorIndex = 1;
Expand Down
3 changes: 2 additions & 1 deletion src/renderer/atlas/Types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
* @license MIT
*/

export const INVERTED_DEFAULT_COLOR = -1;
export const DEFAULT_COLOR = 256;
export const INVERTED_DEFAULT_COLOR = 257;
export const DIM_OPACITY = 0.5;

export interface IGlyphIdentifier {
Expand Down
4 changes: 4 additions & 0 deletions src/renderer/dom/DomRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { EventEmitter } from '../../common/EventEmitter';
import { ColorManager } from '../ColorManager';
import { RenderDebouncer } from '../../ui/RenderDebouncer';
import { BOLD_CLASS, ITALIC_CLASS, CURSOR_CLASS, CURSOR_STYLE_BLOCK_CLASS, CURSOR_STYLE_BAR_CLASS, CURSOR_STYLE_UNDERLINE_CLASS, DomRendererRowFactory } from './DomRendererRowFactory';
import { INVERTED_DEFAULT_COLOR } from '../atlas/Types';

const TERMINAL_CLASS_PREFIX = 'xterm-dom-renderer-owner-';
const ROW_CONTAINER_CLASS = 'xterm-rows';
Expand Down Expand Up @@ -199,6 +200,9 @@ export class DomRenderer extends EventEmitter implements IRenderer {
`${this._terminalSelector} .${FG_CLASS_PREFIX}${i} { color: ${c.css}; }` +
`${this._terminalSelector} .${BG_CLASS_PREFIX}${i} { background-color: ${c.css}; }`;
});
styles +=
`${this._terminalSelector} .${FG_CLASS_PREFIX}${INVERTED_DEFAULT_COLOR} { color: ${this.colorManager.colors.background.css}; }` +
`${this._terminalSelector} .${BG_CLASS_PREFIX}${INVERTED_DEFAULT_COLOR} { background-color: ${this.colorManager.colors.foreground.css}; }`;

this._themeStyleElement.innerHTML = styles;
return this.colorManager.colors;
Expand Down
15 changes: 8 additions & 7 deletions src/renderer/dom/DomRendererRowFactory.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { DEFAULT_ATTR, NULL_CELL_CODE, NULL_CELL_WIDTH, NULL_CELL_CHAR } from '.
import { FLAGS } from '../Types';
import { BufferLine } from '../../BufferLine';
import { IBufferLine } from '../../Types';
import { DEFAULT_COLOR } from '../atlas/Types';

describe('DomRendererRowFactory', () => {
let dom: jsdom.JSDOM;
Expand Down Expand Up @@ -76,7 +77,7 @@ describe('DomRendererRowFactory', () => {
});

it('should add classes for 256 foreground colors', () => {
const defaultAttrNoFgColor = (0 << 9) | (256 << 0);
const defaultAttrNoFgColor = (0 << 9) | (DEFAULT_COLOR << 0);
for (let i = 0; i < 256; i++) {
lineData.set(0, [defaultAttrNoFgColor | (i << 9), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
Expand All @@ -87,7 +88,7 @@ describe('DomRendererRowFactory', () => {
});

it('should add classes for 256 background colors', () => {
const defaultAttrNoBgColor = (257 << 9) | (0 << 0);
const defaultAttrNoBgColor = (DEFAULT_ATTR << 9) | (0 << 0);
for (let i = 0; i < 256; i++) {
lineData.set(0, [defaultAttrNoBgColor | (i << 0), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
Expand All @@ -106,24 +107,24 @@ describe('DomRendererRowFactory', () => {
});

it('should correctly invert default fg color', () => {
lineData.set(0, [(FLAGS.INVERSE << 18) | (257 << 9) | (1 << 0), 'a', 1, 'a'.charCodeAt(0)]);
lineData.set(0, [(FLAGS.INVERSE << 18) | (DEFAULT_ATTR << 9) | (1 << 0), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
'<span class="xterm-fg-1 xterm-bg-15">a</span>'
'<span class="xterm-fg-1 xterm-bg-257">a</span>'
);
});

it('should correctly invert default bg color', () => {
lineData.set(0, [(FLAGS.INVERSE << 18) | (1 << 9) | (256 << 0), 'a', 1, 'a'.charCodeAt(0)]);
lineData.set(0, [(FLAGS.INVERSE << 18) | (1 << 9) | (DEFAULT_COLOR << 0), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
'<span class="xterm-fg-0 xterm-bg-1">a</span>'
'<span class="xterm-fg-257 xterm-bg-1">a</span>'
);
});

it('should turn bold fg text bright', () => {
for (let i = 0; i < 8; i++) {
lineData.set(0, [(FLAGS.BOLD << 18) | (i << 9) | (256 << 0), 'a', 1, 'a'.charCodeAt(0)]);
lineData.set(0, [(FLAGS.BOLD << 18) | (i << 9) | (DEFAULT_COLOR << 0), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
`<span class="xterm-bold xterm-fg-${i + 8}">a</span>`
Expand Down
16 changes: 9 additions & 7 deletions src/renderer/dom/DomRendererRowFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import { CHAR_DATA_CHAR_INDEX, CHAR_DATA_ATTR_INDEX, CHAR_DATA_WIDTH_INDEX, CHAR_DATA_CODE_INDEX, NULL_CELL_CODE } from '../../Buffer';
import { FLAGS } from '../Types';
import { IBufferLine } from '../../Types';
import { DEFAULT_COLOR, INVERTED_DEFAULT_COLOR } from '../atlas/Types';

export const BOLD_CLASS = 'xterm-bold';
export const ITALIC_CLASS = 'xterm-italic';
Expand Down Expand Up @@ -79,16 +80,17 @@ export class DomRendererRowFactory {
const temp = bg;
bg = fg;
fg = temp;
if (fg === 256) {
fg = 0;
if (fg === DEFAULT_COLOR) {
fg = INVERTED_DEFAULT_COLOR;
}
if (bg === 257) {
bg = 15;
if (bg === DEFAULT_COLOR) {
bg = INVERTED_DEFAULT_COLOR;
}
}

if (flags & FLAGS.BOLD) {
// Convert the FG color to the bold variant
// Convert the FG color to the bold variant. This should not happen when
// the fg is the inverse default color as there is no bold variant.
if (fg < 8) {
fg += 8;
}
Expand All @@ -100,10 +102,10 @@ export class DomRendererRowFactory {
}

charElement.textContent = char;
if (fg !== 257) {
if (fg !== DEFAULT_COLOR) {
charElement.classList.add(`xterm-fg-${fg}`);
}
if (bg !== 256) {
if (bg !== DEFAULT_COLOR) {
charElement.classList.add(`xterm-bg-${bg}`);
}
fragment.appendChild(charElement);
Expand Down