Introduce Buffer and BufferSet classes #717

parisk · 2017-06-19T08:28:41Z

This PR introduces the Buffer and BufferSet classes to help:

Organize all buffer information into a single place (Buffer class)
Manage a set of normal and alt buffer (BufferSet class)
Know when a buffer gets activated (BufferSet events)

To-do

Replace usage of lines with Buffer everywhere
Get existing tests work
Write tests for Buffer and BufferSet classes
Document Buffer and BufferSet classes

This will eventually help ship #613.

AndrienkoAleksandr · 2017-06-20T08:54:13Z

src/InputHandler.ts

-      this._terminal.lines.splice(this._terminal.ybase + this._terminal.scrollTop, 1);
-      this._terminal.lines.splice(this._terminal.ybase + this._terminal.scrollBottom, 0, this._terminal.blankLine());
+      this._terminal.buffer.lines.splice(this._terminal.buffer.ybase + this._terminal.scrollTop, 1);
+      this._terminal.buffer.lines.splice(this._terminal.buffer.ybase + this._terminal.scrollBottom, 0, this._terminal.blankLine());
    }
    // this.maxRange();
    this._terminal.updateRange(this._terminal.scrollTop);


this._terminal.ScrollTop and this._terminal.ScrollBottom seems should be in the buffer too.

AndrienkoAleksandr

"ybase", "ydisp" and "lines" should be deleted from ITerminal, because it will be used from buffer.
Seems "cursorHidden", "cursorState", "defAttr" should be moved from ITerminal to the Buffer and restored after switching from alt buffer to normal buffer(and otherwise) => 1047, 47, 1049 in the InputHandler...

…termjs#717

Tyriar · 2017-07-04T15:13:04Z

src/Buffer.ts

+ * This class represents a terminal buffer (an internal state of the terminal)/
+ */
+export class Buffer {
+  public lines: CircularList<string>;


FYI this is a CircularList<[string, number, string]>. Also it will change soon, see #756

AndrienkoAleksandr · 2017-07-04T19:14:02Z

src/InputHandler.ts

@@ -900,7 +900,7 @@ export class InputHandler implements IInputHandler {

          // TODO: Why are params[0] compares nested within a switch for params[0]?

-          this._terminal.x10Mouse = params[0] === 9;
+          this._terminal.buffer.x10Mouse = params[0] === 9;


@parisk Are you sure this line should be changed?

AndrienkoAleksandr · 2017-07-04T19:14:12Z

src/InputHandler.ts

@@ -1094,7 +1078,7 @@ export class InputHandler implements IInputHandler {
        case 1000: // vt200 mouse
        case 1002: // button event mouse
        case 1003: // any event mouse
-          this._terminal.x10Mouse = false;
+          this._terminal.buffer.x10Mouse = false;


@parisk Are you sure this line should be changed?

Tyriar · 2017-07-13T15:11:55Z

src/Interfaces.ts

  defAttr: number;
+  scrollback: number;
+  buffers: any;  // This should be a `BufferSet` class, but it would result in circular dependency


Let's use interfaces here so they're typed.

I tried this and it displays a weird error that ICircularList does not have a _length attribute 😕.

I added the _length attribute to ICircularList and it displays an error that this is a private attribute .

I am not expert with these, so if you have any tip here it could help.

Hmm, I would only expect that error if you added _length to the interface.

I fixed this up and pushed to this branch. The types were a bit messy, the list is actually a CircularList<[number, string, number][]> 😅

Tyriar · 2017-07-13T15:13:36Z

src/xterm.js

@@ -242,21 +219,27 @@ function Terminal(options) {
  // leftover surrogate high from previous write invocation
  this.surrogate_high = '';

+  // Create the terminal's buffers and set the current buffer
+  this.buffers = new BufferSet(this);
+  this.buffer = this.buffers.active;  // Convenience shortcut;


I think this should note:

// TODO: Change to a getter when moved to TypeScript

Are you aware if there any chances of this affecting performance if it becomes a getter since term.buffer is being accessed quite frequently?

It looks like getters should be pretty fast, this TS:

export class CircularList<T> extends EventEmitter { ... public get maxLength(): number { return this._array.length; }

Compiles to:

var CircularList = (function (_super) { ... Object.defineProperty(CircularList.prototype, "maxLength", { get: function () { return this._array.length; },

Tyriar · 2017-07-13T15:14:43Z

src/xterm.js

@@ -2301,9 +2284,13 @@ Terminal.prototype.reset = function() {
  this.options.cols = this.cols;
  var customKeyEventHandler = this.customKeyEventHandler;
  var cursorBlinkInterval = this.cursorBlinkInterval;
+  var inputHandler = this.inputHandler;
+  var buf = this.buffers;


Nit: this is the only one here that doesn't use the full name.

coveralls · 2017-07-14T07:05:10Z

Coverage increased (+0.3%) to 69.832% when pulling 6b98ef2 on buffer-set into 7cfc2ff on master.

coveralls · 2017-07-14T07:15:22Z

Coverage increased (+0.3%) to 69.832% when pulling 575577e on buffer-set into 7cfc2ff on master.

coveralls · 2017-07-14T07:21:48Z

Coverage increased (+0.3%) to 69.832% when pulling 575577e on buffer-set into 7cfc2ff on master.

coveralls · 2017-07-14T07:21:48Z

Coverage increased (+0.3%) to 69.832% when pulling 575577e on buffer-set into 7cfc2ff on master.

Signed-off-by: Aleksandr Andriienko <oandriie@redhat.com>

coveralls · 2017-07-15T08:05:07Z

Coverage increased (+0.3%) to 69.784% when pulling f840997 on buffer-set into c94fdae on master.

coveralls · 2017-07-15T08:46:57Z

Coverage increased (+0.7%) to 70.145% when pulling 1a2e3fb on buffer-set into c94fdae on master.

parisk · 2017-07-15T10:13:50Z

This should be ready for a final review 😄. /cc @Tyriar

coveralls · 2017-07-15T17:40:18Z

Coverage increased (+0.7%) to 70.145% when pulling c27431b on buffer-set into c94fdae on master.

coveralls · 2017-07-15T17:43:47Z

Coverage increased (+0.7%) to 70.145% when pulling c27431b on buffer-set into c94fdae on master.

Tyriar

Looks good, just 1 small comment.

FYI in case you missed the comment above, I pushed to type IBuffer, IBufferSet and also fix up the type of CircularList<[number, string, number][]> everywhere.

This should lead to some fun merge conflicts in #756 😛

Tyriar · 2017-07-15T17:43:09Z

src/xterm.js

+  this.buffers.on('activate', function (buffer) {
+    this._terminal.buffer = buffer;
+  });
+
  /**


This comment is no longer relevant, should be safe to remove as I'm going to heavily document the buffer format in #756

Tyriar · 2017-07-15T17:46:55Z

Also > 70% test coverage 🎉

coveralls · 2017-07-15T17:49:39Z

Coverage increased (+0.7%) to 70.145% when pulling c27431b on buffer-set into c94fdae on master.

coveralls · 2017-07-16T00:36:34Z

Coverage increased (+0.7%) to 70.145% when pulling 4cefa34 on buffer-set into c94fdae on master.

This broke in xtermjs#717. Fixes xtermjs#818

parisk added the work-in-progress Do not merge label Jun 19, 2017

parisk added this to the 2.8.0 milestone Jun 19, 2017

AndrienkoAleksandr reviewed Jun 20, 2017

View reviewed changes

AndrienkoAleksandr reviewed Jun 21, 2017

View reviewed changes

AndrienkoAleksandr added a commit to AndrienkoAleksandr/xterm.js that referenced this pull request Jun 21, 2017

Add todo to split this issue on the tasks and preparing to merge with x…

0101977

…termjs#717

Tyriar mentioned this pull request Jun 21, 2017

Implement getState method #613

Closed

10 tasks

parisk force-pushed the buffer-set branch 2 times, most recently from fbb6605 to 6643472 Compare June 30, 2017 10:20

parisk modified the milestones: 2.9.0, 2.8.0 Jul 3, 2017

Tyriar assigned parisk Jul 4, 2017

Tyriar reviewed Jul 4, 2017

View reviewed changes

AndrienkoAleksandr reviewed Jul 4, 2017

View reviewed changes

parisk force-pushed the buffer-set branch from 8dd4884 to 084475e Compare July 9, 2017 07:10

Tyriar mentioned this pull request Jul 11, 2017

Terminal should provide API to notify data cleaned by control sequences microsoft/vscode#29840

Closed

AndrienkoAleksandr mentioned this pull request Jul 12, 2017

Fix regression. Existing tests work again. #788

Merged

Tyriar reviewed Jul 13, 2017

View reviewed changes

parisk force-pushed the buffer-set branch from c310ab3 to 232e73f Compare July 14, 2017 06:54

parisk and others added 7 commits July 15, 2017 07:58

Introduce new Buffer and BufferSet classes

b3a3501

Create terminal.buffer convenience attribute

74ac1e1

Start isolating buffer attributes into Buffer class

65a2fd4

Fix some references

e1d39fe

Fix more references to buffer.x

60e0678

Move scrollTop and scrollBottom into Buffer

6628e8f

Fix turning from alt screen to normal screen and vice versa. Fix #229 .

5c71356

AndrienkoAleksandr and others added 3 commits July 15, 2017 07:58

Fix regression. Existing tests work again.

f8bea9d

Signed-off-by: Aleksandr Andriienko <oandriie@redhat.com>

Fix variable name

044faa5

Improve documentation of Buffer and BufferSet classes

f840997

parisk force-pushed the buffer-set branch from 575577e to f840997 Compare July 15, 2017 07:58

Implement tests for Buffer and BufferSet

1a2e3fb

parisk removed the work-in-progress Do not merge label Jul 15, 2017

Tyriar force-pushed the buffer-set branch from c244495 to 3979720 Compare July 15, 2017 17:37

Add IBuffer, IBufferSet and fix CircularList type

c27431b

Tyriar force-pushed the buffer-set branch from 3979720 to c27431b Compare July 15, 2017 17:38

Tyriar approved these changes Jul 15, 2017

View reviewed changes

AndrienkoAleksandr approved these changes Jul 15, 2017

View reviewed changes

parisk added 4 commits July 16, 2017 00:55

Remove redundant comment

e1903c6

Rearrange dependencies

6d002ca

Update build dir

1fca4f0

Install npm 5.1.0

4cefa34

parisk merged commit 48dab49 into master Jul 16, 2017

parisk deleted the buffer-set branch July 16, 2017 00:47

Tyriar mentioned this pull request Jul 27, 2017

Output is all messed up when running apt install/remove #818

Closed

Tyriar added a commit to Tyriar/xterm.js that referenced this pull request Jul 27, 2017

Fix save and restore cursor behavior

604959a

This broke in xtermjs#717. Fixes xtermjs#818

Tyriar mentioned this pull request Jul 27, 2017

Fix save and restore cursor behavior #822

Merged

Tyriar added a commit to Tyriar/xterm.js that referenced this pull request Jul 27, 2017

Fix save and restore cursor behavior

3b3f992

This broke in xtermjs#717. Fixes xtermjs#818

cheton added a commit to cncjs/cncjs that referenced this pull request Aug 24, 2017

[xterm 2.9.0] Replace usage of lines with buffer (xtermjs/xterm.js#717)

cef224e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Buffer and BufferSet classes #717

Introduce Buffer and BufferSet classes #717

parisk commented Jun 19, 2017 •

edited

AndrienkoAleksandr Jun 20, 2017

AndrienkoAleksandr left a comment •

edited

Tyriar Jul 4, 2017

AndrienkoAleksandr Jul 4, 2017

AndrienkoAleksandr Jul 4, 2017

Tyriar Jul 13, 2017

parisk Jul 15, 2017

Tyriar Jul 15, 2017

Tyriar Jul 15, 2017

Tyriar Jul 13, 2017

parisk Jul 14, 2017

Tyriar Jul 15, 2017

Tyriar Jul 13, 2017

coveralls commented Jul 14, 2017 •

edited

coveralls commented Jul 14, 2017 •

edited

coveralls commented Jul 14, 2017

coveralls commented Jul 14, 2017

coveralls commented Jul 15, 2017 •

edited

coveralls commented Jul 15, 2017 •

edited

parisk commented Jul 15, 2017

coveralls commented Jul 15, 2017 •

edited

coveralls commented Jul 15, 2017 •

edited

Tyriar left a comment

Tyriar Jul 15, 2017

Tyriar commented Jul 15, 2017

coveralls commented Jul 15, 2017 •

edited

coveralls commented Jul 16, 2017 •

edited

Introduce Buffer and BufferSet classes #717

Introduce Buffer and BufferSet classes #717

Conversation

parisk commented Jun 19, 2017 • edited

To-do

Choose a reason for hiding this comment

AndrienkoAleksandr left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 14, 2017 • edited

coveralls commented Jul 14, 2017 • edited

coveralls commented Jul 14, 2017

coveralls commented Jul 14, 2017

coveralls commented Jul 15, 2017 • edited

coveralls commented Jul 15, 2017 • edited

parisk commented Jul 15, 2017

coveralls commented Jul 15, 2017 • edited

coveralls commented Jul 15, 2017 • edited

Tyriar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tyriar commented Jul 15, 2017

coveralls commented Jul 15, 2017 • edited

coveralls commented Jul 16, 2017 • edited

parisk commented Jun 19, 2017 •

edited

AndrienkoAleksandr left a comment •

edited

coveralls commented Jul 14, 2017 •

edited

coveralls commented Jul 14, 2017 •

edited

coveralls commented Jul 15, 2017 •

edited

coveralls commented Jul 15, 2017 •

edited

coveralls commented Jul 15, 2017 •

edited

coveralls commented Jul 15, 2017 •

edited

coveralls commented Jul 15, 2017 •

edited

coveralls commented Jul 16, 2017 •

edited