Add logic for reflowing lines #644

LucianBuzzo · 2017-04-24T12:11:47Z

Connects to #622

I believe this approach will be more stable and easier to manage than the one previously proposed in #609
One thing I realised after working on the previous reflow PR was that you have a lot of instances where input intentionally writes over EOL and then expects to overwrite the wrapped characters using a carriage return or CSI Ps ; Ps H.
Using the dynamic reflow I had proposed previously, it's very difficult to compensate for this behaviour without a lot of trickery.

As it stands this PR appears to work very well, though I'm sure that it needs more testing.
I'd greatly appreciate people trying to break the implementation and then posting their feedback here.

mofux · 2017-04-24T17:19:09Z

Looks very good, I could only find one last bug, which seems to happen when the input line is wrapped after a resize. It is a little hard to reproduce, but I've been able to capture a video that demonstrates the bug. You can see that I enter lots of text into the input line, then i resize and the characters are shown incorrectly. I then resize back and everything is back to normal. I then run top, and after quitting top you can see that the last line of the top output is not cleared. I then hit enter until I would probably overwrite the uncleared line, but in that moment I get an exception.

Here's the full video that demonstrates my way down to the exception:

Martin1994 · 2017-04-24T18:01:37Z

Here's a much simpler way to reproduce (hopefully) the same bug as @mofux mentioned:

Platform: macOS 10.12.4, stable Chrome 57, node v7.7.1

Steps:

Open xterm.js demo
Change current directory to the root of xterm.js
vim README.md
Move the cursor to line 9 column 11 (on the letter "s" of "## Features")
Change column size to 91

Expected:
Resize to 91*24 and the cursor should be still on letter "s".

Actual:
An exception is thrown. Then xterm.js doesn't respond any more.

Stack trace:

InputHandler.addChar (InputHandler.ts:88)
Parser.parse (Parser.ts:221)
Terminal.innerWrite (xterm.js:1280)
(anonymous) (xterm.js:1259)
setTimeout (async)
Terminal.write (xterm.js:1258)
term._getMessage (attach.js:64)

Tyriar · 2017-04-24T21:45:16Z

Opening in vim and fiddling with the size seems to break the normal buffer when you exit:

Notice the scrollbar, scrolling it does nothing. Looks like the same exception:

Uncaught TypeError: Cannot set property '0' of undefined
    at InputHandler.addChar (InputHandler.ts:88)
    at Parser.parse (Parser.ts:221)
    at Terminal.innerWrite (xterm.js:1280)
    at xterm.js:1259

Tyriar

@LucianBuzzo here's a pretty large review. It would be helpful to get a small high-level write up of the approach you're taking, in particular the following things:

How wrappedLines works
How WrappableList.transform works
What the changes in InputHandler do

Tyriar · 2017-04-24T21:50:42Z

src/Interfaces.ts

@@ -74,6 +74,10 @@ interface ICircularList<T> {
  shiftElements(start: number, count: number, offset: number): void;
 }

+export interface IWrappableList extends ICircularList<any[]> {
+  transform(width: number): void;


This needs a comment as to its purpose.

Tyriar · 2017-04-24T21:52:58Z

src/utils/CircularList.ts

@@ -43,8 +43,13 @@ export class CircularList<T> {
    this._length = newLength;
  }

-  public get forEach(): (callbackfn: (value: T, index: number, array: T[]) => void) => void {
-    return this._array.forEach;
+  public forEach(callbackfn: (value: T, index?: number, array?: T[]) => void): void {


Why did this need to change?

As it stood, the forEach implementation didn't take into account the cyclic index, so as soon as the list started to wrap, the behaviour became unpredictable.

Great point, this may be causing other obscure bugs if it's used anywhere important.

If this is the case though, I think the new forEach needs to take into account the _startIndex since a buffer can start from any index and not necessarily wrap all the way around. https://github.com/sourcelair/xterm.js/blob/944da2808906aa86b6ad096fd0b179e8cf87e0b8/src/utils/CircularList.ts#L140

the get method uses _getCyclicIndex which uses _startIndex though?

Good point 😄

Is there any reason you use .bind? This has a big downside in TypeScript as you lose typing information.

Also I think you should use .length instead of .maxLength for the reason mentioned above?

Good catch on using .length! I think the .bind is left over from earlier experiments and can be removed.

Tyriar · 2017-04-24T21:57:57Z

src/utils/WrappableList.ts

+  return value !== null;
+}
+
+export class WrappableList<T> extends CircularList<any[]> {


WrappableList shouldn't be generic:

export class WrappableList extends CircularList<any[]> {

I remember having issues with the inherited methods when I was typing this, though it was probably down to my bad TS skills! I'll take another look at it

Tyriar · 2017-04-24T21:59:21Z

src/utils/WrappableList.ts

+    if (this._length + 1 === this.maxLength) {
+      this.wrappedLines = this.wrappedLines.map(x => x - 1);
+    }
+    this._array[this._getCyclicIndex(this._length)] = value;


Defer implementation here to super class:

super.push(value);

Tyriar · 2017-04-24T22:03:22Z

src/utils/WrappableList.ts

+  // Need to make sure wrappedlines move when CircularList wraps around
+  public push(value: any[]): void {
+    if (this._length + 1 === this.maxLength) {
+      this.wrappedLines = this.wrappedLines.map(x => x - 1);


Array.prototype.map will create a new array of the same size, initializing it to the result of the function (ie. linear). We need WrappableList.push to remain at a constant time complexity as with a large buffer this happens on every since row added to the list (which can be a lot depending on the command).

Tyriar · 2017-04-24T22:14:04Z

src/utils/WrappableList.ts

+      }
+    };
+
+    this.forEach((line, index) => {


This also won't scale particularly well and will likely cause xterm.js embedders to lock up with a large scrollback buffer.

Can you think of any good real world examples of embedded software that I can test performance with? I think there's quite a few gains that can be made in terms of efficiency

When I was making performance improvements I would up the scrollback buffer and run ls -lR in a reasonably large directory and measure the time/lock ups.

Note that normally time is useful here but it's not accurate in the demo.

Another thing you can do is attach window resize to the resize function, this is what many program do including VS Code as right now resize is a relatively lightweight operation and it would be good to keep it that way.

What's the srollback buffer in vscode?

1000 by default, it's not uncommon for people to want much more than that though.

I've made some improvements and I'm seeing good performance up to a scrollback size of 5000 lines. Above 5000 lines the performance starts to decrease but I think it's still acceptable.
As an example, after setting the scrollback buffer to 10000 and running the command grep -Rl ' ' . in the root of the xterm.js project, running the resize command at 20 cols takes approximately 270ms. This is an extreme example, using the same method above but running resize at 40 cols takes approximately 80ms - I think this is a more likely use case.
To help prevent locking I've wrapped the resize method in a throttle function based on underscore's implementation - this helps a lot when binding the resize method to the window resize event.

I don't think forcing throttling is a particularly good solution here, the lib consumer could always do that if absolutely necessary. Throttling will also potentially cause layout issues for consumers as the terminal will be too large for the container (until the time is hit).

I'd really like to get perf improved much more than this though, 80ms is more than a frame already and remember that when xterm.js is hosted in an application like VS Code everything slows down a whole lot more as xterm.js is just a small portion of the code. As a real example, I use a scrollback of 5000 personally so that the build/test output from VS Code will fit in the buffer.

I think we need some more fundamental changes to the algorithms to get their time complexities down. I'll look through your descriptions below and study the code again and get back to you 😃

Tyriar · 2017-04-24T22:16:55Z

src/utils/WrappableList.ts

+
+    let cachedStartIndex = this._startIndex;
+    const scrollback = temp.length > this.maxLength ? temp.length : this.maxLength;
+    this.maxLength = scrollback;


I think you should be able to override the maxLength getter which would be preferable to assigning over the getter.

Ah yeah; I definitely don't want to assign over the getter!

Tyriar · 2017-04-24T22:18:12Z

src/xterm.js

+  let cachedLines = this.lines.length;
+  this.lines.transform(x);
+
+  let ymove = this.lines.length - cachedLines;


Looks like this will move based on the difference in rows, ideally we would move based on the line index.

Could you write some pseudo code to explain this? I don't quite understand what you mean, sorry!

Tyriar · 2017-04-24T22:19:48Z

src/xterm.js

+
+  if (ymove) {
+    if (this.ydisp === 0) {
+      this.y += ymove;


I believe changing the cursor position like this is dangerous.

Tyriar · 2017-04-24T22:23:59Z

src/InputHandler.ts

@@ -92,6 +94,11 @@ export class InputHandler implements IInputHandler {
        this._terminal.lines.get(row)[this._terminal.x] = [this._terminal.curAttr, '', 0];
        this._terminal.x++;
      }
+
+      let wi = this._terminal.lines.wrappedLines.indexOf(row);


It's not clear what's happening here. It also looks like InputHandler knows a lot more about the WrappableList implementation than it should.

Indeed this needs a higher level of abstraction and comments! 😊

LucianBuzzo · 2017-04-25T12:37:01Z

@Tyriar @mofux The issues you came across should be resolved in the latest commits.
I'll update the PR with comments and descriptions later.

Tyriar · 2017-04-25T15:55:29Z

src/xterm.js

  this.scrollTop = 0;
  this.scrollBottom = y - 1;

  this.charMeasure.measure();

  this.refresh(0, this.rows - 1);

-  this.normal = null;
+  // this.normal = null;


This has been here since the beginning, see #510 (comment) for more info.

@Tyriar It looks like this PR fixes the bug described in #510
We don't need to reset normal to null anymore as the normal screen buffer gets resized and updated after we switch back to it.

I'm a little skeptical of this, it looked like a lot of work to get it working properly when I looked at it. When you resize you need to resize both buffers, otherwise the other one will be out of date?

You could check by empty/full buffer on the normal buffer, switch to alt buffer, resize rows then switch back and check the state of the scroll bar. Not sure if that was the only issue though.

I ran through the repro steps mentioned in the issue and it appears to work fine.

I'll have to check it out then 😃

since the normal buffer is in a pristine state we can just run the resize logic (with the new force flag), when we switch back to the normal biffer and get the correct output

Connects to #622

writing lines to an unscrollable area.

LucianBuzzo · 2017-04-26T09:28:10Z

@Tyriar
How wrappedLines works -
When a line wraps, we store the line index in an array. When reflowing, we look up the index and if the line was previously wrapped we join it to the next line and then chunk it to the new width.

How WrappableList.transform works
Loop over every line in the list and chunk it to the new width, then each chunk is pushed into a new array. If the line was previously wrapped we join the next list item and then chunk the combined line.
The internals of the list are updated to use the new array.

What the changes in InputHandler do
Logic has been added to the InputHandler to mark a line as wrapped if it overflows onto the next line. This allows lines to unwrap when you increase the width of the terminal.
resize is now called when we switch back to a normal buffer from and alt buffer, forcing a reflow calculation. This has required the addition of a force parameter on the resize method, that will force the function to run even if the cols and rows haven't changed.

An additional change that has been made is to change blank character cells from ' ' to null. This has no effect on the renderer but helps differentiate between cells that have been added to pad lines and those that are written to the screen by program output.

LucianBuzzo · 2017-04-28T07:46:12Z

@Tyriar I've made some good performance improvements. 👍
I'm testing the time it takes to run the resize command to resize the terminal between 20 and 21 lines after running grep -Rl ' ' . :

With a scrollback of 10000 resize time is down to around 30ms.
With a scrollback of 5000 resize time is around 11ms.

Running the same tests, but resizing between 40 and 41 cols:

With a scrollback of 10000 resize time is down to around 22ms.
With a scrollback of 5000 resize time is around 8ms.

Tyriar · 2017-04-28T17:28:42Z

@LucianBuzzo I sat down yesterday, studied the code and looked at how we could scale the code a little better by first breaking down the problem again now that we have a better understanding and looking at how we could solve it by doing minimal work on get/push and resize operations.

FYI I checked after your recent perf improvements and am still seeing 300+ms resizes sometimes

Repro steps:

Set scrollback to 10000
Run ls -lR ~
Resize 81 -> 40

Even with these improvements while they may be fine for most users, I think it's still an unacceptable performance regression in VS Code as the team and our users are particularly sensitive to performance issues. Another aspect of VS Code and likely other apps is that multiple terminals will be resized at the same time. Therefore I'm a little reluctant in pushing as it would prevent us from pulling the latest xterm.js.

The community work on this issue has been invaluable so far though as we have a much clearer view of the problem now.

What we need

Wrap lines when reducing width
Unwrap lines when increasing width
Scroll bar needs to support wrapped height
Don't change the way input interacts with the buffer - when an escape sequence operates on row x, it should operate on the unwrapped row at x

Issues with current implementation

Accessing private properties on CircularList could lead to instability. This essentially obsoletes the unit tests on CircularList for how we're using it and makes it more difficult to maintain going forward.
A lot of costly array/string manipulation inside reflow.

Proposal

WrappableBuffer's state

I'm proposing we change WrappableBuffer to store the start index of the row

CircularList     WrappableBuffer where size = 4
[0]: "abcdef"      [0]: 0
[1]: "abcd"        [1]: 2
[2]: "abcdefgh"    [2]: 3

With this we have a basic model of the wrapping with minimal information. For fast access of the actual rows we will also want WrappableBuffer to have a map going the other way:

WrappableBuffer.wrappedToNormalIndex
[0]: [0, 0]
[1]: [0, 1]
[2]: [1, 0]
[3]: [2, 0]
[4]: [2, 1]

These two arrays are all that will be evaluated when the terminal is resized, so the resize of WrappableBuffer will look something like this:

resize(width: number) {
  // Lots of optimization potential here
  this.normalToWrappedIndex = new Array(this.circularList.length);
  this.wrappedToNormalIndex = new Array(this.circularList.length);
  let currentNormal = 0;
  let currentWrappedCount = 0;
  for (let i = 0; i < this.circularList.length; i++) {
    const line = this.circularList.get(i);
    const wrappedCount = Math.ceil(line.length / width);
    this.normalToWrappedIndex[i] = currentWrappedCount;
    for (let j = 0; j < wrappedCount; j++) {
      this.wrappedToNormalIndex[currentWrappedCount + j] = [i, j];
    }
    currentWrappedCount += wrappedCount;
  }
}

Retrieving rows

Since we're only doing the above on a resize we need to actually serve the wrapped rows somehow. This can be done at the time the wrapped row is requested with WrappableBuffer.getWrapped at which point we can cache the chunked row and store it for later use since string construction is relatively expensive.

When the line's wrapped state changes we can discard the cached chunked row.

Composition over inheritance

In order to prevent future misuse let's use a member variable instead of extending and have both CircularList and WrappableBuffer share the ICircularList interface.

CircularList`s internal changes

To deal CircularList's trimming mechanism we could have CircularList extend EventEmitter and throw an onTrim(count: number) event and act accordingly in WrappableBuffer (probably by storing an offset to add to the indexes).

If there are any other internal movement that WrappableBuffer needs to know about it could be exposed in an event.

Exposing both wrapped and unwrapped buffers

I believe we want to expose both wrapped (getWrapped?) and unwrapped buffers (get?) as escape sequences should be operating on the unwrapped rows but we want to display the wrapped rows.

Scrollback

Currently it's a little ambiguous what scrollback means, in the new solution scrollback would strictly define the number of unwrapped rows.

Result

Here's a once over of the ICircularList interface summarizing the changes that WrappableBuffer would make:

length: Similar to CircularList but changes on resize
maxLength: Computed based on CircularList.maxLength and WrappableBuffer.length;
forEach: Little change
get: Create and cache wrapped row
set: Re-evaluates wrapped state of all rows after the current row, this will typically only be called towards the end of the buffer
push: Evaluate wrapped state of new row
pop: Remove row
splice: Like set, should only be used near end of buffer
trimStart: Will throw an event in CircularList
shiftElements: Evaluate wrapped state of new rows, reuse when possible

@LucianBuzzo let me know what you think. You're of course under no obligation to implement this but I'm very interested to hear you thoughts.

LucianBuzzo · 2017-05-01T07:18:53Z

@Tyriar The issue that I think you will run into with this approach is that there are a number of behaviours where characters are expected to wrap and then written over with a carriage return or an escape sequence.
This makes handling "unwrapped lines" very tricky, as you can't tell wether you are dealing with an over long line or characters that are intentionally being sent to trigger a linewrap.
For example, when writing a very long prompt input that wraps, you're sent a string that contains a duped character write before the \r which is then expected to be overwritten by the following character because the terminal has wrapped onto the next line.
I think you can get around this by clever management of how you handle incoming linewraps, but it's something to consider.

Tyriar · 2017-05-01T17:49:43Z

@LucianBuzzo that case where a character is duplicated before the \r, how did you end up handling that in your approach?

LucianBuzzo · 2017-05-01T18:49:40Z

@Tyriar in my previous PR I never resolved the issue. I couldn't think of a robust way of handling these cases whilst taking the approach of ignoring the terminal width and storing the line data in an unwrapped state (I believe that is what you are proposing?). In this PR it's a non issue as we wrap lines when they go over the terminal width.
I think there's a possible approach where your line "source data" includes the line wraps for whatever width it was set at, and then we do on-demand reflow for the lines being rendered.

LucianBuzzo · 2017-05-03T06:56:18Z

@Tyriar I've just tried using the latest branch in vscode with a 10000 line scrollback (full as well) and it works really well.

Tyriar · 2017-05-03T16:59:36Z

@LucianBuzzo I definitely see the difference (2 terminals, 10000 scrollback):

Also 46 tests are failing. https://travis-ci.org/sourcelair/xterm.js/jobs/228226085 @parisk Travis isn't erroring out 😕

I'm going to have a look at perf improvements on top of your stuff now.

Tyriar · 2017-05-04T02:04:05Z

Was trying to get a quick proof of concept together of my design. Was running into a bit of an issue when wrapping get since the array returned is modified in multiple places, so slicing the array to get a subsection of it doesn't work.

Tyriar · 2017-05-05T23:55:55Z

@LucianBuzzo I put out a PR against your branch to clean up some types https://github.com/LucianBuzzo/xterm.js/pull/2

LucianBuzzo · 2017-05-09T08:34:40Z

@Tyriar Should I keep working on this and sort the tests out?

Tyriar · 2017-05-09T16:12:41Z

@LucianBuzzo currently I don't think we can accept this until resize scales better and the problems with the design are fixed (breaking the private API of CircularList, scrollback increasing permanently on a resize) as they represent technical debt we would be taking on.

LucianBuzzo · 2017-05-09T16:33:35Z

@Tyriar WRT the resize functionality scaling better, what are the benchmarks that you want to hit?
You had previously mentioned resizing a 5000 line scrollback in under a frame (~16ms?) - is this still the case?

Tyriar · 2017-05-09T16:46:31Z

@LucianBuzzo I was testing 1-2 terminals with 10000 sized buffers within VS Code. The issues with the scaling are:

At least in VS Code we will resize multiple terminals at once when resizing the window, multiplying the slowness of the function
With a single 10000 sized terminal buffer I was getting 300ms+ pretty consistently on a reasonably specced machine. For example see https://cloud.githubusercontent.com/assets/2193314/25672106/2ac1878c-2fe7-11e7-9595-e7c37325fba2.png which shows that it pushed the frame out to 1445ms
Resizing of the terminal happens not just on window resize but also when resizing the sidebar, toggling the activity bar, moving the window around at an OS-level
This also impacts battery life negatively if we max out the CPU for so long

It's difficult to define an exact benchmark, basically I would like to see a single terminal resizing in as little time as possible. Something more like < 30ms to resize a 10000 buffer while being hosted within VS Code (on my apparently slow machine).

But it's not the only issue with the implementation, the reaching into CircularList's implementation is a pretty big no-no and represents work that would need to be done on our side to fix it, and it doesn't really have an easy solution. For example I'm working on redoing how selection is handled which needs to modify the implementation of CircularList, this will break the reflow logic as it makes assumptions about the form of _array that will be broken.

mofux · 2017-05-10T10:32:31Z

@Tyriar Your comment reads like a dead-end for this PR. I understand your concerns, but I feel a lot of work and ideas that went into this PR and its previous iterations could lay the ground for a re-implementation from your side. I believe you will have to take the lead on this, as this feature has a direct dependency to the virtual selection and the link matcher (multiple lines) features, and you're the only contributor who has an insight knowledge about all of them.

Tyriar · 2017-05-10T17:29:35Z

@mofux unfortunately yeah. While this whole thing has been very valuable as we've learned a lot. I'm definitely going to do a better job in looking into how hard something is before encouraging PRs on it, I had no idea this would have been so hard and touching so many things until I started reviewing the PRs.

…pulate circular list internals

LucianBuzzo · 2017-05-15T04:26:54Z

@Tyriar I've given this some thought and updated this PR with an implementation that attempts to resolve the issues you've described here.

Scrollback will no longer increase over its original size
The CircularList internal implementation is not used (or assumed)
I can see noticeable performance improvements (tested in VScode)

In theory, this means that there should be no conflict with the line selection work you are doing.
This is a rough draft and can certainly be improved on.
If you think this is moving in the right direction let me know.

LucianBuzzo · 2017-05-19T07:32:51Z

ping @Tyriar ^^

Tyriar · 2017-05-22T03:52:13Z

Pretty flat out with work at the moment, will have to defer evaluating the changes for a bit.

LucianBuzzo · 2017-05-22T04:06:45Z

@Tyriar Of course, no problem!

Tyriar · 2017-08-03T16:31:16Z

Closing this as it's not going to get merged for performance/design reasons and it's really out of sync now. It is very useful as a reference though so I've pulled some of the key findings out into #622

LucianBuzzo mentioned this pull request Apr 24, 2017

Add logic for reflowing lines and preserving data when resizing #609

Closed

Tyriar requested changes Apr 24, 2017

View reviewed changes

Tyriar reviewed Apr 25, 2017

View reviewed changes

LucianBuzzo added 4 commits April 26, 2017 10:14

Add logic for reflowing lines

db7b792

Connects to #622

Fixed bug caused by unwrapping lines.

5bdef74

Fixed some issues releated to running in an alt screen buffer and

d6f41ca

writing lines to an unscrollable area.

Addressing PR comments

b9449a1

LucianBuzzo added 6 commits April 27, 2017 10:55

Remove use of .bind and switch to .length in forEach call

893289c

Performance improvements and added throttle to resize function.

d1fb763

Removed Throttle function

e680d4d

Streamlined concatWrapped logic

0e37a42

Improved trimmedLength performance

d176aeb

Dramatic performance improvements by removing use of indexOf

ca62596

LucianBuzzo added 4 commits May 1, 2017 20:38

Reduced trimmed length complexity

09df48d

reducing reflow complexity

54b50a4

Fixed undefined character bug

a3ade37

Fixed tab characters

2722a7d

Tyriar mentioned this pull request May 3, 2017

npm test returns an exit code of 0 when it fails #651

Closed

Fix tests to account for null characters being used instead of spaces.

7ac4119

Increase reflow performance, don't go over max scrollback, don't mani…

fce1d8d

…pulate circular list internals

remove unused functions

4fa6db6

This was referenced Jul 11, 2017

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

Closed

Buffer performance improvements #791

Closed

Tyriar mentioned this pull request Jul 20, 2017

Use Virtual DOM for rendering #807

Closed

Tyriar mentioned this pull request Aug 3, 2017

Support reflowing lines on resize #622

Closed

Tyriar closed this Aug 3, 2017

Tyriar added the work-in-progress Do not merge label Aug 3, 2017

Tyriar added the reference A closed issue/pr that is expected to be useful later as a reference label Sep 13, 2018

Add logic for reflowing lines #644

Add logic for reflowing lines #644

Conversation

LucianBuzzo commented Apr 24, 2017

mofux commented Apr 24, 2017

Martin1994 commented Apr 24, 2017 • edited Loading

Tyriar commented Apr 24, 2017 • edited Loading

Tyriar left a 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

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

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

LucianBuzzo commented Apr 25, 2017

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

LucianBuzzo commented Apr 26, 2017

LucianBuzzo commented Apr 28, 2017

Tyriar commented Apr 28, 2017

What we need

Issues with current implementation

Proposal

WrappableBuffer's state

Retrieving rows

Composition over inheritance

CircularList`s internal changes

Exposing both wrapped and unwrapped buffers

Scrollback

Result

LucianBuzzo commented May 1, 2017

Tyriar commented May 1, 2017

LucianBuzzo commented May 1, 2017

LucianBuzzo commented May 3, 2017

Tyriar commented May 3, 2017

Tyriar commented May 4, 2017

Tyriar commented May 5, 2017

LucianBuzzo commented May 9, 2017

Tyriar commented May 9, 2017

LucianBuzzo commented May 9, 2017

Tyriar commented May 9, 2017

mofux commented May 10, 2017

Tyriar commented May 10, 2017

LucianBuzzo commented May 15, 2017

LucianBuzzo commented May 19, 2017

Tyriar commented May 22, 2017

LucianBuzzo commented May 22, 2017

Tyriar commented Aug 3, 2017

Martin1994 commented Apr 24, 2017 •

edited

Loading

Tyriar commented Apr 24, 2017 •

edited

Loading