[libcamera-devel] [PATCH] libcamera: controls: Extend docs how to identify controls from ControlList

[Laurent Pinchart]

Hi Marian,

Thank you for the patch.

On Fri, Mar 19, 2021 at 06:03:57PM +0100, Sebastian Fricke wrote:
> On 19.03.2021 17:40, Marian Cichy wrote:
> > Informations how to identify Controls from a ControlList is quite
> 
> s/Informations how/Informations on how/
> s/is quite/are quite/
> 
> > scattered around the documentation and not clear if one reads about the
> 
> s/and not clear/and it is not clear/
> 
> > ControlList. Referring to ControlId and ControlIdMap right in the
> > detailed description is probably very helpful.
> 
> agreed :)
> 
> > Signed-off-by: Marian Cichy <m.cichy at pengutronix.de>
> > ---
> >  src/libcamera/controls.cpp | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index c58ed394..b5253f83 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -805,6 +805,10 @@ ControlList::ControlList()
> >   * For ControlList containing libcamera controls, a global map of all libcamera
> >   * controls is provided by controls::controls and can be used as the \a idmap
> >   * argument.
> > + *
> > + * To identify a Control from the ControlList, one needs to find the ControlId
> > + * from the numerical control id saved in this list. A global ControlIdMap
> > + * of all libcamera controls is provided by controls::controls.
> 
> This sentence seems to be very similar to the sentence two rows above.
> How about:
> ```
>   * For ControlList containing libcamera controls, a global map of all libcamera
>   * controls is provided by controls::controls and can be used as the \a idmap
>   * argument.
>   * In order to identify a single control from the ControlList, one needs to find
>   * the ControlId using the contains method in the numerical control IDs of that
>   * map.
> ```
> 
> I feel like when we are at it, just mention how you are supposed to do
> it.

Furthermore, usage information shouldn't be documented in one particular
constructor, but at the class level, or even file level. I think file
level would be more appropriate here, to tie together Control, ControlId
and ControlList.

> >   */
> >  ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)
> >  	: validator_(validator), idmap_(&idmap), infoMap_(nullptr)

-- 
Regards,

Laurent Pinchart