From 95ebba6fdbfe64cb9b93644621b16d4ef3982da9 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Tue, 7 Aug 2018 19:32:30 -0700 Subject: [PATCH 1/8] Add link to running the demo from CONTRIBUTING.md --- CONTRIBUTING.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8b33d183..b4b3a9f2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -23,7 +23,7 @@ opening an issue, read these pointers. - Mention precisely what went wrong. What did you expect to happen? What happened instead? Describe the exact steps a maintainer has to take to make the problem occur. -- If the problem can not be reproduced in the [demo of xterm.js](README.md#demo), please provide an HTML document that demonstrates the problem. +- If the problem can not be reproduced in the [demo of xterm.js](https://github.com/xtermjs/xterm.js/wiki/Contributing#running-the-demo), please provide an HTML document that demonstrates the problem. - Be polite. Issues with an indignant or belligerent tone tend to be moved to the bottom of the pile. @@ -33,6 +33,7 @@ opening an issue, read these pointers. - Make sure you have a [GitHub account](https://github.com/join) - Fork [xterm.js](https://github.com/sourcelair/xterm.js/) ([how to fork a repo](https://help.github.com/articles/fork-a-repo)) +- Get the [xterm.js demo](https://github.com/xtermjs/xterm.js/wiki/Contributing#running-the-demo) running - Make your changes - If your changes are easy to test or likely to regress, add tests. Tests go into `test`, directory. - Follow the general code style of the rest of the project (see below). From 2127128ef58be350137fcf572e9b482342b56130 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Thu, 9 Aug 2018 12:19:37 -0700 Subject: [PATCH 2/8] Only sync viewport in an animation frame Fixes #444 --- src/Viewport.ts | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/src/Viewport.ts b/src/Viewport.ts index a8966d14..e89676d9 100644 --- a/src/Viewport.ts +++ b/src/Viewport.ts @@ -28,6 +28,9 @@ export class Viewport extends Disposable implements IViewport { // quick fix and could have a more robust solution in place that reset the value when needed. private _wheelPartialScroll: number = 0; + private _refreshAnimationFrame: number | null = null; + private _ignoreNextScrollEvent: boolean = false; + /** * Creates a new Viewport. * @param _terminal The terminal this viewport belongs to. @@ -62,6 +65,12 @@ export class Viewport extends Disposable implements IViewport { * necessary. */ private _refresh(): void { + if (this._refreshAnimationFrame === null) { + this._refreshAnimationFrame = requestAnimationFrame(() => this._innerRefresh()); + } + } + + private _innerRefresh(): void { if (this._charMeasure.height > 0) { this._currentRowHeight = this._terminal.renderer.dimensions.scaledCellHeight / window.devicePixelRatio; this._lastRecordedViewportHeight = this._viewportElement.offsetHeight; @@ -71,6 +80,17 @@ export class Viewport extends Disposable implements IViewport { this._scrollArea.style.height = this._lastRecordedBufferHeight + 'px'; } } + + // Sync scrollTop + const scrollTop = this._terminal.buffer.ydisp * this._currentRowHeight; + if (this._viewportElement.scrollTop !== scrollTop) { + // Ignore the next scroll event which will be triggered by setting the scrollTop as we do not + // want this event to scroll the terminal + this._ignoreNextScrollEvent = true; + this._viewportElement.scrollTop = scrollTop; + } + + this._refreshAnimationFrame = null; } /** @@ -90,12 +110,6 @@ export class Viewport extends Disposable implements IViewport { this._refresh(); } } - - // Sync scrollTop - const scrollTop = this._terminal.buffer.ydisp * this._currentRowHeight; - if (this._viewportElement.scrollTop !== scrollTop) { - this._viewportElement.scrollTop = scrollTop; - } } /** @@ -110,6 +124,13 @@ export class Viewport extends Disposable implements IViewport { return; } + // Ignore the event if it was flagged to ignore (when the source of the event is from Viewport) + if (this._ignoreNextScrollEvent) { + this._ignoreNextScrollEvent = false; + return; + } + + const newRow = Math.round(this._viewportElement.scrollTop / this._currentRowHeight); const diff = newRow - this._terminal.buffer.ydisp; this._terminal.scrollLines(diff, true); From 4af2d1ed41524a973759ce6ad4c3afe421bec3b8 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Mon, 27 Aug 2018 07:26:00 -0700 Subject: [PATCH 3/8] Remove duplicate triple-equals rule --- tslint.json | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tslint.json b/tslint.json index 5dac55d6..40e26fcf 100644 --- a/tslint.json +++ b/tslint.json @@ -59,10 +59,7 @@ "esSpecCompliant": true } ], - "triple-equals": [ - true, - "allow-null-check" - ], + "triple-equals": true, "typedef-whitespace": [ true, { @@ -111,7 +108,6 @@ "prefer-const-enum": [ true ], - "prefer-const": true, - "triple-equals": true + "prefer-const": true } } From a194263183dd7523b370951defc209e3c75a368f Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 31 Aug 2018 12:02:23 -0700 Subject: [PATCH 4/8] Document addDisposableListener Part of #1642 --- typings/xterm.d.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/typings/xterm.d.ts b/typings/xterm.d.ts index 1db05639..c8cc2cc6 100644 --- a/typings/xterm.d.ts +++ b/typings/xterm.d.ts @@ -425,6 +425,12 @@ declare module 'xterm' { */ emit(type: string, data?: any): void; + /** + * Adds an event listener to the Terminal, returning an IDisposable that can + * be used to conveniently remove the event listener. + * @param type The type of event. + * @param handler The event handler. + */ addDisposableListener(type: string, handler: (...args: any[]) => void): IDisposable; /** From dc332480cc934de67f8df502a500cbe2b6f521b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Fri, 31 Aug 2018 23:02:05 +0200 Subject: [PATCH 5/8] fix convertEol option --- src/InputHandler.test.ts | 30 ++++++++++++++++++++++++++++++ src/InputHandler.ts | 2 +- src/Terminal.ts | 1 - src/Types.ts | 1 - typings/xterm.d.ts | 9 +++++++++ 5 files changed, 40 insertions(+), 3 deletions(-) diff --git a/src/InputHandler.test.ts b/src/InputHandler.test.ts index 0db6c04f..722cc287 100644 --- a/src/InputHandler.test.ts +++ b/src/InputHandler.test.ts @@ -10,6 +10,7 @@ import { NULL_CELL_CHAR, NULL_CELL_CODE, NULL_CELL_WIDTH, CHAR_DATA_CHAR_INDEX } import { Terminal } from './Terminal'; import { IBufferLine } from './Types'; + // TODO: This and the sections related to this object in associated tests can be // removed safely after InputHandler refactors are finished class OldInputHandler extends InputHandler { @@ -445,4 +446,33 @@ describe('InputHandler', () => { expect(termContent(termNew)).eql(termContent(termOld)); }); }); + it('convertEol setting', function(): void { + // not converting + let s = ''; + const termNotConverting = new Terminal({cols: 15, rows: 10}); + (termNotConverting as any)._inputHandler.parse('Hello\nWorld'); + for (let i = 0; i < termNotConverting.cols; ++i) { + s += termNotConverting.buffer.lines.get(0).get(i)[CHAR_DATA_CHAR_INDEX]; + } + expect(s).equals('Hello '); + s = ''; + for (let i = 0; i < termNotConverting.cols; ++i) { + s += termNotConverting.buffer.lines.get(1).get(i)[CHAR_DATA_CHAR_INDEX]; + } + expect(s).equals(' World '); + + // converting + s = ''; + const termConverting = new Terminal({cols: 15, rows: 10, convertEol: true}); + (termConverting as any)._inputHandler.parse('Hello\nWorld'); + for (let i = 0; i < termConverting.cols; ++i) { + s += termConverting.buffer.lines.get(0).get(i)[CHAR_DATA_CHAR_INDEX]; + } + expect(s).equals('Hello '); + s = ''; + for (let i = 0; i < termConverting.cols; ++i) { + s += termConverting.buffer.lines.get(1).get(i)[CHAR_DATA_CHAR_INDEX]; + } + expect(s).equals('World '); + }); }); diff --git a/src/InputHandler.ts b/src/InputHandler.ts index 3c6aae0f..ad5aad39 100644 --- a/src/InputHandler.ts +++ b/src/InputHandler.ts @@ -477,7 +477,7 @@ export class InputHandler extends Disposable implements IInputHandler { // make buffer local for faster access const buffer = this._terminal.buffer; - if (this._terminal.convertEol) { + if (this._terminal.options.convertEol) { buffer.x = 0; } buffer.y++; diff --git a/src/Terminal.ts b/src/Terminal.ts index 3a0ce0ed..3927d8b5 100644 --- a/src/Terminal.ts +++ b/src/Terminal.ts @@ -134,7 +134,6 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II // TODO: This can be changed to an enum or boolean, 0 and 1 seem to be the only options public cursorState: number; public cursorHidden: boolean; - public convertEol: boolean; private _customKeyEventHandler: CustomKeyEventHandler; diff --git a/src/Types.ts b/src/Types.ts index 5ea2024a..21c6c571 100644 --- a/src/Types.ts +++ b/src/Types.ts @@ -66,7 +66,6 @@ export interface IInputHandlingTerminal extends IEventEmitter { bell(): void; focus(): void; - convertEol: boolean; updateRange(y: number): void; scroll(isWrapped?: boolean): void; setgLevel(g: number): void; diff --git a/typings/xterm.d.ts b/typings/xterm.d.ts index 1db05639..d40c045f 100644 --- a/typings/xterm.d.ts +++ b/typings/xterm.d.ts @@ -39,6 +39,15 @@ declare module 'xterm' { */ bellStyle?: 'none' /*| 'visual'*/ | 'sound' /*| 'both'*/; + /** + * When enabled the cursor will be set to the beginning of the next line with + * every new line. This equivalent to sending '\r\n' for each '\n'. + * Normally the termios settings of the underlying PTY deals with + * the translation of '\n' to '\r\n' and this setting should not be used. + * If you deal with data from a not PTY related source this settings might be useful. + */ + convertEol?: boolean; + /** * The number of columns in the terminal. */ From 467a93c8d9080d66c2329a2616fcfa6197f69388 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sat, 1 Sep 2018 09:34:01 -0700 Subject: [PATCH 6/8] Fix typo/grammar/line width --- typings/xterm.d.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/typings/xterm.d.ts b/typings/xterm.d.ts index d40c045f..e773bf47 100644 --- a/typings/xterm.d.ts +++ b/typings/xterm.d.ts @@ -40,11 +40,12 @@ declare module 'xterm' { bellStyle?: 'none' /*| 'visual'*/ | 'sound' /*| 'both'*/; /** - * When enabled the cursor will be set to the beginning of the next line with - * every new line. This equivalent to sending '\r\n' for each '\n'. - * Normally the termios settings of the underlying PTY deals with - * the translation of '\n' to '\r\n' and this setting should not be used. - * If you deal with data from a not PTY related source this settings might be useful. + * When enabled the cursor will be set to the beginning of the next line + * with every new line. This equivalent to sending '\r\n' for each '\n'. + * Normally the termios settings of the underlying PTY deals with the + * translation of '\n' to '\r\n' and this setting should not be used. If you + * deal with data from a non-PTY related source, this settings might be + * useful. */ convertEol?: boolean; From 6057586ff30ecd837dca9acec3c2222f06c4ccba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Sun, 2 Sep 2018 17:52:56 +0200 Subject: [PATCH 7/8] fixes #1647 --- src/addons/search/SearchHelper.ts | 2 +- src/addons/search/search.test.ts | 53 +++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 src/addons/search/search.test.ts diff --git a/src/addons/search/SearchHelper.ts b/src/addons/search/SearchHelper.ts index 5ba81aa0..9b5c06f3 100644 --- a/src/addons/search/SearchHelper.ts +++ b/src/addons/search/SearchHelper.ts @@ -117,7 +117,7 @@ export class SearchHelper implements ISearchHelper { if (searchIndex >= 0) { const line = this._terminal._core.buffer.lines.get(y); for (let i = 0; i < searchIndex; i++) { - const charData = line[i]; + const charData = line.get(i); // Adjust the searchIndex to normalize emoji into single chars const char = charData[1/*CHAR_DATA_CHAR_INDEX*/]; if (char.length > 1) { diff --git a/src/addons/search/search.test.ts b/src/addons/search/search.test.ts new file mode 100644 index 00000000..7ecb32d3 --- /dev/null +++ b/src/addons/search/search.test.ts @@ -0,0 +1,53 @@ +/** + * Copyright (c) 2018 The xterm.js authors. All rights reserved. + * @license MIT + */ + +import { assert, expect } from 'chai'; +import * as search from './search'; +import { SearchHelper } from './SearchHelper'; +import { ISearchHelper } from './Interfaces'; + + +class MockTerminalPlain {} + +class MockTerminal { + private _core: any; + public searchHelper: ISearchHelper; + constructor(options: any) { + this._core = new (require('../../../lib/Terminal').Terminal)(options); + this.searchHelper = new SearchHelper(this as any); + } + get core(): any { + return this._core; + } +} + +describe('search addon', function(): void { + describe('apply', () => { + it('should register findNext and findPrevious', () => { + search.apply(MockTerminalPlain); + assert.equal(typeof (MockTerminalPlain).prototype.findNext, 'function'); + assert.equal(typeof (MockTerminalPlain).prototype.findPrevious, 'function'); + }); + }); + it('Searchhelper - should find correct position', function(done: () => void): void { + search.apply(MockTerminal); + const term = new MockTerminal({cols: 20, rows: 3}); + term.core.write('Hello World\r\ntest\n123....hello'); + setTimeout(() => { + const hello0 = (term.searchHelper as any)._findInLine('Hello', 0); + const hello1 = (term.searchHelper as any)._findInLine('Hello', 1); + const hello2 = (term.searchHelper as any)._findInLine('Hello', 2); + expect(hello0).eql({col: 0, row: 0, term: 'Hello'}); + expect(hello1).eql(undefined); + expect(hello2).eql({col: 11, row: 2, term: 'Hello'}); + done(); + }, 100); + }); + // selection stuff not testable - depends on window + // the plugin is imho to high level and does more than searching + // maybe separate into: + // - a search addon - get positions for a term + // - a mark addon - highlight positions in output +}); From 559e932a2e4c7da649171cefea298183659b5af1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Sun, 2 Sep 2018 21:46:20 +0200 Subject: [PATCH 8/8] remove comment and setTimeout --- src/addons/search/search.test.ts | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/addons/search/search.test.ts b/src/addons/search/search.test.ts index 7ecb32d3..91ecc4ef 100644 --- a/src/addons/search/search.test.ts +++ b/src/addons/search/search.test.ts @@ -21,6 +21,9 @@ class MockTerminal { get core(): any { return this._core; } + pushWriteData(): void { + this._core._innerWrite(); + } } describe('search addon', function(): void { @@ -30,24 +33,17 @@ describe('search addon', function(): void { assert.equal(typeof (MockTerminalPlain).prototype.findNext, 'function'); assert.equal(typeof (MockTerminalPlain).prototype.findPrevious, 'function'); }); - }); - it('Searchhelper - should find correct position', function(done: () => void): void { + }); + it('Searchhelper - should find correct position', function(): void { search.apply(MockTerminal); const term = new MockTerminal({cols: 20, rows: 3}); term.core.write('Hello World\r\ntest\n123....hello'); - setTimeout(() => { - const hello0 = (term.searchHelper as any)._findInLine('Hello', 0); - const hello1 = (term.searchHelper as any)._findInLine('Hello', 1); - const hello2 = (term.searchHelper as any)._findInLine('Hello', 2); - expect(hello0).eql({col: 0, row: 0, term: 'Hello'}); - expect(hello1).eql(undefined); - expect(hello2).eql({col: 11, row: 2, term: 'Hello'}); - done(); - }, 100); + term.pushWriteData(); + const hello0 = (term.searchHelper as any)._findInLine('Hello', 0); + const hello1 = (term.searchHelper as any)._findInLine('Hello', 1); + const hello2 = (term.searchHelper as any)._findInLine('Hello', 2); + expect(hello0).eql({col: 0, row: 0, term: 'Hello'}); + expect(hello1).eql(undefined); + expect(hello2).eql({col: 11, row: 2, term: 'Hello'}); }); - // selection stuff not testable - depends on window - // the plugin is imho to high level and does more than searching - // maybe separate into: - // - a search addon - get positions for a term - // - a mark addon - highlight positions in output });