[PATCH] _describe and literal \n

[PATCH] _describe and literal \n

[Daniel Shahaf]

Current behaviour:
[[[
$ zsh -f
% autoload compinit
% compinit
% _f() { a=( $'foo:hello\nworld' $'bar:lorem\nipsum' ); _describe descr a }
% compdef _f f
% f <TAB>
bar  -- lorem
ipsum
foo  -- hello
world
]]]

Proposed patch:
[[[
diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index e8f0a6f..9e3ea3c 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -531,7 +531,7 @@ cd_init(char *nam, char *hide, char *mlen, char *sep,
                     tmp++;
 
             if (*tmp)
-                str->desc = ztrdup(rembslash(tmp + 1));
+                str->desc = nicedup(rembslash(tmp + 1), 0 /* heap? */);
             else
                 str->desc = NULL;
             *tmp = '\0';
]]]

Gives:
[[[
% f <TAB>
bar  -- lorem\nipsum
foo  -- hello\nworld
]]]

Is this fix correct?

Re: [PATCH] _describe and literal \n

[Bart Schaefer]

On Jul 23,  6:04pm, Daniel Shahaf wrote:
}
} Gives:
} [[[
} % f <TAB>
} bar  -- lorem\nipsum
} foo  -- hello\nworld
} ]]]
} 
} Is this fix correct?

Boy, that's a tough one.  If the embedded newlines mess up something else,
like for example menuselect keeping track of lines/columns/cursor, then
I'd say this is the closest thing to a correct quick-fix.

On the other hand if the extra newlines are handled correctly, then I'd
say this fix isn't warranted, and it's up to the caller to decide what
is meant when a newline appears in the descriptions.

Ideally the newlines would get noticed and the result would be more like

[[[
% f <TAB>
bar  -- lorem
        ipsum
foo  -- hello
        world
]]]

but that seems a tall hill to climb.

Re: [PATCH] _describe and literal \n

[Daniel Shahaf]

Bart Schaefer wrote on Sat, Jul 23, 2016 at 13:00:59 -0700:
> On Jul 23,  6:04pm, Daniel Shahaf wrote:
> }
> } Gives:
> } [[[
> } % f <TAB>
> } bar  -- lorem\nipsum
> } foo  -- hello\nworld
> } ]]]
> } 
> } Is this fix correct?
> 
> Boy, that's a tough one.  If the embedded newlines mess up something else,
> like for example menuselect keeping track of lines/columns/cursor, then
> I'd say this is the closest thing to a correct quick-fix.
> 
> On the other hand if the extra newlines are handled correctly, then I'd
> say this fix isn't warranted, and it's up to the caller to decide what
> is meant when a newline appears in the descriptions.

I haven't been able to get menu selection's highlighting to misbehave.

However, consider this:

Current master:
    % _f() { a=( $'foo:hello\nworld' $'bar:lorem\nipsum' $'baz:lorem\nipsum' ); _describe descr a }
    % f <TAB>
    baz  
    foo  -- lorem\nipsum
    bar  -- hello\nworld

With the patch:
    % f <TAB>
    baz  bar  -- lorem\nipsum
    foo       -- hello\nworld

Never mind the fact that current, unpatched, master escaped the newline
this time; the more important difference is that current master arranges
the display as though the description of foo is "lorem ipsum" and of bar
is "hello world", which is incorrect.

Cheers,

Daniel

Re: [PATCH] _describe and literal \n

[Oliver Kiddle]

Daniel Shahaf wrote:
> I haven't been able to get menu selection's highlighting to misbehave.
>
> However, consider this:
>
> Current master:
>     % _f() { a=( $'foo:hello\nworld' $'bar:lorem\nipsum' $'baz:lorem\nipsum' ); _describe descr a }
>     % f <TAB>
>     baz  
>     foo  -- lorem\nipsum
>     bar  -- hello\nworld

Is your fix to the internals of _describe or the more general code.
Note that _describe truncates descriptions but by using compadd -ld, you
can have multiline descriptions and it works well. fc completion often
contains newlines and I've never had issues with it. However, I think
the intention in _describe was to not have newlines.

I'm not especially fond of the \n escapes. Would prefer they were mapped
to spaces. However, failing to group baz and foo is clearly wrong.

It could be worth experimenting with long strings that approach the
right edge of the terminal to see if it is affecting the calculations.

Oliver

Re: [PATCH] _describe and literal \n

[Bart Schaefer]

On Jul 24,  1:27am, Oliver Kiddle wrote:
} Subject: Re: [PATCH] _describe and literal \n
}
} Daniel Shahaf wrote:
} > Current master:
} >     % _f() { a=( $'foo:hello\nworld' $'bar:lorem\nipsum' $'baz:lorem\nipsum' ); _describe descr a }
} >     % f <TAB>
} >     baz  
} >     foo  -- lorem\nipsum
} >     bar  -- hello\nworld
} 
} I'm not especially fond of the \n escapes. Would prefer they were mapped
} to spaces. However, failing to group baz and foo is clearly wrong.

Happens with \t, \a, etc. as well, though not with e.g. a backslash before
a space.  So something odd is going on with the string comparisons; maybe
a shared global buffer is getting overwritten during sorting?

Re: [PATCH] _describe and literal \n

[Daniel Shahaf]

Oliver Kiddle wrote on Sun, Jul 24, 2016 at 01:27:08 +0200:
> Daniel Shahaf wrote:
> > I haven't been able to get menu selection's highlighting to misbehave.
> >
> > However, consider this:
> >
> > Current master:
> >     % _f() { a=( $'foo:hello\nworld' $'bar:lorem\nipsum' $'baz:lorem\nipsum' ); _describe descr a }
> >     % f <TAB>
> >     baz  
> >     foo  -- lorem\nipsum
> >     bar  -- hello\nworld
> 
> Is your fix to the internals of _describe or the more general code.

The former: the patch is to cd_init(), which is guts of compdescribe,
which in turn is only used by the _describe function.

> Note that _describe truncates descriptions [...]

I see truncation with 
.
    a=( foo:"$(yes | head -150 | xargs)" bar:"$(yes n | head -150 | xargs)" )
    _describe descr a
.
but not if I remove the |xargs.  The only difference between the two
cases is whether the y's/n's are separated by spaces or newlines.

> I'm not especially fond of the \n escapes. Would prefer they were mapped
> to spaces. However, failing to group baz and foo is clearly wrong.
> 

While the patch adds one case where a newline is escaped as \n, the
incumbent code in master already uses that same escaping in another
situation.

> It could be worth experimenting with long strings that approach the
> right edge of the terminal to see if it is affecting the calculations.

I've found a problem, however, I'm not sure whether it's compdescribe's
fault.  Reproducer:

$ zsh -f
% autoload compinit
% compinit
% zstyle ':completion:*' completer _all_matches _match _expand _complete _ignored
% zstyle -e :completion:\* list-colors 'reply=( "=(#b)(${(b)PREFIX})(?)([^ ]#)*=0=33=34=33" )'
% zstyle ':completion:*' menu 'select=long-list' 'select=3'
% pad=${(r:3*COLUMNS::x :):-}
% _f() { a=( foo:$'fff\nfff'$pad bar:$'ggg\nggg'$pad ); _describe descr a }
% compdef _f f
% f <TAB><TAB><TAB><TAB><TAB>

This garbles the display, it shows a «ba» in reverse video and is offset
one line too far down.  Tabbing further keeps moving it down, eventually
I get lines as though by `repeat $N print -rP -- "%Sba%sf bar foo"`.

Or if you meant, to check whether the patch _break_ some width
calculation, I think that's unlikely: the patch escapes the description
value as soon as it's parsed out of the user-specified $foo:$bar value
into the ->desc struct member, in the (as far as I can tell) only
place that parsing is done.

Cheers,

Daniel
(let me know if I should push the patch I'd posted)

Re: [PATCH] _describe and literal \n

[Oliver Kiddle]

On 24 Jul, Daniel Shahaf wrote:
> (let me know if I should push the patch I'd posted)

I don't think anyone ever gave an answer to this. I don't see any
problem with pushing the patch. While I'm not keen on _describe
putting \n etc in the descriptions that's not a new thing and with
the patch it is certainly more consistent.

Oliver

Re: [PATCH] _describe and literal \n

[Daniel Shahaf]

Oliver Kiddle wrote on Tue, Sep 27, 2016 at 22:32:18 +0200:
> On 24 Jul, Daniel Shahaf wrote:
> > (let me know if I should push the patch I'd posted)
> 
> I don't think anyone ever gave an answer to this. I don't see any
> problem with pushing the patch. While I'm not keen on _describe
> putting \n etc in the descriptions that's not a new thing and with
> the patch it is certainly more consistent.

Thanks for the poke.  I looked again and I think I found the underlying
problem.

compadd prints the description using nice*() functions, but _describe
does its width computations using the raw string length; hence compadd
prints a longer string than _describe computed.  I think that overflowed
the terminal width due to the "spaces-only" fake match that _describe
adds.

Using 'nice' widths in compdescribe fixes the case from 38928, but not
the one from 38925.  I think that one simply needs a s/zputs/nicezputs/
somewhere in the "not part of a group" codepath.

Thanks again for the ping.

Daniel


diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index 27b78cd..6ac880c 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -199,11 +199,11 @@ cd_calc(void)
             set->count++;
             if ((l = strlen(str->str)) > cd_state.pre)
                 cd_state.pre = l;
-            if ((l = MB_METASTRWIDTH(str->str)) > cd_state.premaxw)
+            if ((l = ZMB_nicewidth(str->str)) > cd_state.premaxw)
                 cd_state.premaxw = l;
             if (str->desc) {
                 set->desc++;
-                if ((l = strlen(str->desc)) > cd_state.suf)
+                if ((l = strlen(str->desc)) > cd_state.suf) /* ### strlen() assumes no \n */
                     cd_state.suf = l;
             }
         }
@@ -490,7 +490,7 @@ cd_init(char *nam, char *hide, char *mlen, char *sep,
     setp = &(cd_state.sets);
     cd_state.sep = ztrdup(sep);
     cd_state.slen = strlen(sep);
-    cd_state.swidth = MB_METASTRWIDTH(sep);
+    cd_state.swidth = ZMB_nicewidth(sep);
     cd_state.sets = NULL;
     cd_state.showd = disp;
     cd_state.maxg = cd_state.groups = cd_state.descs = 0;
@@ -526,7 +526,8 @@ cd_init(char *nam, char *hide, char *mlen, char *sep,
             str->other = NULL;
             str->set = set;
 
-            for (tmp = *ap; *tmp && *tmp != ':'; tmp++)
+	    /* Advance tmp to the first unescaped colon. */
+	    for (tmp = *ap; *tmp && *tmp != ':'; tmp++)
                 if (*tmp == '\\' && tmp[1])
                     tmp++;
 
@@ -537,7 +538,7 @@ cd_init(char *nam, char *hide, char *mlen, char *sep,
             *tmp = '\0';
             str->str = str->match = ztrdup(rembslash(*ap));
             str->len = strlen(str->str);
-            str->width = MB_METASTRWIDTH(str->str);
+            str->width = ZMB_nicewidth(str->str);
 	    str->sortstr = NULL;
         }
         if (str)
@@ -692,16 +693,16 @@ cd_get(char **params)
 			 * end of screen as safety margin
 			 */
 			d = str->desc;
-			w = MB_METASTRWIDTH(d);
+			w = ZMB_nicewidth(d);
 			if (w <= remw)
 			    strcpy(p, d);
 			else {
 			    pp = p;
 			    while (remw > 0 && *d) {
-				l = MB_METACHARLEN(d);
+				l = MB_METACHARLEN(d); /* ### should use a _nice variant? */
 				memcpy(pp, d, l);
 				pp[l] = '\0';
-				w = MB_METASTRWIDTH(pp);
+				w = ZMB_nicewidth(pp);
 				if (w > remw) {
 				    *pp = '\0';
 				    break;
@@ -792,17 +793,17 @@ cd_get(char **params)
 			cd_state.swidth - CM_SPACE;
 		    p = pp = dbuf + cd_state.slen;
 		    d = str->desc;
-		    w = MB_METASTRWIDTH(d);
+		    w = ZMB_nicewidth(d);
 		    if (w <= remw) {
 			strcpy(p, d);
 			remw -= w;
 			pp += strlen(d);
 		    } else
 			while (remw > 0 && *d) {
-			    l = MB_METACHARLEN(d);
+			    l = MB_METACHARLEN(d); /* ### should use a _nice variant? */
 			    memcpy(pp, d, l);
 			    pp[l] = '\0';
-			    w = MB_METASTRWIDTH(pp);
+			    w = ZMB_nicewidth(pp);
 			    if (w > remw) {
 				*pp = '\0';
 				break;

Re: [PATCH] _describe and literal \n

[Daniel Shahaf]

Daniel Shahaf wrote on Thu, Sep 29, 2016 at 14:11:39 +0000:
> Using 'nice' widths in compdescribe fixes the case from 38928, but not
> the one from 38925.  I think that one simply needs a s/zputs/nicezputs/
> somewhere in the "not part of a group" codepath.

The following appears to be the missing 'nice'.  On the 38925 case, it
causes the newlines to be output as \n (two characters) rather than
dumpd to stdout literally, but it messes up the cursor position:
pressing <TAB> redraws the prompt two lines above where it started.

Daniel

diff --git a/Src/Zle/compresult.c b/Src/Zle/compresult.c
index 7fec7c8..0e09e1b 100644
--- a/Src/Zle/compresult.c
+++ b/Src/Zle/compresult.c
@@ -2236,7 +2236,7 @@ iprintm(Cmgroup g, Cmatch *mp, UNUSED(int mc), UNUSED(int ml), int lastc, int wi
 	bld_all_str(m);
     if (m->disp) {
 	if (m->flags & CMF_DISPLINE) {
-	    printfmt(m->disp, 0, 1, 0);
+	    printfmt(nicedupstring(m->disp), 0, 1, 0);
 	    return;
 	}
 #ifdef MULTIBYTE_SUPPORT

Re: [PATCH] _describe and literal \n

[Daniel Shahaf]

Daniel Shahaf wrote on Thu, Sep 29, 2016 at 14:11:39 +0000:
> compadd prints the description using nice*() functions, but _describe
> does its width computations using the raw string length; hence compadd
> prints a longer string than _describe computed.
>
> @@ -199,11 +199,11 @@ cd_calc(void)
> -            if ((l = MB_METASTRWIDTH(str->str)) > cd_state.premaxw)
> +            if ((l = ZMB_nicewidth(str->str)) > cd_state.premaxw)

I haven't forgotten about this; I'd like to review it again before
pushing, but haven't yet had time to do so.

Re: [PATCH] _describe and literal \n

[Bart Schaefer]

On Oct 7,  1:50pm, Daniel Shahaf wrote:
} Subject: Re: [PATCH] _describe and literal \n
}
} Daniel Shahaf wrote on Thu, Sep 29, 2016 at 14:11:39 +0000:
} > compadd prints the description using nice*() functions, but _describe
} > does its width computations using the raw string length
} 
} I haven't forgotten about this; I'd like to review it again before
} pushing, but haven't yet had time to do so.

Your reasoning seems sound.

Re: [PATCH] _describe and literal \n

[Daniel Shahaf]

Bart Schaefer wrote on Fri, Oct 07, 2016 at 07:53:13 -0700:
> On Oct 7,  1:50pm, Daniel Shahaf wrote:
> } Subject: Re: [PATCH] _describe and literal \n
> }
> } Daniel Shahaf wrote on Thu, Sep 29, 2016 at 14:11:39 +0000:
> } > compadd prints the description using nice*() functions, but _describe
> } > does its width computations using the raw string length
> } 
> } I haven't forgotten about this; I'd like to review it again before
> } pushing, but haven't yet had time to do so.
> 
> Your reasoning seems sound.

Thanks; it's not the reasoning that I want to review but whether my
s/strlen/nicelen/g surgery had any false positives or false negatives.