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

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

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