[libvirt] [PATCHv3 2/6] virsh: Add vshCmdCompleter and vshOptCompleter

[Tomas Meszaros]

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