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.

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

DOM renderer: Don't output empty cells at end of a line #1792

Merged
merged 7 commits into from
Nov 23, 2018
Merged

DOM renderer: Don't output empty cells at end of a line #1792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
692cdfa
DOM renderer: Don't output empty cells at end
Tyriar Nov 22, 2018
dc077a1
Fix tests and the colCount feature
Tyriar Nov 22, 2018
913adf9
Merge branch 'master' into 1609_dom_no_null_cells
Tyriar Nov 22, 2018
66272eb
Get the line length by going backwards instead
Tyriar Nov 23, 2018
d9513ce
Don't bother checking beyond cols
Tyriar Nov 23, 2018
478dfee
Ensure wide chars don't overflow onto following row
Tyriar Nov 23, 2018
f363872
Merge branch 'master' into 1609_dom_no_null_cells
Tyriar Nov 23, 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
4 changes: 3 additions & 1 deletion src/renderer/dom/DomRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ export class DomRenderer extends EventEmitter implements IRenderer {
element.style.width = `${this.dimensions.canvasWidth}px`;
element.style.height = `${this.dimensions.actualCellHeight}px`;
element.style.lineHeight = `${this.dimensions.actualCellHeight}px`;
// Make sure rows don't overflow onto following row
element.style.overflow = 'hidden';
});

if (!this._dimensionsStyleElement) {
Expand Down Expand Up @@ -330,7 +332,7 @@ export class DomRenderer extends EventEmitter implements IRenderer {
const row = y + terminal.buffer.ydisp;
const lineData = terminal.buffer.lines.get(row);
const cursorStyle = terminal.options.cursorStyle;
rowElement.appendChild(this._rowFactory.createRow(lineData, row === cursorAbsoluteY, cursorStyle, cursorX, terminal.charMeasure.width, terminal.cols));
rowElement.appendChild(this._rowFactory.createRow(lineData, row === cursorAbsoluteY, cursorStyle, cursorX, this.dimensions.actualCellWidth, terminal.cols));
}

this._terminal.emit('refresh', {start, end});
Expand Down
32 changes: 11 additions & 21 deletions src/renderer/dom/DomRendererRowFactory.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,10 @@ describe('DomRendererRowFactory', () => {
});

describe('createRow', () => {
it('should create an element for every character in the row', () => {
it('should not create anything for an empty row', () => {
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
'<span> </span>' +
'<span> </span>'
''
);
});

Expand All @@ -45,8 +44,7 @@ describe('DomRendererRowFactory', () => {
for (const style of ['block', 'bar', 'underline']) {
const fragment = rowFactory.createRow(lineData, true, style, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
`<span class="xterm-cursor xterm-cursor-${style}"> </span>` +
'<span> </span>'
`<span class="xterm-cursor xterm-cursor-${style}"> </span>`
);
}
});
Expand All @@ -65,17 +63,15 @@ describe('DomRendererRowFactory', () => {
lineData.set(0, [DEFAULT_ATTR | (FLAGS.BOLD << 18), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
'<span class="xterm-bold">a</span>' +
'<span> </span>'
'<span class="xterm-bold">a</span>'
);
});

it('should add class for italic', () => {
lineData.set(0, [DEFAULT_ATTR | (FLAGS.ITALIC << 18), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
'<span class="xterm-italic">a</span>' +
'<span> </span>'
'<span class="xterm-italic">a</span>'
);
});

Expand All @@ -85,8 +81,7 @@ describe('DomRendererRowFactory', () => {
lineData.set(0, [defaultAttrNoFgColor | (i << 9), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
`<span class="xterm-fg-${i}">a</span>` +
'<span> </span>'
`<span class="xterm-fg-${i}">a</span>`
);
}
});
Expand All @@ -97,8 +92,7 @@ describe('DomRendererRowFactory', () => {
lineData.set(0, [defaultAttrNoBgColor | (i << 0), 'a', 1, 'a'.charCodeAt(0)]);
const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
`<span class="xterm-bg-${i}">a</span>` +
'<span> </span>'
`<span class="xterm-bg-${i}">a</span>`
);
}
});
Expand All @@ -107,26 +101,23 @@ describe('DomRendererRowFactory', () => {
lineData.set(0, [(FLAGS.INVERSE << 18) | (2 << 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-2">a</span>' +
'<span> </span>'
'<span class="xterm-fg-1 xterm-bg-2">a</span>'
);
});

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, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
'<span class="xterm-fg-1 xterm-bg-15">a</span>' +
'<span> </span>'
'<span class="xterm-fg-1 xterm-bg-15">a</span>'
);
});

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, false, undefined, 0, 5, 20);
assert.equal(getFragmentHtml(fragment),
'<span class="xterm-fg-0 xterm-bg-1">a</span>' +
'<span> </span>'
'<span class="xterm-fg-0 xterm-bg-1">a</span>'
);
});

Expand All @@ -135,8 +126,7 @@ describe('DomRendererRowFactory', () => {
lineData.set(0, [(FLAGS.BOLD << 18) | (i << 9) | (256 << 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>` +
'<span> </span>'
`<span class="xterm-bold xterm-fg-${i + 8}">a</span>`
);
}
});
Expand Down
28 changes: 18 additions & 10 deletions src/renderer/dom/DomRendererRowFactory.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 { CHAR_DATA_CHAR_INDEX, CHAR_DATA_ATTR_INDEX, CHAR_DATA_WIDTH_INDEX } from '../../Buffer';
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';

Expand All @@ -22,18 +22,27 @@ export class DomRendererRowFactory {

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

for (let x = 0; x < lineData.length; x++) {
// Don't allow any buffer to the right to be displayed
if (colCount >= cols) {
continue;
// Find the line length first, this prevents the need to output a bunch of
// empty cells at the end. This cannot easily be integrated into the main
// loop below because of the colCount feature (which can be removed after we
// properly support reflow and disallow data to go beyond the right-side of
// the viewport).
let lineLength = 0;
for (let x = Math.min(lineData.length, cols) - 1; x >= 0; x--) {
const charData = lineData.get(x);
const code = charData[CHAR_DATA_CODE_INDEX];
if (code !== NULL_CELL_CODE || (isCursorRow && x === cursorX)) {
lineLength = x + 1;
break;
}
}

for (let x = 0; x < lineLength; x++) {
const charData = lineData.get(x);
const char: string = charData[CHAR_DATA_CHAR_INDEX];
const attr: number = charData[CHAR_DATA_ATTR_INDEX];
const width: number = charData[CHAR_DATA_WIDTH_INDEX];
const char = charData[CHAR_DATA_CHAR_INDEX];
const attr = charData[CHAR_DATA_ATTR_INDEX];
const width = charData[CHAR_DATA_WIDTH_INDEX];

// The character to the left is a wide character, drawing is owned by the char at x-1
if (width === 0) {
Expand Down Expand Up @@ -98,7 +107,6 @@ export class DomRendererRowFactory {
charElement.classList.add(`xterm-bg-${bg}`);
}
fragment.appendChild(charElement);
colCount += width;
}
return fragment;
}
Expand Down