Re: 'zle redisplay' bug in 5.3?

Re: 'zle redisplay' bug in 5.3?

[Peter Stephenson]

On Thu, 5 Jan 2017 01:09:14 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 5,  4:01pm, Jan Larres wrote:
> }
> }   expand-or-complete-with-dots() {
> }       echo -ne "\e[31m......\e[0m"
> }       zle expand-or-complete
> }       zle redisplay
> }   }
> 
> Hmm.  Indeed, with multi-line prompts, invoking redisplay immediately
> after a completion menu is displayed will move the cursor upward as
> many extra lines as the prompt is tall but then does not finish the
> repainting of the prompt, leaving the cursor in the wrong place.
>
> You don't even need compinit, just do:
> 
>     % PS1=$':first line\n'"$PS1"
>     :first line
>     % ls <TAB>
>     :first line
>     % ls <M-x redisplay><RET>
> 
> and you'll observe zle get confused.
> 
> This is from workers/38048z ... where I asked for additional feedback
> and got none ... discussion starts in users/21315.
> 
> I suspect something is different about the complist module vs. plain
> completion menu and the change in 38048 does not account for the
> latter.

I don't know anything about this beyond the fact that showinglist has
three values: off, an extra special clever value (-1), and an extra extra
special very very clever value (-2), and no normal values (> 0) because
that wouldn't be clever enough.

The documentation says

/* Most lines of the buffer we've shown at once with the current list *
 * showing.  == 0 if there is no list.  == -1 if a new list has just  *
 * been put on the screen.  == -2 if zrefresh() needs to put up a new *
 * list.                                                              */

/**/
mod_export int showinglist;

Is it a case of the two requiring different handling?  You might have
thought the different behaviour was the one where the new list was
needed as in the other cases the screen is already in a consistent state
(perhaps?)  Something like the following?

pws

diff --git a/Src/Zle/zle_refresh.c b/Src/Zle/zle_refresh.c
index 8d173cd..49ce154 100644
--- a/Src/Zle/zle_refresh.c
+++ b/Src/Zle/zle_refresh.c
@@ -2434,8 +2434,13 @@ redisplay(UNUSED(char **args))
     moveto(0, 0);
     zputc(&zr_cr);		/* extra care */
     tc_upcurs(lprompth - 1);
-    resetneeded = !showinglist;
-    clearflag = showinglist;
+    if (showinglist == -2) {
+	resetneeded = 0;
+	clearflag = 1;
+    } else {
+	resetneeded = 1;
+	clearflag = 0;
+    }
     return 0;
 }
 

Re: 'zle redisplay' bug in 5.3?

[Bart Schaefer]

On Jan 5,  3:01pm, Peter Stephenson wrote:
}
} The documentation says
} 
} /* Most lines of the buffer we've shown at once with the current list *
}  * showing.  == 0 if there is no list.  == -1 if a new list has just  *
}  * been put on the screen.  == -2 if zrefresh() needs to put up a new *
}  * list.                                                              */
} 
} Is it a case of the two requiring different handling?

Possibly, but the patch testing for -2 doesn't change anything.

If I change it to test showinglist == -1, then the case in this thread
works but TRAPINT handlers break it again (the original problem from the
thread ending with 38048).

So I begin to suspect that testing only showinglist isn't sufficient, but
I'm not sure what else to look at.

Re: 'zle redisplay' bug in 5.3?

[Bart Schaefer]

} On Jan 5,  3:01pm, Peter Stephenson wrote:
} }
} } The documentation says
} } 
} } /* Most lines of the buffer we've shown at once with the current list *
} }  * showing.  == 0 if there is no list.  == -1 if a new list has just  *
} }  * been put on the screen.  == -2 if zrefresh() needs to put up a new *
} }  * list.                                                              */
} } 
} } Is it a case of the two requiring different handling?

That comment is incomplete.  showinglist may also have a positive value
which is tied to the value of "nlnct" -- which appears to be the number
of newlines in the prompt string.

Something is repeating the "move up nlnct lines" action twice.  This is
similar to the description in the thread from 38048.

Not sure what to do with this information yet, and out of time to look
at it this morning, but maybe it's a clue for someone else.

Re: 'zle redisplay' bug in 5.3?

[Peter Stephenson]

On Sat, 7 Jan 2017 10:05:05 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Something is repeating the "move up nlnct lines" action twice.  This is
> similar to the description in the thread from 38048.

I can see two bits in zrefresh() that look like they have to
do with this

One is to do with listshown, rather than showinglist, that triggers what
I take to be the key bit of the refresh code, together with clearlist.
The interaction between showinglist and listshown appears to be utterly
opaque.

The other uses showinglist and then calls listmatches().  Furthermore,
it then calls zrefresh() recursively.  I modified that in 36416, commit
32f5d3d8, only to get called if there was no error, but the recursive
call has always been there.  This might have something to do with
it, particularly now errflag signals both error and interrupt.  Could
propagation of ERRFLAG_INT or the lack of it or the fact that it affects
the code that calls zrefresh() recursively have something to do with
the interrupt problem that caused the changed to redisplaying?

pws

Re: 'zle redisplay' bug in 5.3?

[Peter Stephenson]

On Sat, 7 Jan 2017 18:53:25 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> The other uses showinglist and then calls listmatches().  Furthermore,
> it then calls zrefresh() recursively.  I modified that in 36416, commit
> 32f5d3d8, only to get called if there was no error, but the recursive
> call has always been there.  This might have something to do with
> it, particularly now errflag signals both error and interrupt.  Could
> propagation of ERRFLAG_INT or the lack of it or the fact that it affects
> the code that calls zrefresh() recursively have something to do with
> the interrupt problem that caused the changed to redisplaying?

OK, how about this as an alternative to the previous change to the
refresh code?  If I'm following properly, this was a problem when

TRAPINT () {
	zle reset-prompt
	return 127
}

was in effect during interruption at a completion list.

If we have been interrupted don't try to list matches at all; abort as
if there ws no list.  Obviously, I can't be sure this doesn't have side
effects of its own but as far as I can see it seems to remove the
TRAPINT problem without resorting to tweaking bits I haven't the
faintest clue about, and it also seems OK without the trap.

An additional fix is that at the zle call from the TRAPINT in this case
we don't know where the command line has been, so should take account of
both possibilities.  In general allowing the command line to be modified
here is dangerous but stopping it without forbidding all zle calls is
difficult, so assume the user is only doing basic stuff here (like
redisplay).

Note that if the discussion returns nonetheless to showlinglist etc. I
shall once again be unable to provide useful replies.

pws

1diff --git a/Src/Zle/complist.c b/Src/Zle/complist.c
index 2edaf6e..0350388 100644
--- a/Src/Zle/complist.c
+++ b/Src/Zle/complist.c
@@ -1993,7 +1993,8 @@ complistmatches(UNUSED(Hookdef dummy), Chdata dat)
     if (noselect > 0)
 	noselect = 0;
 
-    if ((minfo.asked == 2 && mselect < 0) || nlnct >= zterm_lines) {
+    if ((minfo.asked == 2 && mselect < 0) || nlnct >= zterm_lines ||
+	errflag) {
 	showinglist = 0;
 	amatches = oamatches;
 	return (noselect = 1);
diff --git a/Src/Zle/zle_refresh.c b/Src/Zle/zle_refresh.c
index 8d173cd..8391739 100644
--- a/Src/Zle/zle_refresh.c
+++ b/Src/Zle/zle_refresh.c
@@ -2434,8 +2434,8 @@ redisplay(UNUSED(char **args))
     moveto(0, 0);
     zputc(&zr_cr);		/* extra care */
     tc_upcurs(lprompth - 1);
-    resetneeded = !showinglist;
-    clearflag = showinglist;
+    resetneeded = 1;
+    clearflag = 0;
     return 0;
 }
 
diff --git a/Src/Zle/zle_thingy.c b/Src/Zle/zle_thingy.c
index c709285..c003148 100644
--- a/Src/Zle/zle_thingy.c
+++ b/Src/Zle/zle_thingy.c
@@ -703,7 +703,7 @@ bin_zle_call(char *name, char **args, UNUSED(Options ops), UNUSED(char func))
 {
     Thingy t;
     struct modifier modsave = zmod;
-    int ret, saveflag = 0, setbindk = 0;
+    int ret, saveflag = 0, setbindk = 0, remetafy;
     char *wname = *args++, *keymap_restore = NULL, *keymap_tmp;
 
     if (!wname)
@@ -714,7 +714,15 @@ bin_zle_call(char *name, char **args, UNUSED(Options ops), UNUSED(char func))
 	return 1;
     }
 
-    UNMETACHECK();
+    /*
+     * zle is callable in traps, so we can't be sure the line is
+     * in its normal state.
+     */
+    if (zlemetaline) {
+	unmetafy_line();
+	remetafy = 1;
+    } else
+	remetafy = 0;
 
     while (*args && **args == '-') {
 	char *num;
@@ -728,6 +736,8 @@ bin_zle_call(char *name, char **args, UNUSED(Options ops), UNUSED(char func))
 		num = args[0][1] ? args[0]+1 : args[1];
 		if (!num) {
 		    zwarnnam(name, "number expected after -%c", **args);
+		    if (remetafy)
+			metafy_line();
 		    return 1;
 		}
 		if (!args[0][1])
@@ -745,19 +755,26 @@ bin_zle_call(char *name, char **args, UNUSED(Options ops), UNUSED(char func))
 		keymap_tmp = args[0][1] ? args[0]+1 : args[1];
 		if (!keymap_tmp) {
 		    zwarnnam(name, "keymap expected after -%c", **args);
+		    if (remetafy)
+			metafy_line();
 		    return 1;
 		}
 		if (!args[0][1])
 		    *++args = "" - 1;
 		keymap_restore = dupstring(curkeymapname);
-		if (selectkeymap(keymap_tmp, 0))
+		if (selectkeymap(keymap_tmp, 0)) {
+		    if (remetafy)
+			metafy_line();
 		    return 1;
+		}
 		break;
 	    case 'w':
 		setbindk = 1;
 		break;
 	    default:
 		zwarnnam(name, "unknown option: %s", *args);
+		if (remetafy)
+		    metafy_line();
 		return 1;
 	    }
 	}
@@ -775,6 +792,8 @@ bin_zle_call(char *name, char **args, UNUSED(Options ops), UNUSED(char func))
 	zmod = modsave;
     if (keymap_restore)
 	selectkeymap(keymap_restore, 0);
+    if (remetafy)
+	metafy_line();
     return ret;
 }
 

Re: 'zle redisplay' bug in 5.3?

[Bart Schaefer]

On Jan 7,  8:04pm, Peter Stephenson wrote:
} Subject: Re: 'zle redisplay' bug in 5.3?
}
} OK, how about this as an alternative to the previous change to the
} refresh code?  If I'm following properly, this was a problem when
} 
} TRAPINT () {
} 	zle reset-prompt
} 	return 127
} }
} 
} was in effect during interruption at a completion list.

That's correct.  However, this patch (checking errflag) does not fix
the issue with interrupts.

Here's how to reproduce:

    zsh -f
    autoload compinit
    compinit -u -D
    TRAPINT () {
      zle reset-prompt
      return 127
    }
    _delay() { sleep 10 }
    zstyle \* completer _complete _delay

Now complete after e.g. "ls " and press ^C within 10 seconds.  The
complist module does not need to be involved, so a change only there
does not help.

Re: 'zle redisplay' bug in 5.3?

[Peter Stephenson]

On Sat, 7 Jan 2017 20:46:40 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 7,  8:04pm, Peter Stephenson wrote:
> } Subject: Re: 'zle redisplay' bug in 5.3?
> }
> } OK, how about this as an alternative to the previous change to the
> } refresh code?  If I'm following properly, this was a problem when
> } 
> } TRAPINT () {
> } 	zle reset-prompt
> } 	return 127
> } }
> } 
> } was in effect during interruption at a completion list.
> 
> That's correct.  However, this patch (checking errflag) does not fix
> the issue with interrupts.

There isn't a *the* issue, is there?  There can't be; the interrupt can
go off absolutely anywhere.  I've fixed the one with complist, which is
what was originally reported, now you're saying there's something else...

>     autoload compinit
>     compinit -u -D
>     TRAPINT () {
>       zle reset-prompt
>       return 127
>     }
>     _delay() { sleep 10 }
>     zstyle \* completer _complete _delay
> 
> Now complete after e.g. "ls " and press ^C within 10 seconds.  The
> complist module does not need to be involved, so a change only there
> does not help.

That is indeed a completely different problem, at least the problem that
shows up when I try this certainly is.  I'm not sure the _delay has got
much to do with it, though, so it's possible there's yet more.

We're not actually in or under completion code here --- though the hook
mechanism makes that statement less useful than it might otherwise be.
However, that does mean that in this case we're not aborting a display
--- we're aborting to a new command line.  (That's the same as if you
just do the straight compinit then the same key sequence, but the
modification of the completer means that you ought to look at the
internal state when the interrupt happens to confirm, which I've done.)
It turns out the prompt doesn't get redrawn on that line because
clearflag isn't initialised to zero, even though it's value is
meaningless when we restart ZLE for the new line as we should redraw
everything from scratch --- that's the residual effect of the completion
code's hook using dolastprompt, I think.

It looks like the line we're aborting isn't completely tidied up,
either, there's a bit of stray output from the reset-prompt, but I
consider that very minor as we're aborting it completely, and separate
again.

I intend to commit this.  Please we can take problems one at a time?

diff --git a/Src/Zle/complist.c b/Src/Zle/complist.c
index 2edaf6e..0350388 100644
--- a/Src/Zle/complist.c
+++ b/Src/Zle/complist.c
@@ -1993,7 +1993,8 @@ complistmatches(UNUSED(Hookdef dummy), Chdata dat)
     if (noselect > 0)
 	noselect = 0;
 
-    if ((minfo.asked == 2 && mselect < 0) || nlnct >= zterm_lines) {
+    if ((minfo.asked == 2 && mselect < 0) || nlnct >= zterm_lines ||
+	errflag) {
 	showinglist = 0;
 	amatches = oamatches;
 	return (noselect = 1);
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 15ea796..6c271b5 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1245,6 +1245,7 @@ zleread(char **lp, char **rp, int flags, int context, char *init, char *finish)
     resetneeded = 0;
     fetchttyinfo = 0;
     trashedzle = 0;
+    clearflag = 0;
     raw_lp = lp;
     lpromptbuf = promptexpand(lp ? *lp : NULL, 1, NULL, NULL, &pmpt_attr);
     raw_rp = rp;
diff --git a/Src/Zle/zle_refresh.c b/Src/Zle/zle_refresh.c
index 8d173cd..8391739 100644
--- a/Src/Zle/zle_refresh.c
+++ b/Src/Zle/zle_refresh.c
@@ -2434,8 +2434,8 @@ redisplay(UNUSED(char **args))
     moveto(0, 0);
     zputc(&zr_cr);		/* extra care */
     tc_upcurs(lprompth - 1);
-    resetneeded = !showinglist;
-    clearflag = showinglist;
+    resetneeded = 1;
+    clearflag = 0;
     return 0;
 }
 
diff --git a/Src/Zle/zle_thingy.c b/Src/Zle/zle_thingy.c
index c709285..c003148 100644
--- a/Src/Zle/zle_thingy.c
+++ b/Src/Zle/zle_thingy.c
@@ -703,7 +703,7 @@ bin_zle_call(char *name, char **args, UNUSED(Options ops), UNUSED(char func))
 {
     Thingy t;
     struct modifier modsave = zmod;
-    int ret, saveflag = 0, setbindk = 0;
+    int ret, saveflag = 0, setbindk = 0, remetafy;
     char *wname = *args++, *keymap_restore = NULL, *keymap_tmp;
 
     if (!wname)
@@ -714,7 +714,15 @@ bin_zle_call(char *name, char **args, UNUSED(Options ops), UNUSED(char func))
 	return 1;
     }
 
-    UNMETACHECK();
+    /*
+     * zle is callable in traps, so we can't be sure the line is
+     * in its normal state.
+     */
+    if (zlemetaline) {
+	unmetafy_line();
+	remetafy = 1;
+    } else
+	remetafy = 0;
 
     while (*args && **args == '-') {
 	char *num;
@@ -728,6 +736,8 @@ bin_zle_call(char *name, char **args, UNUSED(Options ops), UNUSED(char func))
 		num = args[0][1] ? args[0]+1 : args[1];
 		if (!num) {
 		    zwarnnam(name, "number expected after -%c", **args);
+		    if (remetafy)
+			metafy_line();
 		    return 1;
 		}
 		if (!args[0][1])
@@ -745,19 +755,26 @@ bin_zle_call(char *name, char **args, UNUSED(Options ops), UNUSED(char func))
 		keymap_tmp = args[0][1] ? args[0]+1 : args[1];
 		if (!keymap_tmp) {
 		    zwarnnam(name, "keymap expected after -%c", **args);
+		    if (remetafy)
+			metafy_line();
 		    return 1;
 		}
 		if (!args[0][1])
 		    *++args = "" - 1;
 		keymap_restore = dupstring(curkeymapname);
-		if (selectkeymap(keymap_tmp, 0))
+		if (selectkeymap(keymap_tmp, 0)) {
+		    if (remetafy)
+			metafy_line();
 		    return 1;
+		}
 		break;
 	    case 'w':
 		setbindk = 1;
 		break;
 	    default:
 		zwarnnam(name, "unknown option: %s", *args);
+		if (remetafy)
+		    metafy_line();
 		return 1;
 	    }
 	}
@@ -775,6 +792,8 @@ bin_zle_call(char *name, char **args, UNUSED(Options ops), UNUSED(char func))
 	zmod = modsave;
     if (keymap_restore)
 	selectkeymap(keymap_restore, 0);
+    if (remetafy)
+	metafy_line();
     return ret;
 }