Proposal (with code) to fix breaking of history widgets on reset-prompt

Proposal (with code) to fix breaking of history widgets on reset-prompt

[Roman Perepelitsa]

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

Hi,

If your [up] key is bound to up-line-or-beginning-search, like most people
have it, try the following experiment. Type sleep 6& and hit [enter]. Then
quickly press [up] and wait for the job to finish. Once it finishes, press
[up] again. Oh no! History doesn’t work. You are supposed to see the
command you’d typed before sleep 6& but you are likely still seeing sleep 6&
.

Here’s what’s going on. When jobs finish, zle .reset-prompt is called to
re-expand and display prompt. This sets LASTWIDGET to .reset-prompt. When
you press [up] after that, up-line-or-beginning-search will behave as if
you’ve typed sleep 6& in your prompt and then pressed [up]. That is, it’ll
search for the previous command in your history that starts with sleep 6&.

up-line-or-beginning-search breaks whenever reset-prompt is called. This
happens when jobs finish and also when ZSH themes with async support want
to refresh prompt after collecting some data in the background. For
example, Powerlevel10k <https://github.com/romkatv/powerlevel10k> theme
shows a greyed-out prompt when entering a large git repo and then updates
it once it figures out git status in the background. If the user has
pressed [up] while the prompt was grey, they’ll be surprised to find that
[up] doesn’t work as expected once prompt updates.

One potential fix for this issue is to change reset-prompt widget so that
it keeps LASTWIDGET unchanged. I’ve implemented this change in
https://github.com/romkatv/zsh/tree/gentle-reset-prompt. Diff against
upstream:
https://github.com/zsh-users/zsh/compare/master...romkatv:gentle-reset-prompt
.

The same branch also adds -W option to zle widget command. This option
instructs zle to keep LASTWIDGET unchanged. I added it because it’s a
natural extension of what I’ve done with .reset-prompt and because it has
solved a long-standing issue I had in my zsh config.

The long-stating problem is that I want my [up] key to behave as
up-line-or-beginning-search with local history turned on, so that it
doesn’t see history from other sessions, and my [ctrl+up] key as
up-line-or-beginning-search with local history turned off. This is very
difficult to achieve. The only solution I’ve found is to fork
up-line-or-beginning-search and edit its source code. You can see it in my
dotfiles
<https://github.com/romkatv/dotfiles-public/blob/88218b6fa6801ad4448b04a6893966b9221c1491/bin/local-history.zsh>.
With the new -W flag that I added to zle widget this problem has a clear
solution. Like this:

# This zle widget is the same as the standard
up-line-or-beginning-search# but restricted to local history.
function up-line-or-beginning-search-local() {
  zle .set-local-history -W 1
  up-line-or-beginning-search
  zle .set-local-history -W 0
}
zle -N up-line-or-beginning-search-local

What do you think?

Roman.

Re: Proposal (with code) to fix breaking of history widgets on reset-prompt

[Peter Stephenson]

> By the way, any chance you could take a look at
> https://www.zsh.org/mla/workers//2019/msg00204.html or ping someone who
> might?

On Sat, 2019-03-23 at 14:09 +0100, Roman Perepelitsa wrote:
> If your [up] key is bound to up-line-or-beginning-search, like most people
> have it, try the following experiment. Type sleep 6& and hit [enter]. Then
> quickly press [up] and wait for the job to finish. Once it finishes, press
> [up] again. Oh no! History doesn’t work. You are supposed to see the
> command you’d typed before sleep 6& but you are likely still seeing sleep 6&
> .
>
> Here’s what’s going on. When jobs finish, zle .reset-prompt is called to
> re-expand and display prompt. This sets LASTWIDGET to .reset-prompt. When
> you press [up] after that, up-line-or-beginning-search will behave as if
> you’ve typed sleep 6& in your prompt and then pressed [up]. That is, it’ll
> search for the previous command in your history that starts with sleep
> 6&.
>...
> One potential fix for this issue is to change reset-prompt widget so that
> it keeps LASTWIDGET unchanged. I’ve implemented this change in
> https://github.com/romkatv/zsh/tree/gentle-reset-prompt. Diff against
> upstream:
> https://github.com/zsh-users/zsh/compare/master...romkatv:gentle-reset-prompt
> .

Don't see any problem with doing that.  I suppose we really need a
list of widgets that should have this behaviour.

Ideally, this would be encapsulated as anything that happens not
as part of a user command --- in your example, that's the code executed
at the end of the job (I think as a result of setting "resetneeded" in
trahszle()) that calls last-prompt, rather than last-prompt itself.  But
that could be more major surgery, with largely the same effect since
it's that widget that's causing the problem.  So I'm perfectly happy to
get this fixed by the present means.

If you can produce this as a single change (with appropriate use of
rebase -i or whatever) we can apply it.  A patch to the list would be
fine, since other people get to see it, but I don't mind cherry-picking
/ rebasing from your repo if it's a single squashed change somewhere.

> The same branch also adds -W option to zle widget command. This option
> instructs zle to keep LASTWIDGET unchanged. I added it because it’s a
> natural extension of what I’ve done with .reset-prompt and because it has
> solved a long-standing issue I had in my zsh config.

The functionality certainly seems useful.

It might be easier simply to make LASTWIDGET read-write.  Then you could
do

  local LASTWIDGET=$LASTWIDGET

(you only need the assignment if you expect something down below to look
at it) and no new syntax is needed.  The save/restore just uses existing
mechanisms intended for doing that with variables, which seems more
efficient, too.

pws

Re: Proposal (with code) to fix breaking of history widgets on reset-prompt

[Roman Perepelitsa]

[-- Attachment #1.1: Type: text/plain, Size: 768 bytes --]

On Thu, Mar 28, 2019 at 12:21 PM Peter Stephenson <p.stephenson@samsung.com>
wrote:

> Don't see any problem with doing that.  I suppose we really need a
> list of widgets that should have this behaviour.
>
> [...]
>
> If you can produce this as a single change (with appropriate use of
> rebase -i or whatever) we can apply it.  A patch to the list would be
> fine, since other people get to see it, but I don't mind cherry-picking
> / rebasing from your repo if it's a single squashed change somewhere.
>

Cool! I can do that.

Here's a branch with a single commit that modifies reset-prompt so that it
doesn't alter LASTWIDGET:
https://github.com/zsh-users/zsh/compare/master...romkatv:gentle-reset-prompt2
.

Please find a patch from it in the attachment.

Roman.

[-- Attachment #1.2: Type: text/html, Size: 1278 bytes --]

[-- Attachment #2: gentle-reset-prompt.patch --]
[-- Type: text/x-patch, Size: 5495 bytes --]

diff --git a/Doc/Zsh/zle.yo b/Doc/Zsh/zle.yo
index c2b9f5430..0986e5390 100644
--- a/Doc/Zsh/zle.yo
+++ b/Doc/Zsh/zle.yo
@@ -2415,9 +2415,12 @@ directory, or changes to the value of variables referred to by the
 prompt).
 
 Otherwise, the prompt is only expanded each time zle starts, and
-when the display as been interrupted by output from another part of the
+when the display has been interrupted by output from another part of the
 shell (such as a job notification) which causes the command line to be
 reprinted.
+
+tt(reset-prompt) doesn't alter the special parameter tt(LASTWIDGET).
+
 )
 tindex(send-break)
 item(tt(send-break) (tt(^G ESC-^G)) (unbound) (unbound))(
diff --git a/Src/Zle/iwidgets.list b/Src/Zle/iwidgets.list
index 58310cd74..c95c7a491 100644
--- a/Src/Zle/iwidgets.list
+++ b/Src/Zle/iwidgets.list
@@ -99,7 +99,7 @@
 "recursive-edit", recursiveedit, ZLE_MENUCMP | ZLE_KEEPSUFFIX | ZLE_LASTCOL
 "redisplay", redisplay, ZLE_MENUCMP | ZLE_KEEPSUFFIX | ZLE_LASTCOL
 "redo", redo, ZLE_KEEPSUFFIX
-"reset-prompt", resetprompt, ZLE_MENUCMP | ZLE_KEEPSUFFIX | ZLE_LASTCOL
+"reset-prompt", resetprompt, ZLE_MENUCMP | ZLE_KEEPSUFFIX | ZLE_LASTCOL | ZLE_NOTCOMMAND | ZLE_NOLAST
 "reverse-menu-complete", reversemenucomplete, ZLE_MENUCMP | ZLE_KEEPSUFFIX | ZLE_ISCOMP
 "run-help", processcmd, ZLE_MENUCMP | ZLE_KEEPSUFFIX | ZLE_LASTCOL
 "select-a-word", selectword, ZLE_KEEPSUFFIX
diff --git a/Src/Zle/zle.h b/Src/Zle/zle.h
index f06c56483..609493f8c 100644
--- a/Src/Zle/zle.h
+++ b/Src/Zle/zle.h
@@ -217,6 +217,7 @@ struct widget {
 #define ZLE_ISCOMP      (1<<11)	/* usable for new style completion */
 #define WIDGET_INUSE    (1<<12) /* widget is in use */
 #define WIDGET_FREE     (1<<13) /* request to free when no longer in use */
+#define ZLE_NOLAST	(1<<14)	/* widget should not alter lbindk */
 
 /* thingies */
 
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 71930f76b..d54e928a6 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1073,7 +1073,7 @@ redrawhook(void)
 	 * temporarily reset state for special variable handling etc.
 	 */
 	incompfunc = 0;
-	execzlefunc(initthingy, args, 1);
+	execzlefunc(initthingy, args, 1, 0);
 	incompfunc = old_incompfunc;
 
 	/* Restore errflag and retflag as zlecallhook() does */
@@ -1136,7 +1136,7 @@ zlecore(void)
 		eofsent = 1;
 		break;
 	    }
-	    if (execzlefunc(bindk, zlenoargs, 0)) {
+	    if (execzlefunc(bindk, zlenoargs, 0, 0)) {
 		handlefeep(zlenoargs);
 		if (eofsent)
 		    break;
@@ -1386,7 +1386,7 @@ execimmortal(Thingy func, char **args)
 {
     Thingy immortal = rthingy_nocreate(dyncat(".", func->nam));
     if (immortal)
-	return execzlefunc(immortal, args, 0);
+	return execzlefunc(immortal, args, 0, 0);
     return 1;
 }
 
@@ -1398,13 +1398,14 @@ execimmortal(Thingy func, char **args)
 
 /**/
 int
-execzlefunc(Thingy func, char **args, int set_bindk)
+execzlefunc(Thingy func, char **args, int set_bindk, int set_lbindk)
 {
     int r = 0, ret = 0, remetafy = 0;
     int nestedvichg = vichgflag;
     int isrepeat = (viinrepeat == 3);
     Widget w;
     Thingy save_bindk = bindk;
+    Thingy save_lbindk = lbindk;
 
     if (set_bindk)
 	bindk = func;
@@ -1412,6 +1413,8 @@ execzlefunc(Thingy func, char **args, int set_bindk)
 	unmetafy_line();
 	remetafy = 1;
     }
+    if (set_lbindk)
+	refthingy(save_lbindk);
     if (isrepeat)
 	viinrepeat = 2;
 
@@ -1535,7 +1538,10 @@ execzlefunc(Thingy func, char **args, int set_bindk)
 	    redup(osi, 0);
 	}
     }
-    if (r) {
+    if (set_lbindk) {
+	unrefthingy(lbindk);
+	lbindk = save_lbindk;
+    } else if (r) {
 	unrefthingy(lbindk);
 	refthingy(func);
 	lbindk = func;
diff --git a/Src/Zle/zle_thingy.c b/Src/Zle/zle_thingy.c
index 6b892b822..ce61db27b 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, remetafy;
+    int ret, saveflag = 0, setbindk = 0, setlbindk, remetafy;
     char *wname = *args++, *keymap_restore = NULL, *keymap_tmp;
 
     if (!wname)
@@ -787,7 +787,8 @@ bin_zle_call(char *name, char **args, UNUSED(Options ops), UNUSED(char func))
      * a vi range to detect a repeated key */
     setbindk = setbindk ||
 	(t->widget && (t->widget->flags & (WIDGET_INT | ZLE_VIOPER)) == WIDGET_INT);
-    ret = execzlefunc(t, args, setbindk);
+    setlbindk = t->widget && (t->widget->flags & ZLE_NOLAST) == ZLE_NOLAST;
+    ret = execzlefunc(t, args, setbindk, setlbindk);
     unrefthingy(t);
     if (saveflag)
 	zmod = modsave;
diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index c6df3d89c..0277d4917 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -1733,7 +1733,7 @@ zlecallhook(char *name, char *arg)
 
     args[0] = arg;
     args[1] = NULL;
-    execzlefunc(thingy, args, 1);
+    execzlefunc(thingy, args, 1, 0);
     unrefthingy(thingy);
 
     /* Retain any user interrupt error status */
diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index a5ff9200c..0f198d0e8 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -216,7 +216,7 @@ getvirange(int wf)
 	     * a number of lines is used.  If the function used
 	     * returns 1, we fail.
 	     */
-	    if ((k2 == bindk) ? dovilinerange() : execzlefunc(k2, zlenoargs, 1))
+	    if ((k2 == bindk) ? dovilinerange() : execzlefunc(k2, zlenoargs, 1, 0))
 		ret = -1;
 	    if (viinrepeat)
 		zmult = mult1;

Re: Proposal (with code) to fix breaking of history widgets on reset-prompt

[Peter Stephenson]

On Tue, 2019-04-09 at 20:45 +0200, Roman Perepelitsa wrote:
> On Thu, Mar 28, 2019 at 12:21 PM Peter Stephenson <p.stephenson@samsung.com> wrote:
> > Don't see any problem with doing that.  I suppose we really need a
> > list of widgets that should have this behaviour.
> > 
> > [...]
> > 
> > If you can produce this as a single change (with appropriate use of
> > rebase -i or whatever) we can apply it.  A patch to the list would be
> > fine, since other people get to see it, but I don't mind cherry-picking
> > / rebasing from your repo if it's a single squashed change somewhere.
> Cool! I can do that.
> 
> Here's a branch with a single commit that modifies reset-prompt so that it
> doesn't alter
> LASTWIDGET: https://github.com/zsh-users/zsh/compare/master...romkatv:gentle-reset-prompt2. 
> 
> Please find a patch from it in the attachment.

Thanks, I've applied this.

pws

Re: Proposal (with code) to fix breaking of history widgets on reset-prompt

[Roman Perepelitsa]

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

On Wed, Apr 10, 2019 at 10:46 AM Peter Stephenson <p.stephenson@samsung.com>
wrote:

>
> Thanks, I've applied this.
>

Thanks, Peter!

There is a bug in completion menu I've reported in
https://www.zsh.org/mla/workers/2019/msg00161.html but haven't got a reply.
Unlike zle code, completion code in zsh looks very complex to me, so I
couldn't figure out how to attempt to fix this. Could you give me any
pointers
or poke people who would know where to dig?

Roman.