PATCH: complist with long display lines

PATCH: complist with long display lines

[Peter Stephenson]

Amazingly, I think I may have fixed a bug in completion.

With the following (note long line which might get messed up in the post):

_testcomp() { compadd \
averylongbitoftextthatshouldcausethedisplaytowrapovertothenextlinewitheffectsthatremaintobeseen \
bach cach dach each fach gach hach
}
compdef _testcomp testcomp

and menu selection in effect, type "testcomp <TAB>".  The long line
should appear and be highlighted.  Now hit <TAB> again.  The display is
messed up: if you see what I see, everything is printed two lines too
low.

The following appears to fix it.  It moves the cursor back to get the
line calculation in singledraw() correct.

Index: Src/Zle/complist.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/complist.c,v
retrieving revision 1.85
diff -u -r1.85 complist.c
--- Src/Zle/complist.c	3 Aug 2006 15:37:50 -0000	1.85
+++ Src/Zle/complist.c	7 Aug 2006 15:33:46 -0000
@@ -1649,6 +1649,8 @@
     g = mgtab[ml1 * columns + mc1];
     clprintm(g, mtab[ml1 * columns + mc1], mcc1, ml1, lc1,
              (g->widths ? g->widths[mcc1] : g->width));
+    if (mlprinted)
+	(void) tcmultout(TCUP, TCMULTUP, mlprinted);
     putc('\r', shout);
 
     if (md2 != md1)
@@ -1658,6 +1660,8 @@
     g = mgtab[ml2 * columns + mc2];
     clprintm(g, mtab[ml2 * columns + mc2], mcc2, ml2, lc2,
              (g->widths ? g->widths[mcc2] : g->width));
+    if (mlprinted)
+	(void) tcmultout(TCUP, TCMULTUP, mlprinted);
     putc('\r', shout);
 
     if (mstatprinted) {

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


To access the latest news from CSR copy this link into a web browser:  http://www.csr.com/email_sig.php

Re: PATCH: complist with long display lines

[Bart Schaefer]

On Aug 7,  4:40pm, Peter Stephenson wrote:
}
} Amazingly, I think I may have fixed a bug in completion.

Wow, that's cool.

Now, about zsh-workers/21842 ...

Re: PATCH: complist with long display lines

[DervishD]

    Hi all :)

 * Bart Schaefer <schaefer@brasslantern.com> dixit:
> On Aug 7,  4:40pm, Peter Stephenson wrote:
> }
> } Amazingly, I think I may have fixed a bug in completion.
> 
> Wow, that's cool.
> 
> Now, about zsh-workers/21842 ...

    That's exactly what I was going to write now ;)))

    Raúl Núñez de Arenas Coronado

-- 
Linux Registered User 88736 | http://www.dervishd.net
http://www.pleyades.net & http://www.gotesdelluna.net
It's my PC and I'll cry if I want to... RAmen!

Re: PATCH: complist with long display lines

[Peter Stephenson]

On Mon, 07 Aug 2006 10:19:46 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Now, about zsh-workers/21842 ...

  There was only one catch in the completion code, catch 22, and that
  stated that a concern for one's sanity in the face of complexities that
  were both real and immediate was the product of a rational mind.

The main completion code doesn't count an extra line for the screen for
strings that exactly fit the screen width: see the chunk of code in
compresult.c where I've added the note that it should count character
widths properly, as well as elsewhere in calclist().  This appears to be
correctly reflected in the way the display appears.

The complist code, however, adds 1 to the count of lines (variously held
in ml and mlprinted) if the number of columns just reaches the screen
width.  As it only allocates the number of lines that the main
completion code tells it, this causes bad array addressing.  I've tried
to fix this consistently by doing the same every time the number of
columns printed is divided the screen width, however I haven't the first
clue what part of the process most of those apply to.

The same mistake seems to appear somewhere else, I think, and with some
trepidation I've changed an occurrence in printfmt() in zle_tricky.c;
you'll see that it does subtract the 1 further up in the bit where it's
found a newline.

*IMPORTANT*: please don't ask me if this is really correct as I'll crawl
into a hole in the ground.  However, do continue to report any
reproducible problems, which is the only way we're ever going to sort
this mess out.

The DPUTS() should check that the array accesses are in range.  If these
go off there's another counting problem.

On my xterm (Fedora Core 5, package xterm-213-1.FC5) a line that
fits the width exactly doesn't show the last character.  This doesn't
happen with either gnome-terminal or konsole, so I'm assuming it's
Somebody Else's Problem.  (It's triggered by a termcap CLEAREOL at the
end of the line; xterm moves the cursor back over the character it's
just printed to avoid moving onto the next line, but doesn't seem to
realise that's just cosmetic and so deletes the character when clearing
the line.  We *could* work around this but I'm hanged if I can be
bothered.)

Index: Src/Zle/complist.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/complist.c,v
retrieving revision 1.87
diff -u -r1.87 complist.c
--- Src/Zle/complist.c	7 Aug 2006 15:58:44 -0000	1.87
+++ Src/Zle/complist.c	9 Aug 2006 22:03:26 -0000
@@ -398,6 +398,9 @@
 static int mtab_been_reallocated;
 static Cmgroup *mgtab, *mgtabp;
 static struct listcols mcolors;
+#ifdef DEBUG
+int mgtabsize;
+#endif
 
 /* Used in mtab/mgtab, for explanations. */
 
@@ -660,7 +663,7 @@
 	     * There might be problems with characters of printing width
 	     * greater than one here.
 	     */
-	    if (col >= columns) {
+	    if (col > columns) {
 		ml++;
 		if (mscroll && !--mrestlines && (ask = asklistscroll(ml))) {
 		    mlprinted = ml - oml;
@@ -698,7 +701,7 @@
 		return 0;
 	    }
 	    putc(nc, shout);
-	    if (++col == columns) {
+	    if (++col > columns) {
 		ml++;
 		if (mscroll && !--mrestlines && (ask = asklistscroll(ml))) {
 		    mlprinted = ml - oml;
@@ -1037,7 +1040,8 @@
 		    *stop = 1;
 		    if (stat && n)
 			mfirstl = -1;
-		    return (mlprinted = l + (cc / columns));
+		    mlprinted = l + (cc ? ((cc-1) / columns) : 0);
+		    return mlprinted;
 		}
 	    }
 	}
@@ -1051,7 +1055,8 @@
     if (stat && n)
 	mfirstl = -1;
 
-    return (mlprinted = l + (cc / columns));
+    mlprinted = l + (cc ? ((cc-1) / columns) : 0);
+    return mlprinted;	    
 }
 
 /* This is like zputs(), but allows scrolling. */
@@ -1158,6 +1163,7 @@
 			int mm = (mcols * ml), i;
 
 			for (i = mcols; i--; ) {
+			    DPUTS(mm+i >= mgtabsize, "BUG: invalid position");
 			    mtab[mm + i] = mtmark(NULL);
 			    mgtab[mm + i] = mgmark(NULL);
 			}
@@ -1479,22 +1485,29 @@
 
             if (m->flags & CMF_DUMMY) {
                 for (i = mcols; i--; ) {
+		    DPUTS(mm+i >= mgtabsize, "BUG: invalid position");
                     mtab[mm + i] = mtmark(mp);
                     mgtab[mm + i] = mgmark(g);
                 }
             } else {
                 for (i = mcols; i--; ) {
+		    DPUTS(mm+i >= mgtabsize, "BUG: invalid position");
                     mtab[mm + i] = mp;
                     mgtab[mm + i] = g;
                 }
             }
 	}
 	if (!dolist(ml)) {
-	    mlprinted = printfmt(m->disp, 0, 0, 0) / columns;
+	    int nc = printfmt(m->disp, 0, 0, 0);
+	    if (nc)
+		mlprinted = (nc - 1) / columns;
+	    else
+		mlprinted = 0;
 	    return 0;
 	}
 	if (m->gnum == mselect) {
 	    int mm = (mcols * ml);
+	    DPUTS(mm >= mgtabsize, "BUG: invalid position");
 	    mline = ml;
 	    mcol = 0;
 	    mmtabp = mtab + mm;
@@ -1533,22 +1546,29 @@
 
             if (m->flags & CMF_DUMMY) {
                 for (i = (width ? width : mcols); i--; ) {
+		    DPUTS(mx+mm+i >= mgtabsize, "BUG: invalid position");
                     mtab[mx + mm + i] = mtmark(mp);
                     mgtab[mx + mm + i] = mgmark(g);
                 }
             } else {
                 for (i = (width ? width : mcols); i--; ) {
+		    DPUTS(mx+mm+i >= mgtabsize, "BUG: invalid position");
                     mtab[mx + mm + i] = mp;
                     mgtab[mx + mm + i] = g;
                 }
             }
 	}
 	if (!dolist(ml)) {
-	    mlprinted = ZMB_nicewidth(m->disp ? m->disp : m->str) / columns;
+	    int nc = ZMB_nicewidth(m->disp ? m->disp : m->str);
+	    if (nc)
+		mlprinted = (nc-1) / columns;
+	    else
+		mlprinted = 0;
 	    return 0;
 	}
 	if (m->gnum == mselect) {
 	    int mm = mcols * ml;
+	    DPUTS(mx+mm >= mgtabsize, "BUG: invalid position");
 
 	    mcol = mx;
 	    mline = ml;
@@ -1572,7 +1592,7 @@
 	    return 1;
 	}
 	len = ZMB_nicewidth(m->disp ? m->disp : m->str);
-	mlprinted = len / columns;
+	mlprinted = len ? (len-1) / columns : 0;
 
 	if ((g->flags & CGF_FILES) && m->modec) {
 	    if (m->gnum != mselect) {
@@ -1646,6 +1666,7 @@
         tc_downcurs(md1);
     if (mc1)
         tcmultout(TCRIGHT, TCMULTRIGHT, mc1);
+    DPUTS(ml1 * columns + mc1 >= mgtabsize, "BUG: invalid position");
     g = mgtab[ml1 * columns + mc1];
     clprintm(g, mtab[ml1 * columns + mc1], mcc1, ml1, lc1,
              (g->widths ? g->widths[mcc1] : g->width));
@@ -1657,6 +1678,7 @@
         tc_downcurs(md2 - md1);
     if (mc2)
         tcmultout(TCRIGHT, TCMULTRIGHT, mc2);
+    DPUTS(ml2 * columns + mc2 >= mgtabsize, "BUG: invalid position");
     g = mgtab[ml2 * columns + mc2];
     clprintm(g, mtab[ml2 * columns + mc2], mcc2, ml2, lc2,
              (g->widths ? g->widths[mcc2] : g->width));
@@ -1756,6 +1778,9 @@
 	memset(mtab, 0, i * sizeof(Cmatch **));
 	free(mgtab);
 	mgtab = (Cmgroup *) zalloc(i * sizeof(Cmgroup));
+#ifdef DEBUG
+	mgtabsize = i;
+#endif
 	memset(mgtab, 0, i * sizeof(Cmgroup));
 	mlastcols = mcols = columns;
 	mlastlines = mlines = listdat.nlines;
Index: Src/Zle/compresult.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/compresult.c,v
retrieving revision 1.63
diff -u -r1.63 compresult.c
--- Src/Zle/compresult.c	1 Aug 2006 21:28:04 -0000	1.63
+++ Src/Zle/compresult.c	9 Aug 2006 22:03:28 -0000
@@ -1480,6 +1480,7 @@
 		hidden = 1;
 		while ((sptr = *pp)) {
 		    while (sptr && *sptr) {
+			/* TODO: we need to use wcwidth() here */
 			nlines += (nlptr = strchr(sptr, '\n'))
 			    ? 1 + (nlptr - sptr - 1) / columns
 			    : (ztrlen(sptr) - 1) / columns;
Index: Src/Zle/zle_tricky.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_tricky.c,v
retrieving revision 1.70
diff -u -r1.70 zle_tricky.c
--- Src/Zle/zle_tricky.c	3 Aug 2006 15:37:50 -0000	1.70
+++ Src/Zle/zle_tricky.c	9 Aug 2006 22:03:29 -0000
@@ -2074,6 +2074,7 @@
 
     for (; *p; p++) {
 	/* Handle the `%' stuff (%% == %, %n == <number of matches>). */
+	/* TODO: we need to use wcwidth() to count cc */
 	if (doesc && *p == '%') {
 	    if (*++p) {
 		m = 0;
@@ -2174,7 +2175,7 @@
 		putc(' ', shout);
 	}
     }
-    return l + (cc / columns);
+    return l + ((cc-1) / columns);
 }
 
 /* This is used to print expansions. */

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: PATCH: complist with long display lines

[Bart Schaefer]

On Aug 9, 11:04pm, Peter Stephenson wrote:
}
} The main completion code doesn't count an extra line for the screen for
} strings that exactly fit the screen width
} 
} The complist code, however, adds 1 to the count of lines (variously held
} in ml and mlprinted) if the number of columns just reaches the screen
} width.  As it only allocates the number of lines that the main
} completion code tells it, this causes bad array addressing.

Brilliant, Peter.  How did you ever track this down?

It may be worthwhile (or at least satisfying to paranoid types) to add
the following patch, which I've had sitting around uncommitted because
it failed to resolve the addressing issues that you seem to finally
have identified.  This patch prevents the infinite loop alluded to in
21842, by catching cases where an array index starts out negative and
decrements from there.  It might be unnecessary now, but:

Index: Src/Zle/complist.c
--- Src/Zle/complist.c	2006-08-10 00:34:13.000000000 -0700
+++ Src/Zle/complist.c.defense	2006-08-10 21:19:26.000000000 -0700
@@ -841,7 +831,7 @@
     selectlocalmap(NULL);
     settyinfo(&shttyinfo);
     putc('\r', shout);
-    for (i = columns - 1; i--; )
+    for (i = columns - 1; i-- > 0; )
 	putc(' ', shout);
     putc('\r', shout);
 
@@ -1162,7 +1148,7 @@
 		    if (mselect >= 0) {
 			int mm = (mcols * ml), i;
 
-			for (i = mcols; i--; ) {
+			for (i = mcols; i-- > 0; ) {
			    DPUTS(mm+i >= mgtabsize, "BUG: invalid position");
 			    mtab[mm + i] = mtmark(NULL);
 			    mgtab[mm + i] = mgmark(NULL);
@@ -1484,13 +1469,13 @@
 	    int mm = (mcols * ml), i;
 
             if (m->flags & CMF_DUMMY) {
-                for (i = mcols; i--; ) {
+                for (i = mcols; i-- > 0; ) {
		    DPUTS(mm+i >= mgtabsize, "BUG: invalid position");
                     mtab[mm + i] = mtmark(mp);
                     mgtab[mm + i] = mgmark(g);
                 }
             } else {
-                for (i = mcols; i--; ) {
+                for (i = mcols; i-- > 0; ) {
		    DPUTS(mm+i >= mgtabsize, "BUG: invalid position");
                     mtab[mm + i] = mp;
                     mgtab[mm + i] = g;
@@ -1545,13 +1523,13 @@
 	    int mm = mcols * ml, i;
 
             if (m->flags & CMF_DUMMY) {
-                for (i = (width ? width : mcols); i--; ) {
+                for (i = (width ? width : mcols); i-- > 0; ) {
		    DPUTS(mx+mm+i >= mgtabsize, "BUG: invalid position");
                     mtab[mx + mm + i] = mtmark(mp);
                     mgtab[mx + mm + i] = mgmark(g);
                 }
             } else {
-                for (i = (width ? width : mcols); i--; ) {
+                for (i = (width ? width : mcols); i-- > 0; ) {
		    DPUTS(mx+mm+i >= mgtabsize, "BUG: invalid position");
                     mtab[mx + mm + i] = mp;
                     mgtab[mx + mm + i] = g;
@@ -2184,7 +2142,7 @@
 	    Cmatch **p = mtab;
 
 	    for (y = 0; y < mlines; y++) {
-		for (x = mcols; x; x--, p++)
+		for (x = mcols; x > 0; x--, p++)
 		    if (*p && !mmarked(*p) && **p && mselect == (**p)->gnum)
 			break;
 		if (x) {
@@ -2195,7 +2153,7 @@
 	    int c;
 
 	    while (mlbeg) {
-		for (q = p, c = columns; c; q++, c--)
+		for (q = p, c = columns; c > 0; q++, c--)
 		    if (*q && !mmarked(*q))
 			break;
 		if (c)
@@ -2228,7 +2180,7 @@
 	    int c;
 
 	    while (mlbeg < mlines) {
-		for (q = p, c = columns; c; q++, c--)
+		for (q = p, c = columns; c > 0; q++, c--)
 		    if (*q)
 			break;
 		if (c)

Re: PATCH: complist with long display lines

[Peter Stephenson]

Bart Schaefer wrote:
> How did you ever track this down?

That's three evenings which will never come again.

Once I'd found that array indexing for mgtab was off, which took a
certain amount of digging since it's referenced in some odd ways, it
wasn't so hard to find out how the limits were being set and how the
assumption in the line limit was being violated.

As I indicated, whether the assumption is present at all and only the
places where I've applied the fix is another matter to be determined...

> It may be worthwhile (or at least satisfying to paranoid types) to add
> the following patch, which I've had sitting around uncommitted because
> it failed to resolve the addressing issues that you seem to finally
> have identified.  This patch prevents the infinite loop alluded to in
> 21842, by catching cases where an array index starts out negative and
> decrements from there.  It might be unnecessary now, but:

I don't think I've done anything which would prevent this infinite loop.
mcols comes from the global columns which comes from the environment, so
if that's negative there are problems.  So I think this would be useful.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


To access the latest news from CSR copy this link into a web browser:  http://www.csr.com/email_sig.php

Re: PATCH: complist with long display lines

[Peter Stephenson]

Peter Stephenson wrote:
> The same mistake seems to appear somewhere else, I think, and with some
> trepidation I've changed an occurrence in printfmt() in zle_tricky.c;
> you'll see that it does subtract the 1 further up in the bit where it's
> found a newline.

This appears to be wrong both here and the corresponding change in
complist.c, functions printfmt() and compprintfmt() respectively.  This
turned up when I was using the file-list zstyle with a description which
(including the file name at the end) just happened to hit the right hand
edge of the screen.  In this case, unlike the others, you get a new line at
the end of the display; I'm guessing this makes the difference.  (However,
obviously it isn't the calculation I'm modifying that produces the extra
newline, so I don't know where that comes from.)  The two changes affect
the non-complist and complist display of the same output.

This is different from the other cases because... er... oh look!  There's
Superman!  Over there!

Luckily, changing these doesn't undo the main point of the fix.  Phew.
I've subjected this to the combinations of tests I know about and they all
seem to work.

Index: Src/Zle/complist.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/complist.c,v
retrieving revision 1.88
diff -u -r1.88 complist.c
--- Src/Zle/complist.c	9 Aug 2006 22:08:38 -0000	1.88
+++ Src/Zle/complist.c	10 Aug 2006 16:15:47 -0000
@@ -1055,8 +1055,12 @@
     if (stat && n)
 	mfirstl = -1;
 
-    mlprinted = l + (cc ? ((cc-1) / columns) : 0);
-    return mlprinted;	    
+    /*
+     * *Not* subtracting 1 from cc at this point appears to be
+     * correct.  C.f. printfmt in zle_tricky.c.
+     */
+    mlprinted = l + (cc / columns);
+    return mlprinted;
 }
 
 /* This is like zputs(), but allows scrolling. */
Index: Src/Zle/zle_tricky.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_tricky.c,v
retrieving revision 1.71
diff -u -r1.71 zle_tricky.c
--- Src/Zle/zle_tricky.c	9 Aug 2006 22:08:39 -0000	1.71
+++ Src/Zle/zle_tricky.c	10 Aug 2006 16:15:49 -0000
@@ -2175,7 +2174,12 @@
 		putc(' ', shout);
 	}
     }
-    return l + ((cc-1) / columns);
+    /*
+     * Experiments suggest that at this point not subtracting 1 from
+     * cc is correct, i.e. if just misses wrapping we still add 1.
+     * (Why?)
+     */
+    return l + (cc / columns);
 }
 
 /* This is used to print expansions. */

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


To access the latest news from CSR copy this link into a web browser:  http://www.csr.com/email_sig.php