Re: reverse-menu-complete re-starting completion on 5.2?

Re: reverse-menu-complete re-starting completion on 5.2?

[Oliver Kiddle]

Bart wrote:
> On Feb 26,  9:59am, Danek Duvall wrote:
> } I can cycle forward through the directories
> } just fine, but when I switch into reverse, it takes whatever directory is
> } on the command line and starts completing its subdirectories backwards.
> } Absolutely not what I want.
> 
> This appears to have resulted from this commit:

Even before that commit I can reproduce a variant of the problem by
starting menu completion with reverse-menu-complete and then switching
to a forwards menu complete.

The code only enables cycling of matches when the condition
compwidget == lastcompwidget holds. Before 35627 reverse-menu-complete
was done quite differently and bypassed this code.

One possible fix is to force menucmp to 2 for reversemenucomplete as
follows:
  @@ -346,6 +346,7 @@ reversemenucomplete(char **args)
      zmult = -zmult;
  +    if (menucmp == 1) menucmp = 2;

Apart from that not working for the case of going from reverse to
forwards menu completion, I'm really not sure why we need to be so
strict about the completion widget matching the last completion widget.
With menu selection, we switch to the menuselect keymap and any
completion style widget advances to the next match. I can see why it
would have seemed logical but can't come up with a realistic scenario
where the strict checking of the condition is in any way useful. Can
anyone foresee any problem with just relaxing the condition (see patch).

Oliver

diff --git a/Src/Zle/compcore.c b/Src/Zle/compcore.c
index ae3a640..ae7068f 100644
--- a/Src/Zle/compcore.c
+++ b/Src/Zle/compcore.c
@@ -30,10 +30,6 @@
 #include "complete.mdh"
 #include "compcore.pro"
 
-/* The last completion widget called. */
-
-static Widget lastcompwidget;
-
 /* Flags saying what we have to do with the result. */
 
 /**/
@@ -471,8 +467,7 @@ before_complete(UNUSED(Hookdef dummy), int *lst)
 
     /* If we are doing a menu-completion... */
 
-    if (minfo.cur && menucmp && *lst != COMP_LIST_EXPAND && 
-	(menucmp != 1 || !compwidget || compwidget == lastcompwidget)) {
+    if (minfo.cur && menucmp && *lst != COMP_LIST_EXPAND) {
 	do_menucmp(*lst);
 	return 1;
     }
@@ -481,7 +476,6 @@ before_complete(UNUSED(Hookdef dummy), int *lst)
 	onlyexpl = listdat.valid = 0;
 	return 1;
     }
-    lastcompwidget = compwidget;
 
     /* We may have to reset the cursor to its position after the   *
      * string inserted by the last completion. */
diff --git a/Src/Zle/complist.c b/Src/Zle/complist.c
index 162436b..8aeb6c3 100644
--- a/Src/Zle/complist.c
+++ b/Src/Zle/complist.c
@@ -3399,7 +3399,7 @@ domenuselect(Hookdef dummy, Chdata dat)
 	do_single(*(minfo.cur));
     }
     if (wasnext || broken) {
-	menucmp = 2;
+	menucmp = 1;
 	showinglist = ((validlist && !nolist) ? -2 : 0);
 	minfo.asked = 0;
 	if (!noselect) {
diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index cc4b7d6..a89b2a3 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -100,8 +100,7 @@ mod_export int usemenu, useglob;
 /**/
 mod_export int wouldinstab;
 
-/* != 0 if we are in the middle of a menu completion. May be == 2 to force *
- * menu completion even if using different widgets.                        */
+/* != 0 if we are in the middle of a menu completion. */
 
 /**/
 mod_export int menucmp;

Re: reverse-menu-complete re-starting completion on 5.2?

[Bart Schaefer]

Meant to respond to this a while ago ...

On Feb 29,  1:45am, Oliver Kiddle wrote:
} 
} Even before that commit I can reproduce a variant of the problem by
} starting menu completion with reverse-menu-complete and then switching
} to a forwards menu complete.

Not surprising, really.

} [...] I'm really not sure why we need to be so
} strict about the completion widget matching the last completion widget.

The only reason I can think of is in case menu completion is invoked from
within a user-defined widget, or otherwise entered by a different widget
than simple forward/reverse menu completion.  In that instance the user is
presumably not continuing with the menu in progress but instead wants to
start over.  I.e. similar to tests of $LASTWIDGET in Functions/Zle/*.

} anyone foresee any problem with just relaxing the condition (see patch).

Maybe we'd need another flag action for "zle -f" to continue/interrupt a
menu in progress, but probably it will work as expected more often with
the patch than without.  I suggest you push and we'll find out if a need
for a flag arises.

Re: reverse-menu-complete re-starting completion on 5.2?

[Mikael Magnusson]

On Sun, Mar 6, 2016 at 6:41 PM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> Meant to respond to this a while ago ...
>
> On Feb 29,  1:45am, Oliver Kiddle wrote:
> }
> } Even before that commit I can reproduce a variant of the problem by
> } starting menu completion with reverse-menu-complete and then switching
> } to a forwards menu complete.
>
> Not surprising, really.
>
> } [...] I'm really not sure why we need to be so
> } strict about the completion widget matching the last completion widget.
>
> The only reason I can think of is in case menu completion is invoked from
> within a user-defined widget, or otherwise entered by a different widget
> than simple forward/reverse menu completion.  In that instance the user is
> presumably not continuing with the menu in progress but instead wants to
> start over.  I.e. similar to tests of $LASTWIDGET in Functions/Zle/*.
>
> } anyone foresee any problem with just relaxing the condition (see patch).
>
> Maybe we'd need another flag action for "zle -f" to continue/interrupt a
> menu in progress, but probably it will work as expected more often with
> the patch than without.  I suggest you push and we'll find out if a need
> for a flag arises.

I can tell you I quite often complete a directory name using normal
completion, and then press ctrl-n to complete files inside by latest
modification date. If this continued cycling directories instead, it
would be quite inconvenient. But yeah, push and we'll see what happens
to that.

(I just now noticed that the other way around, switching from ctrl-n
to tab already does continue the same cycle which is not a problem for
me but seems slightly weird).

-- 
Mikael Magnusson

Re: reverse-menu-complete re-starting completion on 5.2?

[Mikael Magnusson]

On Mon, Mar 7, 2016 at 3:05 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
> On Sun, Mar 6, 2016 at 6:41 PM, Bart Schaefer <schaefer@brasslantern.com> wrote:
>> Meant to respond to this a while ago ...
>>
>> On Feb 29,  1:45am, Oliver Kiddle wrote:
>> }
>> } Even before that commit I can reproduce a variant of the problem by
>> } starting menu completion with reverse-menu-complete and then switching
>> } to a forwards menu complete.
>>
>> Not surprising, really.
>>
>> } [...] I'm really not sure why we need to be so
>> } strict about the completion widget matching the last completion widget.
>>
>> The only reason I can think of is in case menu completion is invoked from
>> within a user-defined widget, or otherwise entered by a different widget
>> than simple forward/reverse menu completion.  In that instance the user is
>> presumably not continuing with the menu in progress but instead wants to
>> start over.  I.e. similar to tests of $LASTWIDGET in Functions/Zle/*.
>>
>> } anyone foresee any problem with just relaxing the condition (see patch).
>>
>> Maybe we'd need another flag action for "zle -f" to continue/interrupt a
>> menu in progress, but probably it will work as expected more often with
>> the patch than without.  I suggest you push and we'll find out if a need
>> for a flag arises.
>
> I can tell you I quite often complete a directory name using normal
> completion, and then press ctrl-n to complete files inside by latest
> modification date. If this continued cycling directories instead, it
> would be quite inconvenient. But yeah, push and we'll see what happens
> to that.

Well, this use-case did indeed break with this commit.

-- 
Mikael Magnusson

Re: reverse-menu-complete re-starting completion on 5.2?

[Mikael Magnusson]

On Wed, Mar 9, 2016 at 12:12 PM, Mikael Magnusson <mikachu@gmail.com> wrote:
> On Mon, Mar 7, 2016 at 3:05 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
>> On Sun, Mar 6, 2016 at 6:41 PM, Bart Schaefer <schaefer@brasslantern.com> wrote:
>>> Meant to respond to this a while ago ...
>>>
>>> On Feb 29,  1:45am, Oliver Kiddle wrote:
>>> }
>>> } Even before that commit I can reproduce a variant of the problem by
>>> } starting menu completion with reverse-menu-complete and then switching
>>> } to a forwards menu complete.
>>>
>>> Not surprising, really.
>>>
>>> } [...] I'm really not sure why we need to be so
>>> } strict about the completion widget matching the last completion widget.
>>>
>>> The only reason I can think of is in case menu completion is invoked from
>>> within a user-defined widget, or otherwise entered by a different widget
>>> than simple forward/reverse menu completion.  In that instance the user is
>>> presumably not continuing with the menu in progress but instead wants to
>>> start over.  I.e. similar to tests of $LASTWIDGET in Functions/Zle/*.
>>>
>>> } anyone foresee any problem with just relaxing the condition (see patch).
>>>
>>> Maybe we'd need another flag action for "zle -f" to continue/interrupt a
>>> menu in progress, but probably it will work as expected more often with
>>> the patch than without.  I suggest you push and we'll find out if a need
>>> for a flag arises.
>>
>> I can tell you I quite often complete a directory name using normal
>> completion, and then press ctrl-n to complete files inside by latest
>> modification date. If this continued cycling directories instead, it
>> would be quite inconvenient. But yeah, push and we'll see what happens
>> to that.
>
> Well, this use-case did indeed break with this commit.

While I'm sure everyone would be happy to reimplement this keybind
based on my vague description, here is the relevant setup:

zstyle ':completion:most-recent-file:*' match-original both
zstyle ':completion:most-recent-file:*' file-sort modification
zstyle ':completion:most-recent-file:*' file-patterns '*:all\ files'
zstyle ':completion:most-recent-file:*' hidden all
zstyle ':completion:most-recent-file:*' completer _files
zle -C most-recent-file menu-complete _generic

bindkey "^N"      most-recent-file

I would like ^N after TAB to start completion over (even if this was
originally a ^N completion), but TAB after ^N to continue the same
completion. This is how it worked before the commit. I'm fine with
replacing _generic with something else either custom in my rc or if we
distribute a new helper function.

-- 
Mikael Magnusson

Re: reverse-menu-complete re-starting completion on 5.2?

[Bart Schaefer]

On Mar 9, 12:41pm, Mikael Magnusson wrote:
} Subject: Re: reverse-menu-complete re-starting completion on 5.2?
}
} On Wed, Mar 9, 2016 at 12:12 PM, Mikael Magnusson <mikachu@gmail.com> wrote:
} > On Mon, Mar 7, 2016 at 3:05 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
} >> On Sun, Mar 6, 2016 at 6:41 PM, Bart Schaefer <schaefer@brasslantern.com> wrote:
} >>> On Feb 29,  1:45am, Oliver Kiddle wrote:
} >>> }
} >>> } [...] I'm really not sure why we need to be so
} >>> } strict about the completion widget matching the last completion widget.
} >>>
} >>> The only reason I can think of is in case menu completion is invoked from
} >>> within a user-defined widget, or otherwise entered by a different widget
} >>>
} >>> Maybe we'd need another flag action for "zle -f" to continue/interrupt

Turns out "zle auto-suffix-retain" already serves this purpose.

This is tricky because widgets created with "zle -C" start directly in
completion context, which means that "zle" generates an error; i.e., if
we naively stick this into _generic:

    _generic:zle:1: widgets can only be called when ZLE is active

(The error should probably be more specific, since a completion widget is
merely the wrong *kind* of widget.)

The solution is to make a normal widget for auto-suffix-retain, then invoke
the completion widget with "zle the-widget -w".  This seems a bit more work
than should be necessary, but see example below.

} >> I can tell you I quite often complete a directory name using normal
} >> completion, and then press ctrl-n to complete files inside by latest
} >> modification date.
} 
} zstyle ':completion:most-recent-file:*' match-original both
} zstyle ':completion:most-recent-file:*' file-sort modification
} zstyle ':completion:most-recent-file:*' file-patterns '*:all\ files'
} zstyle ':completion:most-recent-file:*' hidden all
} zstyle ':completion:most-recent-file:*' completer _files
} zle -C most-recent-file menu-complete _generic
} 
} bindkey "^N"      most-recent-file

Just as-is, this works as you desire if the initial directory completion
is not ambiguous, but cycles if there are multiple possible matches in
the current menu.

} I would like ^N after TAB to start completion over (even if this was
} originally a ^N completion), but TAB after ^N to continue the same
} completion.

The following uses the normal-widget-calls-completion trick.  There may
be a better name for it.

-- 8< --
# Replaces bindkey in the above, otherwise all the same

complete-most-recent-file () {
  [[ $LASTWIDGET != $WIDGET ]] && zle auto-suffix-retain
  zle most-recent-file -w
}
zle -N complete-most-recent-file

bindkey "^N" complete-most-recent-file
-- 8< --

If we're OK with this, Oliver's patch can remain, but it's possible that
there are other user-defined widgets that will start behaving unexpectedly.

Re: reverse-menu-complete re-starting completion on 5.2?

[Mikael Magnusson]

On Wed, Mar 9, 2016 at 11:45 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Mar 9, 12:41pm, Mikael Magnusson wrote:
> } Subject: Re: reverse-menu-complete re-starting completion on 5.2?
> }
> } On Wed, Mar 9, 2016 at 12:12 PM, Mikael Magnusson <mikachu@gmail.com> wrote:
> } > On Mon, Mar 7, 2016 at 3:05 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
> } >> On Sun, Mar 6, 2016 at 6:41 PM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> } >>> On Feb 29,  1:45am, Oliver Kiddle wrote:
> } >>> }
> } >>> } [...] I'm really not sure why we need to be so
> } >>> } strict about the completion widget matching the last completion widget.
> } >>>
> } >>> The only reason I can think of is in case menu completion is invoked from
> } >>> within a user-defined widget, or otherwise entered by a different widget
> } >>>
> } >>> Maybe we'd need another flag action for "zle -f" to continue/interrupt
>
> Turns out "zle auto-suffix-retain" already serves this purpose.
>
> This is tricky because widgets created with "zle -C" start directly in
> completion context, which means that "zle" generates an error; i.e., if
> we naively stick this into _generic:
>
>     _generic:zle:1: widgets can only be called when ZLE is active
>
> (The error should probably be more specific, since a completion widget is
> merely the wrong *kind* of widget.)
>
> The solution is to make a normal widget for auto-suffix-retain, then invoke
> the completion widget with "zle the-widget -w".  This seems a bit more work
> than should be necessary, but see example below.
>
> } >> I can tell you I quite often complete a directory name using normal
> } >> completion, and then press ctrl-n to complete files inside by latest
> } >> modification date.
> }
> } zstyle ':completion:most-recent-file:*' match-original both
> } zstyle ':completion:most-recent-file:*' file-sort modification
> } zstyle ':completion:most-recent-file:*' file-patterns '*:all\ files'
> } zstyle ':completion:most-recent-file:*' hidden all
> } zstyle ':completion:most-recent-file:*' completer _files
> } zle -C most-recent-file menu-complete _generic
> }
> } bindkey "^N"      most-recent-file
>
> Just as-is, this works as you desire if the initial directory completion
> is not ambiguous, but cycles if there are multiple possible matches in
> the current menu.
>
> } I would like ^N after TAB to start completion over (even if this was
> } originally a ^N completion), but TAB after ^N to continue the same
> } completion.
>
> The following uses the normal-widget-calls-completion trick.  There may
> be a better name for it.
>
> -- 8< --
> # Replaces bindkey in the above, otherwise all the same
>
> complete-most-recent-file () {
>   [[ $LASTWIDGET != $WIDGET ]] && zle auto-suffix-retain
>   zle most-recent-file -w
> }
> zle -N complete-most-recent-file
>
> bindkey "^N" complete-most-recent-file
> -- 8< --
>
> If we're OK with this, Oliver's patch can remain, but it's possible that
> there are other user-defined widgets that will start behaving unexpectedly.

I did it like this,

## Helper function for most-*-file
function _complete_separately() {
  [[ $LASTWIDGET != $WIDGET ]] && zle auto-suffix-retain
  zle ${WIDGET#complete-} -w
}
zle -N complete-most-recent-file _complete_separately
zle -N complete-most-accessed-file _complete_separately

Using accept-search instead of auto-suffix-retain also works, but it
eats up the first press without doing anything for some reason (only
if a menu is active).

I don't think anyone else is likely to figure this out by themselves though :).

-- 
Mikael Magnusson