Bug 10381

Summary:	(patch inside) lineedit: multiline PS1: fix extra scrolls
Product:	Busybox	Reporter:	avih <avihpit>
Component:	Other	Assignee:	unassigned
Status:	RESOLVED FIXED
Severity:	normal	CC:	busybox-cvs
Priority:	P5
Version:	unspecified
Target Milestone:	---
Hardware:	All
OS:	Linux
Host:		Target:
Build:
Attachments:	lineedit: multiline PS1: fix extra scrolls - redraw using last PS1 line lineedit: multiline PS1: fix extra scrolls - redraw using last PS1 line (applied on top of git master) lineedit: multiline PS1: fix extra scrolls - redraw using last PS1 line (v3)

Description avih 2017-10-07 19:21:11 UTC

Created attachment 7266 [details]
lineedit: multiline PS1: fix extra scrolls - redraw using  last PS1 line

The commit message:

---------------
lineedit: multiline PS1: fix extra scrolls - redraw using last PS1 line

This patch only affects prompts with newlines.

We redraw the prompt [+ input] occasionally, e.g. during tab completion,
history browsing or search, etc, and we expect it to align with prior
redraws, such that the visible effect is that only the input changes.

With multi-line PS1, redraw always printed the prompt some lines below
the old one, which resulted in terminal scroll during every redraw.

Now we only redraw the last PS1 line, so vertical alignment is easier to
manage (we already calculated it using only the last line, but re-drew
all lines - that was the culprit), which fixes those extra scrolls.

Notes:
- Clear-screen (^L) is now the only case where we redraw all prompt lines.

- During terminal resize we now only redraw the last[/sole] prompt line,
  which is arguably better because it's hard to do right (and we never did).

- Good side effect for reverse-i-search: its prompt now replaces only the
  last line of the original prompt - like other shells do.
---------------


Note, when applied to busybox git master, the patch has a conflict at the context of the last hunk because it's applied on top of the patch at bug 10376 . However, the patches and fixes are independent and just happen to modify near eachother.

Comment 1 avih 2017-10-07 20:11:30 UTC

Created attachment 7271 [details]
lineedit: multiline PS1: fix extra scrolls - redraw using last PS1 line (applied on top of git master)

Same as the previous patch, but applied on top of git master. Identical to the previous one other than some line offsets and different context for the last hunk.

Comment 2 avih 2017-10-09 20:34:08 UTC

Created attachment 7276 [details]
lineedit: multiline PS1: fix extra scrolls - redraw using  last PS1 line (v3)

I noticed two issues with my previous patch:

- Other than after ^L, we also need the full prompt after displaying tab-completion choices, since we don't render it in-place.

- The input_backward function, if ENABLE_UNICODE_WIDE_WCHARS, is printing the prompt itself, which was all the prompt lines with my previous patch.


Patch v3 refactors the code slightly, which I think makes it cleaner, and also addressed the issues above - explicitly with tab-completions, implicitly with input_backward (now the patch doesn't change cmdedit_prompt to point to cmdedit_prompt_full, even not temporarily, so input_backward doesn't accidentally print the full prompt).

This patch applies on top of busybox git master, and so will have a context conflict if applied on top of the patch at bug 10376 .

Comment 3 Denys Vlasenko 2017-10-12 15:19:08 UTC

Thanks! Fixed in git!

Comment 4 avih 2017-10-12 16:05:58 UTC

Thank for taking it. Glad to help.

I did notice that it appears at the repo as if you're both the author and the one who signed it off, i.e. my name has completely disappeared from the commit.

Is this expected?

Comment 5 avih 2017-10-12 16:13:26 UTC

Also, something is weird with the commit. The end of the commit at the repo now looks like this:

                vi_case(CTRL('L')|VI_CMDMODE_BIT:)
                        /* Control-l -- clear screen */
-                       printf(ESC"[H"); /* cursor to top,left */
-                       redraw(0, command_len - cursor);
+                       /* cursor to top,left; clear to the end of screen */
+                       printf(ESC"[H" ESC"[J");
+                       draw_full(command_len - cursor);
                        break;


But this new comment: "/* cursor to top,left; clear to the end of screen */" is not part of this bug and is not at my patch. It belongs to my patch at bug 10376 (and you took the comment but didn't add the escape sequence which add the clear...)

Comment 6 avih 2017-10-12 18:08:45 UTC

Thanks for the authorship change, and it was my misunderstanding with the incorrect commit thingy. All is good. Thanks!