Bug somewhere in verbose output for completion listing

Bug somewhere in verbose output for completion listing

[Peter Stephenson]

I expect this to be greeted with something like the cough from the back
of the room when Krusty the Clown tells a joke --- I can't reproduce
this reliably at the moment --- but something determining output width
in completion listings (I'm using the zsh/complist module) isn't being
reset properly.

After a while, the following:

_wtf () {
	local stuff
	stuff=('SpoobleOne:2012/11/02 Description of SpoobleOne'
               'SpoobleTwo:2013/02/13 Description of SpoobleTwo')
	_describe -t stuff 'Stuff to complete.' stuff
}
compdef _wtf wtf

starts producing completion listings in verbose mode (the default) with

zstyle ':completion:*' list-separator '#'

(hence the "#" below) that look like

Completing Stuff to complete.
SpoobleOne                                                                                               # 
SpoobleTwo                                                                                               # 

From that point on this happens every time.  In case they get wrapped,
there are only three lines there, the second and thrid long with lots of
spaces between the completion and what should be the description, which
is missing.

This doesn't happen when I first start the shell; I get the expected

Completing Stuff to complete.
SpoobleOne     # 2012/11/02 Description of SpoobleOne
SpoobleTwo     # 2013/02/13 Description of SpoobleTwo

I think some extra long output from _describe or similar is causing it
to go haywire thereafter.  In my case that seems to be to do with long
completions generated by _perforce, but it's unlikely to be specific to
that.  I suspect some static in complist isn't being reset: I have a
memory the logic is a bit tortuous.

If I find a moment I'll try to make it more reproducible (although
simply attaching to a running shell might be good enough to track it
down).

pws

Re: Bug somewhere in verbose output for completion listing

[Peter Stephenson]

On Thu, 03 Oct 2013 14:00:25 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> If I find a moment I'll try to make it more reproducible (although
> simply attaching to a running shell might be good enough to track it
> down).

Appears to be trivial to reproduce in the obvious manner, by generating
a single long completion.


autoload -Uz compinit
compinit
_wtf2 () {
  local stuff
  stuff=('SpoobleOneVariantWithAVeryLongCompletionThatCouldCauseAllSortsOfMayhemAfterAllWhoKnowsWhatEvilLurksEtc:2012/11/02 Description of SpoobleOne'
  'SpoobleTwo:2013/02/13 Description of SpoobleTwo')
  _describe -t stuff 'Stuff to complete.' stuff
}
compdef _wtf2 wtf2
_wtf () {
  local stuff
  stuff=('SpoobleOne:2012/11/02 Description of SpoobleOne'
  'SpoobleTwo:2013/02/13 Description of SpoobleTwo')
  _describe -t stuff 'Stuff to complete.' stuff
}
compdef _wtf wtf


Complete after wtf2 as far as getting a listing (the separator is the
default "--" instead of the "#" in the previous posting).  This is as
easy as typing "wtf2 ^D".  It already looks a bit screwy at this point
as the comment is missing even though the separator is present.  Then
whenever you get a listing after wtf you see the problem.

pws

Re: Bug somewhere in verbose output for completion listing

[Bart Schaefer]

On Oct 3,  2:00pm, Peter Stephenson wrote:
} Subject: Bug somewhere in verbose output for completion listing
}
} I expect this to be greeted with something like the cough from the back
} of the room when Krusty the Clown tells a joke --- I can't reproduce
} this reliably at the moment --- but something determining output width
} in completion listings (I'm using the zsh/complist module) isn't being
} reset properly.

This looks as though it's related to this from a few months ago:

    http://www.zsh.org/mla/workers/2013/msg00368.html

Unfortunately I don't have any new insights.

Re: Bug somewhere in verbose output for completion listing

[Bart Schaefer]

On Oct 3,  7:50am, Bart Schaefer wrote:
}
}     http://www.zsh.org/mla/workers/2013/msg00368.html
} 
} Unfortunately I don't have any new insights.

Well, one maybe:  There's a static global struct in computil.c called
cd_state that has maxmlen and minmlen fields.  Those are based on the
exported globals maxmlen and minmlen from compcore.c, which the comment
(there's a comment!) describes as "Length of the longest/shortest match".

A bit of grepping does not find anywhere that cd_state.maxmlen is reset
across ZLE exit/restart.  I'm a bit worried that forcibly doing so may
slow down or actually break _oldlist, but maybe cd_state is the place
to start looking.

Re: Bug somewhere in verbose output for completion listing

[Bart Schaefer]

On Oct 3,  8:09am, Bart Schaefer wrote:
}
} Well, one maybe:  There's a static global struct in computil.c called
} cd_state that has maxmlen and minmlen fields.  Those are based on the
} exported globals maxmlen and minmlen from compcore.c, which the comment
} (there's a comment!) describes as "Length of the longest/shortest match".

OK, right track, wrong details.  (There is no cdstate.minmlen)  The
problem is with cd_state.premaxw, which is the padding width.  This
gets set in cd_calc() but is never changed unless it gets larger.

The following superfically fixes it, but I haven't checked in detail
what happens in cases where the same list is being re-used (e.g. by
_oldlist) for successive completions.

I also haven't checked the case of multiple completions in different
groups (the case from the old thread I linked earlier), or whether the
LIST_PACKED option has any effect either way.

diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index f8983c3..ee39185 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -465,6 +465,7 @@ cd_init(char *nam, char *hide, char *mlen, char *sep,
     cd_state.showd = disp;
     cd_state.maxg = cd_state.groups = cd_state.descs = 0;
     cd_state.maxmlen = atoi(mlen);
+    cd_state.premaxw = 0;
     itmp = zterm_columns - cd_state.swidth - 4;
     if (cd_state.maxmlen > itmp)
         cd_state.maxmlen = itmp;

Re: Bug somewhere in verbose output for completion listing

[Peter Stephenson]

On Thu, 03 Oct 2013 08:09:56 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:

> On Oct 3,  7:50am, Bart Schaefer wrote:
> }
> }     http://www.zsh.org/mla/workers/2013/msg00368.html
> } 
> } Unfortunately I don't have any new insights.
> 
> Well, one maybe:  There's a static global struct in computil.c called
> cd_state that has maxmlen and minmlen fields.  Those are based on the
> exported globals maxmlen and minmlen from compcore.c, which the comment
> (there's a comment!) describes as "Length of the longest/shortest match".
> 
> A bit of grepping does not find anywhere that cd_state.maxmlen is reset
> across ZLE exit/restart.  I'm a bit worried that forcibly doing so may
> slow down or actually break _oldlist, but maybe cd_state is the place
> to start looking.

Thanks, I think that may have been all the assistance that was needed.
I discovered that before the problem took effect, completing after wtf
gave

(gdb) p cd_state
$1 = {showd = 1, sep = 0xa0f64e8 "# ", slen = 2, swidth = 2, maxmlen = 40, sets = 0xa1183b0, pre = 10, premaxw = 10, suf = 36, maxg = 1, maxglen = 12, groups = 0, descs = 2, gprew = 0, runs = 0x0}

and with the problem showing up it gave

(gdb) p cd_state
$2 = {showd = 1, sep = 0xa0f64e8 "# ", slen = 2, swidth = 2, maxmlen = 40, sets = 0xa1183b0, pre = 10, premaxw = 102, suf = 36, maxg = 1, maxglen = 12, groups = 0, descs = 2, gprew = 0, runs = 0x0}

so premaxw has gone up.  Sure enough it doesn't get reset with most of the
other stuff in cd_init(), and I can't see any reason why it shouldn't
be.  It must start off initially at 0 so resetting it to 0 each time
we initialise the state can't make anything worse (theoretically).

This removes the persistent problem: you still don't get descriptions
with very long completions, even though list-separator is output, which
seems a dodgy combination of things, but that's obviously a different
issue.

diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index f8983c3..ee39185 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -465,6 +465,7 @@ cd_init(char *nam, char *hide, char *mlen, char *sep,
     cd_state.showd = disp;
     cd_state.maxg = cd_state.groups = cd_state.descs = 0;
     cd_state.maxmlen = atoi(mlen);
+    cd_state.premaxw = 0;
     itmp = zterm_columns - cd_state.swidth - 4;
     if (cd_state.maxmlen > itmp)
         cd_state.maxmlen = itmp;

pws

Re: Bug somewhere in verbose output for completion listing

[Peter Stephenson]

On Thu, 03 Oct 2013 17:20:07 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
> index f8983c3..ee39185 100644
> --- a/Src/Zle/computil.c
> +++ b/Src/Zle/computil.c
> @@ -465,6 +465,7 @@ cd_init(char *nam, char *hide, char *mlen, char *sep,
>      cd_state.showd = disp;
>      cd_state.maxg = cd_state.groups = cd_state.descs = 0;
>      cd_state.maxmlen = atoi(mlen);
> +    cd_state.premaxw = 0;
>      itmp = zterm_columns - cd_state.swidth - 4;
>      if (cd_state.maxmlen > itmp)
>          cd_state.maxmlen = itmp;
> 
> pws

Bart wrote:
> diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
> index f8983c3..ee39185 100644
> --- a/Src/Zle/computil.c
> +++ b/Src/Zle/computil.c
> @@ -465,6 +465,7 @@ cd_init(char *nam, char *hide, char *mlen, char *sep,
>      cd_state.showd = disp;
>      cd_state.maxg = cd_state.groups = cd_state.descs = 0;
>      cd_state.maxmlen = atoi(mlen);
> +    cd_state.premaxw = 0;
>      itmp = zterm_columns - cd_state.swidth - 4;
>      if (cd_state.maxmlen > itmp)
>          cd_state.maxmlen = itmp;

These look pretty similar.  I'll let you commit yours.

pws

Re: Bug somewhere in verbose output for completion listing

[Bart Schaefer]

Already committed 31781.  A bit additional:

On Oct 3,  2:32pm, Peter Stephenson wrote:
}
} Complete after wtf2 as far as getting a listing (the separator is the
} default "--" instead of the "#" in the previous posting).  This is as
} easy as typing "wtf2 ^D".  It already looks a bit screwy at this point
} as the comment is missing even though the separator is present.

The seperator was being inserted into the string before the width was
compared to zterm_columns.  Most of the below is just re-indentation,
but because the listing already has to deal with completions so wide
that the line has wrapped, there doesn't seem to be any reason not to
use the space on the last additional line to tack on the description.

What this means is that if you have a long completion match that almost
exactly fills the screen, you'll get all the matches with none of the
descriptions.  If the longest matches is a bit less than screen width,
you'll see a truncated description for each match, and if the longest
wraps the screen, you'll see the descriptions again.

Minimizing the number of lines used is the reason for all of this.
Some people might prefer to always see the whole description.  At
least this makes it obvious where you'd put a zstyle hook for that.


diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index ee39185..f5e6ba1 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -644,35 +644,43 @@ cd_get(char **params)
 		    p += str->len;
                     memset(p, ' ', (l = (cd_state.premaxw - str->width + CM_SPACE)));
 		    p += l;
-		    strcpy(p, cd_state.sep);
-		    p += cd_state.slen;
 
-		    /*
-		     * copy a character at once until no more screen width
-		     * is available. Leave 1 character at the end of screen
-		     * as safety margin
-		     */
 		    remw = zterm_columns - cd_state.premaxw -
 			cd_state.swidth - 3;
-		    d = str->desc;
-		    w = MB_METASTRWIDTH(d);
-		    if (w <= remw)
-			strcpy(p, d);
-		    else {
-			pp = p;
-			while (remw > 0 && *d) {
-			    l = MB_METACHARLEN(d);
-			    memcpy(pp, d, l);
-			    pp[l] = '\0';
-			    w = MB_METASTRWIDTH(pp);
-			    if (w > remw) {
-				*pp = '\0';
-				break;
-			    }
+		    while (remw < 0 && zterm_columns) {
+			/* line wrapped, use remainder of the extra line */
+			remw += zterm_columns;
+		    }
+		    if (cd_state.slen < remw) {
+			strcpy(p, cd_state.sep);
+			p += cd_state.slen;
+			remw -= cd_state.slen;
 
-			    pp += l;
-			    d += l;
-			    remw -= w;
+			/*
+			 * copy a character at once until no more screen
+			 * width is available. Leave 1 character at the
+			 * end of screen as safety margin
+			 */
+			d = str->desc;
+			w = MB_METASTRWIDTH(d);
+			if (w <= remw)
+			    strcpy(p, d);
+			else {
+			    pp = p;
+			    while (remw > 0 && *d) {
+				l = MB_METACHARLEN(d);
+				memcpy(pp, d, l);
+				pp[l] = '\0';
+				w = MB_METASTRWIDTH(pp);
+				if (w > remw) {
+				    *pp = '\0';
+				    break;
+				}
+
+				pp += l;
+				d += l;
+				remw -= w;
+			    }
 			}
 		    }
 

-- 
Barton E. Schaefer

Re: Bug somewhere in verbose output for completion listing

[Bart Schaefer]

A final thought.

On Oct 3,  5:20pm, Peter Stephenson wrote:
}
} so premaxw has gone up.  Sure enough it doesn't get reset with most of the
} other stuff in cd_init(), and I can't see any reason why it shouldn't
} be.  It must start off initially at 0 so resetting it to 0 each time
} we initialise the state can't make anything worse (theoretically).

We're now back to the state that Felipe Contreras complained about in
the earlier thread that I referenced:

torch% _foobar ()            
{
    local -a commands extra
    commands=('one:command one' 'two:command two')
    _describe -t commands 'commands' commands
    extra=('extraone:extra command one'
	   'zbiggertoshowthealignissue:extra command two')  
    _describe -t extra 'extra' extra
}
torch% compdef _foobar foobar
torch% foobar
extraone                    -- extra command one
one  -- command one
two  -- command two
zbiggertoshowthealignissue  -- extra command two
torch% 

This happens because there are two calls to _describe for the different
groups, and each call to _describe calls compdescribe -I which then
resets the maximum width.  (Previously it would look strange like this
the first time you tried it, and then [because premaxw was NOT reset]
they would all line up on the widest stance on subsequent attempts.
Now at least it's consistently "mis-aligned" every time.)

The behavior looks a little better if you sort the matches by group:

torch% zstyle :completion:\* group-name ''
torch% foobar
one  -- command one
two  -- command two
extraone                    -- extra command one
zbiggertoshowthealignissue  -- extra command two

To add a bit to the discussion from the previous thread, this is as it
is because the entire "one  -- command one" (et al.) is assembled as a
single string by compdescribe.  The data structure does not store pairs
of (match,description) that can be realigned upon display.