[PATCH] Fix backspace on wrapped lines in console (virtual terminal)

[PATCH] Fix backspace on wrapped lines in console (virtual terminal)

[Joe Peterson]

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

Attached is a patch that fixes virtual terminal problems when backspace
is used on wrapped lines (see patch text for the specific issues).  The
issues can be demonstrated by trying two things in the console (vt):

#1

1) issue the "cat" command
2) type characters until you have wrapped cursor to next line
3) hit backspace (nothing happens visually, but char(s) are erased
   in buf, as seen when enter is hit and line is printed to display)

#2

1) issue the "cat" command
2) type characters until you are at the very end of the line
   (cursor now on last char - i.e. in "need_wrap" state)
3) hit backspace (it moves back and visually erases 2nd to last char,
   but hitting enter shows that last char was actually erased in buffer)

							-Joe

[-- Attachment #2: vt-fix-wrapped-backspace.patch --]
[-- Type: text/plain, Size: 1392 bytes --]

Fix backspace in the virtual terminal when line is wrapped:

1) Enable backspace to work when wrapped to next line.

2) Correct backspace behavior when most recently typed
   character is in the rightmost column (i.e. in need_wrap state).

Signed-off-by: Joe Peterson <joe@skyrush.com>
---

--- linux-2.6.27-rc6-git5/drivers/char/vt.c.orig	2008-09-20 09:04:55.000000000 -0600
+++ linux-2.6.27-rc6-git5/drivers/char/vt.c	2008-09-20 09:49:22.000000000 -0600
@@ -1130,12 +1130,36 @@ static inline void cr(struct vc_data *vc
 
 static inline void bs(struct vc_data *vc)
 {
-	if (vc->vc_x) {
-		vc->vc_pos -= 2;
-		vc->vc_x--;
+	if (vc->vc_need_wrap) {
+		/*
+		 * If in need_wrap state, do not move cursor,
+		 * but unset need_wrap.
+		 */
 		vc->vc_need_wrap = 0;
-		notify_write(vc, '\b');
+	} else {
+		if (vc->vc_x == 0) {
+			/*
+			 * If at leftmost column, move cursor to end
+			 * of previous line (only if autowrap is on).
+			 */
+			if (vc->vc_decawm) {
+				vc->vc_x = vc->vc_cols - 1;
+				if (vc->vc_y == vc->vc_top) {
+					scrdown(vc,
+						vc->vc_top, vc->vc_bottom, 1);
+					vc->vc_pos += vc->vc_size_row - 2;
+				} else if (vc->vc_y > 0) {
+					vc->vc_y--;
+					vc->vc_pos -= 2;
+				}
+			}
+		} else {
+			/* Normal case: just move cursor back */
+			vc->vc_x--;
+			vc->vc_pos -= 2;
+		}
 	}
+	notify_write(vc, '\b');
 }
 
 static inline void del(struct vc_data *vc)

Re: [PATCH] Fix backspace on wrapped lines in console (virtual terminal)

[Alan Cox]

On Sat, 20 Sep 2008 10:25:49 -0600
Joe Peterson <joe@skyrush.com> wrote:

> Attached is a patch that fixes virtual terminal problems when backspace
> is used on wrapped lines (see patch text for the specific issues).  The
> issues can be demonstrated by trying two things in the console (vt):

I would need to power up my vt420 to check (yes I have one lurking in a
corner!) but I have a feeling the current behaviour (not going up a line)
is correct for ansi type terminals.

Alan

Re: [PATCH] Fix backspace on wrapped lines in console (virtual terminal)

[Joe Peterson]

Alan Cox wrote:
> On Sat, 20 Sep 2008 10:25:49 -0600
> Joe Peterson <joe@skyrush.com> wrote:
> 
>> Attached is a patch that fixes virtual terminal problems when backspace
>> is used on wrapped lines (see patch text for the specific issues).  The
>> issues can be demonstrated by trying two things in the console (vt):
> 
> I would need to power up my vt420 to check (yes I have one lurking in a
> corner!) but I have a feeling the current behaviour (not going up a line)
> is correct for ansi type terminals.

Would it still erase the actual characters input, though?  This would
cause one to believe they were not erased, since the visual feedback
does not match...  I wonder if "fixing" this in Linux would really break
the needed vt hardware compatibility - maybe that case lies in the
"undefined" area in a sense.

What about the need_wrap (end of line) case - I wonder what the real
vt420 does for that.  If it doesn't match the behavior this patch
"fixes", it would probably be a vt420 bug...

Yes, these things are certainly getting harder to check against real
hardware these days!  :)

						-Joe

Re: [PATCH] Fix backspace on wrapped lines in console (virtual terminal)

[Joe Peterson]

Alan Cox wrote:
> On Sat, 20 Sep 2008 10:25:49 -0600
> Joe Peterson <joe@skyrush.com> wrote:
> 
>> Attached is a patch that fixes virtual terminal problems when backspace
>> is used on wrapped lines (see patch text for the specific issues).  The
>> issues can be demonstrated by trying two things in the console (vt):
> 
> I would need to power up my vt420 to check (yes I have one lurking in a
> corner!) but I have a feeling the current behaviour (not going up a line)
> is correct for ansi type terminals.

One more thought about this:

xterm has a "private mode 45", which turns on reverseWrap, allowing
backspace to wrap.  This is similar to mode 7 (supported on xterm and
vt), which is DEVAWM (auto wrap mode).

I agree that hard-coding backspace/reverse wrap would likely be wrong
(even though it looks like some terminal emulators I've seen, like
"screen" add the feature), but would it be appropriate to add mode 45 to
vt, allowing reverse wrap as an option?

Also note that vt enables DEVAWM (mode 7) by default, whereas the vt100
docs say that it is off by default.  Since reverse wrap is probably
almost always useful for general Linux in the console, should it also be
on by default if we were to add that option?

						-Joe

Re: [PATCH] Fix backspace on wrapped lines in console (virtual terminal)

[Joe Peterson]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

Joe Peterson wrote:
> xterm has a "private mode 45", which turns on reverseWrap, allowing
> backspace to wrap.  This is similar to mode 7 (supported on xterm and
> vt), which is DEVAWM (auto wrap mode).
> 
> I agree that hard-coding backspace/reverse wrap would likely be wrong
> (even though it looks like some terminal emulators I've seen, like
> "screen" add the feature), but would it be appropriate to add mode 45 to
> vt, allowing reverse wrap as an option?

I have worked up a new patch (attached) for review.  It adds the private
mode 45 I mentioned above, but defaults it to off, preserving the old
default of no backspace wrap to the previous line.  This matches xterm's
reverse-wrap behavior (and default settings), so the same escape
sequence can be used to control this mode in both xterm and console/vt.

						-Joe

[-- Attachment #2: vt-fix-wrapped-backspace.patch --]
[-- Type: text/plain, Size: 4289 bytes --]

Fix backspace in the virtual terminal when line is wrapped:

1) Add option for backspace to work when wrapped to next line by adding
   a new private mode 45 to enable reverse wrap (default: off).
   This matches xterm's behavior, which already supports private
   mode 45.

2) Correct the backspace behavior when the most recently typed
   character is in the rightmost column (i.e. in need_wrap state).
   Use high bit of need_wrap (now 2 bits) allow this backspace logic
   to work even when autowrap is off.

Signed-off-by: Joe Peterson <joe@skyrush.com>
---

diff -Nurp a/drivers/char/vt.c b/drivers/char/vt.c
--- a/drivers/char/vt.c	2008-09-21 09:27:43.186681676 -0600
+++ b/drivers/char/vt.c	2008-09-21 09:15:30.826683557 -0600
@@ -1130,12 +1130,36 @@ static inline void cr(struct vc_data *vc
 
 static inline void bs(struct vc_data *vc)
 {
-	if (vc->vc_x) {
-		vc->vc_pos -= 2;
-		vc->vc_x--;
+	if (vc->vc_need_wrap) {
+		/*
+		 * If in need_wrap state (regardless of autowrap mode),
+		 * do not move cursor, but unset need_wrap.
+		 */
 		vc->vc_need_wrap = 0;
-		notify_write(vc, '\b');
+	} else {
+		if (vc->vc_x == 0) {
+			/*
+			 * If at leftmost column, move cursor to end
+			 * of previous line (only if reverse wrap is on).
+			 */
+			if (vc->vc_reverse_wrap) {
+				vc->vc_x = vc->vc_cols - 1;
+				if (vc->vc_y == vc->vc_top) {
+					scrdown(vc,
+						vc->vc_top, vc->vc_bottom, 1);
+					vc->vc_pos += vc->vc_size_row - 2;
+				} else if (vc->vc_y > 0) {
+					vc->vc_y--;
+					vc->vc_pos -= 2;
+				}
+			}
+		} else {
+			/* Normal case: just move cursor back */
+			vc->vc_x--;
+			vc->vc_pos -= 2;
+		}
 	}
+	notify_write(vc, '\b');
 }
 
 static inline void del(struct vc_data *vc)
@@ -1437,6 +1461,9 @@ static void set_mode(struct vc_data *vc,
 			case 25:		/* Cursor on/off */
 				vc->vc_deccm = on_off;
 				break;
+			case 45:		/* Reverse wrap on/off */
+				vc->vc_reverse_wrap = on_off;
+				break;
 			case 1000:
 				vc->vc_report_mouse = on_off ? 2 : 0;
 				break;
@@ -1621,6 +1648,7 @@ static void reset_terminal(struct vc_dat
 	vc->vc_decscnm		= 0;
 	vc->vc_decom		= 0;
 	vc->vc_decawm		= 1;
+	vc->vc_reverse_wrap	= 0;
 	vc->vc_deccm		= 1;
 	vc->vc_decim		= 0;
 
@@ -2320,9 +2348,9 @@ rescan_last_byte:
 			}
 
 			while (1) {
-				if (vc->vc_need_wrap || vc->vc_decim)
+				if ((vc->vc_need_wrap & 1) || vc->vc_decim)
 					FLUSH
-				if (vc->vc_need_wrap) {
+				if (vc->vc_need_wrap & 1) {
 					cr(vc);
 					lf(vc);
 				}
@@ -2337,7 +2365,12 @@ rescan_last_byte:
 					draw_from = vc->vc_pos;
 				}
 				if (vc->vc_x == vc->vc_cols - 1) {
-					vc->vc_need_wrap = vc->vc_decawm;
+					/*
+					 * Set high bit of need_wrap regardless
+					 * of autowrap mode (backspace logic
+					 * relies on this).
+					 */
+					vc->vc_need_wrap = 2 | vc->vc_decawm;
 					draw_to = vc->vc_pos + 2;
 				} else {
 					vc->vc_x++;
@@ -2496,12 +2529,12 @@ static void vt_console_print(struct cons
 	 * Problems caused when we have need_wrap set on '\n' character */
 	while (count--) {
 		c = *b++;
-		if (c == 10 || c == 13 || c == 8 || vc->vc_need_wrap) {
+		if (c == 10 || c == 13 || c == 8 || (vc->vc_need_wrap & 1)) {
 			if (cnt > 0) {
 				if (CON_IS_VISIBLE(vc))
 					vc->vc_sw->con_putcs(vc, start, cnt, vc->vc_y, vc->vc_x);
 				vc->vc_x += cnt;
-				if (vc->vc_need_wrap)
+				if (vc->vc_need_wrap & 1)
 					vc->vc_x--;
 				cnt = 0;
 			}
diff -Nurp a/include/linux/console_struct.h b/include/linux/console_struct.h
--- a/include/linux/console_struct.h	2008-09-21 09:15:16.136681289 -0600
+++ b/include/linux/console_struct.h	2008-09-21 09:15:30.956686803 -0600
@@ -71,6 +71,7 @@ struct vc_data {
 	unsigned int	vc_decscnm	: 1;	/* Screen Mode */
 	unsigned int	vc_decom	: 1;	/* Origin Mode */
 	unsigned int	vc_decawm	: 1;	/* Autowrap Mode */
+	unsigned int	vc_reverse_wrap	: 1;	/* Reverse wrap Mode */
 	unsigned int	vc_deccm	: 1;	/* Cursor Visible */
 	unsigned int	vc_decim	: 1;	/* Insert Mode */
 	unsigned int	vc_deccolm	: 1;	/* 80/132 Column Mode */
@@ -87,7 +88,7 @@ struct vc_data {
 	unsigned int	vc_s_reverse	: 1;
 	/* misc */
 	unsigned int	vc_ques		: 1;
-	unsigned int	vc_need_wrap	: 1;
+	unsigned int	vc_need_wrap	: 2;
 	unsigned int	vc_can_do_color	: 1;
 	unsigned int	vc_report_mouse : 2;
 	unsigned int	vc_kmalloced	: 1;

Re: [PATCH] Fix backspace on wrapped lines in console (virtual terminal)

[Joe Peterson]

[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]

Joe Peterson wrote:
> I have worked up a new patch (attached) for review.

After discussing this with Thomas Dickey (maintainer of xterm), and
running vttest on both xterm and vt, I have generated an even newer
patch for review.  This replaces previous patches in this thread.

xterm only "fixes" the backspace behavior if both reverse wrap and
autowrap modes are enabled.  Although the "fix" is required for accurate
erasure of characters on wrapped lines (and therefore is really nice
when using Linux), the vttest results are not what it expects test 1
(cursor movement).  I do not have a VTxxx to test, but I assume vttest
is true to the hardware.

It makes sense, therefore, to retain the older behavior in vt unless
reverse wrap (which did not exist in the hardware) is enabled.  This
makes the patch true to vttest (and presumably the hardware) as well the
same as xterm in this respect.

							Thanks, Joe

P.S.  If anyone has a VTxxx, it would be interesting to know what
happens with autowrap mode on when backspace is hit after a character is
typed in the rightmost column.  If the cursor moves back, and the 2nd to
last character is then erased, this means the VTxxx behavior will match
the patched vt and xterm (when reverse wrap is not on - i.e. using
standard modes).


[-- Attachment #2: vt-add-reverse-wrap-mode.patch --]
[-- Type: text/plain, Size: 2582 bytes --]

head vt-add-reverse-wrap-mode.patch.old
Add option for backspace to work when wrapped to next line by adding
a new private mode 45 to enable reverse wrap (default: off).
This matches xterm's behavior, which already supports private
mode 45.  In addition, when this new mode is active, fix backspace
behavior when the most recently typed character is in the rightmost
column (i.e. in need_wrap state).  This enables correct erasure in
wrapped lines and also matches xterm behavior.

Signed-off-by: Joe Peterson <joe@skyrush.com>
---

diff -Nurp a/drivers/char/vt.c b/drivers/char/vt.c
--- a/drivers/char/vt.c	2008-09-21 09:27:43.186681676 -0600
+++ b/drivers/char/vt.c	2008-09-23 13:52:44.000000000 -0600
@@ -1130,12 +1130,26 @@ static inline void cr(struct vc_data *vc
 
 static inline void bs(struct vc_data *vc)
 {
-	if (vc->vc_x) {
-		vc->vc_pos -= 2;
-		vc->vc_x--;
-		vc->vc_need_wrap = 0;
-		notify_write(vc, '\b');
+	if (!vc->vc_need_wrap || !vc->vc_reverse_wrap || !vc->vc_decawm) {
+		if (vc->vc_x == 0) {
+			if (vc->vc_reverse_wrap && vc->vc_decawm) {
+				vc->vc_x = vc->vc_cols - 1;
+				if (vc->vc_y == vc->vc_top) {
+					scrdown(vc,
+						vc->vc_top, vc->vc_bottom, 1);
+					vc->vc_pos += vc->vc_size_row - 2;
+				} else if (vc->vc_y > 0) {
+					vc->vc_y--;
+					vc->vc_pos -= 2;
+				}
+			}
+		} else {
+			vc->vc_x--;
+			vc->vc_pos -= 2;
+		}
 	}
+	vc->vc_need_wrap = 0;
+	notify_write(vc, '\b');
 }
 
 static inline void del(struct vc_data *vc)
@@ -1437,6 +1451,9 @@ static void set_mode(struct vc_data *vc,
 			case 25:		/* Cursor on/off */
 				vc->vc_deccm = on_off;
 				break;
+			case 45:		/* Reverse wrap on/off */
+				vc->vc_reverse_wrap = on_off;
+				break;
 			case 1000:
 				vc->vc_report_mouse = on_off ? 2 : 0;
 				break;
@@ -1621,6 +1638,7 @@ static void reset_terminal(struct vc_dat
 	vc->vc_decscnm		= 0;
 	vc->vc_decom		= 0;
 	vc->vc_decawm		= 1;
+	vc->vc_reverse_wrap	= 0;
 	vc->vc_deccm		= 1;
 	vc->vc_decim		= 0;
 
diff -Nurp a/include/linux/console_struct.h b/include/linux/console_struct.h
--- a/include/linux/console_struct.h	2008-09-21 09:15:16.136681289 -0600
+++ b/include/linux/console_struct.h	2008-09-23 10:23:56.000000000 -0600
@@ -71,6 +71,7 @@ struct vc_data {
 	unsigned int	vc_decscnm	: 1;	/* Screen Mode */
 	unsigned int	vc_decom	: 1;	/* Origin Mode */
 	unsigned int	vc_decawm	: 1;	/* Autowrap Mode */
+	unsigned int	vc_reverse_wrap	: 1;	/* Reverse wrap Mode */
 	unsigned int	vc_deccm	: 1;	/* Cursor Visible */
 	unsigned int	vc_decim	: 1;	/* Insert Mode */
 	unsigned int	vc_deccolm	: 1;	/* 80/132 Column Mode */