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.

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

this.buffer.lines.get(i) returns undefined #824

Closed
Tyriar opened this issue Jul 28, 2017 · 7 comments
Closed

this.buffer.lines.get(i) returns undefined #824

Tyriar opened this issue Jul 28, 2017 · 7 comments
Labels
type/bug Something is misbehaving
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Jul 28, 2017

Original issue: #821
PR for quick fix: #823

The resize rows section below is meant to handle this row resizing, if this is actually happening we should figure out the root cause. this.buffer.lines.get(i) should always return an array when i < this.buffer.lines.length which is guaranteed by line 1942.

@jpmasters any more details on how you're using xterm.js?

@Tyriar Tyriar added the type/bug Something is misbehaving label Jul 28, 2017
@jpmasters
Copy link

Hi @Tyriar I can reproduce it in the demo using the following after refreshing the demo page:

// create a scroll region from lines 13 to 19
term.write('\x1b[13;19r');
// move the cursor to the top of the scroll region
term.write('\x1b[13;0H');
// RI
term.write('\x1bM');
// now the last entries are undefined
console.log(term.buffer.lines.length);
console.log(term.buffer.lines[term.buffer.lines.length - 5]);

If you now call term.resize() with a rows value > the current rows it'll throw a type error if the terminal has had the chance to render in the meantime so try the resize() as a new command.

term.resize(50, 50);

The key to reproducing it seems to be to get (Terminal.buffer.y === Terminal.buffer.scrollTop) &&((Terminal.buffer.y + Terminal.buffer.ybase) > 0) then calling reverseIndex. We managed to do that by using a scroll region but I don't know if that's the only way it can happen. The code as it is at the moment assumes a RI will only need to insert a new line when buffer.y === 0.

Does that make sense?

@parisk
Copy link
Contributor

parisk commented Aug 2, 2017

I confirm that this is happening on SourceLair as well. Here is a public Sentry event:

https://sentry.io/share/issue/35323433302e333139343137383232/

@amejia1
Copy link
Contributor

amejia1 commented Jan 12, 2018

Tracked this down to a problem with the 'buffer' property here. Seems now with the latest version of xterm.js, this keeps whatever buffer value was set to it initially or on activate. As I resize the terminal, I can see the buffers in the bufferset get updated, but it doesn't seem to get reflected in this property.

Changing this property to be implemented through a getter resolves the resizing issue for me but now I get a problem with selecting content to be copied from the terminal. In any case, this is the diff I have so far.

diff --git a/src/Buffer.ts b/src/Buffer.ts
index 623c385..3e7d612 100644
--- a/src/Buffer.ts
+++ b/src/Buffer.ts
@@ -121,11 +121,6 @@ export class Buffer implements IBuffer {
       if (this._terminal.cols < newCols) {
         const ch: CharData = [this._terminal.defAttr, ' ', 1, 32]; // does xterm use the default attr?
         for (let i = 0; i < this._lines.length; i++) {
-          // TODO: This should be removed, with tests setup for the case that was
-          // causing the underlying bug, see https://github.com/sourcelair/xterm.js/issues/824
-          if (this._lines.get(i) === undefined) {
-            this._lines.set(i, this._terminal.blankLine(undefined, undefined, newCols));
-          }
           while (this._lines.get(i).length < newCols) {
             this._lines.get(i).push(ch);
           }
diff --git a/src/Terminal.ts b/src/Terminal.ts
index f95cc0e..d653138 100644
--- a/src/Terminal.ts
+++ b/src/Terminal.ts
@@ -293,10 +293,6 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT

     // Create the terminal's buffers and set the current buffer
     this.buffers = new BufferSet(this);
-    this.buffer = this.buffers.active;  // Convenience shortcut;
-    this.buffers.on('activate', (buffer: Buffer) => {
-      this.buffer = buffer;
-    });

     // Ensure the selection manager has the correct buffer
     if (this.selectionManager) {
@@ -304,6 +300,13 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT
     }
   }

+  /**
+   * Convenience property to active buffer.
+   */
+  public get buffer(): Buffer {
+      return this.buffers.active;
+  }
+
   /**
    * back_color_erase feature for xterm.
    */

@Tyriar
Copy link
Member Author

Tyriar commented Jan 12, 2018

The getter is definitely something we want, not sure what you mean by "problem with selecting content to be copied from the terminal"

@amejia1
Copy link
Contributor

amejia1 commented Jan 12, 2018

I mean when you are selecting content inside of the terminal to copy, like say the diff above from git diff.

@anandsiddharth
Copy link

anandsiddharth commented Feb 16, 2019

seems like this issue still exist
i am using xterm v3.11.0

screenshot 2019-02-16 at 8 58 08 pm

@Tyriar
Copy link
Member Author

Tyriar commented Feb 20, 2019

@anandsiddharth same underlying caused as #1932

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something is misbehaving
Projects
None yet
Development

No branches or pull requests

5 participants