Fix #948 #1108
Fix #948 #1108
Conversation
src/Buffer.ts
Outdated
| @@ -11,6 +11,7 @@ export const CHAR_DATA_ATTR_INDEX = 0; | |||
| export const CHAR_DATA_CHAR_INDEX = 1; | |||
| export const CHAR_DATA_WIDTH_INDEX = 2; | |||
| export const CHAR_DATA_CODE_INDEX = 3; | |||
| export const MAX_BUFFER_SIZE = -1 >>> 0; // 2^32 - 1 | |||
There was a problem hiding this comment.
Are you sure this works consistently across all browsers? Doing an unsigned bit operation on a signed number seems dangerous. Might be better to just stick the number in there.
src/Buffer.ts
Outdated
| @@ -68,7 +69,10 @@ export class Buffer implements IBuffer { | |||
| if (!this._hasScrollback) { | |||
| return rows; | |||
| } | |||
| return rows + this._terminal.options.scrollback; | |||
|
|
|||
| const correctBuferLength = rows + this._terminal.options.scrollback; | |||
There was a problem hiding this comment.
correctBuferLength → correctBufferLength
Also, is this here only to cover the case where scrollback is set from the Terminal constructor? If so it might be good to validate at in the Terminal constructor instead of every time _getCorrectBufferLength is requested
There was a problem hiding this comment.
When i first bumped into this was via terminal.setOption(), and if my understanding is correct we could also trigger this by resizing the terminal, and increasing the number of rows if the buffer size was MAX_BUFFER_SIZE.
src/Terminal.ts
Outdated
| @@ -384,6 +384,8 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT | |||
| } | |||
| break; | |||
| case 'scrollback': | |||
| value = value > 0 && value <= MAX_BUFFER_SIZE ? value : MAX_BUFFER_SIZE; | |||
There was a problem hiding this comment.
Math.min would be preferable to this ternary, I was surprised how fast it was when I looked into it.
|
Thanks for the contribution and quick follow up 🎉 |
Fix suggested in #948
This is my first pull request, so all feedback is welcome.