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

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

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