Re: [PATCH 2/3] Make dynamic dir completion easier to implement

Re: [PATCH 2/3] Make dynamic dir completion easier to implement

[Marlon Richert]

Thanks for the feedback! Below is a new patch.

---

This patch implements most of the logic needed to write a custom
completion function for dynamic directory names into
_dynamic_directory_name.  This includes:

- Default tag and group names
- Correct logic for inserting `]` as a suffix
- A completion tag loop

This way, these don't need to be implemented and maintained for each
such function individually.
---
 .../Zsh/Context/_dynamic_directory_name       | 30 +++++++++----
 Doc/Zsh/expn.yo                               | 44 +++++++------------
 Test/Y01completion.ztst                       | 11 +++++
 3 files changed, 50 insertions(+), 35 deletions(-)

diff --git a/Completion/Zsh/Context/_dynamic_directory_name b/Completion/Zsh/Context/_dynamic_directory_name
index f449c3b12..5e0d73a8d 100644
--- a/Completion/Zsh/Context/_dynamic_directory_name
+++ b/Completion/Zsh/Context/_dynamic_directory_name
@@ -1,15 +1,29 @@
 #autoload
+local -a dirfuncs=(
+    ${(k)functions[zsh_directory_name]}
+    $zsh_directory_name_functions
+)
+local descr='dynamically named directory'

-local func
-integer ret=1
+if (( $#dirfuncs )); then
+  local -a expl
+  local -i ret
+  local func suf tag=dynamically-named-directories

-if [[ -n $functions[zsh_directory_name] || \
-  ${+zsh_directory_name_functions} -ne 0 ]] ; then
-  [[ -n $functions[zsh_directory_name] ]] && zsh_directory_name c && ret=0
-  for func in $zsh_directory_name_functions; do
-    $func c && ret=0
+  [[ $ISUFFIX != \]* ]] &&
+      suf=-S]
+
+  _tags "$tag"
+  while _tags; do
+    while _next_label "$tag" expl "$descr" $suf; do
+      for func in $dirfuncs; do
+        $func c && ret=0
+      done
+    done
+    (( ret )) || break
   done
   return ret
+
 else
-  _message 'dynamic directory name: implemented as zsh_directory_name c'
+  _message "${descr}: implement as zsh_directory_name c"
 fi
diff --git a/Doc/Zsh/expn.yo b/Doc/Zsh/expn.yo
index 19f5909ea..6f86d0c54 100644
--- a/Doc/Zsh/expn.yo
+++ b/Doc/Zsh/expn.yo
@@ -2066,34 +2066,24 @@ tt(/home/pws/perforce).  In this simple case a static name for the
 directory would be just as effective.

 example(zsh_directory_name+LPAR()RPAR() {
-  emulate -L zsh
-  setopt extendedglob
+  emulate -L zsh -o extendedglob
   local -a match mbegin mend
-  if [[ $1 = d ]]; then
-    # turn the directory into a name
-    if [[ $2 = (#b)(/home/pws/perforce/)([^/]##)* ]]; then
-      typeset -ga reply
-      reply=(p:$match[2] $(( ${#match[1]} + ${#match[2]} )) )
-    else
-      return 1
-    fi
-  elif [[ $1 = n ]]; then
-    # turn the name into a directory
-    [[ $2 != (#b)p:(?*) ]] && return 1
-    typeset -ga reply
-    reply=(/home/pws/perforce/$match[1])
-  elif [[ $1 = c ]]; then
-    # complete names
-    local expl
-    local -a dirs
-    dirs=(/home/pws/perforce/*(/:t))
-    dirs=(p:${^dirs})
-    _wanted dynamic-dirs expl 'dynamic directory' compadd -S\] -a dirs
-    return
-  else
-    return 1
-  fi
-  return 0
+  local base=/home/pws/perforce
+  case $1 in
+  ( d )  # Turn the directory into a name.
+    [[ $2 == (#b)($base/)([^/]##)* ]] &&
+        reply=( p:$match[2] $(( $#match[1] + $#match[2] )) )
+  ;;
+  ( n )  # Turn the name into a directory.
+    [[ $2 == (#b)p:(?*) ]] &&
+        reply=( $base/$match[1] )
+  ;;
+  ( c )  # Complete names.
+    local -a dirs=( $base/*(/:t) )
+    # Completion system populates $expl with flags for compadd.
+    compadd "$expl[@]" p:$^dirs
+  ;;
+  esac
 })

 texinode(Static named directories)(`=' expansion)(Dynamic named directories)(Filename Expansion)
diff --git a/Test/Y01completion.ztst b/Test/Y01completion.ztst
index 51e246307..2524c43bd 100644
--- a/Test/Y01completion.ztst
+++ b/Test/Y01completion.ztst
@@ -93,6 +93,17 @@
 >line: {: ~user2}{}
 >line: {: ~user1}{}

+  comptesteval 'zsh_directory_name() { compadd "$expl[@]" -- name1 name2 }'
+  comptest $': ~[\t\t\t\t'
+0:dynamic directory names after ~[
+>line: {: ~[name}{}
+>line: {: ~[name}{}
+>DESCRIPTION:{dynamically named directory}
+>NO:{name1}
+>NO:{name2}
+>line: {: ~[name1]}{}
+>line: {: ~[name2]}{}
+
   comptest $'echo ;:\C-b\C-b\t'
 0:directories and files before separator
 >line: {echo }{;:}
--
2.39.2 (Apple Git-143)

Re: [PATCH 2/3] Make dynamic dir completion easier to implement

[Oliver Kiddle]

Sorry for not reviewing this sooner. Thanks for following up to Aaron's
reply with a bit more detail as that does help.

On 5 May, Marlon Richert wrote:
>
> In the pre-patch example, the recommended way of implementing this was
> as follows:
>   _wanted dynamic-dirs expl 'dynamic directory' compadd -S\] ...
>
> However, this is problematic for the following reasons:
> - The proper place for handling _wanted or other tag logic is in the
>   completion function _dynamic_directory_name, not in each dynamic dir
>   name function separately.

That's questionable. Adding a tag loop outside as in your patch probably
makes it easier for someone to setup dynamic directories. It does
simplify the example. And it doesn't stop people using their own tag
loop - that can be necessary where the dynamic names are broken down
into components for the purposes of completion. So I've no objection
to the change. There are aspects of having nested tag loops that isn't
ideal but it's better than no tag loop.

> - The naming styles of the tag and group are not consistent with those
>   used for non-dynamic named directories, namely 'named-directories' and
>   'named directory', respectively.

That's specific to the example in the documentation. For many real
cases, the text used for dynamic directories can be more usefully
described in some other way. Such as 'recent directory index' in the
cdr example.

> - The example always inserted a `]` suffix, even if the suffix was
>   already present on the command line, leading to a double `]]` on the
>   command line.

Yes, that's not good. And passing the suffix to the function as a
parameter is a good approach to take.

> -local func
> -integer ret=1
> +if (( $#dirfuncs )); then
> +  local -a expl=()
> +  local -i ret=1
> +  local func=

There's no need to explicitly assign empty values. The function isn't
even relying on the initial value and local does reset the value to be
empty anyway.

> +  local tag=dynamically-named-directories
> +  [[ -z $ISUFFIX ]] &&
> +      local suf=-S]

This will only declare the variable suf local if the condition evaluates
to true. If it is false, we could be picking an unwanted value and
pass it to compadd.

Aside from that, I don't think the condition is correct because the user
could be completing in the middle of a word. zsh_directory_name_cdr uses

  [[ $ISUFFIX = *\]* ]] || addsuffix='-S]'

Within _dynamic_directory_name, I think it should be just
  [[ $ISUFFIX = \]* ]]

> -if [[ -n $functions[zsh_directory_name] || \
> -  ${+zsh_directory_name_functions} -ne 0 ]] ; then
> -  [[ -n $functions[zsh_directory_name] ]] && zsh_directory_name c && ret=0
> -  for func in $zsh_directory_name_functions; do
> -    $func c && ret=0
> +  _tags "$tag"
> +  while _tags; do
> +    while _next_label "$tag" expl "$descr"; do
> +      expl+=( $suf )

You can simply add $suf to the end of the call to _next_label and it
will add extra options to $expl:

  while _next_label "$tag" expl "$descr" $suf; do

The cdr functions that Bart mentioned need their own descriptions but at
least zsh_directory_name_cdr could be tweaked to take advantage of the
suffix now being passed to it.

I don't see this breaking any existing dynamic directory functions which
was my initial worry before looking at the changes more closely.

Oliver

Re: [PATCH 2/3] Make dynamic dir completion easier to implement

[Bart Schaefer]

On Sat, May 6, 2023 at 10:28 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> Not necessarily asking Marlon to do this, but if this patch is going
> to be included we need to repair these two functions:
>
> Functions/Chpwd/zsh_directory_name_cdr
> Functions/Chpwd/zsh_directory_name_generic

Attempting to extrapolate from Marlon's edits to the doc example, I
think the only part of these that really needs to change is the "if
completion" branch.  However, I'm not a user of the "cdr" functions so
I'm not sure what to do with the former, or if the "Can't complete in
the middle" warning still applies in the latter (and if it does, is
that a problem with Marlon's patch?); nor if the explanatory text
passed to _describe in either function has to move somewhere.

Re: [PATCH 2/3] Make dynamic dir completion easier to implement

[Bart Schaefer]

Not necessarily asking Marlon to do this, but if this patch is going
to be included we need to repair these two functions:

Functions/Chpwd/zsh_directory_name_cdr
Functions/Chpwd/zsh_directory_name_generic

[PATCH 2/3] Make dynamic dir completion easier to implement

[Marlon Richert]

From: Marlon Richert <marlon.richert@gmail.com>

In the pre-patch example, the recommended way of implementing this was
as follows:
  _wanted dynamic-dirs expl 'dynamic directory' compadd -S\] ...

However, this is problematic for the following reasons:
- The proper place for handling _wanted or other tag logic is in the
  completion function _dynamic_directory_name, not in each dynamic dir
  name function separately.
- The naming styles of the tag and group are not consistent with those
  used for non-dynamic named directories, namely 'named-directories' and
  'named directory', respectively.
- The example always inserted a `]` suffix, even if the suffix was
  already present on the command line, leading to a double `]]` on the
  command line.
- Code addressing the points the above needed to be duplicated in each
  dynamic dir name function.

This patch fixes these problems by
- moving the completion logic from the example into
  _dynamic_directory_name, so that users don't have to duplicated this
  scaffolding for each dynamic dir name function, and
- adds logic to _dynamic_directory_name for inserting the `]` suffix
  only when needed.
---
 .../Zsh/Context/_dynamic_directory_name       | 30 +++++++++----
 Doc/Zsh/expn.yo                               | 44 +++++++------------
 Test/Y01completion.ztst                       | 11 +++++
 3 files changed, 50 insertions(+), 35 deletions(-)

diff --git a/Completion/Zsh/Context/_dynamic_directory_name b/Completion/Zsh/Context/_dynamic_directory_name
index f449c3b12..e173dcb7f 100644
--- a/Completion/Zsh/Context/_dynamic_directory_name
+++ b/Completion/Zsh/Context/_dynamic_directory_name
@@ -1,15 +1,29 @@
 #autoload
+local -a dirfuncs=(
+   ${(k)functions[zsh_directory_name]}
+   $zsh_directory_name_functions
+)
+local descr='dynamically named directory'

-local func
-integer ret=1
+if (( $#dirfuncs )); then
+  local -a expl=()
+  local -i ret=1
+  local func=
+  local tag=dynamically-named-directories
+  [[ -z $ISUFFIX ]] &&
+      local suf=-S]

-if [[ -n $functions[zsh_directory_name] || \
-  ${+zsh_directory_name_functions} -ne 0 ]] ; then
-  [[ -n $functions[zsh_directory_name] ]] && zsh_directory_name c && ret=0
-  for func in $zsh_directory_name_functions; do
-    $func c && ret=0
+  _tags "$tag"
+  while _tags; do
+    while _next_label "$tag" expl "$descr"; do
+      expl+=( $suf )
+      for func in $dirfuncs; do
+        $func c && ret=0
+      done
+    done
+    (( ret )) || break
   done
   return ret
 else
-  _message 'dynamic directory name: implemented as zsh_directory_name c'
+  _message "${descr}: implement as zsh_directory_name c"
 fi
diff --git a/Doc/Zsh/expn.yo b/Doc/Zsh/expn.yo
index 19f5909ea..6f86d0c54 100644
--- a/Doc/Zsh/expn.yo
+++ b/Doc/Zsh/expn.yo
@@ -2066,34 +2066,24 @@ tt(/home/pws/perforce).  In this simple case a static name for the
 directory would be just as effective.

 example(zsh_directory_name+LPAR()RPAR() {
-  emulate -L zsh
-  setopt extendedglob
+  emulate -L zsh -o extendedglob
   local -a match mbegin mend
-  if [[ $1 = d ]]; then
-    # turn the directory into a name
-    if [[ $2 = (#b)(/home/pws/perforce/)([^/]##)* ]]; then
-      typeset -ga reply
-      reply=(p:$match[2] $(( ${#match[1]} + ${#match[2]} )) )
-    else
-      return 1
-    fi
-  elif [[ $1 = n ]]; then
-    # turn the name into a directory
-    [[ $2 != (#b)p:(?*) ]] && return 1
-    typeset -ga reply
-    reply=(/home/pws/perforce/$match[1])
-  elif [[ $1 = c ]]; then
-    # complete names
-    local expl
-    local -a dirs
-    dirs=(/home/pws/perforce/*(/:t))
-    dirs=(p:${^dirs})
-    _wanted dynamic-dirs expl 'dynamic directory' compadd -S\] -a dirs
-    return
-  else
-    return 1
-  fi
-  return 0
+  local base=/home/pws/perforce
+  case $1 in
+  ( d )  # Turn the directory into a name.
+    [[ $2 == (#b)($base/)([^/]##)* ]] &&
+        reply=( p:$match[2] $(( $#match[1] + $#match[2] )) )
+  ;;
+  ( n )  # Turn the name into a directory.
+    [[ $2 == (#b)p:(?*) ]] &&
+        reply=( $base/$match[1] )
+  ;;
+  ( c )  # Complete names.
+    local -a dirs=( $base/*(/:t) )
+    # Completion system populates $expl with flags for compadd.
+    compadd "$expl[@]" p:$^dirs
+  ;;
+  esac
 })

 texinode(Static named directories)(`=' expansion)(Dynamic named directories)(Filename Expansion)
diff --git a/Test/Y01completion.ztst b/Test/Y01completion.ztst
index 51e246307..2524c43bd 100644
--- a/Test/Y01completion.ztst
+++ b/Test/Y01completion.ztst
@@ -93,6 +93,17 @@
 >line: {: ~user2}{}
 >line: {: ~user1}{}

+  comptesteval 'zsh_directory_name() { compadd "$expl[@]" -- name1 name2 }'
+  comptest $': ~[\t\t\t\t'
+0:dynamic directory names after ~[
+>line: {: ~[name}{}
+>line: {: ~[name}{}
+>DESCRIPTION:{dynamically named directory}
+>NO:{name1}
+>NO:{name2}
+>line: {: ~[name1]}{}
+>line: {: ~[name2]}{}
+
   comptest $'echo ;:\C-b\C-b\t'
 0:directories and files before separator
 >line: {echo }{;:}
--
2.39.2 (Apple Git-143)