From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4307 invoked by alias); 15 Jun 2016 23:25:00 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 38689 Received: (qmail 25816 invoked from network); 15 Jun 2016 23:24:53 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=0.6 required=5.0 tests=BAYES_00,TO_NO_BRKTS_PCNT autolearn=no autolearn_force=no version=3.4.1 Date: Wed, 15 Jun 2016 23:24:47 +0000 From: Daniel Shahaf To: zsh-workers@zsh.org Subject: Re: [PATCH] add-zle-hook-widget Message-ID: <20160615232447.GA29225@tarsus.local2> References: <160612184453.ZM11316@torch.brasslantern.com> <20160613085219.GA23148@tarsus.local2> <160614111054.ZM17893@torch.brasslantern.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <160614111054.ZM17893@torch.brasslantern.com> User-Agent: Mutt/1.5.23 (2014-03-12) 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.