zsh-workers
 help / color / mirror / code / Atom feed
* Bug + patch: _expand() fails to insert unambiguous prefix
@ 2021-03-04 19:49 Marlon Richert
  2021-03-20  1:50 ` Lawrence Velázquez
  0 siblings, 1 reply; 7+ messages in thread
From: Marlon Richert @ 2021-03-04 19:49 UTC (permalink / raw)
  To: Zsh hackers list

Test case:
```
cd $(mktemp -d)
exec zsh -f
autoload -Uz compinit; compinit
zstyle ':completion:*' completer _expand _complete
zstyle ':completion:*' tag-order '! original all-expansions'
zstyle ':completion:*' show-ambiguity yes
bindkey '^I' complete-word
touch bar baz
file ^I
file *^I
```

Patch:
```
diff --git Completion/Base/Core/_main_complete
Completion/Base/Core/_main_complete
index 663f755..ec2d954 100644
--- Completion/Base/Core/_main_complete
+++ Completion/Base/Core/_main_complete
@@ -248,11 +248,10 @@ if [[ $compstate[old_list] = keep || nm -gt 1 ]]; then
         ( -n "$_menu_style[(r)select=long-list]" ||
           -n "$_menu_style[(r)(yes|true|on|1)=long-list]" ) ]]; then
     compstate[insert]=menu
-  elif [[ "$compstate[insert]" = "$_saved_insert" ]]; then
-    if [[ -n "$compstate[insert]" &&
+  elif [[ -n "$compstate[insert]" &&
       -n "$_menu_style[(r)(yes|true|1|on)=long]" && tmp -gt LINES ]]; then
     compstate[insert]=menu
-    else
+  elif [[ -n "$_saved_insert" && -n "$compstate[insert]" ]]; then
       sel=( "${(@M)_menu_style:#(yes|true|1|on)*}" )

     if (( $#sel )); then
@@ -291,15 +290,18 @@ if [[ $compstate[old_list] = keep || nm -gt 1 ]]; then
     fi
     if [[ ( -n "$min" && nm -ge min && ( -z "$max" || nm -lt max ) ) ||
           ( -n "$_menu_style[(r)auto*]" &&
-              "$compstate[insert]" = automenu ) ]]; then
+              "$_saved_insert" = automenu ) ]]; then
       compstate[insert]=menu
     elif [[ -n "$max" && nm -ge max ]]; then
       compstate[insert]=unambiguous
     elif [[ -n "$_menu_style[(r)auto*]" &&
-              "$compstate[insert]" != automenu ]]; then
+              "$_saved_insert" != automenu ||
+            "$PREFIX$SUFFIX" != *"$compstate[unambiguous]"* ]]; then
       compstate[insert]=automenu-unambiguous
+    elif [[ -z "$compstate[old_list]" ]]; then
+      compstate[insert]=
     fi
-    fi
+    compstate[list]="${compstate[list]/ambiguous/list}"
   fi

   if [[ "$compstate[insert]" = *menu* ]]; then
```


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Bug + patch: _expand() fails to insert unambiguous prefix
  2021-03-04 19:49 Bug + patch: _expand() fails to insert unambiguous prefix Marlon Richert
@ 2021-03-20  1:50 ` Lawrence Velázquez
  2021-03-20 11:20   ` Marlon Richert
  0 siblings, 1 reply; 7+ messages in thread
From: Lawrence Velázquez @ 2021-03-20  1:50 UTC (permalink / raw)
  To: zsh-workers; +Cc: Marlon Richert

On Thu, Mar 4, 2021, at 2:49 PM, Marlon Richert wrote:
> Test case:
> [...]
> 
> Patch:
> [...]

ping

vq


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Bug + patch: _expand() fails to insert unambiguous prefix
  2021-03-20  1:50 ` Lawrence Velázquez
@ 2021-03-20 11:20   ` Marlon Richert
  2021-03-20 14:42     ` Lawrence Velázquez
  0 siblings, 1 reply; 7+ messages in thread
From: Marlon Richert @ 2021-03-20 11:20 UTC (permalink / raw)
  To: Lawrence Velázquez; +Cc: Zsh hackers list

On Sat, Mar 20, 2021 at 3:50 AM Lawrence Velázquez <vq@larryv.me> wrote:
> ping

Thanks for the ping. I will add an automated test case to that one, too.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Bug + patch: _expand() fails to insert unambiguous prefix
  2021-03-20 11:20   ` Marlon Richert
@ 2021-03-20 14:42     ` Lawrence Velázquez
  2021-03-20 18:22       ` Marlon Richert
  2021-03-20 20:54       ` Oliver Kiddle
  0 siblings, 2 replies; 7+ messages in thread
From: Lawrence Velázquez @ 2021-03-20 14:42 UTC (permalink / raw)
  To: Marlon Richert; +Cc: zsh-workers

On Sat, Mar 20, 2021, at 7:20 AM, Marlon Richert wrote:
> On Sat, Mar 20, 2021 at 3:50 AM Lawrence Velázquez <vq@larryv.me> wrote:
> > ping
> 
> Thanks for the ping. I will add an automated test case to that one, too.

Sorry, that was more to elicit a review from a dev, but you're welcome to expand the patch of course.

vq


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Bug + patch: _expand() fails to insert unambiguous prefix
  2021-03-20 14:42     ` Lawrence Velázquez
@ 2021-03-20 18:22       ` Marlon Richert
  2021-03-20 20:54       ` Oliver Kiddle
  1 sibling, 0 replies; 7+ messages in thread
From: Marlon Richert @ 2021-03-20 18:22 UTC (permalink / raw)
  To: Lawrence Velázquez; +Cc: Zsh hackers list

On Sat, Mar 20, 2021 at 4:43 PM Lawrence Velázquez <vq@larryv.me> wrote:
> Sorry, that was more to elicit a review from a dev, but you're welcome to expand the patch of course.

New patch with accompanying test and all existing tests passing:

diff --git a/Completion/Base/Completer/_expand
b/Completion/Base/Completer/_expand
index def522a76..ea63d97ce 100644
--- a/Completion/Base/Completer/_expand
+++ b/Completion/Base/Completer/_expand
@@ -11,7 +11,7 @@ setopt localoptions nonomatch

 [[ _matcher_num -gt 1 ]] && return 1

-local exp word sort expr expl subd suf=" " force opt asp tmp opre pre epre
+local exp word sort expr expl subd pref suf=" " force opt asp tmp opre pre epre
 local continue=0

 (( $# )) &&
@@ -181,6 +181,7 @@ if (( $#exp == 1 )); then
   fi
 fi

+pref="${${${word:#[~/]*}:+$PWD/}:-/}"
 if [[ -z "$compstate[insert]" ]] ;then
   if [[ "$sort" = menu ]]; then
     _description expansions expl expansions "o:$word"
@@ -188,7 +189,7 @@ if [[ -z "$compstate[insert]" ]] ;then
     _description -V expansions expl expansions "o:$word"
   fi

-  compadd "$expl[@]" -UQ -qS "$suf" -a exp
+  compadd "$expl[@]" -fW "$pref" -UQ -qS "$suf" -a exp
 else
   _tags all-expansions expansions original

@@ -214,9 +215,9 @@ else
         normal=( "$normal[@]" "$i" )
       fi
     done
-    (( $#dir ))    && compadd "$expl[@]" -UQ -qS/ -a dir
-    (( $#space ))  && compadd "$expl[@]" -UQ -qS " " -a space
-    (( $#normal )) && compadd "$expl[@]" -UQ -qS "" -a normal
+    (( $#dir ))    && compadd "$expl[@]" -fW "$pref" -UQ -qS/ -a dir
+    (( $#space ))  && compadd "$expl[@]" -fW "$pref" -UQ -qS " " -a space
+    (( $#normal )) && compadd "$expl[@]" -fW "$pref" -UQ -qS "" -a normal
   fi
   if _requested all-expansions; then
     local disp dstr
@@ -232,8 +233,7 @@ else
     else
       disp=()
     fi
-    [[ -o multios ]] && exp=($exp[1] $compstate[redirect]${^exp[2,-1]})
-    compadd "$disp[@]" "$expl[@]" -UQ -qS "$suf" - "$exp"
+    compadd "$disp[@]" "$expl[@]" -UQ -qS "$suf" -C
   fi

   _requested original expl original && compadd "$expl[@]" -UQ - "$word"
diff --git a/Completion/Base/Core/_main_complete
b/Completion/Base/Core/_main_complete
index 169ca1f40..ebf04dc25 100644
--- a/Completion/Base/Core/_main_complete
+++ b/Completion/Base/Core/_main_complete
@@ -248,11 +248,10 @@ if [[ $compstate[old_list] = keep || nm -gt 1 ]]; then
         ( -n "$_menu_style[(r)select=long-list]" ||
           -n "$_menu_style[(r)(yes|true|on|1)=long-list]" ) ]]; then
     compstate[insert]=menu
-  elif [[ "$compstate[insert]" = "$_saved_insert" ]]; then
-    if [[ -n "$compstate[insert]" &&
+  elif [[ -n "$compstate[insert]" &&
           -n "$_menu_style[(r)(yes|true|1|on)=long]" && tmp -gt LINES ]]; then
     compstate[insert]=menu
-    else
+  elif [[ -n "$_saved_insert" && -n "$compstate[insert]" ]]; then
     sel=( "${(@M)_menu_style:#(yes|true|1|on)*}" )

     if (( $#sel )); then
@@ -289,16 +288,19 @@ if [[ $compstate[old_list] = keep || nm -gt 1 ]]; then
         (( max )) || break
       done
     fi
+    [[ $compstate[insert] = menu ]] &&
+      compstate[list]="${compstate[list]/ambiguous/list}"
     if [[ ( -n "$min" && nm -ge min && ( -z "$max" || nm -lt max ) ) ||
           ( -n "$_menu_style[(r)auto*]" &&
-              "$compstate[insert]" = automenu ) ]]; then
+            "$_saved_insert" = automenu ) ]]; then
       compstate[insert]=menu
     elif [[ -n "$max" && nm -ge max ]]; then
       compstate[insert]=unambiguous
     elif [[ -n "$_menu_style[(r)auto*]" &&
-              "$compstate[insert]" != automenu ]]; then
+            "$_saved_insert" != automenu ]]; then
       compstate[insert]=automenu-unambiguous
-      fi
+    else
+      compstate[insert]="$_saved_insert"
     fi
   fi

diff --git a/Test/Y01completion.ztst b/Test/Y01completion.ztst
index f6474c4a1..cf0abe85d 100644
--- a/Test/Y01completion.ztst
+++ b/Test/Y01completion.ztst
@@ -44,6 +44,25 @@
 >line: {: dir1/}{}
 >line: {: dir2/}{}

+  comptesteval "zstyle ':completion:*' completer _expand"
+  comptesteval "zstyle ':completion:*' tag-order '! original'"
+  comptesteval "zstyle ':completion:*:expand:*' keep-prefix no"
+  comptest $': d*\C-K\C-Wf*\C-K'
+0:_expand inserts unambiguous prefix and highlights file types
+>line: {: dir}{}
+>DESCRIPTION:{expansions}
+>DI:{dir1}
+>DI:{dir2}
+>DESCRIPTION:{all expansions}
+>NO:{dir1 dir2}
+>line: {: file}{}
+>DESCRIPTION:{expansions}
+>FI:{file1}
+>FI:{file2}
+>DESCRIPTION:{all expansions}
+>NO:{file1 file2}
+
+  comptesteval "zstyle -d ':completion:*' completer"
   comptesteval '_users () { compadd user1 user2 }'
   comptest $': ~\t\t\t\t\t'
 0:tilde
diff --git a/Test/comptest b/Test/comptest
index a36e301e0..2069e48ac 100644
--- a/Test/comptest
+++ b/Test/comptest
@@ -57,6 +57,13 @@ expand-or-complete-with-report () {
   zle clear-screen
   zle -R
 }
+complete-word-with-report () {
+  print -lr "<WIDGET><complete-word>"
+  zle complete-word
+  print -lr - "<LBUFFER>$LBUFFER</LBUFFER>" "<RBUFFER>$RBUFFER</RBUFFER>"
+  zle clear-screen
+  zle -R
+}
 list-choices-with-report () {
   print -lr "<WIDGET><list-choices>"
   zle list-choices
@@ -81,10 +88,12 @@ zle-finish () {
   zle accept-line
 }
 zle -N expand-or-complete-with-report
+zle -N complete-word-with-report
 zle -N list-choices-with-report
 zle -N comp-finish
 zle -N zle-finish
 bindkey "^I" expand-or-complete-with-report
+bindkey "^K" complete-word-with-report
 bindkey "^D" list-choices-with-report
 bindkey "^Z" comp-finish
 bindkey "^X" zle-finish


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Bug + patch: _expand() fails to insert unambiguous prefix
  2021-03-20 14:42     ` Lawrence Velázquez
  2021-03-20 18:22       ` Marlon Richert
@ 2021-03-20 20:54       ` Oliver Kiddle
  2021-03-21  9:49         ` Marlon Richert
  1 sibling, 1 reply; 7+ messages in thread
From: Oliver Kiddle @ 2021-03-20 20:54 UTC (permalink / raw)
  To: Marlon Richert, zsh-workers

Lawrence Velázquez wrote:
> Sorry, that was more to elicit a review from a dev, but you're welcome to expand the patch of course.

_expand takes a deliberate approach of setting
  compstate[insert]=menu

That forces menu completion and was a conscious design decision. This
tends to be applicable for completions that aren't taking an incomplete
prefix and adding characters but are transforming what was entered.

It is reasonable that you might prefer different behaviour here so I'm
not averse to enabling it via a style. A simple pattern like a plain *
makes for a rather contrived example. With a more realistic pattern,
the case is less clear. I've always used just the all-expansions tag
with _expand. I don't see why you'd want completion to turn a carefully
written pattern into two characters of common prefix.

The trouble is that this patch has needed to change _main_complete, so
it results in a change in the behaviour of any other completion function
that sets compstate[insert]. In quite a few cases, _pids for instance, I
don't think the change is an improvement.

The relevant part of _main_complete checks whether compstate[insert]
has been changed by the function and applies the values from styles and
defaults if not. The patch continues regardless of the compstate[insert]
change. In general, it is good if the user can override defaults but
this is changing the default behaviour.

The menu style does get looked up with the context
:completion::expand:::expansions but _expand setting compstate[insert]
overrides it. Ideally, the precedence order for preferences should be:

1 User style configured for a specific context (:completion:*:expand:*:expansions)
2 Specific default set explicitly by a completion function, _expand in this case
3 User style configured for the general context (menu style default tag in this case)
4 A general default

Your patch is throwing away (2) in a limited manner. The old code
gave (2) precedence over (1). Getting it right is not simple, though
- consider also the fact that the menu style is not really meant
to control the unambiguous aspect of compstate[insert]. But I think
we can do better while preserving existing behaviour.

For what it's worth, I'm also not keen on the patch leaving a block with
the wrong indentation level (apart from a final fi and preceding line).
It kept the patch short but makes it harder to follow the logic.

Oliver


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Bug + patch: _expand() fails to insert unambiguous prefix
  2021-03-20 20:54       ` Oliver Kiddle
@ 2021-03-21  9:49         ` Marlon Richert
  0 siblings, 0 replies; 7+ messages in thread
From: Marlon Richert @ 2021-03-21  9:49 UTC (permalink / raw)
  To: Oliver Kiddle; +Cc: zsh-workers

Forgot to CC this to the mailing list. I shouldn’t use email on my phone, apparently.

>>> On 20. Mar 2021, at 22.54, Oliver Kiddle <opk@zsh.org> wrote:
>> 
>> Thanks for your well-thought-out response. I think you have good 
> 
> Whoops! Replying on my phone, I accidentally touched the send button. :/
> 
> Anyway, what I meant to say was, I took your points to heart and spent some time trying to come up with a better solution, only to realize that I cannot find a way to make it work in a good way that takes your points into account _and_ I’m not really that invested in this feature in the first. It just seemed like a nice idea at the time that wouldn’t be too hard to implement. Turns out it has more consequences than I thought about and I don’t really need the feature myself.
> 
> So, long story short: I decided to abandon this patch and go work on something else. If anyone else wants to continue where I left off, feel free to do so.
> 
> Having said that, this exercise have shown me some other areas where _expand could be improved. I’ll submit a new patch for _expand soon (just not for this particular feature).


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-03-21  9:50 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-04 19:49 Bug + patch: _expand() fails to insert unambiguous prefix Marlon Richert
2021-03-20  1:50 ` Lawrence Velázquez
2021-03-20 11:20   ` Marlon Richert
2021-03-20 14:42     ` Lawrence Velázquez
2021-03-20 18:22       ` Marlon Richert
2021-03-20 20:54       ` Oliver Kiddle
2021-03-21  9:49         ` Marlon Richert

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).