* 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