Zsh Mailing List Archive
Messages sorted by: Reverse Date, Date, Thread, Author

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

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

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

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

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



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

Messages sorted by: Reverse Date, Date, Thread, Author