Fill lines inserted from scrolling with erase attributes #1706
Conversation
src/Terminal.ts
Outdated
| @@ -1170,7 +1170,8 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II | |||
| * @param isWrapped Whether the new line is wrapped from the previous line. | |||
| */ | |||
| public scroll(isWrapped?: boolean): void { | |||
| const newLine = BufferLine.blankLine(this.cols, DEFAULT_ATTR, isWrapped); | |||
| const blankAttr = isWrapped ? DEFAULT_ATTR : this.eraseAttr(); | |||
There was a problem hiding this comment.
To be compliant to xterm & vte imho the new line should always follow the erase char so this conditional is not needed. To see the difference you have to test it at the lower viewport border (not within the viewport, new lines are only spawned at the end).
There was a problem hiding this comment.
xterm (and others) always use the erase character.
vte (since 0.48.3) uses the erase character unless it is a new line caused by line-wrapping, diverging from xterm for reasons given in the following issue and commit.
So for vte behavior, you could accept the PR as-is.
If you do in-fact want xterm behavior instead, let me know and I will adjust accordingly.
There was a problem hiding this comment.
Ah well, this is kinda messed up. Need to test first the behavior with a newer vte (tested with an older version above) as I think this will need more adjustments to get it right. So this has to wait for now.
Also up for discussion, whether to go with xterm (+most of others) or vte.
|
@whydoubt thanks for the PR! Is there an issue tracking this or just the PR? Also what's the test you're using to repro the issue? |
|
No issue, just the PR. If you |
|
For these sorts of issues we tend to opt for whatever the majority does, so it would be good to test this out in Terminal.app, iTerm2 as well as VTE. |
|
While I don't have a Mac to check what those terminals do, I have checked several that work on Linux. I suspect that VTE is unique. As I understand it, they rejected the majority on the grounds that—for the identified use case—their deviation was visually superior. I will change this PR to follow xterm behavior, and may later submit a separate PR for VTE behavior. Improving the appearance of vim is my primary concern, and for that the xterm behavior is sufficient. |
When scrolling occurs due to a LF or scroll code, fill the new line with the currently-set background color.
whydoubt
force-pushed
the
blank_line_attributes
branch
from
f5accf1
to
8bd49cb
Compare
|
@whydoubt LGTM 👍 Maybe we can add vte special behavior with a later PR and option. I see a valid point in vte's way, still I wonder whether the whole wrapped line should carry the customized background or just the wrapped content cells (dont like the latter either as it would introduce awkward state handling conditions). Maybe @egmontkob could shed some light on this too? |
|
Our side of the story was discussed in VTE 754596 along with the initial implementation, plus the partial revert we unfortunately had to make to please ncurses. I'm not sure what you mean by "or just the wrapped content cells", could you please clarify? As far as I recall, this story is only relevant when a new line (consisting of unused aka empty cells) is scrolled in. Cells will get content as text is printed, which obviously needs to use the currently active attributes. But it was a long time ago and I might be missing something. The point is that commands like should behave "sanely" even if the linebreak occurs while having a custom background color set, and even if this linebreak causes scrolling. But there's no standard to adhere to, or if there's one then the standard is the "insane" behavior VTE intentionally deviates from. |
When scrolling occurs due to a LF or scroll code, the new line should be
filled with the currently set background color. As with VTE, fill with
the default color if the new line was caused by line-wrap.