Skip to content

Commit

Permalink
Updates as per code review comments
Browse files Browse the repository at this point in the history 
Getting rid of the ICursorOptions interface to improve performance
  • Loading branch information
@hindol
hindol committed Sep 8, 2018
1 parent 374a8b7 commit 2716413
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 28 deletions.
8 changes: 0 additions & 8 deletions src/Types.ts
View file
Expand Up @@ -522,11 +522,3 @@ export interface IBufferLine {
deleteCells(pos: number, n: number, fill: CharData): void;
replaceCells(start: number, end: number, fill: CharData): void;
}

/**
* Interface for cursor options in the cursor line of the terminal buffer.
*/
export interface ICursorOptions {
isCursorRow: boolean;
cursorStyle?: string;
}
3 changes: 2 additions & 1 deletion src/renderer/dom/DomRenderer.ts
View file
Expand Up @@ -325,7 +325,8 @@ export class DomRenderer extends EventEmitter implements IRenderer {

const row = y + terminal.buffer.ydisp;
const lineData = terminal.buffer.lines.get(row);
rowElement.appendChild(this._rowFactory.createRow(lineData, { isCursorRow: row === cursorAbsoluteY, cursorStyle: terminal.getOption('cursorStyle') }, cursorX, terminal.charMeasure.width, terminal.cols));
const cursorStyle = terminal.options.cursorStyle;
rowElement.appendChild(this._rowFactory.createRow(lineData, row === cursorAbsoluteY, cursorStyle, cursorX, terminal.charMeasure.width, terminal.cols));
}

this._terminal.emit('refresh', {start, end});
Expand Down
24 changes: 12 additions & 12 deletions src/renderer/dom/DomRendererRowFactory.test.ts
View file
Expand Up @@ -24,7 +24,7 @@ describe('DomRendererRowFactory', () => {

describe('createRow', () => {
it('should create an element for every character in the row', () => {
const fragment = rowFactory.createRow(lineData, { isCursorRow: false }, 0, 5, 20);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
'<span> </span>' +
'<span> </span>'
Expand All @@ -35,15 +35,15 @@ describe('DomRendererRowFactory', () => {
lineData.set(0, [DEFAULT_ATTR, '語', 2, '語'.charCodeAt(0)]);
// There should be no element for the following "empty" cell
lineData.set(1, [DEFAULT_ATTR, '', 0, undefined]);
const fragment = rowFactory.createRow(lineData, { isCursorRow: false }, 0, 5, 20);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
'<span style="width: 10px;">語</span>'
);
});

it('should add class for cursor and cursor style', () => {
for (const style of ['block', 'bar', 'underline']) {
const fragment = rowFactory.createRow(lineData, { isCursorRow: true, cursorStyle: style }, 0, 5, 20);
const fragment = rowFactory.createRow(lineData, true, style, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
`<span class="xterm-cursor xterm-cursor-${style}"> </span>` +
'<span> </span>'
Expand All @@ -54,7 +54,7 @@ describe('DomRendererRowFactory', () => {
it('should not render cells that go beyond the terminal\'s columns', () => {
lineData.set(0, [DEFAULT_ATTR, 'a', 1, 'a'.charCodeAt(0)]);
lineData.set(1, [DEFAULT_ATTR, 'b', 1, 'b'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, { isCursorRow: false }, 0, 5, 1);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 1);
assert.equal(getFragmentHtml(fragment),
'<span>a</span>'
);
Expand All @@ -63,7 +63,7 @@ describe('DomRendererRowFactory', () => {
describe('attributes', () => {
it('should add class for bold', () => {
lineData.set(0, [DEFAULT_ATTR | (FLAGS.BOLD << 18), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, { isCursorRow: false }, 0, 5, 20);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
'<span class="xterm-bold">a</span>' +
'<span> </span>'
Expand All @@ -72,7 +72,7 @@ describe('DomRendererRowFactory', () => {

it('should add class for italic', () => {
lineData.set(0, [DEFAULT_ATTR | (FLAGS.ITALIC << 18), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, { isCursorRow: false }, 0, 5, 20);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
'<span class="xterm-italic">a</span>' +
'<span> </span>'
Expand All @@ -83,7 +83,7 @@ describe('DomRendererRowFactory', () => {
const defaultAttrNoFgColor = (0 << 9) | (256 << 0);
for (let i = 0; i < 256; i++) {
lineData.set(0, [defaultAttrNoFgColor | (i << 9), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, { isCursorRow: false }, 0, 5, 20);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
`<span class="xterm-fg-${i}">a</span>` +
'<span> </span>'
Expand All @@ -95,7 +95,7 @@ describe('DomRendererRowFactory', () => {
const defaultAttrNoBgColor = (257 << 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, { isCursorRow: false }, 0, 5, 20);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
`<span class="xterm-bg-${i}">a</span>` +
'<span> </span>'
Expand All @@ -105,7 +105,7 @@ describe('DomRendererRowFactory', () => {

it('should correctly invert colors', () => {
lineData.set(0, [(FLAGS.INVERSE << 18) | (2 << 9) | (1 << 0), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, { isCursorRow: false }, 0, 5, 20);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
'<span class="xterm-fg-1 xterm-bg-2">a</span>' +
'<span> </span>'
Expand All @@ -114,7 +114,7 @@ describe('DomRendererRowFactory', () => {

it('should correctly invert default fg color', () => {
lineData.set(0, [(FLAGS.INVERSE << 18) | (257 << 9) | (1 << 0), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, { isCursorRow: false }, 0, 5, 20);
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> </span>'
Expand All @@ -123,7 +123,7 @@ describe('DomRendererRowFactory', () => {

it('should correctly invert default bg color', () => {
lineData.set(0, [(FLAGS.INVERSE << 18) | (1 << 9) | (256 << 0), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, { isCursorRow: false }, 0, 5, 20);
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> </span>'
Expand All @@ -133,7 +133,7 @@ describe('DomRendererRowFactory', () => {
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)]);
const fragment = rowFactory.createRow(lineData, { isCursorRow: false }, 0, 5, 20);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
`<span class="xterm-bold xterm-fg-${i + 8}">a</span>` +
'<span> </span>'
Expand Down
14 changes: 7 additions & 7 deletions src/renderer/dom/DomRendererRowFactory.ts
View file
Expand Up @@ -5,7 +5,7 @@

import { CHAR_DATA_CHAR_INDEX, CHAR_DATA_ATTR_INDEX, CHAR_DATA_WIDTH_INDEX } from '../../Buffer';
import { FLAGS } from '../Types';
import { IBufferLine, ICursorOptions } from '../../Types';
import { IBufferLine } from '../../Types';

export const BOLD_CLASS = 'xterm-bold';
export const ITALIC_CLASS = 'xterm-italic';
Expand All @@ -20,7 +20,7 @@ export class DomRendererRowFactory {
) {
}

public createRow(lineData: IBufferLine, cursorOptions: ICursorOptions, cursorX: number, cellWidth: number, cols: number): DocumentFragment {
public createRow(lineData: IBufferLine, isCursorRow: boolean, cursorStyle: string | undefined, cursorX: number, cellWidth: number, cols: number): DocumentFragment {
const fragment = this._document.createDocumentFragment();
let colCount = 0;

Expand Down Expand Up @@ -49,19 +49,19 @@ export class DomRendererRowFactory {
let bg = attr & 0x1ff;
let fg = (attr >> 9) & 0x1ff;

if (cursorOptions.isCursorRow && x === cursorX) {
if (isCursorRow && x === cursorX) {
charElement.classList.add(CURSOR_CLASS);

switch (cursorOptions.cursorStyle) {
case 'block':
charElement.classList.add(CURSOR_STYLE_BLOCK_CLASS);
break;
switch (cursorStyle) {
case 'bar':
charElement.classList.add(CURSOR_STYLE_BAR_CLASS);
break;
case 'underline':
charElement.classList.add(CURSOR_STYLE_UNDERLINE_CLASS);
break;
default:
charElement.classList.add(CURSOR_STYLE_BLOCK_CLASS);
break;
}
}

Expand Down

0 comments on commit 2716413

Please sign in to comment.