Support reflowing lines on resize #622
Comments
|
I'm closing off the PR for this #644, if anything it was a good learning experience! Basically to tackle this feature properly we need to come up with a lightweight view model, it's probably best to be left to one of the core maintainers as it's quite complex and needs an intimate knowledge of the internals of xterm.js to be done right. I'll copy over the proposal from #644 (comment) for reference, it's still mostly correct and useful. I would also add to this that we need to consider the memory cost of the view model as well as potentially using the view model as a way to gain deeper insights into what's happening in the buffer for #731 (eg. knowing that a single character was added when a line is redrawn). What we need
Issues with current implementation
ProposalWrappableBuffer's stateI'm proposing we change With this we have a basic model of the wrapping with minimal information. For fast access of the actual rows we will also want These two arrays are all that will be evaluated when the terminal is resized, so the resize of 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 rowsSince 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 When the line's wrapped state changes we can discard the cached chunked row. Composition over inheritanceIn order to prevent future misuse let's use a member variable instead of extending and have both CircularList`s internal changesTo deal If there are any other internal movement that Exposing both wrapped and unwrapped buffersI believe we want to expose both wrapped ( ScrollbackCurrently it's a little ambiguous what scrollback means, in the new solution scrollback would strictly define the number of unwrapped rows. ResultHere's a once over of the
|
|
About this issue, in the section (What we need) " is exactly what we are looking for, a way to display in the xterm on a browser the wrap line when increasing/reducing the width of the window, be able to see all the data for the executed command. |
|
I'm planning on tackling this for 3.4.0, I recently went over the history of this and wrote up a refreshed design document: Requirements
Performance considerations
ImplementationThe solution will keep a separate view model This will provide fast O(1) access to wrapped or unwrapped indexes at the cost of a O(n) operation on resize to update the lists. This operation could probably be optimized pretty heavily. abstract class BaseWrappedList extends IWrappedList<LineData, CharData> {
private _circularList: ICircularList<LineData>;
// These should use TypedArrays/ArrayBuffers (may need to reduce MAX_BUFFER_SIZE)
private wrappedToNormalIndex: [number, number][];
private normalToWrappedIndex: number[];
// O(n) where n is length of _circularList
public onResize(cols: number): void {
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);
// TODO: Use _isListEntryDoubleWidth to determine the line of the
// last wrapped cell in each line
const wrappedCount = Math.ceil(line.length / cols);
this.normalToWrappedIndex[i] = currentWrappedCount;
for (let j = 0; j < wrappedCount; j++) {
this.wrappedToNormalIndex[currentWrappedCount + j] = [i, j];
}
currentWrappedCount += wrappedCount;
}
}
protected _isListEntryDoubleWidth(unwrappedRow: number, unwrappedCol: number): boolean {
return false;
}
}
class WrappedBuffer extends IWrappedList<LineData, CharData> {
protected _isListEntryDoubleWidth(unwrappedRow: number, unwrappedCol: number): boolean {
// Used to check if the last char is double width in order to wrap it to
// the next row or not
// Null check as needed
return this.get(unwrappedRow, unwrappedCol)[2] === 2;
}
}Beforeexport interface ICircularList<T> extends IEventEmitter {
length: number;
maxLength: number;
forEach: (callbackfn: (value: T, index: number) => void) => void; // Not sure this is used anymore
get(index: number): T;
set(index: number, value: T): void;
push(value: T): void;
pop(): T;
splice(start: number, deleteCount: number, ...items: T[]): void;
trimStart(count: number): void;
shiftElements(start: number, count: number, offset: number): void;
}
class CircularList extends ICircularList<LineData> { ... }Afterexport interface IWrappedList<T extends Array<U>, U> {
// How to handle wide chars on the last cell? Protected method
// _isListEntryDoubleWidth which is called on the last char to figure out the
// right wrapped spot
readonly length: number;
get(index: number): T;
splice(start: number, deleteCount: number, ...items: T[]): void;
shiftElements(start: number, count: number, offset: number): void;
onResize(cols: number): void;
}
export interface ICircularList<T> extends IEventEmitter {
length: number;
maxLength: number;
get(index: number): T;
set(index: number, value: T): void;
push(value: T): void;
pop(): T;
trimStart(count: number): void;
}Notes
|
|
is it possible to recalculate the visible lines first? this to make
resizing fast.
Maybe during resizing, reflow lines "manually", and then after a debounce
rebuild the wrappedIndex and thus update the scrollbar?
Premature optimization probably ;)
…On Thu, Mar 22, 2018, 7:03 PM Daniel Imms, ***@***.***> wrote:
I'm planning on tackling this for 3.4.0, I recently went over the history
of this and wrote up a refreshed design document:
Requirements
- Wrap lines when reducing width
- Unwrap lines when increasing width
- Scroll bar needs to use the total number of *wrapped* lines
- Escape sequences need to act upon the *wrapped* buffer, not the
unwrapped buffer.
Performance considerations
- Do not reconstruct characters/rows/strings while maintaining the
wrapped state
- The total number of wrapped lines needs to be fetched very quickly
as that is needed to position the scrollbar
- Resize needs to be as fast as possible as this can fire many times
on windows resize (and the terminal is just one component in VS Code)
- Ideally scrollback would represent the number of wrapped rows, this
isn't too important though if that's tough
Implementation
The solution will keep a separate view model WrappedBuffer which
maintains arrays that map from normal to wrapped index and the other way
around. For example:
CircularList WrappedBuffer.normalToWrappedIndex where size = 4
[0]: "abcdef" [0]: 0
[1]: "abcd" [1]: 2
[2]: "abcdefgh" [2]: 3
WrappableBuffer.wrappedToNormalIndex
[0]: [0, 0]
[1]: [0, 4]
[2]: [1, 0]
[3]: [2, 0]
[4]: [2, 4]
This will provide fast O(1) access to wrapped or unwrapped indexes at the
cost of a O(n) operation on resize to update the lists. This operation
could probably be optimized pretty heavily.
abstract class BaseWrappedList extends IWrappedList<LineData, CharData> {
private _circularList: ICircularList<LineData>;
// These should use TypedArrays/ArrayBuffers (may need to reduce MAX_BUFFER_SIZE) private wrappedToNormalIndex: [number, number][];
private normalToWrappedIndex: number[];
// O(n) where n is length of _circularList public onResize(cols: number): void {
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);
// TODO: Use _isListEntryDoubleWidth to determine the line of the // last wrapped cell in each line const wrappedCount = Math.ceil(line.length / cols);
this.normalToWrappedIndex[i] = currentWrappedCount;
for (let j = 0; j < wrappedCount; j++) {
this.wrappedToNormalIndex[currentWrappedCount + j] = [i, j];
}
currentWrappedCount += wrappedCount;
}
}
protected _isListEntryDoubleWidth(unwrappedRow: number, unwrappedCol: number): boolean {
return false;
}
}
class WrappedBuffer extends IWrappedList<LineData, CharData> {
protected _isListEntryDoubleWidth(unwrappedRow: number, unwrappedCol: number): boolean {
// Used to check if the last char is double width in order to wrap it to // the next row or not // Null check as needed return this.get(unwrappedRow, unwrappedCol)[2] === 2;
}
}
Before
export interface ICircularList<T> extends IEventEmitter {
length: number;
maxLength: number;
forEach: (callbackfn: (value: T, index: number) => void) => void; // Not sure this is used anymore
get(index: number): T;
set(index: number, value: T): void;
push(value: T): void;
pop(): T;
splice(start: number, deleteCount: number, ...items: T[]): void;
trimStart(count: number): void;
shiftElements(start: number, count: number, offset: number): void;
}
class CircularList extends ICircularList<LineData> { ... }
After
export interface IWrappedList<T extends Array<U>, U> {
// How to handle wide chars on the last cell? Protected method // _isListEntryDoubleWidth which is called on the last char to figure out the // right wrapped spot
readonly length: number;
get(index: number): T;
splice(start: number, deleteCount: number, ...items: T[]): void;
shiftElements(start: number, count: number, offset: number): void;
onResize(cols: number): void;
}
export interface ICircularList<T> extends IEventEmitter {
length: number;
maxLength: number;
get(index: number): T;
set(index: number, value: T): void;
push(value: T): void;
pop(): T;
trimStart(count: number): void;
}
Notes
-
When a line is wrapped and it is deleted, the delete line operation is
perfermed on the wrapped portion of the line, breaking the line
https://github.com/Microsoft/vscode/issues?utf8=%E2%9C%93&q=is%3Aopen%20no%3Aassignee%20-label%3Afeature-request%20-label%3Atestplan-item%20-label%3Aplan-item%20-label%3Aextension-candidate
echo -e '\e[44m\e[A\e[A\e[A\e[A\e[A\e[1M\e[0m'
If deleted from the middle of a wrapped line (eg. line 2 of a 3 line
wrapped line), VTE will join the other portions of the line
This means that the wrapped state will also affect how manipulations
happen on the actual circular list.
-
When a line is inserted into the middle of a wrapped line, it will be
marked as wrapped.
echo 'https://github.com/Microsoft/vscode/issues?utf8=%E2%9C%93&q=is%3Aopen%20no%3Aassignee%20-label%3Afeature-request%20-label%3Atestplan-item%20-label%3Aplan-item%20-label%3Aextension-candidate'
echo -e '\e[44m\e[A\e[A\e[A\e[A\e[A\e[1L%20-label%3Afeature-request%20-label%3Atestplan-item%20-label%3Aplan-item%20-label%3A\e[0m'
The line will not join with a following wrapped portion unless the
line wraps over to that line:
echo 'https://github.com/Microsoft/vscode/issues?utf8=%E2%9C%93&q=is%3Aopen%20no%3Aassignee%20-label%3Afeature-request%20-label%3Atestplan-item%20-label%3Aplan-item%20-label%3Aextension-candidate'
echo -e '\e[44m\e[A\e[A\e[A\e[A\e[A\e[1L%20-label%3Afeature-request%20-label%3Atestplan-item%20-label%3Aplan-item%20-label%3AA\e[0m'
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#622 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADWlh30vLRjSOMvgv_NDKLA6DzwPETeks5tg-d4gaJpZM4MuuoY>
.
|
|
How about making the resize interruptable: on resize:
this way it should remain responsive at all times. |
The issue with this is getting the scroll position correct, we could do more guessing but that would probably involve a lot more changes and may not be necessary. I'm hoping that because we're working on TypedArrays and just basic math it should be reasonable for a few terminals with 1000 scrollback (typical VS Code case). Time will tell though.
This is an interesting idea, I'm not 100% sure if the synchronous JS will block another resize event from coming in. If not, we should definitely be able to cancelp 👍 |
Sorry I don't know how the scrolling is implemented, can't you keep the top line at the top of the visible range? So can you leave the scroll bar unchanged until the reflow is complete, and then update. |
Well that's the idea, but reflow will be synchronous so it will happen immediately after.
The thing with the scroll bar is that we could maybe get away with only reflowing the viewport if not for the scroll bar. For the scroll bar we need to know the number of wrapped lines, and to get that we need to iteration over all the lines and perform the above resize algorithm. I'm hoping it's just fast enough due to the TypedArrays but if not then maybe we have to fallback on being lazy and guessing the scroll bar but I can see weird bugs coming out of doing something like this. My thinking was that we could just wrap the viewport (and maybe surrounding rows), scrolling with the mouse wheel could be left unchanged. Scrolling by dragging the bar however would scroll the unwrapped rows or to some percentage of the viewport. I think this is over engineering the solution though and would cause some things to just not feel right (scroll bar height, jumping over an inconsistent amount of rows when scrolling). |
|
Hmm right. Come to think of it, assuming getting the length of a string is
O(1), knowing the number of lines is O(n) but with few operations: the sum
of ceil(line[i].length/width)
Creating the wrapped line arrays is a heavier O(n*avg_nr_lines/line)
Maybe it is indeed best to do the naive implementation first and then
optimize if needed.
UPDATE: not quite, escape sequences mess with line length. So that first operation is also quite heavy. Or are lines stored as UTF-8 + attribute ranges?
|
|
My work on this got deferred because some other higher priority stuff came up 🙁 |
|
DomTerm supports Common Lisp-style "pretty-printing". There are certain escape sequence to indicate logical grouping, optional newlines (only break line if too long to fit), indentation etc. Then DomTerm's line-breaking engine calculates the line-breaks. On window resize the line-breaks are re-calculated. This may not be something you want to do, but if would be nice if it is something an addon could do. I.e. there needs to be a hook into reflow. It would be nice to be able to add and remove "soft" newlines that aren't always at the right of the line. |
On resize ideally we want the text to reflow and wrap to the next line where appropriate.
The text was updated successfully, but these errors were encountered: