[PATCH] add-zle-hook-widget

[PATCH] add-zle-hook-widget

[Bart Schaefer]

The idea and ultimate implementation are the same as in my "crude sketch"
but the function has been rewritten to parallel add-zsh-hook very closely.
This means among other things that you can only add one hook widget per
call to add-zle-hook-widget.

Also as briefly discussed, I removed add-zle-hook-function entirely and
made it a requirement that all the hooks be actual widgets.  However, as
a parallel to add-zsh-hook, the given name is both defined as a widget
and marked as an autoload function if the widget doesn't yet exist.

There are probably still bugs in there somewhere, but all worked as I
expected in a few simple tests.  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.

Patch includes repairs to the add-zsh-hook documentation.


diff --git a/Doc/Zsh/contrib.yo b/Doc/Zsh/contrib.yo
index 923bb29..dd643a1 100644
--- a/Doc/Zsh/contrib.yo
+++ b/Doc/Zsh/contrib.yo
@@ -292,11 +292,11 @@ cindex(hook function utility)
 
 startitem()
 findex(add-zsh-hook)
-item(tt(add-zsh-hook) [ tt(-dD) ] [ tt(-Uzk) ] var(hook) var(function))(
+item(tt(add-zsh-hook) [ tt(-L) | tt(-dD) ] [ tt(-Uzk) ] var(hook) var(function))(
 Several functions are special to the shell, as described in the section
 ifnzman(Special Functions, noderef(Functions))\
 ifzman(SPECIAL FUNCTIONS, see zmanref(zshmisc)),
-in that they are automatic called at a specific point during shell execution.
+in that they are automatically called at specific points during shell execution.
 Each has an associated array consisting of names of functions to be
 called at the same point; these are so-called `hook functions'.
 The shell function tt(add-zsh-hook) provides a simple way of adding or
@@ -312,6 +312,9 @@ var(function) is name of an ordinary shell function.  If no options
 are given this will be added to the array of functions to be executed
 in the given context.
 
+If the option tt(-L) is given, the current values for the hook arrays
+are listed with tt(typeset).
+
 If the option tt(-d) is given, the var(function) is removed from
 the array of functions to be executed.
 
@@ -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))(
+Several widget names are special to the line editor, as described in the section
+ifnzman(Special Widgets, noderef(Zle Widgets))\
+ifzman(Special Widgets, see zmanref(zshzle)),
+in that they are automatically called at specific points during editing.
+Unlike function hooks, these do not use a predefined array of other names
+to call at the same point; the shell function tt(add-zle-hook-widget)
+maintains a similar array and arranges for the special widget to invoke
+those additional widgets.
+
+var(hook) is one of tt(isearch-exit), tt(isearch-update),
+tt(line-pre-redraw), tt(line-init), tt(line-finish), tt(history-line-set),
+or tt(keymap-select), corresponding to each of the special widgets
+tt(zle-isearch-exit), etc.
+
+var(widgetname) is the name of a ZLE widget.  If no options are given this
+is added to the array of widgets to be invoked in the given hook context.
+Note that the hooks are called as widgets, that is, with
+`tt(zle var(widgetname) -Nw)' rather than as a function call.
+
+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.
+
+If the option tt(-d) is given, the var(widgename) is removed from
+the array of widgets to be executed.
+
+If the option tt(-D) is given, the var(widgetname) is treated as a pattern
+and any matching names of widgets are removed from the array.
+
+If var(widgetname) does not name an existing widget when added to the
+array, it is assumed that a shell function also named var(widgetname) is
+meant to provide the implementation of the widget.  This name is therefore
+marked for autoloading, and the options tt(-U), tt(-z) and tt(-k) are
+passed as arguments to tt(autoload) as with tt(add-zsh-hook).  The
+widget is also created with `tt(zle -N var(widgetname))' to cause the
+corresponding function to be loaded the first time the hook is called.
+
+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.
+)
 enditem()
 
 texinode(Recent Directories)(Other Directory Functions)(Utilities)(User Contributions)
diff --git a/Functions/Zle/add-zle-hook-widget b/Functions/Zle/add-zle-hook-widget
new file mode 100644
index 0000000..eeb0191
--- /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.
+
+emulate -L zsh
+
+# 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
+}
+
+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
+
+for hook in $hooktypes
+do
+  function zle-$hook {
+      local -a hook_widgets
+      local hook
+      # Values of these styles look like number:name
+      # and we run them in number order
+      # $funcstack is more reliable than $0
+      # Also, ksh_arrays is annoying
+      emulate zsh -c 'zstyle -a $funcstack[2] widgets hook_widgets'
+      for hook in "${@${(@on)hook_widgets}#*:}"
+      do
+	zle "$hook" -Nw || return
+      done
+      return 0
+  }
+done
+
+# Redefine ourself with the setup left out
+
+function add-zle-hook-widget {
+    local -a hooktypes
+    zstyle -a zle-hook types hooktypes
+
+    # This part copied from add-zsh-hook
+    local usage="Usage: $0 hook widgetname\nValid hooks are:\n  $hooktypes"
+
+    local opt
+    local -a autoopts
+    integer del list help
+
+    while getopts "dDhLUzk" opt; do
+	case $opt in
+	    (d)
+	    del=1
+	    ;;
+
+	    (D)
+	    del=2
+	    ;;
+
+	    (h)
+	    help=1
+	    ;;
+
+	    (L)
+	    list=1
+	    ;;
+
+	    ([Uzk])
+	    autoopts+=(-$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-}"
+    local fn="$2"
+
+    if (( del )); then
+        # delete, if hook is set
+	if zstyle -g extant_hooks "$hook" widgets; then
+	    if (( del == 2 )); then
+		set -A extant_hooks ${extant_hooks:#(<->:|)${~fn}}
+	    else
+		set -A extant_hooks ${extant_hooks:#(<->:|)$fn}
+	    fi
+            # unset if no remaining entries
+	    if (( ${#extant_hooks} )); then
+		zstyle "$hook" widgets "${extant_hooks[@]}"
+	    else
+		zstyle -d "$hook" widgets
+	    fi
+	fi
+    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"
+	fi
+	if [[ -z "${widgets[$hook]}" ]]; then
+	    zle -N "$hook"
+	fi
+    fi
+}
+
+# Handle zsh autoloading conventions
+if [[ "$zsh_eval_context" = *loadautofunc && ! -o kshautoload ]]; then
+    add-zle-hook-widget "$@"
+fi

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

[Daniel Shahaf]

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

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

[Bart Schaefer]

On Jun 13,  8:52am, Daniel Shahaf wrote:
} Subject: Re: [PATCH] add-zle-hook-widget
}
} Bart Schaefer wrote on Sun, Jun 12, 2016 at 18:44:53 -0700:
} > +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.)

Mostly I made that decision because add-zsh-hook explicitly declares that
its values are arrays and displays their values with "typeset" if the -L
option is passed.

Of course add-zsh-hook doesn't have much choice because the arrays are
builtin names documented elsewhere, but it seemed odd to expose this
detail in one case and hide it in the other.  If the zstyle detail were
not called out, readers would assume by analogy that there were array
variables they could be assigning.

} > +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).

The whole ordering thing depends on the cooperation of whoever declared
widgets A and B in the first place.  Declarer(A) could as easily make
capricious changes to his index as not provide it at all.  add-zsh-hook
doesn't support ANY ordering except by direct manipulation of the array
variables.

Yeah, I'm drawing a somewhat arbitrary line at "this is now complicated
enough, the rest is someone else's problem."

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

Because add-zsh-hook does, and so do assorted other Functions/*.

} > +# 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?

I probably needn't even have bothered with the test.

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

I debated that.  I don't want add-zle-hook-widget to fail if called from
an init file in e.g. a non-interactive shell where zle will never be
loaded, but I don't want to needlessly test for the module every time
the hooks are called, either.

} (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 )
} 
} Bikeshed, but I'd have used the names with "zle-" prefixes here

Again a debate.  add-zsh-hook doesn't require the "_functions" suffix.
And though not documented, it's written so that either

    add-zle-hook-widget isearch-exit ...
or
    add-zle-hook-widget zle-isearch-exit ...

are equivalent.

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

For some reason this argument fills me with a vast ennui that I struggle
to overcome just because I can't think why.

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

I'm strongly of the opinion that this is the WRONG way to manipulate a
non-special editor widget, and that the right way needs more help than
this kind of facility can provide, and that I'm not going to attempt
to explain all the details in this thread.

On the other hand if we had the "right" facility for handling other
widgets we could probably re-implement add-zle-hook-widget in those
terms.

} 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.

"zle-" was intended to be that prefix.  I'm sick of perpetuating the
colon-separated-fields thing outside of compsys.

} This parameter expansion causes the following warning on every prompt:

Oops, copy-paste error that I forgot to fix in the sandbox before
committing.  Should be

      for hook in "${(@)${(@on)hook_widgets}#<->:}"

I'll send a patch later.

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

Because the necessary context to be more specific isn't passed down the
stack into the parameter expansion / substitution code.

} 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?

Hmm, how did that happen?  The test for [[ -z "${widgets[$fn]}" ]] was
intended to cause your existing hook to prevail, i.e., the added hook
is discarded instead.

I suppose something should happen here, but it's potentially tricky
to determine when an existing hook was created by add-zle-hook-widget.
Perhaps another style name to track this state.

} > +      do
} > +	zle "$hook" -Nw || return
} 
} The indentation in the email is wrong.

It's not wrong; it has a leading 8-space tab instead of all indentation
being done with literal spaces, so it looks odd when prefixed.

} What happens if $hook starts with a minus?

It can't.  It's one of a list of predefined names.

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

I couldn't think of a situation in which a hook widget could receive
a meaningful non-empty argument list.

} 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?

Again, where would a meaningful value of NUMERIC come from here?

I think this all goes back to "this is the WRONG way".

} > +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.

Again, copying add-zsh-hook.  If someone else wants to undertake to
overhaul both of these functions after the rest of this dust settles,
I wouldn't object too much.

} 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.

Except now $hooktypes wants to have the zle- prefix (overcoming ennui).
So other stuff like this is going to have to change too.

} > +	    autoload "${autoopts[@]}" -- "$fn"
} > +	    zle -N "$fn"
} 
} If the 'autoload' needs that '--' guard, then 'zle -N' needs it too.

Good point.  The zle documentation could be clearer about where "--"
is permitted/needed; the doc implication is that no other options are
valid after -N so the next word should always be taken as a widget
name, but that's not how the parsing is implemented.

(I wonder why the widget is NOT parsed as an argument of the -N option,
now that I think about it.)

-- 
Barton E. Schaefer

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

[Bart Schaefer]

On Jun 14, 11:10am, Bart Schaefer wrote:
}
} On Jun 13,  8:52am, Daniel Shahaf wrote:
} } 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?
} 
} Hmm, how did that happen?  The test for [[ -z "${widgets[$fn]}" ]]

Two thinkos:

(1) I meant [[ -z "${widgets[$hook]}" ]]

(2) but that doesn't stop a *function* named $hook from being replaced,
    which is probably what you meant.

Yes, something should be done about that.

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

[Daniel Shahaf]

Bart Schaefer wrote on Tue, Jun 14, 2016 at 11:10:54 -0700:
> On Jun 13,  8:52am, Daniel Shahaf wrote:
> } Subject: Re: [PATCH] add-zle-hook-widget
> }
> } Bart Schaefer wrote on Sun, Jun 12, 2016 at 18:44:53 -0700:
> } > +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.)
> 
> Mostly I made that decision because add-zsh-hook explicitly declares that
> its values are arrays and displays their values with "typeset" if the -L
> option is passed.
> 
> Of course add-zsh-hook doesn't have much choice because the arrays are
> builtin names documented elsewhere, but it seemed odd to expose this
> detail in one case and hide it in the other.  If the zstyle detail were
> not called out, readers would assume by analogy that there were array
> variables they could be assigning.
> 

Then how about saying that registrations may only be added/removed
through calls to add-z-h-w [-d], without documenting how its storage is
implemented?

> } > +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).
> 
> The whole ordering thing depends on the cooperation of whoever declared
> widgets A and B in the first place.  Declarer(A) could as easily make
> capricious changes to his index as not provide it at all.
>

Let's not assume the author of (A) is malicious.  All I'm assuming that
(B) would like his own code to run after (A)'s, for whatever reason.

>
> add-zsh-hook doesn't support ANY ordering except by direct
> manipulation of the array variables.
> 
> Yeah, I'm drawing a somewhat arbitrary line at "this is now complicated
> enough, the rest is someone else's problem."
> 

The question is whether the API enables the problem to happen, and the
answer is it does: permitting registrations that specify no index means
plugins _will_ register without specifying an index.

I strongly suspect that the releasing the current interface would be
a mistake: with the current interface, the majority of registrants will
specify no index, and then regitrants that _do_ wish to take advantage
of the ordering facility won't be able to.

Basically, we can't have both "indices are optional to specify" and "no
index means index=infinity"; we need to give up one or the other or
both.

So, what I think _will_ work is either of three options: drop indices
altogether (restoring parity with add-zsh-hook); declare "no index" as
equivalent to index == 42 (for some well-known value 42); make indices
mandatory.  I don't have a preference among these three options.

> } There's also a dependency on zsh/zleparameter (${widgets} is accessed),
> } perhaps that module's presence should be checked as well?
> 
> I debated that.  I don't want add-zle-hook-widget to fail if called from
> an init file in e.g. a non-interactive shell where zle will never be
> loaded, but I don't want to needlessly test for the module every time
> the hooks are called, either.
> 

I imagined just one check when the autoloaded function is first loaded.

Or the "first loaded" code could run zmodload -e || zmodload || bail
out, and the "every widget call" code then just checks (( ${+widgets} )).

> } 1) To make it easier for people who grep for uses of the zle-foo widget
> } to find this callsite.
> 
> For some reason this argument fills me with a vast ennui that I struggle
> to overcome just because I can't think why.
> 

To me, "being greppable" is on par with "correctly indented" and "well
commented": it's a property that improves readability and maintainability
and whose absence is fair game to be pointed out in code reviews.

> } 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.)
> 
> I'm strongly of the opinion that this is the WRONG way to manipulate a
> non-special editor widget, and that the right way needs more help than
> this kind of facility can provide, and that I'm not going to attempt
> to explain all the details in this thread.
> 

You do not have to agree with me, but it is common for whoever states
a disagreeing opinion to give at least a brief glimpse of the technical
considerations underlying their different assessment.

> On the other hand if we had the "right" facility for handling other
> widgets we could probably re-implement add-zle-hook-widget in those
> terms.
> 
> } 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.
> 
> "zle-" was intended to be that prefix.  I'm sick of perpetuating the
> colon-separated-fields thing outside of compsys.
> 

Colons are a red herring here; «zstyle tuesdayzle-isearch-update …» would
address my concern just as well.

(This point only matters if add-z-h-w may in the future be extended to
non-special widgets.)

> } 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?
> 
> Hmm, how did that happen?  The test for [[ -z "${widgets[$fn]}" ]] was
> intended to cause your existing hook to prevail, i.e., the added hook
> is discarded instead.

I have a 'zle -N zle-line-init' in my zshrc.  The function gets
redefined.

> } Shouldn't argv be passed down to the hook?  In case some future special
> } widget gets passed argv from zle.
> 
> I couldn't think of a situation in which a hook widget could receive
> a meaningful non-empty argument list.
> 

zle-keymap-select takes a non-empty argument list.  (and is one of the
widgets handled by add-z-h-w)

> } 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?
> 
> Again, where would a meaningful value of NUMERIC come from here?
> 
> I think this all goes back to "this is the WRONG way".
> 

Yes, it would be easier to conceive of a use-case for NUMERIC related to
wrapping a non-special widget than to wrapping a special widget.
 
> } > +	    autoload "${autoopts[@]}" -- "$fn"
> } > +	    zle -N "$fn"
> } 
> } If the 'autoload' needs that '--' guard, then 'zle -N' needs it too.
> 
> Good point.  The zle documentation could be clearer about where "--"
> is permitted/needed; the doc implication is that no other options are
> valid after -N so the next word should always be taken as a widget
> name, but that's not how the parsing is implemented.
> 
> (I wonder why the widget is NOT parsed as an argument of the -N option,
> now that I think about it.)

The -N option doesn't take an argument; the widget name is a positional
argument.  (The optspec is "aAcCDfFgGIKlLmMNrRTUw" with no colon after
the 'N'.)

>

New issue: There seems to be a problem with the autoload boilerplate:

[[[
$ zsh -f
% echo $ZSH_PATCHLEVEL
zsh-5.2-dev-1-181-gbc1acf5
% f() {} 
% autoload -U +X add-zle-hook-widget 
% functions -T add-zle-hook-widget
% add-zle-hook-widget zle-line-pre-redraw f 
+add-zle-hook-widget:21> emulate -L zsh
+add-zle-hook-widget:25> zmodload zsh/parameter
+add-zle-hook-widget:30> local -a hooktypes=( isearch-exit isearch-update line-pre-redraw line-init line-finish history-line-set keymap-select )
+add-zle-hook-widget:34> zstyle zle-hook types isearch-exit isearch-update line-pre-redraw line-init line-finish history-line-set keymap-select
+add-zle-hook-widget:36> hook=isearch-exit
+add-zle-hook-widget:36> hook=isearch-update
+add-zle-hook-widget:36> hook=line-pre-redraw
+add-zle-hook-widget:36> hook=line-init
+add-zle-hook-widget:36> hook=line-finish
+add-zle-hook-widget:36> hook=history-line-set
+add-zle-hook-widget:36> hook=keymap-select
+add-zle-hook-widget:138> [[ 'toplevel shfunc' == *loadautofunc ]]
% zstyle -L
zstyle zle-hook types isearch-exit isearch-update line-pre-redraw line-init line-finish history-line-set keymap-select
]]]

As you can see, the hook 'f' isn't installed.

I did the z-sy-h patch, it works fine once this autoload boilerplate
issue and the "bad substitution" issue are patched.

?,

Daniel

P.S. Whatever that loadautofunc issue is, it probably applies to
bracketed-paste-magic as well, as it uses the same idiom.

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

[Bart Schaefer]

On Jun 15, 11:24pm, Daniel Shahaf wrote:
}
} Bart Schaefer wrote on Tue, Jun 14, 2016 at 11:10:54 -0700:
} > Mostly I made that decision because add-zsh-hook explicitly declares
} > that its values are arrays and displays their values with "typeset"
} > if the -L option is passed.
} 
} Then how about saying that registrations may only be added/removed
} through calls to add-z-h-w [-d], without documenting how its storage
} is implemented?

  > it seemed odd to expose this
  > detail in one case and hide it in the other.

} > The whole ordering thing depends on the cooperation of whoever declared
} > widgets A and B in the first place.  Declarer(A) could as easily make
} > capricious changes to his index as not provide it at all.
} 
} Let's not assume the author of (A) is malicious.

I'm not assuming malice, just inconsistency, or a well-meaning decision
that (A) should come after (B) even though declarer(B) thinks exactly
the opposite.

} The question is whether the API enables the problem to happen, and
} the answer is it does: permitting registrations that specify no index
} means plugins _will_ register without specifying an index.

I'm not convinced that's a problem; no index means "I don't care when
this runs, and it shouldn't matter."  If it turns out otherwise for
some other widget, then that other widget merely has to delete the
un-indexed widget and re-declare it with an index.

I think this is better than the situation with add-zsh-hook where
somebody has to be sure ALL the add-* calls are made in exactly the
right sequence ... or else has to know the implementation and muck
with it directly.

} I strongly suspect that the releasing the current interface would be
} a mistake: with the current interface, the majority of registrants will
} specify no index, and then regitrants that _do_ wish to take advantage
} of the ordering facility won't be able to.

The choice of index numbers can't be made in a vaccuum, and maybe it
can't even be made by anyone other than the user editing his .zshrc.
In the previous example, declarer(B) has to be aware of widget (A)
and know what index it uses in order to choose another index that
comes after it; how does that differ from being aware of (A) and
therefore asserting new indices for both?  If (B) is NOT aware of (A)
then what difference could it make what index declarer(A) chose?

} So, what I think _will_ work is either of three options: drop indices
} altogether (restoring parity with add-zsh-hook); declare "no index" as
} equivalent to index == 42 (for some well-known value 42); make indices
} mandatory.  I don't have a preference among these three options.

These all degenerate to the same problem in the right circumstances;
e.g. what happens if (A) and (B) both have index 42 and then (C) wants
to run *between* them?

What you have convinced me is that in the absence of an explicit index
there's some value in retaining the order in which the add-* calls
were made, which the code as last pushed doesn't.

} > I'm strongly of the opinion that this is the WRONG way to manipulate a
} > non-special editor widget, and that the right way needs more help than
} > this kind of facility can provide, and that I'm not going to attempt
} > to explain all the details in this thread.
} 
} You do not have to agree with me, but it is common for whoever states
} a disagreeing opinion to give at least a brief glimpse of the technical
} considerations underlying their different assessment.

The gist is that these special widgets by definition do nothing unless
user-defined, so there's no distinction between replacing the widget
action and augmenting it.  Non-special widgets each have an intended
semantic, so there *is* that distinction, and *usually* it's desired
to augment rather than replace.  For special widgets it's reasonable
to simply enumerate a list of actions; for non-special that's not good
enough.

} I have a 'zle -N zle-line-init' in my zshrc.  The function gets
} redefined.

Yeah, I've concluded that for add-zsh-hook the hook widget name and the
implementation function name can't be the same.

} zle-keymap-select takes a non-empty argument list.  (and is one of the
} widgets handled by add-z-h-w)

Good point.

} > (I wonder why the widget is NOT parsed as an argument of the -N option,
} > now that I think about it.)
} 
} The -N option doesn't take an argument; the widget name is a positional
} argument.  (The optspec is "aAcCDfFgGIKlLmMNrRTUw" with no colon after
} the 'N'.)

Yes, I know that; rephrased:  "I wonder why the optspec for bin_zle chose
to place the widget name in a positional argument rather than make it an
argument to -N, given that there are no other valid options that can be
passed along with -N."

} % autoload -U +X add-zle-hook-widget 

Oh.  It doesn't work with +X, it only works with -X.  That's because the
file is designed to be "source"-able rather than "autoload +X"-able.
I'm not sure there's a way to make it safe for all three of autoload +X,
source, and kshautoload.

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

[Bart Schaefer]

On Jun 16, 10:20pm, Bart Schaefer wrote:
}
} What you have convinced me is that in the absence of an explicit index
} there's some value in retaining the order in which the add-* calls
} were made, which the code as last pushed doesn't.

The following implements this, as well as fixing some bugs, most notably
that "setopt ksharrays" broke a bunch of stuff.

There may still be other problems, and of course the discussion with Daniel
hasn't really finished yet.


diff --git a/Doc/Zsh/contrib.yo b/Doc/Zsh/contrib.yo
index 64d93f9..b9c1c0a 100644
--- a/Doc/Zsh/contrib.yo
+++ b/Doc/Zsh/contrib.yo
@@ -345,14 +345,16 @@ tt(zle-isearch-exit), etc.
 var(widgetname) is the name of a ZLE widget.  If no options are given this
 is added to the array of widgets to be invoked in the given hook context.
 Note that the hooks are called as widgets, that is, with
-`tt(zle )var(widgetname)tt( -Nw)' rather than as a function call.
+example(tt(zle )var(widgetname)tt( -Nw "$@"))
+rather than as a function call.
 
-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.
+In typical usage, var(widgetname) has 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.  However, var(widgetname) may omit the index, in which case an
+index is computed for it to arrange for it to be called in the order
+in which it was added to the array.
 
 If the option tt(-d) is given, the var(widgename) is removed from
 the array of widgets to be executed.
@@ -368,12 +370,14 @@ passed as arguments to tt(autoload) as with tt(add-zsh-hook).  The
 widget is also created with `tt(zle -N )var(widgetname)' to cause the
 corresponding function to be loaded the first time the hook is called.
 
-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.
+
+The arrays of var(widgetname) are currently maintained in 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)'.  This implementation may change, and 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, so if this function is used for any
+hooks, then all hooks should be managed only via this function.
 )
 enditem()
 
diff --git a/Functions/Zle/add-zle-hook-widget b/Functions/Zle/add-zle-hook-widget
index eeb0191..608a776 100644
--- a/Functions/Zle/add-zle-hook-widget
+++ b/Functions/Zle/add-zle-hook-widget
@@ -6,6 +6,7 @@
 #
 # 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.
+# Othewise the widget is assigned an index that appends it to the array.
 # 
 # With -d, remove the widget from the hook instead; delete the hook
 # variable if it is empty.
@@ -17,38 +18,45 @@
 # 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.
+#
+# The -L option lists the hooks and their associated widgets.
 
 emulate -L zsh
 
-# Setup - create the base functions for hook widgets that call the others
-
-zmodload zsh/parameter || {
-    print -u2 "Need parameter module for zle hooks"
+# This is probably more safeguarding than necessary
+zmodload -e zsh/zle || return 1
+{ zmodload zsh/parameter && zmodload zsh/zleparameter } || {
+    print -u2 "Need parameter modules for zle hooks"
     return 1
 }
 
-local -a hooktypes=( isearch-exit isearch-update
-                     line-pre-redraw line-init line-finish
-                     history-line-set keymap-select )
+# Setup - create the base functions for hook widgets that call the others
+
+local -a hooktypes=( zle-isearch-exit zle-isearch-update
+                     zle-line-pre-redraw zle-line-init zle-line-finish
+                     zle-history-line-set zle-keymap-select )
 # Stash in zstyle to make it global
-zstyle zle-hook types $hooktypes
+zstyle zle-hook types ${hooktypes#zle-}
 
 for hook in $hooktypes
 do
-  function zle-$hook {
+  function azhw:$hook {
       local -a hook_widgets
       local hook
       # Values of these styles look like number:name
       # and we run them in number order
-      # $funcstack is more reliable than $0
-      # Also, ksh_arrays is annoying
-      emulate zsh -c 'zstyle -a $funcstack[2] widgets hook_widgets'
-      for hook in "${@${(@on)hook_widgets}#*:}"
-      do
-	zle "$hook" -Nw || return
+      zstyle -a $WIDGET widgets hook_widgets
+      for hook in "${(@)${(@on)hook_widgets[@]}#<->:}"; do
+	  zle "$hook" -Nw "$@" || return
       done
       return 0
   }
+  # Check for an existing widget, add it as the first hook
+  if [[ ${widgets[$hook]} = user:* ]]; then
+      zle -A "$hook" "${widgets[$hook]}"
+      zstyle -- "$hook" widgets 0:"${widgets[$hook]}"
+      zle -N "$hook" azhw:"$hook"
+  fi
 done
 
 # Redefine ourself with the setup left out
@@ -93,25 +101,27 @@ function add-zle-hook-widget {
     done
     shift $(( OPTIND - 1 ))
 
+    1=${1#zle-}	# Strip prefix not stored in zle-hook types style
+
     if (( list )); then
-	zstyle -L "zle-(${1:-${(@j:|:)hooktypes}})" widgets
+	zstyle -L "zle-(${1:-${(@j:|:)hooktypes[@]}})" widgets
 	return $?
-    elif (( help || $# != 2 || ${hooktypes[(I)${1#zle-}]} == 0 )); then
+    elif (( help || $# != 2 || ${hooktypes[(I)$1]} == 0 )); then
 	print -u$(( 2 - help )) $usage
 	return $(( 1 - help ))
     fi
 
     local -aU extant_hooks
-    local hook="zle-${1#zle-}"
+    local hook="zle-$1"
     local fn="$2"
 
     if (( del )); then
         # delete, if hook is set
 	if zstyle -g extant_hooks "$hook" widgets; then
 	    if (( del == 2 )); then
-		set -A extant_hooks ${extant_hooks:#(<->:|)${~fn}}
+		set -A extant_hooks ${extant_hooks[@]:#(<->:|)${~fn}}
 	    else
-		set -A extant_hooks ${extant_hooks:#(<->:|)$fn}
+		set -A extant_hooks ${extant_hooks[@]:#(<->:|)$fn}
 	    fi
             # unset if no remaining entries
 	    if (( ${#extant_hooks} )); then
@@ -121,15 +131,28 @@ function add-zle-hook-widget {
 	    fi
 	fi
     else
+	integer i=${#options[ksharrays]}-2
 	zstyle -g extant_hooks "$hook" widgets
-	extant_hooks+=("$fn")
+	if [[ "$fn" != <->:* ]]; then
+	    if [[ -z ${(M)extant_hooks[@]:#(<->:|)$fn} ]]; then
+	        # no index and not already hooked
+		# assign largest existing index plus 10
+		i=${${(On@)${(@M)extant_hooks[@]#<->:}%:}[i]}+10
+	    else
+		return 0
+	    fi
+	else
+	    i=${${(M)fn#<->:}%:}
+	    fn=${fn#<->:}
+	fi
+	extant_hooks+=("${i}:${fn}")
 	zstyle -- "$hook" widgets "${extant_hooks[@]}"
 	if [[ -z "${widgets[$fn]}" ]]; then
 	    autoload "${autoopts[@]}" -- "$fn"
-	    zle -N "$fn"
+	    zle -N -- "$fn"
 	fi
 	if [[ -z "${widgets[$hook]}" ]]; then
-	    zle -N "$hook"
+	    zle -N "$hook" azhw:"$hook"
 	fi
     fi
 }

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

[Daniel Shahaf]

Bart Schaefer wrote on Thu, Jun 16, 2016 at 22:20:55 -0700:
> On Jun 15, 11:24pm, Daniel Shahaf wrote:
> }
> } Bart Schaefer wrote on Tue, Jun 14, 2016 at 11:10:54 -0700:
> } > Mostly I made that decision because add-zsh-hook explicitly declares
> } > that its values are arrays and displays their values with "typeset"
> } > if the -L option is passed.
> } 
> } Then how about saying that registrations may only be added/removed
> } through calls to add-z-h-w [-d], without documenting how its storage
> } is implemented?
> 
>   > it seemed odd to expose this
>   > detail in one case and hide it in the other.
> 

I see your later patch adopted this suggestion.  I of course concede the
asymmetry but I think leaving ourselves room to change the implementation
is preferable.

> } > The whole ordering thing depends on the cooperation of whoever declared
> } > widgets A and B in the first place.  Declarer(A) could as easily make
> } > capricious changes to his index as not provide it at all.
> } 
> } Let's not assume the author of (A) is malicious.
> 
> I'm not assuming malice, just inconsistency, or a well-meaning decision
> that (A) should come after (B) even though declarer(B) thinks exactly
> the opposite.

Good point: perhaps (A) does have a reason to want to be last.
(I should have thought about that sooner: z-sy-h asks to be last...)

> } The question is whether the API enables the problem to happen, and
> } the answer is it does: permitting registrations that specify no index
> } means plugins _will_ register without specifying an index.
> 
> I'm not convinced that's a problem; no index means "I don't care when
> this runs, and it shouldn't matter."  If it turns out otherwise for
> some other widget, then that other widget merely has to delete the
> un-indexed widget and re-declare it with an index.
> 

Having (B) unhook/rehook (A)'s hook sounds like a recipe for
hard-to-diagnose bugs.

I think making indices mandatory would resolve this: there could be
a convention that "use index=999 to run last" and then if (A) sets 999
and (B) consciously override (A), (B) can register at 1000.  There are
enough integers so this design wouldn't give either (A) or (B) preference.
[But see below for another idea]

> I think this is better than the situation with add-zsh-hook where
> somebody has to be sure ALL the add-* calls are made in exactly the
> right sequence ... or else has to know the implementation and muck
> with it directly.
> 

Agreed, although there _is_ something to be said for the "or else"
alternative: it is a thin/KISS solution, "here's the explicit order
hooks will be called in, have a field day".

> } I strongly suspect that the releasing the current interface would be
> } a mistake: with the current interface, the majority of registrants will
> } specify no index, and then regitrants that _do_ wish to take advantage
> } of the ordering facility won't be able to.
> 
> The choice of index numbers can't be made in a vaccuum, and maybe it
> can't even be made by anyone other than the user editing his .zshrc.
> In the previous example, declarer(B) has to be aware of widget (A)
> and know what index it uses in order to choose another index that
> comes after it; how does that differ from being aware of (A) and
> therefore asserting new indices for both?  If (B) is NOT aware of (A)
> then what difference could it make what index declarer(A) chose?
> 

Writing (B) to correctly register itself in the correct relative order 
both to plugins it knows about and to one it doesn't is an interesting
question.

Perhaps we should ditch indexing and use topological sorting [tsort(1)
is in POSIX] instead: then we can let registrants specify "before:" and
"after:" clauses, so B could do "add-zsh-hook zle-line-init B_hook
before:A_hook".  That would handle dependencies to known other plugins;
to handle dependencies to unknown other plugins — say, having all
$BUFFER-modifying hooks to run before all non-$BUFFER-modifying hooks —
we could add a virtual "BUFFER-modifying-done" sequence point (which
won't correspond to a hook) to let plugins register themselves with
"before:BUFFER-modifying-done".  If no before: nor after: clauses are
specified, all registered hooks would be run in unspecified order (or
perhaps in registration order).

I'm not sure whether this is simpler or more complicated than indices.

> } So, what I think _will_ work is either of three options: drop indices
> } altogether (restoring parity with add-zsh-hook); declare "no index" as
> } equivalent to index == 42 (for some well-known value 42); make indices
> } mandatory.  I don't have a preference among these three options.
> 
> These all degenerate to the same problem in the right circumstances;
> e.g. what happens if (A) and (B) both have index 42 and then (C) wants
> to run *between* them?
> 

Good point.  Using tsort would enable (C) to achieve this:
add-z-h-w C_${hook} before:B_${hook} after:A_${hook}

One issue with topological sort is cycles (add-z-h-w x A_hook before:B;
add-z-h-w x B_hook before:A;).  On both linux and freebsd, tsort warns
about cycles to stderr but prints _some_ ordering to stdout anyway,
which is good for add-zsh-hook-widget's use-case.

> What you have convinced me is that in the absence of an explicit index
> there's some value in retaining the order in which the add-* calls
> were made, which the code as last pushed doesn't.
> 

Okay :)

> } > I'm strongly of the opinion that this is the WRONG way to manipulate a
> } > non-special editor widget, and that the right way needs more help than
> } > this kind of facility can provide, and that I'm not going to attempt
> } > to explain all the details in this thread.
> } 
> } You do not have to agree with me, but it is common for whoever states
> } a disagreeing opinion to give at least a brief glimpse of the technical
> } considerations underlying their different assessment.
> 
> The gist is that these special widgets by definition do nothing unless
> user-defined, so there's no distinction between replacing the widget
> action and augmenting it.  Non-special widgets each have an intended
> semantic, so there *is* that distinction, and *usually* it's desired
> to augment rather than replace.  For special widgets it's reasonable
> to simply enumerate a list of actions; for non-special that's not good
> enough.
> 

Thank you for detailing.  I am not convinced, but as I don't have
a strong opinion on the issue I'll defer to your judgement.

> } % autoload -U +X add-zle-hook-widget 
> 
> Oh.  It doesn't work with +X, it only works with -X.  That's because the
> file is designed to be "source"-able rather than "autoload +X"-able.
> I'm not sure there's a way to make it safe for all three of autoload +X,
> source, and kshautoload.

Wouldn't the following work?

$ cat f
f() { echo I have been called with "$@" }
if [[ "$zsh_eval_context" != *\ file && ! -o kshautoload ]]; then
    f "$@"
fi
$ zsh -f
% fpath+=$PWD
% autoload f
% f arg
I have been called with arg

$ zsh -f
% fpath+=$PWD
% setopt kshautoload 
% autoload -k f 
% f arg 
I have been called with arg

$ zsh -f
% fpath+=$PWD
% source f
% f arg
I have been called with arg

$ zsh -f
% fpath+=$PWD
% autoload +X f 
% f arg
I have been called with arg

$ zsh -f
% fpath+=$PWD
% setopt kshautoload
% autoload +X -k f
% f arg
I have been called with arg

Cheers,

Daniel

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

[Daniel Shahaf]

Bart Schaefer wrote on Sat, Jun 18, 2016 at 16:25:58 -0700:
> On Jun 16, 10:20pm, Bart Schaefer wrote:
> }
> } What you have convinced me is that in the absence of an explicit index
> } there's some value in retaining the order in which the add-* calls
> } were made, which the code as last pushed doesn't.
> 
> The following implements this, as well as fixing some bugs, most notably
> that "setopt ksharrays" broke a bunch of stuff.
> 
> There may still be other problems, and of course the discussion with Daniel
> hasn't really finished yet.

Yes, I'm replying as often as I can, but these days that's not as often
as I'd like ☹

>  for hook in $hooktypes
>  do
> +  # Check for an existing widget, add it as the first hook
> +  if [[ ${widgets[$hook]} = user:* ]]; then
> +      zle -A "$hook" "${widgets[$hook]}"
> +      zstyle -- "$hook" widgets 0:"${widgets[$hook]}"

The last parameter expansion should strip the "user:" prefix.

> +      zle -N "$hook" azhw:"$hook"
> +  fi

Cheers,

Daniel

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

[Bart Schaefer]

On Jun 21,  1:41am, Daniel Shahaf wrote:
} Subject: Re: [PATCH] add-zle-hook-widget
}
} Bart Schaefer wrote on Sat, Jun 18, 2016 at 16:25:58 -0700:
} >  for hook in $hooktypes
} >  do
} > +  # Check for an existing widget, add it as the first hook
} > +  if [[ ${widgets[$hook]} = user:* ]]; then
} > +      zle -A "$hook" "${widgets[$hook]}"
} > +      zstyle -- "$hook" widgets 0:"${widgets[$hook]}"
} 
} The last parameter expansion should strip the "user:" prefix.

No, it shouldn't, because the "zle -A" command creates a new widget
whose name begins with "user:".

This way if you run "add-zle-hook-widget -L" it will show you that the
widget with index zero was the user's previously defined widget.

Yes, this breaks things if the user's widget compares itself to $WIDGET
but there's no way around that [*] because the newly created hook is
the only widget allowed to use the reserved hook-widget name.

[*] A possible fix for this is to omit the -w when calling the widget
from zle, so that $WIDGET remains the hook-widget.  That would look
something like

      for hook in "${(@)${(@on)hook_widgets[@]}#<->:}"; do
          if [[ "$hook" = user:* ]]; then
              # Do not change $WIDGET, it may be tested in $hook
              zle "$hook" -N "$@" || return
          else
              zle "$hook" -Nw "$@" || return
          fi
      done

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

[Bart Schaefer]

On Jun 21,  1:41am, Daniel Shahaf wrote:
} Subject: Re: [PATCH] add-zle-hook-widget
}
} > I'm not sure there's a way to make it safe for all three of autoload +X,
} > source, and kshautoload.
} 
} Wouldn't the following work?
} 
} $ cat f
} f() { echo I have been called with "$@" }
} if [[ "$zsh_eval_context" != *\ file && ! -o kshautoload ]]; then
}     f "$@"
} fi

I don't think this correctly handles the case where "autoload -k" is used
but kshautoload is not actually set when the function is first called.
There's also the reverse case, where kshautoload IS set but the function
was declared with "autoload -z", to be considered.

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

[Daniel Shahaf]

Bart Schaefer wrote on Wed, Jun 22, 2016 at 13:25:41 -0700:
> On Jun 21,  1:41am, Daniel Shahaf wrote:
> } Subject: Re: [PATCH] add-zle-hook-widget
> }
> } > I'm not sure there's a way to make it safe for all three of autoload +X,
> } > source, and kshautoload.
> } 
> } Wouldn't the following work?
> } 
> } $ cat f
> } f() { echo I have been called with "$@" }
> } if [[ "$zsh_eval_context" != *\ file && ! -o kshautoload ]]; then
> }     f "$@"
> } fi
> 
> I don't think this correctly handles the case where "autoload -k" is used
> but kshautoload is not actually set when the function is first called.
> There's also the reverse case, where kshautoload IS set but the function
> was declared with "autoload -z", to be considered.

Any comments on the rest of my email, particularly on the idea to use
topological sorting instead of indices?  I realise you may not have time
to implement it yourself, but I wanted to hear your opinion on the
proposed change.

(I wouldn't normally ping, but this is a not-yet-released API so I'd
like to get it right before we're bound by backwards compat.)

Cheers,

Daniel

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

[Bart Schaefer]

Delayed response.

On Jun 21,  1:41am, Daniel Shahaf wrote:
} Subject: Re: [PATCH] add-zle-hook-widget
}
} Bart Schaefer wrote on Thu, Jun 16, 2016 at 22:20:55 -0700:
} > I think this is better than the situation with add-zsh-hook where
} > somebody has to be sure ALL the add-* calls are made in exactly the
} > right sequence ... or else has to know the implementation and muck
} > with it directly.
} 
} Agreed, although there _is_ something to be said for the "or else"
} alternative: it is a thin/KISS solution, "here's the explicit order
} hooks will be called in, have a field day".

You can't have it both ways -- either the implementation must be
documented, which you objected to, or it has to be possible to do
what's needed without knowing.

} Perhaps we should ditch indexing and use topological sorting [tsort(1)
} is in POSIX] instead: then we can let registrants specify "before:" and
} "after:" clauses

I don't think we want to get into the business of storing a partial
orderting and attempting to resolve it to a total one.  For one thing,
more information arrives every time add-zsh-hook is called.  Redo the
whole tsort each time?

} so B could do "add-zsh-hook zle-line-init B_hook before:A_hook"

How does that differ from:

} Having (B) unhook/rehook (A)'s hook sounds like a recipe for
} hard-to-diagnose bugs.

??  It's just the internals heuristically doing the unhook/re-hook at
B's behest, instead of B doing it explicitly.

Still, there might be something.  Just thinking aloud:

Firstly, I chose the syntax "index:name" to allow multiple items to be
added at once, but add-zsh-hook doesn't work that way so there's no
longer any reason for add-zle-hook-widget to work that way.  So let's
make the syntax be
	add-zle-hook-widget HOOK WIDGETNAME [INDEX]
instead of embedding the index in the widgetname.

(This might be tricky/confusing given that the widget list would still
need to be stored with each index attached.)

Secondly, the index could be symbolic as you suggest; perhaps first,
last, "before:otherwidget" and "after:otherwidget".  The difference
from a topological sort would be that before/after apply immediately,
so if there is no "otherwidget" present yet, they mean nothing.  You
still have to call add-zle-hook-widget in some kind of sequence, but
you don't have to know what index was assigned to otherwidget.  Also,
"otherwidget" should be a pattern.

Thirdly, allow multiple symbolic indices, to cover "before:X after:Y".
Try them until one fails, ignoring any that are meaningless.  If all
succeed, add the hook, else report error?

One other thought -- it probably doesn't work to have a single sequence
point for e.g. whether buffer modification happens.

} I'm not sure whether this is simpler or more complicated than indices.

Well, it sounds a lot more complicated to me, but it depends on whether
you mean complicated for the user to figure out beforehand what index to
use, or complicated for the shell to manage and the user to understand
afterward why a particular order of execution was chosen.

As tsort(1) itself says:
> Note that for a given partial ordering, generally there is no unique
> total ordering.

That means the widgets might start running in different order after a
new hook is added, for no reason the user is able to plainly see.  I like
the notion that a known total ordering can be imposed without having to
express increasingly detailed partial orders for each new addition.

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

[Daniel Shahaf]

Bart Schaefer wrote on Fri, Jul 01, 2016 at 13:11:58 -0700:
> On Jun 21,  1:41am, Daniel Shahaf wrote:
> } Subject: Re: [PATCH] add-zle-hook-widget
> }
> } Bart Schaefer wrote on Thu, Jun 16, 2016 at 22:20:55 -0700:
> } > I think this is better than the situation with add-zsh-hook where
> } > somebody has to be sure ALL the add-* calls are made in exactly the
> } > right sequence ... or else has to know the implementation and muck
> } > with it directly.
> } 
> } Agreed, although there _is_ something to be said for the "or else"
> } alternative: it is a thin/KISS solution, "here's the explicit order
> } hooks will be called in, have a field day".
> 
> You can't have it both ways -- either the implementation must be
> documented, which you objected to, or it has to be possible to do
> what's needed without knowing.

I haven't changed my opinion; I was simply trying to say that each of
these options has its merits.

> } Perhaps we should ditch indexing and use topological sorting [tsort(1)
> } is in POSIX] instead: then we can let registrants specify "before:" and
> } "after:" clauses
> 
> I don't think we want to get into the business of storing a partial
> orderting and attempting to resolve it to a total one.  For one thing,
> more information arrives every time add-zsh-hook is called.  Redo the
> whole tsort each time?

Considering that vcs_info works, and is presumably more expensive than
a tsort, we could probably do the tsort every precmd() and nobody would
complain (*cough* cygwin *cough*).

Or we could use a just-in-time approach: have add-zle-hook-widget just
stash $argv[2,-1] somewhere and invalidate the cache, and have azhw:$1()
tsort the somewhere, cache the resulting sorted order, and thereafter
use the cached answer.  (But that may be a premature optimisation...)

> } so B could do "add-zsh-hook zle-line-init B_hook before:A_hook"
> 
> How does that differ from:
> 
> } Having (B) unhook/rehook (A)'s hook sounds like a recipe for
> } hard-to-diagnose bugs.
> 

The difference is whether the order of A_hook relative to hooks *other
than* B_hook may be affected.

If B changes the index A_hook is registered at, that order may be
affected.  If B adds a "after:B_hook" constraint to A_hook, that order
shouldn't be affected [unless the additional constraint creates
a circular dependency].

> ??  It's just the internals heuristically doing the unhook/re-hook at
> B's behest, instead of B doing it explicitly.

See previous paragraphs.

> Still, there might be something.  Just thinking aloud:
> 
> Firstly, I chose the syntax "index:name" to allow multiple items to be
> added at once, but add-zsh-hook doesn't work that way so there's no
> longer any reason for add-zle-hook-widget to work that way.  So let's
> make the syntax be
> 	add-zle-hook-widget HOOK WIDGETNAME [INDEX]
> instead of embedding the index in the widgetname.
> 

+1

> (This might be tricky/confusing given that the widget list would still
> need to be stored with each index attached.)

I'm not sure what's tricky/confusing here.?

> Secondly, the index could be symbolic as you suggest; perhaps first,
> last, "before:otherwidget" and "after:otherwidget".  The difference
> from a topological sort would be that before/after apply immediately,
> so if there is no "otherwidget" present yet, they mean nothing.  You
> still have to call add-zle-hook-widget in some kind of sequence, but
> you don't have to know what index was assigned to otherwidget.  Also,
> "otherwidget" should be a pattern.
> 
> Thirdly, allow multiple symbolic indices, to cover "before:X after:Y".
> Try them until one fails, ignoring any that are meaningless.  If all
> succeed, add the hook, else report error?
> 

This sounds exactly like topological sort except that the before:/after:
constraints may only refer to already-registered widgets, and
consequently, the responsibility for declaring interdependencies between
two plugins lies entirely with the plugin sourced latter of the two.

In other words: a plugin cannot declare what order its hook should be
run in relative to plugins that have not yet been sourced.  Perhaps
virtual sequence points (see next paragraphs) can serve instead,
though.

> One other thought -- it probably doesn't work to have a single sequence
> point for e.g. whether buffer modification happens.
> 

I was thinking that would be a convention, not enforced by code.  For
example, zsh-syntax-highlighting wants to run once BUFFER is settled
down in its final form, but doesn't care which hooks are installed that
modify BUFFER.  What arguments should it pass to add-zle-hook-widget then?

This may be comparable to the 'first' / 'last' ordering constraints you
described.

> } I'm not sure whether this is simpler or more complicated than indices.
> 
> Well, it sounds a lot more complicated to me, but it depends on whether
> you mean complicated for the user to figure out beforehand what index to
> use, or complicated for the shell to manage and the user to understand
> afterward why a particular order of execution was chosen.
> 

I meant "is tsort the right way to model and solve this problem".

> As tsort(1) itself says:
> > Note that for a given partial ordering, generally there is no unique
> > total ordering.
> 
> That means the widgets might start running in different order after a
> new hook is added, for no reason the user is able to plainly see.  I like
> the notion that a known total ordering can be imposed without having to
> express increasingly detailed partial orders for each new addition.

Fair point.

Note there are alternatives to specifying detailed partial orders: for
example, one could hook a single function that invokes a few other ones:
.
    order-sensitive() {
       local w
       for w in foo bar baz; do zle -- $w -Nw -- "$@"; done
    }
    add-zle-hook-widget line-init order-sensitive
    add-zle-hook-widget line-init qux             # qux can be ordered either before or after the foo,bar,baz group

Cheers,

Daniel

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

[Bart Schaefer]

On Jul 5,  4:57am, Daniel Shahaf wrote:
}
} Note there are alternatives to specifying detailed partial orders: for
} example, one could hook a single function that invokes a few other ones

This gives me an interesting idea:  The list of hook types is stored
in a zstyle.  So you could theoretically add your own new hook types
to that style, and hey presto, add-zle-hook-widget will let you manage
a hook "array" for your new hook name.

    azhw:zle-new-hook-name() { ... }
    zle -N zle-new-hook-name azhw:zle-new-hook-name
    $(zstyle -L zle-hook types) new-hook-name

    add-zle-hook-widget new-hook-name some-other-widget
    add-zle-hook-widget new-hook-name yet-another-widget

Then you can add all of your hooks to one of the built-in hooks like

    add-zle-hook-widget line-init zle-new-hook-name

Hmm.