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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list