Repository navigation
Refactor so isWrapped isn't public writeable #5788
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
Changes from 59 commits
97a5069
1c58f1f
e9ea22f
8773207
a88bd17
1d562d1
7e7dbc9
8315000
46fcebb
918c5f0
8653029
0780cfc
cf799ac
10d2b1d
1a06b92
9c5e1d7
26d1ddd
673db7c
2998cf7
5c0434f
fe7611a
b54f918
7ecb43d
91f816c
b4c9024
bdea328
05b8c14
ffd194a
4dc3aff
648779a
658ce2b
9088d1e
6b00ba6
d8f504f
bf35a21
d99c173
d7129e5
7205b06
553d0dc
31e2fe7
552b7a0
eb3c7e7
97cb26a
5107158
ca70702
227ea0c
8757f76
17e7ec5
d92a1a9
1b9c9c4
424722a
6adfde4
36a9f5d
d027be4
34dd3ca
874825e
fe302d0
a11f91a
0117604
5f2f03b
ef936ed
18dc877
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -592,7 +592,7 @@ export class InputHandler extends Disposable implements IInputHandler { | |
| } | ||
| // The line already exists (eg. the initial viewport), mark it as a | ||
| // wrapped line | ||
| this._activeBuffer.lines.get(this._activeBuffer.ybase + this._activeBuffer.y)!.isWrapped = true; | ||
| this._activeBuffer.setWrapped(this._activeBuffer.ybase + this._activeBuffer.y, true); | ||
| } | ||
| // row changed, get it again | ||
| bufferRow = this._activeBuffer.lines.get(this._activeBuffer.ybase + this._activeBuffer.y); | ||
|
|
@@ -756,7 +756,7 @@ export class InputHandler extends Disposable implements IInputHandler { | |
| // reprint is common, especially on resize. Note that the windowsMode wrapped line heuristics | ||
| // can mess with this so windowsMode should be disabled, which is recommended on Windows build | ||
| // 21376 and above. | ||
| this._activeBuffer.lines.get(this._activeBuffer.ybase + this._activeBuffer.y)!.isWrapped = false; | ||
| this._activeBuffer.setWrapped(this._activeBuffer.ybase + this._activeBuffer.y, false); | ||
| } | ||
| // If the end of the line is hit, prevent this action from wrapping around to the next line. | ||
| if (this._activeBuffer.x >= this._bufferService.cols) { | ||
|
|
@@ -820,7 +820,7 @@ export class InputHandler extends Disposable implements IInputHandler { | |
| && this._activeBuffer.y > this._activeBuffer.scrollTop | ||
| && this._activeBuffer.y <= this._activeBuffer.scrollBottom | ||
| && this._activeBuffer.lines.get(this._activeBuffer.ybase + this._activeBuffer.y)?.isWrapped) { | ||
| this._activeBuffer.lines.get(this._activeBuffer.ybase + this._activeBuffer.y)!.isWrapped = false; | ||
| this._activeBuffer.setWrapped(this._activeBuffer.ybase + this._activeBuffer.y, false); | ||
| this._activeBuffer.y--; | ||
| this._activeBuffer.x = this._bufferService.cols - 1; | ||
| // find last taken cell - last cell can have 3 different states: | ||
|
|
@@ -1184,7 +1184,7 @@ export class InputHandler extends Disposable implements IInputHandler { | |
| respectProtect | ||
| ); | ||
| if (clearWrap) { | ||
| line.isWrapped = false; | ||
| this._activeBuffer.setWrapped(this._activeBuffer.ybase + y, false); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1198,7 +1198,7 @@ export class InputHandler extends Disposable implements IInputHandler { | |
| if (line) { | ||
| line.fill(this._activeBuffer.getNullCell(this._eraseAttrData()), respectProtect); | ||
| this._bufferService.buffer.clearMarkers(this._activeBuffer.ybase + y); | ||
| line.isWrapped = false; | ||
| this._activeBuffer.setWrapped(this._activeBuffer.ybase + y, false); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question as above - why the absolute addressing now?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reply. |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -1246,10 +1246,7 @@ export class InputHandler extends Disposable implements IInputHandler { | |
| this._eraseInBufferLine(j, 0, this._activeBuffer.x + 1, true, respectProtect); | ||
| if (this._activeBuffer.x + 1 >= this._bufferService.cols) { | ||
| // Deleted entire previous line. This next line can no longer be wrapped. | ||
| const nextLine = this._activeBuffer.lines.get(j + 1); | ||
| if (nextLine) { | ||
| nextLine.isWrapped = false; | ||
| } | ||
| this._activeBuffer.setWrapped(j + 1, false); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These n+1 lookups for wrapped state are really annoying. I think I already addressed it in some comment elsewhere - we basically have a slightly off handling of wrapped state: But that is a major rewrite and we are in a state, where our more complicated abstraction works "good enough" - haha.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, the |
||
| } | ||
| while (j--) { | ||
| this._resetBufferLine(j, respectProtect); | ||
|
|
@@ -1516,9 +1513,10 @@ export class InputHandler extends Disposable implements IInputHandler { | |
| } | ||
| const param = params.params[0] || 1; | ||
| for (let y = this._activeBuffer.scrollTop; y <= this._activeBuffer.scrollBottom; ++y) { | ||
| const line = this._activeBuffer.lines.get(this._activeBuffer.ybase + y)!; | ||
| const row = this._activeBuffer.ybase + y; | ||
| const line = this._activeBuffer.lines.get(row)!; | ||
| line.deleteCells(0, param, this._activeBuffer.getNullCell(this._eraseAttrData())); | ||
| line.isWrapped = false; | ||
| this._activeBuffer.setWrapped(row, false); | ||
| } | ||
| this._dirtyRowTracker.markRangeDirty(this._activeBuffer.scrollTop, this._activeBuffer.scrollBottom); | ||
| return true; | ||
|
|
@@ -1549,9 +1547,10 @@ export class InputHandler extends Disposable implements IInputHandler { | |
| } | ||
| const param = params.params[0] || 1; | ||
| for (let y = this._activeBuffer.scrollTop; y <= this._activeBuffer.scrollBottom; ++y) { | ||
| const line = this._activeBuffer.lines.get(this._activeBuffer.ybase + y)!; | ||
| const row = this._activeBuffer.ybase + y; | ||
| const line = this._activeBuffer.lines.get(row)!; | ||
| this._activeBuffer.setWrapped(row, false); | ||
| line.insertCells(0, param, this._activeBuffer.getNullCell(this._eraseAttrData())); | ||
| line.isWrapped = false; | ||
| } | ||
| this._dirtyRowTracker.markRangeDirty(this._activeBuffer.scrollTop, this._activeBuffer.scrollBottom); | ||
| return true; | ||
|
|
@@ -1572,9 +1571,10 @@ export class InputHandler extends Disposable implements IInputHandler { | |
| } | ||
| const param = params.params[0] || 1; | ||
| for (let y = this._activeBuffer.scrollTop; y <= this._activeBuffer.scrollBottom; ++y) { | ||
| const line = this._activeBuffer.lines.get(this._activeBuffer.ybase + y)!; | ||
| const row = this._activeBuffer.ybase + y; | ||
| this._activeBuffer.setWrapped(row, false); | ||
| const line = this._activeBuffer.lines.get(row)!; | ||
| line.insertCells(this._activeBuffer.x, param, this._activeBuffer.getNullCell(this._eraseAttrData())); | ||
| line.isWrapped = false; | ||
| } | ||
| this._dirtyRowTracker.markRangeDirty(this._activeBuffer.scrollTop, this._activeBuffer.scrollBottom); | ||
| return true; | ||
|
|
@@ -1595,9 +1595,10 @@ export class InputHandler extends Disposable implements IInputHandler { | |
| } | ||
| const param = params.params[0] || 1; | ||
| for (let y = this._activeBuffer.scrollTop; y <= this._activeBuffer.scrollBottom; ++y) { | ||
| const line = this._activeBuffer.lines.get(this._activeBuffer.ybase + y)!; | ||
| const row = this._activeBuffer.ybase + y; | ||
| const line = this._activeBuffer.lines.get(row)!; | ||
| this._activeBuffer.setWrapped(row, false); | ||
| line.deleteCells(this._activeBuffer.x, param, this._activeBuffer.getNullCell(this._eraseAttrData())); | ||
| line.isWrapped = false; | ||
| } | ||
| this._dirtyRowTracker.markRangeDirty(this._activeBuffer.scrollTop, this._activeBuffer.scrollBottom); | ||
| return true; | ||
|
|
@@ -3478,11 +3479,8 @@ export class InputHandler extends Disposable implements IInputHandler { | |
| this._setCursor(0, 0); | ||
| for (let yOffset = 0; yOffset < this._bufferService.rows; ++yOffset) { | ||
| const row = this._activeBuffer.ybase + this._activeBuffer.y + yOffset; | ||
| const line = this._activeBuffer.lines.get(row); | ||
| if (line) { | ||
| line.fill(cell); | ||
| line.isWrapped = false; | ||
| } | ||
| this._activeBuffer.setWrapped(row, false); | ||
| this._activeBuffer.lines.get(row)?.fill(cell); | ||
| } | ||
| this._dirtyRowTracker.markAllDirty(); | ||
| this._setCursor(0, 0); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,9 +19,7 @@ export function updateWindowsModeWrappedState(bufferService: IBufferService): vo | |
| // wrapped. | ||
| const line = bufferService.buffer.lines.get(bufferService.buffer.ybase + bufferService.buffer.y - 1); | ||
| const lastChar = line?.get(bufferService.cols - 1); | ||
|
|
||
| const nextLine = bufferService.buffer.lines.get(bufferService.buffer.ybase + bufferService.buffer.y); | ||
| if (nextLine && lastChar) { | ||
| nextLine.isWrapped = (lastChar[CHAR_DATA_CODE_INDEX] !== NULL_CELL_CODE && lastChar[CHAR_DATA_CODE_INDEX] !== WHITESPACE_CELL_CODE); | ||
| if (lastChar) { | ||
| bufferService.buffer.setWrapped(bufferService.buffer.ybase + bufferService.buffer.y, lastChar[CHAR_DATA_CODE_INDEX] !== NULL_CELL_CODE && lastChar[CHAR_DATA_CODE_INDEX] !== WHITESPACE_CELL_CODE); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a really hackish assumption over data state and def. will wrongly merge lines, that happened to end on the last cell and had a hard break following. (no critic on your code, I see that you just transformed it)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I haven't really thought about this, but I followed the existing logic. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -125,6 +125,11 @@ export class Buffer extends Disposable implements IBuffer { | |
| return correctBufferLength > MAX_BUFFER_SIZE ? MAX_BUFFER_SIZE : correctBufferLength; | ||
| } | ||
|
|
||
| public setWrapped(absrow: number, value: boolean): void { | ||
| const line = this.lines.get(absrow); | ||
| line instanceof BufferLine && line.setWrapped(value); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The && invocation is an interesting pattern here, I guess you did it for speed reasons. I also found && to be much faster on v8 compared to ordinary branching. Minor perf suggestion - if the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC I used the The |
||
| } | ||
|
|
||
| /** | ||
| * Fills the buffer's viewport with blank lines. | ||
| */ | ||
|
|
||
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.
Is there a reason for using absolute addressing here now, why not on the line?
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.
Not sure I understand the question.
Buffer.setWrappedtakes an absolute line number. We don't want to useBufferLine.setWrapped(now removed) because it is internal.Note one reason for removing
BufferLine.setWrapped(which is the same reason for makingBufferLine.isWrappedread-only) is because some implementations (including the proposedLogicalLineimplementation) need more state, and modifyingisWrappedcan't be done without access to at least the previous line.