Refactor so isWrapped isn't public writeable by PerBothner · Pull Request #5788 · xtermjs/xterm.js

PerBothner · 2026-03-31T19:08:18Z

Having a publicly-writable isWrapped property in BufferLine restricts our implementation options for how wrapped lines are represented in a Buffer. For example, it conflicts with the proposal in #5673.

To hide this implementation choice, this PR makes isWrapped read-only, and adds a Buffer.setWrapped method (taking an absolute row number) for modifying the isWrapped state. There is also an internal-only BufferLine.setWrapped method used to implement Buffer.setWrapped and in one other place.

This gives greater flexibility in how BufferLine and line-wrapping are implemented.

This is likely to trivially conflict with PR #5785. If one of these is merged, I'll update the other one to fix any conflicts.

Instead to change isWrapped state use a Buffer.setWrapped method. This allows for potential flexibility in how BufferLine and line-wrapping are implemented.

PerBothner · 2026-04-25T21:15:48Z

It would be great if someone could merge this. It allows (but does not require) alternative implementation stategies for BufferLine, such as PR #5797. I believe this should be trivially reviewable and hopefully non-controversial.

jerch · 2026-04-26T13:37:45Z

This gives greater flexibility in how BufferLine and line-wrapping are implemented.

Can you expand on why this would be needed? Is this an attempt to hide the row logic behind Buffer.* access (which is prolly needed for your logical line implementation)? (Btw our isWrapped logic is flawed currently, we hold this flag on the wrong line leading to all sorts of forward/backward lookups to correct the behavior.)

If we go that route with hiding abstraction behind Buffer.* access, then this also would apply for getters and prolly data getters/setters. The latter smells like a performance grave to me, getter/setter interfaces are always slower than direct access (with property getter/setter being even worse). I suggest to discuss this route first and to back it up with some perf measures.

PerBothner · 2026-04-26T16:34:52Z

The motivation of this PR was to split off part of PR #5797 that can be reviewed separately. Like ad hoc "stacked PRs".

For #5797 (and probably other alternative Buffer implementation strategies) changing a line from wrapped to non-wrapped or vice versa isn't a matter of just flipping a bit - there are more extensive changes that need to be done. For PR #5797 you would need to split or merge the related LogicalLine objects. So changing the isWrapped state becomes a more expensive operation. (Though note that setting isWrapped is usually when a new line is scrolled in, when a new BufferLine is created. Clearing isWrapped is usually when a line or buffer is erased, so those operations can be combined.)

Is this an attempt to hide the row logic behind Buffer.* access (which is prolly needed for your logical line implementation)?

Most of the other changes involving LogicalLine are hidden inside BufferLine.js (except for buffer reflow in Buffer.ts, which becomes vastly simpler and more efficient), but changing isWrapped to read-only is the main API change.

It would probably be straightforward to make isWrapped is a readonly property (rather than a getter), but I haven't looked into that. Either way, I would be astounded if the difference from direct access is measureable, as isWrapped/setWrapped are not called on a per-character basis.

Btw our isWrapped logic is flawed currently, we hold this flag on the wrong line leading to all sorts of forward/backward lookups to correct the behavior.)

PR #5797 makes it trivial to see if the current line wraps to the next one: Just check line.nextBufferLine.

Perhaps it would be more useful to come back this PR after looking at PR #5797. If you prefer, I can merge this into the latter, making the latter slightly larger, but perhaps better justified.

I am also interested in exploring this Marker proposal, which I think will have significant performance and extensibility advantages, but I think PR #5797 needs to be merged first.

Bumps [axios](https://github.com/axios/axios) from 1.13.2 to 1.15.0. - [Release notes](https://github.com/axios/axios/releases) - [Changelog](https://github.com/axios/axios/blob/v1.x/CHANGELOG.md) - [Commits](axios/axios@v1.13.2...v1.15.0) --- updated-dependencies: - dependency-name: axios dependency-version: 1.15.0 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com>

Disable desynchronized 2D canvas mode for image layers so resize paint stays synchronized and avoids blank-frame jitter on some platforms. Signed-off-by: sergiochan <yuheng9211@qq.com>

Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.11 to 1.16.0. - [Release notes](https://github.com/follow-redirects/follow-redirects/releases) - [Commits](follow-redirects/follow-redirects@v1.15.11...v1.16.0) --- updated-dependencies: - dependency-name: follow-redirects dependency-version: 1.16.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>

- change interface to IFunctionIdentifier (API change) - extend parser states APC_ENTRY, APC_INTERMEDIATE, APC_PASSTHROUGH - remove APC_STRING - change ApcParser.start to retrieve ident - change actions APC_START and APC_PUT

Fixes xtermjs#5863

Broke due to 2 recent conflicting PRs

This allows O(1) clearing of the cache, a single TTL timer and we can avoid making BufferLine's Disposable

Bit hacky, but this is due to the way we're serializing the line in the test

PerBothner · 2026-10-07T21:06:10Z

This is part of the large PR #6160.

jerch

LGTM 👍

I have a few remarks below, but nothing blocking. Plz let me know, if you want to address some of them, otherwise this looks ready to merge.

jerch · 2026-10-11T10:04:41Z


+  public setWrapped(absrow: number, value: boolean): void {
+    const line = this.lines.get(absrow);
+    line instanceof BufferLine && line.setWrapped(value);


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 instanceof check is not needed (because .get() will always return either something nullish or a bufferline), then we should prolly just test for truthiness.

IIRC I used the X && Y pattern because lint would complain about if (X) Y, though it did allow if (X) {Y}, which I though a bit inelegant. I actually think if (X) Y is slightly for readable - but if X && Y is faster we might as well keep it.

The instanceof check is because IBufferLine specifically does not provide setWrapped, which is supposed to used only to implement Buffer.setWrapped. I made a change to get rid of BufferLine.setWrapped completely - instead you have to do (line as any)._isWrapped. That should make it clear(er) you're mucking about with internals.

jerch · 2026-10-11T10:09:51Z

      this._cachedBlankLine = newLine;
    }
-    newLine.isWrapped = isWrapped;
+    (newLine as BufferLine).setWrapped(isWrapped);


Do we have an interface issue here? Isn't buffer.getBlankLine meant to always return a bufferline?

As above. Replaced by (newLine as any)._isWrapped = isWrapped.

Note the assignment is only needed for the case we don't call getBlankLine, so I put it in an else branch..

jerch · 2026-10-11T10:12:34Z

    );
    if (clearWrap) {
-      line.isWrapped = false;
+      this._activeBuffer.setWrapped(this._activeBuffer.ybase + y, false);


Is there a reason for using absolute addressing here now, why not on the line?

Not sure I understand the question. Buffer.setWrapped takes an absolute line number. We don't want to use BufferLine.setWrapped (now removed) because it is internal.

Note one reason for removing BufferLine.setWrapped (which is the same reason for making BufferLine.isWrapped read-only) is because some implementations (including the proposed LogicalLine implementation) need more state, and modifying isWrapped can't be done without access to at least the previous line.

jerch · 2026-10-11T10:13:15Z

      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);


Same question as above - why the absolute addressing now?

Same reply.

jerch · 2026-10-11T10:22:00Z

-          if (nextLine) {
-            nextLine.isWrapped = false;
-          }
+          this._activeBuffer.setWrapped(j + 1, false);


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:
We set the flag on the next line to indicate, that it continues from the previous one. This introduces a lot of nasty edge cases, where we have to fix wrapped on n+1.
A more logical place would be to store it on the n-th line to indicate, that it continues into the next line. This plays much nicer with left-right and top-down progressing of the default cursor movement.

But that is a major rewrite and we are in a state, where our more complicated abstraction works "good enough" - haha.

FWIW, the LogicalLine re-write makes it trivial to implement a isWrapping (read-only) property, which is true when a line continues into the next line:

public get isWrapping(): boolean { return !!this.nextBufferLine; }

jerch · 2026-10-11T10:27:45Z

-  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);


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)

Yes, I haven't really thought about this, but I followed the existing logic.

Though only for internal use (and only used twice), it could lead to improper use, even if not available in IBufferLine. Instead, change _isWrapped directly after a cast to any - that makes it more obvious we're tweaking internals.

jerch

Plz see my comments below.

jerch · 2026-10-12T22:21:07Z

    const line = this.lines.get(absrow);
-    line instanceof BufferLine && line.setWrapped(value);
+    (line as any)._isWrapped = value;


This looks now like it will throw, if get() returns undefined? I think the truthiness check will be needed, as get may return nothing.

Also the as any is not good, as will swallow lit. anything. Imho it is better to add _isWrapped to the interface type the ring buffer returns. This makes the later rework bark loud, once that flag vanishes - so we will miss places, where is needs to be fixed.

Replaced the truthiness check, and avoided the as any. Instead, make _isWrapped public in BufferLine (but not IBufferLine), with some wrong warnings.

jerch · 2026-10-12T22:32:38Z

      newLine = buffer.getBlankLine(eraseAttr, isWrapped);
      this._cachedBlankLine = newLine;
+    } else {
+      (newLine as any)._isWrapped = isWrapped;


Same as above.

Instead, make _isWrapped public in BufferLine - but add strong warnings.

PerBothner added 3 commits March 31, 2026 11:52

Refactor so isWrapped isn't public writeable.

97a5069

Instead to change isWrapped state use a Buffer.setWrapped method. This allows for potential flexibility in how BufferLine and line-wrapping are implemented.

Merge branch 'master' into setWrapped

1c58f1f

Fix MockBuffer.setWrapped.

e9ea22f

PerBothner mentioned this pull request Apr 7, 2026

move _data from BufferLine to new LogicalLine class #5797

Closed

Merge branch 'master' into setWrapped

8773207

dependabot Bot and others added 22 commits May 2, 2026 16:48

Fix image layer resize jitter

1d562d1

Disable desynchronized 2D canvas mode for image layers so resize paint stays synchronized and avoids blank-frame jitter on some platforms. Signed-off-by: sergiochan <yuheng9211@qq.com>

Update ImageRenderer.ts

7e7dbc9

add missing separator for test arguments

8315000

fix web-fonts tests

46fcebb

newer safari promotes different loading state

918c5f0

remove broken test condition

8653029

fix safari

0780cfc

fix safari

cf799ac

fix safari

10d2b1d

disable wrong webgl tests

1a06b92

disable firefox webgl

9c5e1d7

Make agent instructions harness agnostic

673db7c

Add Orca to real-world uses

2998cf7

fix xtermjs#5849

5c0434f

adjust image addon defaults

fe7611a

multiple changes in APC handling:

b54f918

- change interface to IFunctionIdentifier (API change) - extend parser states APC_ENTRY, APC_INTERMEDIATE, APC_PASSTHROUGH - remove APC_STRING - change ApcParser.start to retrieve ident - change actions APC_START and APC_PUT

make linter happy

7ecb43d

add state and transition tests

91f816c

basic read ahead optimization

b4c9024

APC perf test

bdea328

jerch and others added 18 commits May 2, 2026 16:48

activate tests for webkit

553d0dc

IIP resizing tests for QOI

31e2fe7

merge resize tests for PNG & QOI into one block

552b7a0

Add instructions around MutableDisposables and const enums

eb3c7e7

Convert a lot of top level consts to const enums

97cb26a

Cache translateToString calls inside BufferLine

5107158

Fixes xtermjs#5863

Fix test/startCol handling

ca70702

Fix integration test defaults

227ea0c

Broke due to 2 recent conflicting PRs

Support caching of trimRight

8757f76

Store a boolean on whether the cache is trimmed

17e7ec5

Move cache into Buffer owned object, use WeakRefs

d92a1a9

This allows O(1) clearing of the cache, a single TTL timer and we can avoid making BufferLine's Disposable

Cache/buffer lifecycle

1b9c9c4

Convert cache to Disposable

424722a

Avoid any in test

6adfde4

Avoid thrashing timeouts

36a9f5d

Fix build, lint

d027be4

Ignore cache keys in serialize addon

34dd3ca

Bit hacky, but this is due to the way we're serializing the line in the test

Fix lint

874825e

PerBothner mentioned this pull request Jun 18, 2026

Am I wasting my time here? #6022

Open

coderabbitai Bot mentioned this pull request Aug 6, 2026

feat(terminal): redact copied command blocks rssh-org/rssh#222

Merged

PerBothner mentioned this pull request Oct 7, 2026

replace _combined map and string cache with _chars variable in LogicalLine #6160

Open

PerBothner added 3 commits October 10, 2026 15:00

Merge remote-tracking branch 'upstream/master' into setWrapped

fe302d0

More merge fixes.

a11f91a

More merge cleanup.

0117604

jerch approved these changes Oct 11, 2026

View reviewed changes

Remove BufferLine setWrapped method

5f2f03b

Though only for internal use (and only used twice), it could lead to improper use, even if not available in IBufferLine. Instead, change _isWrapped directly after a cast to any - that makes it more obvious we're tweaking internals.

jerch requested changes Oct 12, 2026

View reviewed changes

PerBothner added 2 commits October 12, 2026 21:33

Remove cast to any for accessing internal _isWrapped property

ef936ed

Instead, make _isWrapped public in BufferLine - but add strong warnings.

Merge branch 'master' into setWrapped

18dc877

Conversation

PerBothner commented Mar 31, 2026

Uh oh!

PerBothner commented Apr 25, 2026

Uh oh!

jerch commented Apr 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PerBothner commented Apr 26, 2026

Uh oh!

PerBothner commented Oct 7, 2026

Uh oh!

jerch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerch left a comment

Choose a reason for hiding this comment

Uh oh!

jerch Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jerch commented Apr 26, 2026 •

edited

Loading

jerch Oct 12, 2026 •

edited

Loading