Updates as per code review comments

Getting rid of the ICursorOptions interface to improve performance

src/Types.ts

@@ -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;
-}

src/renderer/dom/DomRenderer.ts

@@ -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});

src/renderer/dom/DomRendererRowFactory.test.ts

@@ -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>'
@@ -35,7 +35,7 @@ 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>'
       );
@@ -43,7 +43,7 @@ describe('DomRendererRowFactory', () => {
 
     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>'
@@ -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>'
       );
@@ -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>'
@@ -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>'
@@ -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>'
@@ -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>'
@@ -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>'
@@ -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>'
@@ -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>'
@@ -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>'

src/renderer/dom/DomRendererRowFactory.ts

@@ -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';
@@ -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;
 
@@ -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;
         }
       }