From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20144 invoked by alias); 14 Jun 2016 18:10:38 -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: 38681 Received: (qmail 13 invoked from network); 14 Jun 2016 18:10:37 -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=-1.9 required=5.0 tests=BAYES_00,T_DKIM_INVALID autolearn=ham autolearn_force=no version=3.4.1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=brasslantern-com.20150623.gappssmtp.com; s=20150623; h=from:message-id:date:in-reply-to:comments:references:to:subject :mime-version; bh=hsicrmEFLCFCqc5ewDgUUM2Wet7Fb+uHDc/mlPmuaRc=; b=c+PxRV+JNZXRhsGprViZ5yNeaZwLxaQqv3BxwfA3fXVCW3qErl/2OiFdOextCgZ920 SwC5R4rWI4AHZj8WruIHB45ZGjVKm6hkOsZQYOdWiN3rvPsc5m+TMggyaCAI/g8PfwG+ diUX6NA9Ruh9jlX35pk/QzOSOWc3ufN3mqSVM1QHHUTqgVqBL70Wre1N0VEGHE0JYF4c WqE0luZjqV/9Rey2ZQTsrzMu1vITlDyD8A1YSQO8JOc99e4+HVx+Fd0Ix6XFsipZ5VUZ /gBBZCTodGYG3Hb30dF8fYGKERkBpT7/LH5ukm1rYS6EyyFguZL3jGZlrKegNVu2F9y0 QIlQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:message-id:date:in-reply-to:comments :references:to:subject:mime-version; bh=hsicrmEFLCFCqc5ewDgUUM2Wet7Fb+uHDc/mlPmuaRc=; b=F8XleVdX3sUZy2YuoA7bMDrWnvtnu3nN+sMFKdcjx88XWYu1L7xEmVOSLBllRCX5Wh Dacxpu5BPq2rNGJSqJi03PeWPt58Wo3BHcOA902R3FIs3Ngqm5bLlkc58j/9gU/qvawH F9mhMYNlNjxPbgHyI64EXeAG79NLsSrucYXv9oOXPGXs2qHRRNnO7RgJFphVF4EV965Z V+NIQD4VVT6pfzRLdMI6n/XRyE8abixPWxnJwJgctuaNo0Bv+eXntp+TstLdcoR2ireG nXfWWawLFW6zxqiiS6Yga+RLFeLVe2IDJH1pbel8aZzrzhisYmkjktT7+tVcO0nKdm25 Q66Q== X-Gm-Message-State: ALyK8tJOTzCETRe0w2WazBYQClMtHsbnkrRQ2/MQwMBucWp28RJWxtQzAXI8ZWivOH4jFg== X-Received: by 10.98.18.131 with SMTP id 3mr5122717pfs.102.1465927835303; Tue, 14 Jun 2016 11:10:35 -0700 (PDT) From: Bart Schaefer Message-Id: <160614111054.ZM17893@torch.brasslantern.com> Date: Tue, 14 Jun 2016 11:10:54 -0700 In-Reply-To: <20160613085219.GA23148@tarsus.local2> Comments: In reply to Daniel Shahaf "Re: [PATCH] add-zle-hook-widget" (Jun 13, 8:52am) References: <160612184453.ZM11316@torch.brasslantern.com> <20160613085219.GA23148@tarsus.local2> X-Mailer: OpenZMail Classic (0.9.2 24April2005) To: zsh-workers@zsh.org Subject: Re: [PATCH] add-zle-hook-widget MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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