Re: [libvirt] [PATCHv3 2/6] virsh: Add vshCmdCompleter and vshOptCompleter
On 26/08/13 at 11:47am, Eric Blake wrote:
On 08/26/2013 06:36 AM, Tomas Meszaros wrote:
> completer and completer_flags added to the _vshCmdDef and _vshCmdOptDef
> structures so it will be possible for completion generators to
> conveniently call completer functions with desired flags.
> ---
> tools/virsh.h | 7 +++++++
> 1 file changed, 7 insertions(+)
In isolation, this patch looks reasonable, but I want to see how it gets
used before acking it.
>
> diff --git a/tools/virsh.h b/tools/virsh.h
> index 466ca2d..064acde 100644
> --- a/tools/virsh.h
> +++ b/tools/virsh.h
> @@ -147,6 +147,9 @@ typedef struct _vshCmdOptDef vshCmdOptDef;
> typedef struct _vshControl vshControl;
> typedef struct _vshCtrlData vshCtrlData;
>
> +typedef char **(*vshCmdCompleter)(unsigned int flags);
> +typedef char **(*vshOptCompleter)(unsigned int flags);
Do we need two typedefs, or is (*vshCompleter) a sufficient reusable name?
You are right, we don't need two typedefs. I will merge them into
(*vshCompleter) as you are suggesting.
> +
> /*
> * vshCmdInfo -- name/value pair for information about command
> *
> @@ -168,6 +171,8 @@ struct _vshCmdOptDef {
> unsigned int flags; /* flags */
> const char *help; /* non-NULL help string; or for VSH_OT_ALIAS
> * the name of a later public option */
> + vshOptCompleter completer; /* option completer */
> + unsigned int completer_flags; /* option completer flags */
Per-option completions make sense. For example, 'virsh vol-key --pool
<TAB>' wants to use a pool completer, while 'virsh vol-key --vol
<TAB>'
wants to use a volume completer; furthermore, 'virsh vol-key <TAB>'
should be the combination of the option completer (showing --vol and
--pool) AND the volume completer filtered to names not starting with '-'
(since virsh has the semantics that arguments are positional, where the
option '--vol' is implied if the argument that appears in that position
does not resemble an option).
So If I get it right, you are suggesting that it should work like this:
virsh # vol-key <TAB>
vol1 vol2 pool1 pool2
as you said, combination of --vol and --pool completers.
I was initially thinking that completion should work like this:
virsh # vol-key <TAB>
vol1 vol2
It is completing <vol> first becasue <vol> is only mandatory argument
for this command.
Only if someone type:
virsh # vol-key --pool <TAB>
pool1 pool2
then call --pool completer.
> };
>
> /*
> @@ -199,6 +204,8 @@ struct _vshCmdDef {
> const vshCmdOptDef *opts; /* definition of command options */
> const vshCmdInfo *info; /* details about command */
> unsigned int flags; /* bitwise OR of VSH_CMD_FLAG */
> + vshCmdCompleter completer; /* command completer */
> + unsigned int completer_flags; /* command completer flags */
> };
I'm not so sure about per-command completers, though. What can a
command complete differently than per-option completers would already
provide? Maybe this argues that you need more rationale in the commit
message about how this will be used. Or maybe I'll figure it out by the
time I get to the end of your series.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
--
Tomas Meszaros