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

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

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.