-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add logic for reflowing lines #644
Closed
Closed
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
db7b792
Add logic for reflowing lines
LucianBuzzo 5bdef74
Fixed bug caused by unwrapping lines.
LucianBuzzo d6f41ca
Fixed some issues releated to running in an alt screen buffer and
LucianBuzzo b9449a1
Addressing PR comments
LucianBuzzo 893289c
Remove use of `.bind` and switch to `.length` in forEach call
LucianBuzzo d1fb763
Performance improvements and added throttle to resize function.
LucianBuzzo e680d4d
Removed Throttle function
LucianBuzzo 0e37a42
Streamlined concatWrapped logic
LucianBuzzo d176aeb
Improved trimmedLength performance
LucianBuzzo ca62596
Dramatic performance improvements by removing use of indexOf
LucianBuzzo 09df48d
Reduced trimmed length complexity
LucianBuzzo 54b50a4
reducing reflow complexity
LucianBuzzo a3ade37
Fixed undefined character bug
LucianBuzzo 2722a7d
Fixed tab characters
LucianBuzzo 7ac4119
Fix tests to account for null characters being used instead of spaces.
LucianBuzzo fce1d8d
Increase reflow performance, don't go over max scrollback, don't mani…
LucianBuzzo 4fa6db6
remove unused functions
LucianBuzzo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,150 @@ | ||
import { CircularList } from './CircularList'; | ||
import { RowData } from '../Types'; | ||
|
||
import { IWrappableList } from '../Interfaces'; | ||
|
||
function fastForeach(array, fn) { | ||
let i = 0; | ||
let len = array.length; | ||
for (i; i < len; i++) { | ||
fn(array[i], i, array); | ||
} | ||
} | ||
|
||
function trimmedLength(line) { | ||
let i = 0; | ||
let len = line.length; | ||
for (i; i < len; i++) { | ||
if (!line[i] || (line[i] && line[i][1] === null)) { | ||
break; | ||
} | ||
} | ||
|
||
return i; | ||
} | ||
|
||
export class WrappableList extends CircularList<RowData> { | ||
private _wrappedLineIncrement: number[] = []; | ||
private _blankline: RowData; | ||
public wrappedLines: number[] = []; | ||
|
||
constructor(maxLength: number, private _terminal) { | ||
super(maxLength); | ||
|
||
this._blankline = this._terminal.blankLine(); | ||
} | ||
|
||
public push(value: RowData): void { | ||
// Need to make sure wrappedlines move when CircularList wraps around, but without increasing | ||
// the time complexity of `push`. We push the number of `wrappedLines` that should be | ||
// incremented so that it can be calculated later. | ||
if (this._length + 1 === this.maxLength) { | ||
this._wrappedLineIncrement.push(this.wrappedLines.length); | ||
} | ||
super.push(value); | ||
} | ||
|
||
public addWrappedLine(row: number): void { | ||
this.wrappedLines.push(row); | ||
} | ||
|
||
// Adjusts `wrappedLines` using `_wrappedLineIncrement` | ||
private _adjustWrappedLines(): void { | ||
fastForeach(this._wrappedLineIncrement, (end) => { | ||
let i = 0; | ||
for (i; i < end; i++) { | ||
this.wrappedLines[i] -= 1; | ||
} | ||
}); | ||
this._wrappedLineIncrement = []; | ||
} | ||
|
||
private _numArrayToObject(array: number[]) { | ||
let i = 0; | ||
let len = array.length; | ||
let returnObject = {}; | ||
for (i; i < len; i++) { | ||
returnObject[array[i]] = null; | ||
} | ||
return returnObject; | ||
} | ||
|
||
/** | ||
* Reflow lines to a new max width. | ||
* A record of which lines are wrapped is stored in `wrappedLines` and is used to join and split | ||
* lines correctly. | ||
*/ | ||
public reflow(width: number, oldWidth: number) { | ||
const temp = []; | ||
const tempWrapped = []; | ||
const wrappedLines = this.wrappedLines; | ||
let masterIndex = 0; | ||
let len = this.length; | ||
let line; | ||
let trim; | ||
let isWidthDecreasing = width < oldWidth; | ||
let i = 0; | ||
let xj; | ||
|
||
this._adjustWrappedLines(); | ||
// Using in index accessor is much quicker when we need to calculate previouslyWrapped many times | ||
const wrappedLinesObject = this._numArrayToObject(this.wrappedLines); | ||
|
||
const concatWrapped = (data, index) => { | ||
let next = index; | ||
while (wrappedLinesObject[next] !== undefined) { | ||
next++; | ||
masterIndex++; | ||
Array.prototype.push.apply(data, this.get(next)); | ||
} | ||
return data; | ||
}; | ||
|
||
// A for loop is used here so that masterIndex can be advanced when concatting lines in | ||
// the 'concatWrapped' method | ||
for (masterIndex; masterIndex < len; masterIndex++) { | ||
line = concatWrapped(this.get(masterIndex), masterIndex); | ||
trim = trimmedLength(line); | ||
|
||
if (trim > width) { | ||
line.length = trim; | ||
xj = fastCeil(trim / width) - 1; | ||
for (i = 0; i < trim; i += width) { | ||
if (width > line.length) { | ||
temp.push(line); | ||
} else { | ||
temp.push(line.splice(0, width)); | ||
} | ||
|
||
if (xj-- > 0) { | ||
tempWrapped.push(temp.length - 1); | ||
} | ||
} | ||
} else { | ||
if (isWidthDecreasing) { | ||
line.length = width; | ||
} | ||
temp.push(line); | ||
} | ||
} | ||
|
||
// Chop the reflow list to length and push it into a new CircularList, also compensate wrapped | ||
// lines for new start point of list | ||
const scrollback = this.maxLength; | ||
let pushStart = temp.length > scrollback ? | ||
temp.length - scrollback : | ||
0; | ||
if (pushStart > 0) { | ||
for (i = 0; i < tempWrapped.length; i++) { | ||
tempWrapped[i] -= pushStart; | ||
} | ||
} | ||
let newList = new WrappableList(scrollback, this._terminal); | ||
for (i = pushStart; i < temp.length; i++) { | ||
newList.push(temp[i]); | ||
} | ||
newList.wrappedLines = tempWrapped; | ||
|
||
return newList; | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this need to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the
get
method uses_getCyclicIndex
which uses_startIndex
though?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on using
.length
! I think the.bind
is left over from earlier experiments and can be removed.