Re: [PATCH] add-zle-hook-widget

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Bart Schaefer wrote on Sun, Jun 12, 2016 at 18:44:53 -0700:
> I have not made much attempt to make this backward-compatible with
> past zsh versions, but I think the only conflict is with the
> arraname+=(value list) appending syntax.

You also use the reserved word variant of 'local -a'.

> +++ b/Doc/Zsh/contrib.yo
> @@ -323,6 +326,55 @@ The options tt(-U), tt(-z) and tt(-k) are passed as arguments to
>  tt(autoload) for var(function).  For functions contributed with zsh, the
>  options tt(-Uz) are appropriate.
>  )
> +findex(add-zle-hook-widget)
> +item(tt(add-zle-hook-widget) [ tt(-L) | tt(-dD) ] [ tt(-Uzk) ] var(hook) var(widgetname))(
> +
> +The arrays of var(widgetname) are maintained in several tt(zstyle)
> +contexts, one for each var(hook) context, with a style of `tt(widgets)'.
> +If the tt(-L) option is given, this set of styles is listed with
> +`tt(zstyle -L)'.  These styles may be updated directly with tt(zstyle)
> +commands, but the special widgets that refer to the styles are created
> +only if tt(add-zle-hook-widget) is called to add at least one widget.

I don't see why we should document the use of zstyles as an API, as
opposed to keeping it an implementation detail.  (And then -L could
return the list of registered widgets in $reply.)

> +In addition, var(widgetname) may be of the form var(index)tt(:)var(name).
> +In this case var(index) is an integer which determines the order in
> +which the widget var(name) will be called relative to other widgets in
> +the array.  Widgets having the same var(index) are called in unspecified
> +order, and all widgets declared with an index are called before any
> +widgets that have no index.

I'm not sure I like the bit described in the last sentence: it means
there is no way for widget B to force itself to be called after widget A
if widget A does not specify a var(index).  Instead, we could either
require an index to be specified or make a missing index be equivalent
to giving index == 500 (or any other well-known value, e.g., have "no
index" interpreted as "index == 0" and allow negative indices).

> --- /dev/null
> +++ b/Functions/Zle/add-zle-hook-widget
> @@ -0,0 +1,140 @@
> +# Add to HOOK the given WIDGET
> +# 
> +# HOOK is one of isearch-exit, isearch-update, line-pre-redraw, line-init,
> +# line-finish, history-line-set, keymap-select (the zle- prefix is not
> +# required).
> +#
> +# WIDGET may be of the form INDEX:NAME in which case the INDEX determines
> +# the order in which the widget executes relative to other hook widgets.
> +# 
> +# With -d, remove the widget from the hook instead; delete the hook
> +# variable if it is empty.
> +#
> +# -D behaves like -d, but pattern characters are active in the
> +# widget name, so any matching widget will be deleted from the hook.
> +#
> +# Without -d, if the WIDGET is not already defined, a function having the
> +# same name is marked for autoload; -U is passed down to autoload if that
> +# is given, as are -z and -k.  (This is harmless if the function is
> +# already defined.)  The WIDGET is then created with zle -N.

Any particular reason to repeat the docs twice, both here and in the
manual?  As opposed to just leaving a pointer.

> +# Setup - create the base functions for hook widgets that call the others
> +
> +zmodload zsh/parameter || {
> +    print -u2 "Need parameter module for zle hooks"
> +    return 1
> +}
> +

Should this dependency be documented?

There's also a dependency on zsh/zleparameter (${widgets} is accessed),
perhaps that module's presence should be checked as well?

(For those following along at home: it should be reasonably straightforward
to remove either of these dependencies, if their requirement gets in
someone's way...)

> +local -a hooktypes=( isearch-exit isearch-update
> +                     line-pre-redraw line-init line-finish
> +                     history-line-set keymap-select )
> +# Stash in zstyle to make it global
> +zstyle zle-hook types $hooktypes
> +

Bikeshed, but I'd have used the names with "zle-" prefixes here, for two
reasons:

1) To make it easier for people who grep for uses of the zle-foo widget
to find this callsite.

2) To make it easier to extend this facility to non-special widgets in
5.4, should we want to do so.  (z-sy-h will no longer need to wrap
individual widgets in 5.3, but other plugins might still wish to do
exactly that.)

For the same reason (5.4 compatibility), I would suggest using some
prefix on the zstyle names, e.g., «zstyle :foobar:zle-isearch-update …»
instead of the current bare «zstyle zle-isearch-update …».  Having
a prefix won't break anything but might avoid a problem down the road.

> +for hook in $hooktypes
> +do
> +  function zle-$hook {
> +      for hook in "${@${(@on)hook_widgets}#*:}"

This will break on function name such as
"mynamespace:myfunctionname() {}".  Suggest ${…#<->:} instead.

This parameter expansion causes the following warning on every prompt:

[[[
$ zsh -f
% fpath+=($PWD)
% zle-line-init() {}
% zle -N zle-line-init
% autoload -Uz add-zle-hook-widget
% add-zle-hook-widget
Usage: add-zle-hook-widget hook widgetname
Valid hooks are:
  isearch-exit isearch-update line-pre-redraw line-init line-finish history-line-set keymap-select
%
zsh:8: bad substitution
%
]]]

(Side note: why does the errorr say "zsh" instead of "zle" or the widget's or function's name?)

What happens when a zle-line-init already exists when add-zle-hook-widget
is first called?  I had such a hook and it was simply discarded.  Should
add-z-h-w warn that it is overwriting/redefining an existing hook?

> +      do
> +	zle "$hook" -Nw || return

The indentation in the email is wrong.

What happens if $hook starts with a minus?

Shouldn't argv be passed down to the hook?  In case some future special
widget gets passed argv from zle.

Why pass -N?  If we ever define a special widget that takes a NUMERIC,
surely widgets registered through add-z-h-w should have access to the
argument's value?

So, all in all, how about:
.
    zle -- "$hook" -w -- "$@"
.
?

> +# Redefine ourself with the setup left out
> +
> +function add-zle-hook-widget {
> +    local usage="Usage: $0 hook widgetname\nValid hooks are:\n  $hooktypes"

If the value of $0 or $hooktypes contained literal backslashes, they
wouldn't be printed correctly.  I know the current values are safe, but
I still think it's worth fixing.  (Code evolves; someone might add another
parameter expansion to the string literal someday without realizing the
problem.)

> +    local opt
> +    local -a autoopts
> +    integer del list help
> +
> +    while getopts "dDhLUzk" opt; do
> +	case $opt in
> +	    (d)
> +	    del=1
> +	    ;;
> +
> +	    (*)

echo >&2 "add-zle-hook-widget: unknown option ${(qq)opt}"

> +	    return 1
> +	    ;;
> +	esac
> +    done
> +    shift $(( OPTIND - 1 ))
> +
> +    if (( list )); then
> +	zstyle -L "zle-(${1:-${(@j:|:)hooktypes}})" widgets
> +	return $?
> +    elif (( help || $# != 2 || ${hooktypes[(I)${1#zle-}]} == 0 )); then
> +	print -u$(( 2 - help )) $usage
> +	return $(( 1 - help ))
> +    fi
> +
> +    local -aU extant_hooks
> +    local hook="zle-${1#zle-}"

A few lines above (in the 'zstyle -L' call) you use $1 without
idempotently adding/stripping the "zle-" prefix.  It'd probably be
easier to do «1=zle-${1#zle-}» or «1=${1#zle-}» once at the very top of
the function, than to remember to account for both possibilities at
every reference to $1 throughout the function.

> +    local fn="$2"
> +
> +    if (( del )); then
> +        # delete, if hook is set
> +    else
> +	zstyle -g extant_hooks "$hook" widgets
> +	extant_hooks+=("$fn")
> +	zstyle -- "$hook" widgets "${extant_hooks[@]}"
> +	if [[ -z "${widgets[$fn]}" ]]; then
> +	    autoload "${autoopts[@]}" -- "$fn"
> +	    zle -N "$fn"

If the 'autoload' needs that '--' guard, then 'zle -N' needs it too.

> +	fi

Thanks for this!  I'll test it further and teach z-sy-h about it when
I have a minute (which may not be for a few days).

Cheers,

Daniel