Re: PATCH: list units in brackets at the end of completion group descriptions

[Oliver Kiddle <opk@xxxxxxx>]

On 27 Aug, Daniel wrote:
> Oliver Kiddle wrote on Fri, 27 Aug 2021 11:52 +00:00:
> > There might be some more useful way we might indicate where suffixes can
> > be appended to a value to denote units. Completing them after a number
> > doesn't really make the options clear before a number has been entered.
> > Perhaps an initial slash, e.g. 'timeout (seconds) [5] /ms /us /ns'

> This design first says "Input a value in seconds", then the default
> value, then goes back to say that other units are possible.

On further relection, for handling unit suffixes, we ideally also want
to complete them. So having to specify them in the description would
only lead to duplication.

Units are nearly always only applicable on the end of numbers. So how
about we add _numbers as a helper function to take care of completing
unit suffixes. This also side-steps Daniel's concern about locality - at
least if we don't display the units in the description in the default
format.

(I've always been somewhat unhappy about the existence of
__git_guard_number - there's nothing git specific about it and the guard
behaviour is questionable. I still wouldn't bother adding calls to
_numbers for plain values where it doesn't add anything.)

The attached patch shows a possible approach. It includes _numbers and a
patch to _description to parse the description for units, range and a
default. That's turning what has hitherto only been a convention into an
explicit part of the _descriptions interface. Should we worry about that
and the effect it might have on existing descriptions. The patterns can
perhaps be tightened up if they pick up things that aren't meant to be
units etc. I can't think of an alternative way to add the functionality
if we want it applied throughout and not just sporadically. With this
patch the parsing can be disabled by passing an extra zformat spec
(H:0 may be a good choice).

_description has always had the ability to accept extra zformat
specifications. It isn't used as often as was perhaps originally
envisaged (just %o with expansions). I've used %m for units (measurement
- u is taken for underline), %r for range (e.g 1-30), %o for default
(otherwise - d is taken), %x for the list of suffixes (s is taken) and
%h for the unadorned description (heading). For backward compatibility
%d is contructed to look as before. I'm open to better suggestions on
these if someone has better ideas?

Corresponding to these are uppercase forms for use with the zformat
conditional. This allows, e.g. %(M. %F{28}(%m%)%f.) for adding the units
conditionally. Perhaps we should change zformat so %(m. .) can be used
directly but that would break compatibility.

_numbers also constructs the list of suffixes using zformat; looked up
using unit-suffixes as the tag. (A name that wouldn't clash is needed
because _description looks up the format style first under the tag it is
passed before checking with the tag "descriptions".) In this case %x is
used for an individual suffix, %X the description, %i and %r for index
and reverse index and %d for a default. The results then being
concatenated. So, e.g. for blank space before the first, underline for a
default and vertical bars after all but the last it would need the
following style:

  zstyle ':completion:*:unit-suffixes' format \
      '%(i:       :)%(d.%U.)%x%(d.%u.)%(r..|)'

This _numbers function takes quite a few options. Should we allow a way
to configure how a range is presented - this hardcodes a dash separating
min and max and has no way to indicate for decimal numbers whether the
min and max values are inclusive?

The patterns don't handle exponents, hex or an initial unary + operator
but would be easy to add if there's a need.

In some existing completions we fully expand the numbers as matches
with, e.g. {1..32}. Perhaps a style can enable this to a specified
limit. There are also cases where a specific set of numbers are
possible, e.g. baud-rates but I've not bothered to consider that case
because they typically wouldn't be followed by unit suffixes.

Does anyone have any other thoughts or ideas on this?

Oliver

diff --git a/Completion/BSD/Command/_ipfw b/Completion/BSD/Command/_ipfw
index 2354a70fe..49d0ef1e8 100644
--- a/Completion/BSD/Command/_ipfw
+++ b/Completion/BSD/Command/_ipfw
@@ -249,8 +249,8 @@ actions=(
     $'/(pipe|queue|sched)[ \t\0]/' -'pqs=${match%?}' ':dummynet-commands:dummynet configuration:$ca pipe queue sched'
     $word ': _message -e numbers number'
     $word ':options:config:$ca config'
-    \( $'/bw[ \t\0]/' \( $'/<->/' ': _guard "[0-9]#" bandwidth'
-         $word ':units:unit:compadd -M "m:{a-z}={A-Z}" {K,M,G}{bit,Byte}/s'
+    \( $'/bw[ \t\0]/'
+      \( $word ':bandwidths: :_numbers -M "m:{a-z}={A-Z}" bandwidth {K,M,G}{bit,Byte}/s'
       \| $word ':devices:device:_net_interfaces -qS " "' \)
     \| $'/delay[ \t\0]/' $word ': _message -e numbers "propagation delay (ms)"'
     \| $'/burst[ \t\0]/' $word ': _message -e numbers "size (bytes)"'
diff --git a/Completion/Base/Core/_description b/Completion/Base/Core/_description
index bdb4007a6..48d9f6e55 100644
--- a/Completion/Base/Core/_description
+++ b/Completion/Base/Core/_description
@@ -2,6 +2,7 @@
 
 local name nopt xopt format gname hidden hide match opts tag
 local -a ign gropt sort
+local -a match mbegin mend
 
 opts=()
 
@@ -78,6 +79,13 @@ shift 2
 if [[ -z "$1" && $# -eq 1 ]]; then
   format=
 elif [[ -n "$format" ]]; then
+  if [[ -z $2 ]]; then
+    argv+=( h:${1%%( ##\((#b)([^\)]#[^0-9-][^\)]#)(#B)\)|)( ##\((#b)([0-9-]##)(#B)\)|)( ##\[(#b)([^\]]##)(#B)\]|)} H:1 )
+    [[ -n $match[1] ]] && argv+=( m:$match[1] M:1 )
+    [[ -n $match[2] ]] && argv+=( r:$match[2] R:1 )
+    [[ -n $match[3] ]] && argv+=( o:$match[3] O:1 )
+  fi
+
   zformat -f format "$format" "d:$1" "${(@)argv[2,-1]}"
 fi
 
diff --git a/Completion/Base/Utility/_numbers b/Completion/Base/Utility/_numbers
new file mode 100644
index 000000000..312d0a7a0
--- /dev/null
+++ b/Completion/Base/Utility/_numbers
@@ -0,0 +1,78 @@
+#autoload
+
+# Usage: _numbers [compadd options] [-t tag] [-f|-N] [-u units] [-l min] [-m max] \
+#                 [-d default] ["description"] [unit-suffix...]
+
+#   -t : specify a tag (defaults to 'numbers')
+#   -u : indicate the units, e.g. seconds
+#   -l : lowest possible value
+#   -m : maximum possible value
+#   -d : default value
+#   -N : allow negative numbers (implied by range including a negative)
+#   -f : allow decimals (float)
+
+# For a unit-suffix, an initial colon indicates a unit that asserts the default
+# otherwise, colons allow for descriptions, e.g:
+
+#   :s:seconds m:minutes h:hours
+
+# unit-suffixes are not sorted by the completion system when listed
+# Specify them in order of magnitude, this tends to be ascending unless
+# the default is of a higher magnitude, in which case, descending.
+# So for, example
+#   bytes kB MB GB
+#   s ms us ns
+# Where the compadd options include matching control or suffixes, these
+# are applied to the units
+
+# For each unit-suffix, the format style is looked up with the
+# unit-suffixes tag and the results concatenated.
+
+# _description is called with u and U tokens set
+
+local desc tag range suffixes suffix suffixfmt pat='<->' partial=''
+local -a expl formats
+local -a default max min keep tags units
+local -i i
+local -A opts
+
+zparseopts -K -D -A opts M+:=keep q:=keep s+:=keep S+:=keep J+: V+: 1 2 o+: n F: x+: X+: \
+  t:=tags u:=units l:=min m:=max d:=default f=type e=type N=type
+
+desc="${1:-number}" tag="${tags[2]:-numbers}"
+(( $# )) && shift
+
+[[ -n ${(M)type:#-f} ]] && pat='(<->.[0-9]#|[0-9]#.<->|<->)' partial='(|.)'
+[[ -n ${(M)type:#-N} || $min[2] = -* || $max[2] = -* ]] && \
+    pat="(|-)$pat" partial="(|-)$partial"
+
+if (( $#argv )) && compset -P "$pat"; then
+  zstyle -s ":completion:${curcontext}:units" list-separator sep || sep=--
+  _description -V units expl unit
+  disp=( ${${argv#:}/:/ $sep } )
+  compadd -M 'r:|/=* r:|=*' -d disp "$keep[@]" "$expl[@]" - ${${argv#:}%%:*}
+  return
+elif [[ -prefix $~pat || $PREFIX = $~partial ]]; then
+  formats=( "h:$desc" H:1 )
+  (( $#units )) && formats+=( m:${units[2]} M:${#units} ) desc+=" ($units[2])"
+  (( $#min )) && range="$min[2]-"
+  (( $#max )) && range="${range:--}$max[2]"
+  [[ -n $range ]] && formats+=( r:$range R:1 ) desc+=" ($range)"
+  (( $#default )) && formats+=( o:${default[2]} O:1 ) desc+=" [$default[2]]"
+
+  zstyle -s ":completion:${curcontext}:unit-suffixes" format suffixfmt
+  for ((i=0;i<$#;i++)); do
+    zformat -f suffix "$suffixfmt" "x:${${argv[i+1]#:}%%:*}" \
+        "X:${${argv[i+1]#:}#*:}" "d:${#${argv[i+1]}[1]#:}" \
+	i:i r:$(( $# - i - 1))
+    suffixes+="$suffix"
+  done
+  [[ -n $suffixes ]] && formats+=( x:$suffixes X:1 )
+
+  _comp_mesg=yes
+  _description -x $tag expl "$desc" $formats
+  compadd "$expl[@]"
+  return 0
+fi
+
+return 1
diff --git a/Completion/Unix/Command/_killall b/Completion/Unix/Command/_killall
index 36accb2e0..3ddd36341 100644
--- a/Completion/Unix/Command/_killall
+++ b/Completion/Unix/Command/_killall
@@ -38,15 +38,9 @@ if _pick_variant psmisc=PSmisc unix --version; then
 
   case $state in
     (time)
-      local -a units=( 's:seconds' 'm:minutes' 'h:hours' 'd:days'
-			'w:weeks' 'M:months' 'y:years' )
-      if compset -P '[0-9]##(|.[0-9]#)'; then
-	_alternative 'float-numbers:: _message "float number"' \
-		    'units:: _describe unit units' && ret=0
-      else
-	_message 'float number and unit' && ret=0
-      fi
-      ;;
+      _numbers -fN age 's:seconds' 'm:minutes' 'h:hours' 'd:days' \
+          'w:weeks' 'M:months' 'y:years'
+    ;;
   esac
 
   return ret
diff --git a/Completion/Unix/Command/_pv b/Completion/Unix/Command/_pv
index 68f8e8586..d02d3a35d 100644
--- a/Completion/Unix/Command/_pv
+++ b/Completion/Unix/Command/_pv
@@ -25,7 +25,7 @@ _arguments -s -S $args \
   '(-q --quiet)'{-q,--quiet}"[don't output any transfer information at all, useful with -L]" \
   '(-W --wait)'{-W,--wait}'[display nothing until first byte transferred]' \
   '(-D --delay-start -R --remote)'{-D+,--delay-start=}'[display nothing until delay has passed]:delay (seconds)' \
-  '(-s --size)'{-s+,--size=}'[set estimated data size]:size (bytes):->size-unit' \
+  '(-s --size)'{-s+,--size=}'[set estimated data size]: :_numbers -u bytes size K M G T' \
   '(-l --line-mode -R --remote)'{-l,--line-mode}'[count lines instead of bytes]' \
   '(-0 --null -l --line-mode)'{-0,--null}'[lines are null-terminated]' \
   '(-i --interval)'{-i+,--interval=}'[update every after specified interval]:interval (seconds) [1]' \
@@ -34,8 +34,8 @@ _arguments -s -S $args \
   '(-N --name)'{-N+,--name=}'[prefix visual information with given name]:name' \
   '(-f --force -R --remote)'{-f,--force}'[output even if standard error is not a terminal]' \
   '(-c --cursor -R --remote)'{-c,--cursor}'[use cursor positioning escape sequences]' \
-  '(-L --rate-limit)'{-L+,--rate-limit=}'[limit transfer rate]:rate (bytes per second):->size-unit' \
-  '(-B --buffer-size)'{-B+,--buffer-size=}'[use a buffer size of given size]:size (bytes):->size-unit' \
+  '(-L --rate-limit)'{-L+,--rate-limit=}'[limit transfer rate]: :_numbers -u "bytes per second" rate K M G T' \
+  '(-B --buffer-size)'{-B+,--buffer-size=}'[use a buffer size of given size]: :_numbers -u bytes size K M G T' \
   '(-C --no-splice)'{-C,--no-splice}'[never use splice(), always use read/write]' \
   '(-R --remote)*'{-E,--skip-errors}"[skip read errors in input${Edesc}]" \
   '(-S --stop-at-size -R --remote)'{-S,--stop-at-size}'[stop after --size bytes have been transferred]' \
@@ -70,18 +70,6 @@ case $state in
       _pids $suf && ret=0
     fi
   ;;
-  size-unit)
-    if compset -P '<->'; then
-      _tags values units
-    else
-      _tags values
-    fi
-    while _tags; do
-      _requested values && _message -e values "$state_descr" && ret=0
-      _requested units expl unit compadd -o nosort - K M G T && ret=0
-      (( ret )) || break
-    done
-  ;;
 esac
 
 return ret
diff --git a/Completion/Unix/Command/_rclone b/Completion/Unix/Command/_rclone
index 27b4dd926..a2e3429f5 100644
--- a/Completion/Unix/Command/_rclone
+++ b/Completion/Unix/Command/_rclone
@@ -62,7 +62,7 @@ _arguments -C \
   '--backup-dir[make backups into hierarchy based at specified directory]:directory:_directories' \
   '--bind[specify socal address to bind to for outgoing connections]:IPv4, IPv6 or name' \
   '--buffer-size[specify in memory buffer size when reading files for each --transfer]:size [16M]' \
-  '--bwlimit[specify bandwidth limit]:BwTimetable (kBytes/s or b|k|M|G suffix)' \
+  '--bwlimit[specify bandwidth limit]: :_numbers -u kBytes/s limit b k M G' \
   '--cache-dir[specify directory rclone will use for caching]:directory [~/.cache/rclone]:_directories' \
   '--checkers[specify number of checkers to run in parallel]:number [8]': \
   '(-c --checksum)'{-c,--checksum}'[skip based on checksum & size, not mod-time & size]' \
@@ -99,15 +99,15 @@ _arguments -C \
   '--log-format[specify comma separated list of log format options]:string ["date,time"]' \
   '--log-level[specify log level]:string [NOTICE]:(DEBUG INFO NOTICE ERROR)'  \
   '--low-level-retries[number of low level retries to do]:int [10]' \
-  '--max-age[only transfer files younger than this in s or suffix ms|s|m|h|d|w|M|y]:duration [default off]' \
+  '--max-age[only transfer files younger than specified age]: :_numbers -u seconds age ms\:milliseconds \:s\:seconds m\:minutes h\:hours d\:days w\:weeks M\:months y\:years' \
   '--max-backlog[maximum number of objects in sync or check backlog]:int [10000]' \
   '--max-delete[when synchronizing, limit the number of deletes]:delete limit [-1]' \
   '--max-depth[limit the recursion depth]:depth [-1]' \
-  '--max-size[only transfer files smaller than this in k or suffix b|k|M|G]:int [default off]' \
+  '--max-size[only transfer files smaller than specified size]: :_numbers -u kBytes size \:k M G' \
   '--max-transfer[maximum size of data to transfer]:int [default off]' \
   '--memprofile[write memory profile to file]:file:_files' \
-  '--min-age[only transfer files older than this in s or suffix ms|s|m|h|d|w|M|y]:duration [default off]' \
-  '--min-size[only transfer files bigger than this in k or suffix b|k|M|G]:int [default off]' \
+  '--min-age[only transfer files older than specified age]: :_numbers -u seconds age ms\:milliseconds \:s\:seconds m\:minutes h\:hours d\:days w\:weeks M\:months y\:years' \
+  '--min-size[only transfer files bigger than specified size]: :_numbers -u kBytes size \:k M G' \
   '--modify-window[specify max time delta to be considered the same]:duration [1ns]' \
   '--multi-thread-cutoff[use multi-threaded downloads for files above specified size]:size (250M)' \
   '--multi-thread-streams[specify max number of streams to use for multi-threaded downloads]:number (4)' \
diff --git a/Completion/Unix/Command/_rsync b/Completion/Unix/Command/_rsync
index b1a4f6046..eb906e974 100644
--- a/Completion/Unix/Command/_rsync
+++ b/Completion/Unix/Command/_rsync
@@ -99,7 +99,7 @@ _rsync() {
   _arguments -s \
     '*'{-v,--verbose}'[increase verbosity]' \
     {--no-v,--no-verbose}'[turn off --verbose]' \
-    '--bwlimit=[limit I/O bandwidth]:limit (KiB per second)' \
+    '--bwlimit=[limit I/O bandwidth]: :_numbers -f -u "KiB per second" -d 1g limit B K M G T P' \
     '--outbuf=[set output buffering]:buffering:(none line block)' \
     '--port=[specify alternate port number]:port:(873)' \
     '--address=[bind to the specified address]:bind address:_bind_addresses' \
@@ -181,7 +181,7 @@ _rsync() {
     {--no-W,--no-whole-file}'[turn off --whole-file]' \
     '(--cc --checksum-choice)'{--cc,--checksum-choice}'=[choose the checksum algorithms]:algorithm:_sequence -n 2 compadd - auto md4 md5 none' \
     '(-x --one-file-system)'{-x,--one-file-system}"[don't cross filesystem boundaries]" \
-    '(-B --block-size)'{-B+,--block-size=}'[force a fixed checksum block-size]:block size (bytes)' \
+    '(-B --block-size)'{-B+,--block-size=}'[force a fixed checksum block-size]: :_numbers -f -u bytes -d 1g "block size" B K M G T P' \
     '(-e --rsh)'{-e+,--rsh=}'[specify the remote shell to use]:remote-shell command:(rsh ssh)' \
     '--rsync-path=[specify path to rsync on the remote machine]:remote command' \
     '--ignore-existing[ignore files that already exist on receiving side]' \
@@ -199,10 +199,10 @@ _rsync() {
     '--force-change[affect user-/system-immutable files/dirs]' \
     '--force-uchange[affect user-immutable files/dirs]' \
     '--force-schange[affect system-immutable files/dirs]' \
-    '--max-delete=[do not delete more than NUM files]:number' \
-    '--max-size=[do not transfer any file larger than specified size]:number' \
+    "--max-delete=[don't delete more than NUM files]: :_numbers -f -u bytes size B K M G T P" \
+    "--max-size=[don't transfer any file larger than specified size]: :_numbers -f -u bytes size B K M G T P" \
     '--min-size=[do not transfer any file smaller than specified size]:number' \
-    '--max-alloc=[set limit to individual memory allocation]:size (bytes) [1g]' \
+    '--max-alloc=[set limit to individual memory allocation]: :_numbers -f -u bytes -d 1g size B K M G T P' \
     '(-P)--partial[keep partially transferred files]' \
     '--no-partial[turn off --partial]' \
     '--partial-dir=[put a partially transferred file into specified directory]:directory:_directories' \
diff --git a/Completion/Unix/Command/_zfs b/Completion/Unix/Command/_zfs
index 452e1160d..a83ce12f4 100644
--- a/Completion/Unix/Command/_zfs
+++ b/Completion/Unix/Command/_zfs
@@ -238,8 +238,8 @@ _zfs() {
 			':filesystem:_zfs_dataset -t fs -e "parent dataset"' \
 			- set2 \
 			'-s[Create sparse volume]' \
-			'-b[Set volblocksize]:blocksize:' \
-			'-V[Set size]:size:' \
+			'-b+[set volblocksize]:block size:_numbers -M 'm:{a-zA-Z}={A-Za-z}' -u bytes {k,M,G,T,P,E,Z}{,B}' \
+			'-V+[set size]:size:_numbers -M 'm:{a-zA-Z}={A-Za-z}' -u bytes {k,M,G,T,P,E,Z}{,B}' \
 			':volume:_zfs_dataset -t fs -e "parent dataset"'
 		;;
 
diff --git a/Completion/X/Command/_xset b/Completion/X/Command/_xset
index b35a6466b..adda47f01 100644
--- a/Completion/X/Command/_xset
+++ b/Completion/X/Command/_xset
@@ -91,8 +91,8 @@ _regex_arguments _xset_parse \
     \( "/(blank|noblank|expose|noexpose|default|on|activate|reset)$nul/" \
        ':option-s:screen saver:(blank noblank expose noexpose default on activate reset off)' \
     \| "/off$nul/" \( "/off$nul/" ':option-s-off-period:period off:(off)' \| \) \
-    \| "/[0-9]##$nul/" ':option-s-timeout:length:' \
-      \( "/[0-9]##$nul/" ':option-s-period:period:' \
+    \| "/[0-9]##$nul/" ':option-s-timeout: :_numbers -u seconds length' \
+      \( "/[0-9]##$nul/" ':option-s-period: :_numbers -u seconds period' \
       \| \) \
     \| \) \
   \| "/-r$nul/" "$guard" \