PATCH: several bug fixes in countprompt

PATCH: several bug fixes in countprompt

[Roman Perepelitsa]

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

This patch fixes multiple bugs in countprompt.

1. Height off by one in the presence of meta characters at the end of the line.

The following prompt has height 2 but countprompt used to return 3.

    PROMPT="${(pl.$COLUMNS..-.)}%f"$'\n'

You can observe the effects of the bug by hitting esc-x followed by
reset-prompt.

2. Width off by one when a line is broken in the middle of a wide character.

Assuming COLUMNS=79, the following prompt has width 2 but countprompt
used to return 0. Note that \u3050 has width 2.

    PROMPT="${(pl.40..\u3050.)}" zsh -df

Press ctrl-r or type ls<tab> to observe the effects of the bug.

3. Width off by 1-7 when a line is broken in the middle of a tab.

Assuming COLUMNS=79, the following prompt has width 1 but countprompt
used to return 0.

    PROMPT="${(pl.10..\t.)}" zsh -df

Press ctrl-r or type ls<tab> to observe the effects of the bug.

On github: https://github.com/zsh-users/zsh/compare/master...romkatv:countprompt-fix.

I've only tested it on one terminal. Before I test it on a dozen of
different terminals, is there anything I'm missing? Anything I should
be on lookout for?

Roman.

[-- Attachment #2: countprompt-fix.patch --]
[-- Type: application/octet-stream, Size: 1740 bytes --]

diff --git a/Src/prompt.c b/Src/prompt.c
index e8d50d161..7f4d7a70e 100644
--- a/Src/prompt.c
+++ b/Src/prompt.c
@@ -1075,10 +1075,9 @@ putstr(int d)
 mod_export void
 countprompt(char *str, int *wp, int *hp, int overf)
 {
-    int w = 0, h = 1, multi = 0;
+    int w = 0, h = 1, multi = 0, wcw = 0;
     int s = 1;
 #ifdef MULTIBYTE_SUPPORT
-    int wcw;
     char inchar;
     mbstate_t mbs;
     wchar_t wc;
@@ -1092,10 +1091,23 @@ countprompt(char *str, int *wp, int *hp, int overf)
 	 * prompt and the line it terminates takes up exactly the width of the
 	 * terminal
 	 */
-	if (w >= zterm_columns && overf >= 0 && !multi && *str != '\n') {
-	    w = 0;
+	while (w > zterm_columns && overf >= 0 && !multi) {
 	    h++;
+	    if (wcw) {
+		/*
+		 * Wide characters don't get split off. They move to the
+		 * next line if there is not enough space.
+		 */
+		w = wcw;
+		break;
+	    } else {
+		/*
+		 * Tabs overflow to the next line as if they were made of spaces.
+		 */
+		w -= zterm_columns;
+	    }
 	}
+	wcw = 0;
 	/*
 	 * Input string should be metafied, so tokens in it should
 	 * be real tokens, even if there are multibyte characters.
@@ -1176,12 +1188,19 @@ countprompt(char *str, int *wp, int *hp, int overf)
      * This isn't easy to handle generally; just assume there's no
      * output.
      */
-    if(w >= zterm_columns && overf >= 0) {
-	if (!overf || w > zterm_columns) {
-	    w = 0;
-	    h++;
+    while (w > zterm_columns && overf >= 0) {
+	h++;
+	if (wcw) {
+	    w = wcw;
+	    break;
+	} else {
+	    w -= zterm_columns;
 	}
     }
+    if (w == zterm_columns && overf == 0) {
+	w = 0;
+	h++;
+    }
     if(wp)
 	*wp = w;
     if(hp)

Re: PATCH: several bug fixes in countprompt

[Peter Stephenson]

On Wed, 2019-06-19 at 12:22 +0200, Roman Perepelitsa wrote:
> This patch fixes multiple bugs in countprompt.
>...
> I've only tested it on one terminal. Before I test it on a dozen of
> different terminals, is there anything I'm missing? Anything I should
> be on lookout for?

Thanks.  I think the experience has usually been wide characters either
work on a terminal or don't work, but I don't think anybody's specifcally
looked for edge cases before.  Other characters and effects are usually
fairly predictable.

pws

Re: PATCH: several bug fixes in countprompt

[Roman Perepelitsa]

On Wed, Jun 19, 2019 at 12:56 PM Peter Stephenson
<p.stephenson@samsung.com> wrote:
>
> On Wed, 2019-06-19 at 12:22 +0200, Roman Perepelitsa wrote:
> > This patch fixes multiple bugs in countprompt.
> >...
> > I've only tested it on one terminal. Before I test it on a dozen of
> > different terminals, is there anything I'm missing? Anything I should
> > be on lookout for?
>
> Thanks.  I think the experience has usually been wide characters either
> work on a terminal or don't work, but I don't think anybody's specifcally
> looked for edge cases before.  Other characters and effects are usually
> fairly predictable.

Got it.

I've tested the patch on several terminals. Here's what I've found.

1. All terminals suffer from bug #1 and the patch fixes it everywhere.

2. All terminals that support wide characters suffer from bug #2 and
the patch fixes it. Terminals that don't support wide characters have
incorrect cursor position both with and without my patch.

3. All terminals suffer from bug #3. However, my patch fixes it only
for some terminals but not for all. There are two ways terminals
handle tabs that don't fit on the line. The first is what I've
described in the patch: tabs are treated as sequences of spaces, and
some spaces can overflow to the next line. Thus, with COLUMNS=77 and
PROMPT consisting of 10 tabs, prompt ends up taking two lines with the
second line having just 3 spaces on it. The second way is that the
cursor remains on the last column of the first line without ever
wrapping around to the next line. In addition, cursor behaves rather
unexpectedly when you start typing in such prompt. The first typed
character appears at the end of the line, the second typed character
also appears at the end of the line while pushing the previous
character to the left, but the third character overflows to the next
line.

To summarize, my patch doesn't seem to break anything, fixes bug #1,
fixes bug #2 everywhere where it makes sense, and fixes bug #3 where
it's feasible.

Roman.

Re: PATCH: several bug fixes in countprompt

[Peter Stephenson]

On Wed, 2019-06-19 at 16:44 +0200, Roman Perepelitsa wrote:
> To summarize, my patch doesn't seem to break anything, fixes bug #1,
> fixes bug #2 everywhere where it makes sense, and fixes bug #3 where
> it's feasible.

Thanks, let's commit it and see what happens.  It sounds like most
people weren't particularly sensitive to the problem anyway so
presumably the potential fall out for most users is in any case quite
small.  But I've just used lots of words like "sounds like",
"particularly", "presumably" and "potential".

pws

Re: PATCH: several bug fixes in countprompt

[Bart Schaefer]

On Wed, Jun 19, 2019 at 7:45 AM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
>
> 3. There are two ways terminals
> handle tabs that don't fit on the line.

Sounds as though we should either:
-- document that using a tab character in your prompt is not advisable, or
-- always convert tabs to spaces before emitting the prompt

The latter suffers from a need to understand tab stop positions, which
is going to be ugly no matter how it is approached.

Re: PATCH: several bug fixes in countprompt

[Roman Perepelitsa]

On Wed, Jun 19, 2019 at 4:58 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
> Sounds as though we should either:
> -- document that using a tab character in your prompt is not advisable, or
> -- always convert tabs to spaces before emitting the prompt

I like both of these. That is, really do both. Right now, in addition
to making unjustified assumptions about how tabs overflow, ZSH also
assumes that tab stops are set every 8 columns (this is usually true
but not always). RPROMPT breaks when it includes tabs, too.

Roman.