Reverse wraparound #2724

jerch · 2020-02-16T19:54:23Z

There are a few things to consider, see @egmontkob's listing here: https://gitlab.gnome.org/GNOME/vte/issues/62

Basic implementation is done. ~~I had to disable the _restrictCursor() call, which is abit unfortunate. This needs further investigation and is prolly related to our weird way of handling pending wraps.~~ Turns out that xterm allows the last cell to be deleted if reverse wrap-around is set, otherwise not. Fixed.

Furthermore this PR revealed another bug - the last char before EOL is not shown under the "sticky" cursor, prolly the renderer does not take the char into account under the cursor in that position (data is correct in buffer).

Another bug: we currently dont clear cells to the right when early wraparound happens. Fixed.

TODO:

create DECSET 45 (default = false)
decide whether to operate on all lines or only softwraps (latter seems preferrable)
skip empty cells at EOL after wide chars
write backspace tests
~~write reverse wrap-around xterm test case files?~~ No comparison possible due to different behavior.
investigate _restrictCursor() behavior / scroll margins
clear cells to the right on early wraparound in print, write tests
followup issue for renderer bug of last cell --> Renderer bug for last cell in row #2729
followup issue to investigate correct lifting of isWrapped flag (related isWrapped does not get lifted for longish bash commands #1882) --> Revise isWrapped behavior #2728

Closes #2716.

jerch · 2020-02-17T13:22:20Z

Update: Eww, reverse wrap-around does some weird things on xterm - any cursor movement sequence does not respect grid borders anymore, instead the cursor wraps as well. Not sure why it was made like this, it leads to a totally different behavior for all of those sequences. DECAWM does not do this, from a "rule of least surprise" i'd call this behavior flawed. Ofc this is a bold statement since it seems to be an xterm feature originally. Still it changes the way the terminal content gets addressed, I think we should not follow that path.

Suggestion, up for discussion:

limit reverse wrap-around to soft NLs (can only happen in a line, that was wrapped beforehand)
always stop at hard NLs
within a line BS should act as it would normally w'o reverse wrap-around set
cursor movement sequences do not change anything:
- they are bound to top/bottom/left/right and cannot pass these borders
- a following BS would be eval'ed within the new position and apply the rules above
- cursor jumps into an empty line "realize" that line up to the new cursor position, line itself is treated as not wrapped (cursor cannot pass into prev line)
scrollborders should work/respected as with DECAWM

This is more in line how DECAWM works and makes reverse wrap-around kinda the opposite/undo action.

FYI: @Tyriar, @egmontkob

egmontkob · 2020-02-17T20:43:29Z

limit reverse wrap-around to soft NLs

This is what tmux does, and is probably the most reasonable behavior.

jerch · 2020-02-18T19:11:06Z

@egmontkob Thx for your feedback. Yes I think so too, seems to be much more reasonable than altering the behavior of 50% of the common terminal sequences. Though I wonder why xterm did it this way in the first place (pros/cons compared to a stricter "DECAWM reverse") and whether there is some significant usage in the wild with that odd behavior. Not sure if it is worth to be brought up in terminal-wg...

FYI: @textshell

…everse_wraparound

jerch · 2020-02-21T17:37:55Z

@Tyriar Done with the implementation and tests. Up for review.

Btw, you can test the difference with the following:

enable reverse wrap with echo -e "\x1b[?45h"
run cat in terminal
input some longish stuff over several lines w'o pressing enter
use BS to delete chars from the end:
- before: would always stop at cursor.x == 0
- this PR: soft wraps can be deleted into the previous line

@jerch

Excitingly enough, @jerch has made this PR in response to my feature request for reverse-wraparound mode, which obviates me having to add mediocre implementation of it: xtermjs/xterm.js#2724

Tyriar · 2020-04-10T17:44:56Z

Btw, you can test the difference with the following:

@jerch I don't see any difference with this branch and master when doing this? 😕

jerch · 2020-04-11T12:06:24Z

@jerch I don't see any difference with this branch and master when doing this?

Oh thats weird. Maybe you are "holding it wrong" 😸 Here are the exact steps to repro the difference:

switch on reverse-wrap with echo -e "\x1b[?45h"
open cat in demo
hold down some character key until it fills more than one line (thus wrapped at least once)
dont press Enter
hold down <--:
- before: deleting chars would always stop at the beginning of the current cursor line
- this PR: deleting wraps back into prev line up to last '\n' or top-left of the viewport

Tyriar · 2020-04-11T13:41:24Z

@jerch oh I ran cat README.md 😅, I see it now

src/InputHandler.ts

Tyriar · 2020-04-11T13:51:23Z

src/InputHandler.ts

+       * reverse wrap-around:
+       * Our implementation deviates from xterm on purpose. Details:
+       * - only previous soft NLs can be reversed (isWrapped=true)
+       * - only works within scrollborders (top/bottom, left/right not yet supported)


Not yet supported? Do we need to note this in vt features?

I dont think so, we dont support left/right scroll margins at all atm, it is still on the TODO list of missing vt features. The comment is more a reminder to cover it here as well once we implement it (which is abit more involved, as it needs to be respected in several vt commands, nothing to deal with here).

src/InputHandler.ts

…everse_wraparound

jerch · 2020-04-27T14:18:22Z

src/common/InputHandler.test.ts

+        assert.equal(bufferService.buffer.x, 0);
+      });
+      /*
+      it.only('should not reverse outside of scroll margins', () => {


@Tyriar I cannot get this test working anymore without TestTerminal, seems the scrollback is not respected at all. Any clue why?

Yep, move it over here:

xterm.js/src/browser/Terminal.test.ts

Lines 1421 to 1430 in 45077d7

describe('Terminal InputHandler integration', () => {

function getLines(term: TestTerminal, limit: number = term.rows): string[] {

const res: string[] = [];

for (let i = 0; i < limit; ++i) {

res.push(term.buffer.lines.get(i)!.translateToString(true));

}

return res;

}

// This suite cannot live in InputHandler unless Terminal.scroll moved into IBufferService

Terminal still owns scroll, scroll should probably live into BufferService and then Terminal reacts to it via an event. Created #2881 for this

Tyriar · 2020-04-27T14:23:43Z

I'll give this another review and hopefully merge next week 😃

Tyriar

Works great and well tested 👌

jerch added 2 commits February 16, 2020 20:41

working draft

6f886fe

comment missing empty cell handling

f736b6c

jerch self-assigned this Feb 16, 2020

jerch added area/parser type/enhancement Features or improvements to existing features labels Feb 16, 2020

jerch added 3 commits February 17, 2020 12:22

fix behavior, first BS tests

0c70327

test case for clearing cells to the right on early wrap-around

7de600c

wide char tests

62ea2c6

jerch added 3 commits February 18, 2020 20:12

Merge branch 'master' into reverse_wraparound

5cf0aad

respect scroll margins, tests

cb31179

Merge branch 'reverse_wraparound' of github.com:jerch/xterm.js into r…

d35d12f

…everse_wraparound

jerch marked this pull request as ready for review February 21, 2020 17:38

jerch mentioned this pull request Feb 21, 2020

Renderer bug for last cell in row #2729

Closed

Merge branch 'master' into reverse_wraparound

23cacbf

This was referenced Mar 8, 2020

Support reverse-wraparound mode? #2716

Closed

wrap issue when prompt is over 2 lines #2752

Closed

This was referenced Mar 18, 2020

UI: add exec terminal hashicorp/nomad#6697

Merged

UI: migrate exec to use release of Xterm.js that includes reverse wraparound mode hashicorp/nomad#7461

Closed

Merge branch 'master' into reverse_wraparound

2e6f547

Merge branch 'master' into reverse_wraparound

5669cf8

Tyriar requested changes Apr 11, 2020

View reviewed changes

jerch added 5 commits April 26, 2020 15:59

Merge branch 'master' into reverse_wraparound

ab2c8c7

Merge branch 'reverse_wraparound' of github.com:jerch/xterm.js into r…

71c2551

…everse_wraparound

some refactoring

51a808c

Merge branch 'master' into reverse_wraparound

e1be294

resolve master conflicts

0d0f04d

jerch commented Apr 27, 2020

View reviewed changes

Tyriar mentioned this pull request Apr 27, 2020

Terminal.scroll should be moved into BufferService #2881

Closed

Tyriar added this to the 4.6.0 milestone Apr 27, 2020

Tyriar added 2 commits May 3, 2020 07:04

Move commented test into Terminal.test

210fa7f

Merge branch 'master' into reverse_wraparound

076d51f

Tyriar approved these changes May 3, 2020

View reviewed changes

Tyriar merged commit 3ea9a75 into xtermjs:master May 3, 2020

isontheline mentioned this pull request Jan 27, 2022

Rendering : New Terminal Rendering Engine isontheline/pro.webssh.net#150

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reverse wraparound #2724

Reverse wraparound #2724

jerch commented Feb 16, 2020 •

edited

Loading

jerch commented Feb 17, 2020

egmontkob commented Feb 17, 2020

jerch commented Feb 18, 2020

jerch commented Feb 21, 2020 •

edited

Loading

Tyriar commented Apr 10, 2020

jerch commented Apr 11, 2020

Tyriar commented Apr 11, 2020

Tyriar Apr 11, 2020

jerch Apr 26, 2020

jerch Apr 27, 2020

Tyriar Apr 27, 2020

Tyriar commented Apr 27, 2020

Tyriar left a comment

	describe('Terminal InputHandler integration', () => {
	function getLines(term: TestTerminal, limit: number = term.rows): string[] {
	const res: string[] = [];
	for (let i = 0; i < limit; ++i) {
	res.push(term.buffer.lines.get(i)!.translateToString(true));
	}
	return res;
	}

	// This suite cannot live in InputHandler unless Terminal.scroll moved into IBufferService

Reverse wraparound #2724

Reverse wraparound #2724

Conversation

jerch commented Feb 16, 2020 • edited Loading

jerch commented Feb 17, 2020

egmontkob commented Feb 17, 2020

jerch commented Feb 18, 2020

jerch commented Feb 21, 2020 • edited Loading

Tyriar commented Apr 10, 2020

jerch commented Apr 11, 2020

Tyriar commented Apr 11, 2020

Tyriar Apr 11, 2020

Choose a reason for hiding this comment

jerch Apr 26, 2020

Choose a reason for hiding this comment

jerch Apr 27, 2020

Choose a reason for hiding this comment

Tyriar Apr 27, 2020

Choose a reason for hiding this comment

Tyriar commented Apr 27, 2020

Tyriar left a comment

Choose a reason for hiding this comment

jerch commented Feb 16, 2020 •

edited

Loading

jerch commented Feb 21, 2020 •

edited

Loading