Re: lib/53682

[Nikhil Benesch <nikhil.benesch%gmail.com@localhost>]

The following reply was made to PR lib/53682; it has been noted by GNATS.

From: Nikhil Benesch <nikhil.benesch%gmail.com@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: 
Subject: Re: lib/53682
Date: Tue, 20 Nov 2018 16:47:03 -0500

 Attached is a patch for PR 53682. I based the patch on
 https://www.thrysoee.dk/editline/, not the NetBSD source tree, so the
 filename will need to be adjusted. I've reviewed the recent changes to
 refresh.c in the NetBSD tree, however, and I expect the patch to
 otherwise apply cleanly.
 
 For reference, this is the RCS header from my version of refresh.c:
 
 /* $NetBSD: refresh.c,v 1.54 2017/06/30 20:26:52 kre Exp $ */
 
 I should note that I am rather perplexed by what the code I am removing
 was trying to accomplish. It was introduced in [0] to improve handling
 of the rightmost column on terminals with the automargin (am) termcap,
 and appears to have been copied from tcsh. Most of that patch seems
 sensible. For example, terminal_overwrite was taught to account for
 automargin when writing the rightmost character.
 
 What I don't understand is why terminal_move_to_line was given an
 entirely new implementation on terminals with automargin. Namely,
 instead of outputting DO or a newline, like it would on non-automargin
 terminals, it moves to the last character on the line and overwrites
 that character with whatever was already there, relying on automargin
 to move the cursor to the next line as a side effect. In the common
 case where the line does not take up the full width of the terminal,
 the rightmost character is a padding character (a space) that was not
 actually output; outputting it here causes my terminal emulator to fill
 up the entire line with spaces, resulting in the behavior that Jordan
 observed.
 
 This patch removes this special case for terminal_move_to_line. This
 prevents the space padding on my terminal emulator and hasn't caused any
 other aberrations, even when writing to the rightmost column.
 
 If there is some edge case here that I'm not seeing, I'd be happy to
 find an alternative workaround that does not result in space padding,
 provided someone can point me to a terminal emulator that exhibits a
 problem with this patch.
 
 [0]: https://github.com/NetBSD/src/commit/dd263dcc3c17e30437fcbe4b3427caf0eb226635
 
 diff --git a/src/terminal.c b/src/terminal.c
 index 9c74fcd..cc0169e 100644
 --- a/src/terminal.c
 +++ b/src/terminal.c
 @@ -509,36 +509,14 @@ terminal_move_to_line(EditLine *el, int where)
   return;
   }
   if ((del = where - el->el_cursor.v) > 0) {
 - while (del > 0) {
 - if (EL_HAS_AUTO_MARGINS &&
 -    el->el_display[el->el_cursor.v][0] != '\0') {
 -                                size_t h = (size_t)
 -    (el->el_terminal.t_size.h - 1);
 -                                for (; h > 0 &&
 -                                         el->el_display[el->el_cursor.v][h] ==
 -                                                 MB_FILL_CHAR;
 -                                         h--)
 -                                                continue;
 - /* move without newline */
 - terminal_move_to_char(el, (int)h);
 - terminal_overwrite(el, &el->el_display
 -    [el->el_cursor.v][el->el_cursor.h],
 -    (size_t)(el->el_terminal.t_size.h -
 -    el->el_cursor.h));
 - /* updates Cursor */
 - del--;
 - } else {
 - if ((del > 1) && GoodStr(T_DO)) {
 - terminal_tputs(el, tgoto(Str(T_DO), del,
 -    del), del);
 - del = 0;
 - } else {
 - for (; del > 0; del--)
 - terminal__putc(el, '\n');
 - /* because the \n will become \r\n */
 - el->el_cursor.h = 0;
 - }
 - }
 + if ((del > 1) && GoodStr(T_DO)) {
 + terminal_tputs(el, tgoto(Str(T_DO), del, del), del);
 + del = 0;
 + } else {
 + for (; del > 0; del--)
 + terminal__putc(el, '\n');
 + /* because the \n will become \r\n */
 + el->el_cursor.h = 0;
   }
   } else { /* del < 0 */
   if (GoodStr(T_UP) && (-del > 1 || !GoodStr(T_up)))