* 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).