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

Make buffer length equal rows + scrollback, make alt buffer always 0 scrollback #855

Merged
merged 7 commits into from
Aug 16, 2017

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Aug 7, 2017

Fixes #847
Fixes #802

This is needed so that resize is still performant. Now the list size is only
rebuild (O(n)) when it goes beyond 40 entries of the initial max length. The
List is also rebuilt when the size shrinks below 120 of the actual backing max
length (init_max_length + 40 - 120).
Unfortunately this removed the padding idea from CircularList as it
would only work when the list is not full.
@Tyriar Tyriar added this to the 3.0.0 milestone Aug 7, 2017
@Tyriar Tyriar self-assigned this Aug 7, 2017
@Tyriar Tyriar requested a review from parisk August 7, 2017 01:58
@coveralls
Copy link

coveralls commented Aug 7, 2017

Coverage Status

Coverage increased (+0.3%) to 71.585% when pulling e7ee3ec on Tyriar:847_proper_scrollback_value into 687a5e2 on sourcelair:v3.

Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern with this PR is that a buffer becomes aware if it is the alt buffer or not.

Instead of doing this, I propose setting the length of each buffer explicitly via the BufferSet which is aware of both the normal and the alt buffer, and essentially manages them.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 9, 2017

@parisk I'm not sure that's better though, right now the "alt buffer scrollback is 0" logic is contained in a single place (as well as the comment explaining it). If we had BufferSet manage it, then we would need a scrollback parameter on the Buffer.constructor, Buffer.clear and Buffer.resize.

If preferable we could change the _isAltBuffer property to something like _hasScrollback to be more generic?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 71.825% when pulling 996eb1b on Tyriar:847_proper_scrollback_value into 91bf319 on sourcelair:v3.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 12, 2017

Ping @parisk

@parisk
Copy link
Contributor

parisk commented Aug 16, 2017

we would need a scrollback parameter on the Buffer.constructor, Buffer.clear and Buffer.resize

Couldn't we just pass it only in the constructor and have clear and resize sourcing it from the buffer's attributes?

Renaming _isAltBuffer to _hasScrollback is 👌 too.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 16, 2017

@parisk but they change and the values are needed every time these functions are called. We would either have to pipe through the scrollback (or total height) every time from BufferSet or just have Buffer manage it itself.

@coveralls
Copy link

coveralls commented Aug 16, 2017

Coverage Status

Coverage increased (+0.3%) to 71.89% when pulling 9dd1083 on Tyriar:847_proper_scrollback_value into aa438c8 on sourcelair:v3.

@coveralls
Copy link

coveralls commented Aug 16, 2017

Coverage Status

Coverage increased (+0.3%) to 71.89% when pulling 7b0d0e8 on Tyriar:847_proper_scrollback_value into aa438c8 on sourcelair:v3.

@Tyriar Tyriar merged commit f3d375a into xtermjs:v3 Aug 16, 2017
@Tyriar Tyriar deleted the 847_proper_scrollback_value branch August 16, 2017 16:38
@Tyriar Tyriar mentioned this pull request Sep 5, 2017
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants