[libcamera-devel] [PATCH 3/3] cam: options: Add support for repeatable options

[Jacopo Mondi]

Hi Niklas,

On Tue, Mar 26, 2019 at 11:41:23AM +0100, Niklas Söderlund wrote:
> Hi Laurent,
>
> Thanks for your feedback.
>
> On 2019-03-26 08:28:21 +0200, Laurent Pinchart wrote:
> > > -	values_[opt] = value;
> > > +	if (option.array)
> > > +		values_[opt].add(value);
> > > +	else
> > > +		values_[opt] = value;
> >
> > I would be tempted to overload the [] operator for values_ to just do
> > the right thing depending on the type, but I think it would be bother
> > overkill and confusing. Still tempting to play with the available toys
> > though :-)
>
> Playing with new toys are always fun :-) In this particular instance I
> think overloading [] would just add to the confusion.
>
> > > diff --git a/src/cam/options.h b/src/cam/options.h
> > > index 6a887416c0070c41..1dac15ea90f2ffd2 100644
> > > --- a/src/cam/options.h
> > > +++ b/src/cam/options.h
> > > @@ -36,6 +36,7 @@ struct Option {
> > >  	const char *argumentName;
> > >  	const char *help;
> > >  	KeyValueParser *keyValueParser;
> > > +	bool array;
> >
> > Maybe isArray ?
>
> Better, will switch in next version.
>
> >
> > I think the rest is fine. How does the help output look like ?
>
> $ ./cam --help
> Options:
>   -c, --camera camera                                   Specify which camera to operate on
>   -C, --capture                                         Capture until interrupted by user
>   -F, --file[=filename]                                 Write captured frames to disk
>                                                         The first '#' character in the file name is expanded to the frame sequence number.
>                                                         The default file name is 'frame-#.bin'.
>   -f, --format key=value[,key=value,...] ...            Set format of the camera's first stream
>                                                         height=integer          Height in pixels
>                                                         id=integer              ID of stream
>                                                         pixelformat=integer     Pixel format
>                                                         width=integer           Width in pixels
>   -h, --help                                            Display this help message
>   -l, --list                                            List all cameras
>
> I let you comment on if you like the ... to indicate that an option can
> be repeated and post v2 after that.
>

I might not be paying too much attention, but I missed the three
additiona dots completely. Should we find something more explicit to
indicate an option can be repeated multiple times?

Also, be aware the comment reports "camera's first stream". I assume
this will be changed as well.

Thanks
   j

> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190326/9113958c/attachment.sig>