reverse-menu-complete

[Oliver Kiddle <okiddle@xxxxxxxxxxx>]

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;