From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27652 invoked by alias); 13 Jun 2016 08:52:24 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 38674 Received: (qmail 27265 invoked from network); 13 Jun 2016 08:52:22 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00 autolearn=ham autolearn_force=no version=3.4.1 Date: Mon, 13 Jun 2016 08:52:19 +0000 From: Daniel Shahaf To: zsh-workers@zsh.org Subject: Re: [PATCH] add-zle-hook-widget Message-ID: <20160613085219.GA23148@tarsus.local2> References: <160612184453.ZM11316@torch.brasslantern.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <160612184453.ZM11316@torch.brasslantern.com> User-Agent: Mutt/1.5.23 (2014-03-12) 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