reverse-menu-complete

reverse-menu-complete

[Oliver Kiddle]

Something that has bothered me for some time is that when menu selection
is started with reverse-menu-complete, it will initially select the
first match rather than the last. This behaviour is not consistent with
traditional menu completion where reverse-menu-complete will start with
the last match.

The cause of this relates to how reversemenucomplete() is implemented:
It calls menucomplete() and then there is a subsequent reverse_menu hook
which moves the current match back. If menu selection starts, that call
to menucomplete() doesn't actually return until menu-selection is over
so the hook doesn't get run until it's too late. Within menu-selection,
reversemenucomplete() is called directly in response to subsequent keys
and the hook is able to work in those cases.

The quick fix would be to the makecomplist() function where insmnum is
initialised to 1. Given a value of -1, the last match will be selected
but that function somehow needs to know whether or not it has been
called from reversemenucomplete(). That just needs some sort of flag.

Before I looked at the implementation of reversemenucomplete(), my guess
of what it would contain was:
  zmult = -zmult;
  return menucomplete(zlenoargs);

The patch below is an experiment in taking that approach. It works as
far as I can tell and I think it is actually simpler but it is not
without consequences:

- A numeric argument to completion is currently used to toggle the
ALWAYS_LAST_PROMPT option. Does anyone use that feature? I use the
end-of-list widget which I'd have thought is more useful because you can
decide after the list is displayed that you want to keep it. Does this
need peserving?

- As a further complication, there is a "reverse_menu" hook which does
the backwards movement. That becomes superfluous but I'm not sure how
much of the hook would need to stay. Is the hook in theory an interface
to third-party modules?

- And because this isn't the minimal fix, there's a good chance that
I've overlooked something.

While this also allows numeric arguments to, say, jump to the fifth
match, it doesn't really work once menu-selection is active because
there's currently no way to enter numeric arguments within menu
selection. It works in normal menu completion, however.

I also intended to look into why a basic widget defined with
  zle -C xxx reverse-menu-complete _xxx
doesn't cycle between matches in a normal old-style menu completion but
that issue disappeared with this patch.

Any thoughts on this or ideas for other approaches to solving this?

Oliver

diff --git a/Src/Zle/compcore.c b/Src/Zle/compcore.c
index d4051bd..ba538ca 100644
--- a/Src/Zle/compcore.c
+++ b/Src/Zle/compcore.c
@@ -327,9 +327,7 @@ do_completion(UNUSED(Hookdef dummy), Compldat dat)
     haspattern = 0;
     complistmax = getiparam("LISTMAX");
     zsfree(complastprompt);
-    complastprompt = ztrdup(((isset(ALWAYSLASTPROMPT) && zmult == 1) ||
-			     (unset(ALWAYSLASTPROMPT) && zmult != 1)) ?
-			    "yes" : "");
+    complastprompt = ztrdup(isset(ALWAYSLASTPROMPT) ? "yes" : "");
     dolastprompt = 1;
     zsfree(complist);
     complist = ztrdup(isset(LISTROWSFIRST) ?
@@ -975,7 +973,7 @@ makecomplist(char *s, int incmd, int lst)
 	mnum = 0;
 	unambig_mnum = -1;
 	isuf = NULL;
-	insmnum = 1;
+	insmnum = zmult;
 #if 0
 	/* group-numbers in compstate[insert] */
 	insgnum = 1;
diff --git a/Src/Zle/complete.c b/Src/Zle/complete.c
index ea5e41f..63e5b91 100644
--- a/Src/Zle/complete.c
+++ b/Src/Zle/complete.c
@@ -1625,7 +1625,6 @@ boot_(Module m)
     addhookfunc("before_complete", (Hookfn) before_complete);
     addhookfunc("after_complete", (Hookfn) after_complete);
     addhookfunc("accept_completion", (Hookfn) accept_last);
-    addhookfunc("reverse_menu", (Hookfn) reverse_menu);
     addhookfunc("list_matches", (Hookfn) list_matches);
     addhookfunc("invalidate_list", (Hookfn) invalidate_list);
     (void)addhookdefs(m, comphooks, sizeof(comphooks)/sizeof(*comphooks));
@@ -1640,7 +1639,6 @@ cleanup_(Module m)
     deletehookfunc("before_complete", (Hookfn) before_complete);
     deletehookfunc("after_complete", (Hookfn) after_complete);
     deletehookfunc("accept_completion", (Hookfn) accept_last);
-    deletehookfunc("reverse_menu", (Hookfn) reverse_menu);
     deletehookfunc("list_matches", (Hookfn) list_matches);
     deletehookfunc("invalidate_list", (Hookfn) invalidate_list);
     (void)deletehookdefs(m, comphooks,
diff --git a/Src/Zle/complist.c b/Src/Zle/complist.c
index ccee9a7..aae6504 100644
--- a/Src/Zle/complist.c
+++ b/Src/Zle/complist.c
@@ -2578,6 +2578,7 @@ domenuselect(Hookdef dummy, Chdata dat)
     getk:
 
     	if (!do_last_key) {
+	    zmult = 1;
 	    cmd = getkeycmd();
 	    if (mtab_been_reallocated) {
 		do_last_key = 1;
diff --git a/Src/Zle/compresult.c b/Src/Zle/compresult.c
index 9f383f4..7fec7c8 100644
--- a/Src/Zle/compresult.c
+++ b/Src/Zle/compresult.c
@@ -1220,25 +1220,39 @@ do_menucmp(int lst)
 	was_meta = 1;
 
     /* Otherwise go to the next match in the array... */
-    do {
-	if (!*++(minfo.cur)) {
-	    do {
-		if (!(minfo.group = (minfo.group)->next)) {
-		    minfo.group = amatches;
+    while (zmult) {
+	do {
+	    if (zmult > 0) {
+		if (!*++(minfo.cur)) {
+		    do {
+			if (!(minfo.group = (minfo.group)->next)) {
+			    minfo.group = amatches;
 #ifdef ZSH_HEAP_DEBUG
-		    if (memory_validate(minfo.group->heap_id)) {
-			HEAP_ERROR(minfo.group->heap_id);
-		    }
+			    if (memory_validate(minfo.group->heap_id)) {
+				HEAP_ERROR(minfo.group->heap_id);
+			    }
 #endif
+			}
+		    } while (!(minfo.group)->mcount);
+		    minfo.cur = minfo.group->matches;
 		}
-	    } while (!(minfo.group)->mcount);
-	    minfo.cur = minfo.group->matches;
-	}
-    } while ((menuacc &&
-	      !hasbrpsfx(*(minfo.cur), minfo.prebr, minfo.postbr)) ||
-             ((*minfo.cur)->flags & CMF_DUMMY) ||
-	     (((*minfo.cur)->flags & (CMF_NOLIST | CMF_MULT)) &&
-	      (!(*minfo.cur)->str || !*(*minfo.cur)->str)));
+	    } else {
+		if (minfo.cur == (minfo.group)->matches) {
+		    do {
+			if (!(minfo.group = (minfo.group)->prev))
+			    minfo.group = lmatches;
+		    } while (!(minfo.group)->mcount);
+		    minfo.cur = (minfo.group)->matches + (minfo.group)->mcount - 1;
+		} else
+		    minfo.cur--;
+	    }
+	} while ((menuacc &&
+		!hasbrpsfx(*(minfo.cur), minfo.prebr, minfo.postbr)) ||
+		((*minfo.cur)->flags & CMF_DUMMY) ||
+		(((*minfo.cur)->flags & (CMF_NOLIST | CMF_MULT)) &&
+		(!(*minfo.cur)->str || !*(*minfo.cur)->str)));
+	zmult -= (0 < zmult) - (zmult < 0);
+    }
     /* ... and insert it into the command line. */
     do_single(*minfo.cur);
 
@@ -1246,43 +1260,6 @@ do_menucmp(int lst)
 	unmetafy_line();
 }
 
-/**/
-int
-reverse_menu(UNUSED(Hookdef dummy), UNUSED(void *dummy2))
-{
-    int was_meta;
-
-    if (minfo.cur == NULL)
-	return 1;
-
-    do {
-	if (minfo.cur == (minfo.group)->matches) {
-	    do {
-		if (!(minfo.group = (minfo.group)->prev))
-		    minfo.group = lmatches;
-	    } while (!(minfo.group)->mcount);
-	    minfo.cur = (minfo.group)->matches + (minfo.group)->mcount - 1;
-	} else
-	    minfo.cur--;
-    } while ((menuacc &&
-	      !hasbrpsfx(*(minfo.cur), minfo.prebr, minfo.postbr)) ||
-	     ((*minfo.cur)->flags & CMF_DUMMY) ||
-	     (((*minfo.cur)->flags & (CMF_NOLIST | CMF_MULT)) &&
-	      (!(*minfo.cur)->str || !*(*minfo.cur)->str)));
-    /* May already be metafied if called from within a selection */
-    if (zlemetaline == NULL) {
-	metafy_line();
-	was_meta = 0;
-    }
-    else
-	was_meta = 1;
-    do_single(*(minfo.cur));
-    if (!was_meta)
-	unmetafy_line();
-
-    return 0;
-}
-
 /* Accepts the current completion and starts a new arg, *
  * with the next completions. This gives you a way to   *
  * accept several selections from the list of matches.  */
diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index f18ad17..81a2395 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -345,14 +345,8 @@ mod_export int
 reversemenucomplete(char **args)
 {
     wouldinstab = 0;
-    if (!menucmp) {
-	menucomplete(args);
-	/*
-	 * Drop through, since we are now on the first item instead of
-	 * the last.  We've already updated the display, so this is a
-	 * bit inefficient, but it's simple and it works.
-	 */
-    }
+    zmult = -zmult;
+    menucomplete(args);
 
     runhookdef(REVERSEMENUHOOK, NULL);
     return 0;

Re: reverse-menu-complete

[Peter Stephenson]

On Sat, 27 Jun 2015 08:52:52 +0200
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> Any thoughts on this or ideas for other approaches to solving this?

I don't think there's anything down here that's obvious.  If anyone is
comeing to come up with suggestions without you just applying this and
seeing what happens, I'd be surprised...

pws

Re: reverse-menu-complete

[Bart Schaefer]

(Please pardon lengthy context for very short comment below.)

On Jun 27,  8:52am, Oliver Kiddle wrote:
}
} - As a further complication, there is a "reverse_menu" hook which does
} the backwards movement. That becomes superfluous but I'm not sure how
} much of the hook would need to stay. Is the hook in theory an interface
} to third-party modules?
} 
} diff --git a/Src/Zle/compresult.c b/Src/Zle/compresult.c
} index 9f383f4..7fec7c8 100644
} --- a/Src/Zle/compresult.c
} +++ b/Src/Zle/compresult.c
} @@ -1246,43 +1260,6 @@ do_menucmp(int lst)
}  	unmetafy_line();
}  }
}  
} -/**/
} -int
} -reverse_menu(UNUSED(Hookdef dummy), UNUSED(void *dummy2))
} -{
[...]
} diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
} index f18ad17..81a2395 100644
} --- a/Src/Zle/zle_tricky.c
} +++ b/Src/Zle/zle_tricky.c
} @@ -345,14 +345,8 @@ mod_export int
}  reversemenucomplete(char **args)
}  {
}      wouldinstab = 0;
} +    zmult = -zmult;
} +    menucomplete(args);
}  
}      runhookdef(REVERSEMENUHOOK, NULL);
}      return 0;


So what does the REVERSEMENUHOOK do, now that the reverse_menu function
is gone?

Re: reverse-menu-complete

[Oliver Kiddle]

Bart wrote:
> }  reversemenucomplete(char **args)
> }  {
> }      wouldinstab = 0;
> } +    zmult = -zmult;
> } +    menucomplete(args);
> }  
> }      runhookdef(REVERSEMENUHOOK, NULL);
> }      return 0;
> 
> So what does the REVERSEMENUHOOK do, now that the reverse_menu function
> is gone?

Essentially nothing.

As the development guide says, "Modules can also define function hooks.
Other modules can then add functions to these hooks to make the first
module call these functions instead of the default."

The fact that even complist didn't need to add a function to
REVERSEMENUHOOK is perhaps good evidence that it isn't useful.

So shall I bin it?

Oliver

diff --git a/Src/Zle/zle.h b/Src/Zle/zle.h
index 3c65290..ab2428e 100644
--- a/Src/Zle/zle.h
+++ b/Src/Zle/zle.h
@@ -352,8 +352,7 @@ struct brinfo {
 #define BEFORECOMPLETEHOOK (zlehooks + 2)
 #define AFTERCOMPLETEHOOK  (zlehooks + 3)
 #define ACCEPTCOMPHOOK     (zlehooks + 4)
-#define REVERSEMENUHOOK    (zlehooks + 5)
-#define INVALIDATELISTHOOK (zlehooks + 6)
+#define INVALIDATELISTHOOK (zlehooks + 5)
 
 /* complete hook data struct */
 
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 7ccfb68..fe561fc 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1992,8 +1992,6 @@ mod_export struct hookdef zlehooks[] = {
     HOOKDEF("after_complete", NULL, 0),
     /* ACCEPTCOMPHOOK */
     HOOKDEF("accept_completion", NULL, 0),
-    /* REVERSEMENUHOOK */
-    HOOKDEF("reverse_menu", NULL, 0),
     /* INVALIDATELISTHOOK */
     HOOKDEF("invalidate_list", NULL, 0),
 };
diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index 81a2395..937b910 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -346,10 +346,7 @@ reversemenucomplete(char **args)
 {
     wouldinstab = 0;
     zmult = -zmult;
-    menucomplete(args);
-
-    runhookdef(REVERSEMENUHOOK, NULL);
-    return 0;
+    return menucomplete(args);
 }
 
 /**/

Re: reverse-menu-complete

[Bart Schaefer]

On Jun 27, 11:34pm, Oliver Kiddle wrote:
} Subject: Re: reverse-menu-complete
}
} The fact that even complist didn't need to add a function to
} REVERSEMENUHOOK is perhaps good evidence that it isn't useful.
} 
} So shall I bin it?

It's been there since before the zsh sources were in CVS, and it must
have been added by Sven during the time when he was sending patches
as encoded tarballs because a search of the archives turns up nothing.

(Curiously, reversemenucomplete() was modified in a commit that Sven's
commit log attributes to workers/10609, but the corresponding diffs
are not in that message.)

Anyway, there is no hook for *forward* menu-completion, but one for
reverse was explicitly included, so someone at some point must have
thought there was a reason a module would need to know when the list
was supposed to start at the wrong end.

I dunno.

Re: reverse-menu-complete

[Oliver Kiddle]

Bart wrote:
> It's been there since before the zsh sources were in CVS, and it must
> have been added by Sven during the time when he was sending patches
> as encoded tarballs because a search of the archives turns up nothing.

That's it. The patch is 8478 but see 8475 for the explanation. It seems
the idea was only to separate completion specific code into the complete
module and not to provide a hook for addon modules. With that in mind
along with the fact that it wouldn't be hard to put it back, I'll remove
it unless anyone says otherwise.

Does anyone have backups of the old development releases such as the
-pws ones? I could put them into a git repository to make it easier to
track early things down. There's even RCS history files from around
3.1.1 on the ftp site.

> (Curiously, reversemenucomplete() was modified in a commit that Sven's
> commit log attributes to workers/10609, but the corresponding diffs
> are not in that message.)

That's odd. I was wondering if it was something that went wrong due to
the git conversion but no (it is 1.2 of zle_tricky.c in CVS).

Oliver

Re: reverse-menu-complete

[Bart Schaefer]

On Jun 28,  4:22pm, Oliver Kiddle wrote:
}
} Does anyone have backups of the old development releases such as the
} -pws ones? I could put them into a git repository to make it easier to
} track early things down. There's even RCS history files from around
} 3.1.1 on the ftp site.

I have

torch<501> print $ZSH_VERSION
2.4.306 beta

with a corresponding CVS repository, but it'll be cluttered with my own
commits and won't match the way articles arrived in the archives.  This
is from the time before any of the directory names were capitalized in
the source tree.

I have similarly non-canonical repositories for 3.0 and most of 3.1.

I quit following zsh during the "we're going to become a ksh clone and
throw away all this csh stuff" interregnum across the 2.5 series, so I
don't have any of that.

Re: reverse-menu-complete

[Bart Schaefer]

On Sat, Jun 27, 2015 at 2:34 PM, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
>
> >
> > So what does the REVERSEMENUHOOK do, now that the reverse_menu function
> > is gone?
>
> Essentially nothing.
>
> The fact that even complist didn't need to add a function to
> REVERSEMENUHOOK is perhaps good evidence that it isn't useful.

So apparently part of the operation of that function was to allow
menu-complete and reverse-menu-complete to work in tandem to go
forward/backward through the same completion listing, when NOT in menu
selection.  See users/21319 and subsequent.