[libcamera-devel] [PATCH] libcamera: media_device: Documentation fix

[Kieran Bingham]

Quoting Umang Jain (2021-11-19 05:21:13)
> Hi Laurent,
> 
> On 11/19/21 1:20 AM, Laurent Pinchart wrote:
> > Hi Umang,
> >
> > Thank you for the patch.
> >
> > On Thu, Nov 18, 2021 at 11:37:07PM +0530, Umang Jain wrote:
> >> Fix a couple of sentence formations to clarify the intent.
> >>
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >>   src/libcamera/media_device.cpp | 12 ++++++------
> >>   1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> >> index aa93da75..dd5def20 100644
> >> --- a/src/libcamera/media_device.cpp
> >> +++ b/src/libcamera/media_device.cpp
> >> @@ -40,15 +40,15 @@ LOG_DEFINE_CATEGORY(MediaDevice)
> >>    * instance.
> >>    *
> >>    * The instance is created with an empty media graph. Before performing any
> >> - * other operation, it must be populate by calling populate(). Instances of
> >> + * other operation, it must be populated by calling populate(). Instances of
> > Oops. Good catch.
> >
> >>    * MediaEntity, MediaPad and MediaLink are created to model the media graph,
> >>    * and stored in a map indexed by object id.
> >>    *
> >> - * The graph is valid once successfully populated, as reported by the isValid()
> >> - * function. It can be queried to list all entities(), or entities can be
> >> - * looked up by name with getEntityByName(). The graph can be traversed from
> >> - * entity to entity through pads and links as exposed by the corresponding
> >> - * classes.
> >> + * The graph is valid once it has been successfully populated,
> > I think the sentence is grammatically correct already. Is it confusing ?
> 
> 

Ayyee wrapped lines are making that really hard to interpret.

As far as I can tell:

Original:

 The graph is valid once successfully populated, as reported by the
 isValid() function.

Suggested:

 The graph is valid once it has been successfully populated,
 and is reported as valid by the isValid() function.

diff:

A) s/once/once it has been/
B) s/as reported/and is reported/


Diff-A certainly works for me.

Diff-B makes it sound like calling isValid() is a requirement to
making it valid it, which it isn't.

To me a clearer diff would be:

B s/as reported/which is reported/



> It seems confusing to me: Other options were:
> 
>      * The graph is valid once populated successfully, as reported by 
> the isValid() ...
> 
> But I don't think it goes with the explanation you gave below about 
> isValid()
> >
> >> and is reported
> >> + * as valid by the isValid() function. It can be queried to list all entities
> > That doesn't mean the same. The current text meant to say that the
> > successful population of the graph is reported by isValid().
> 
> 
> Oh. So I re-read the isValid() to clarify this -
> 
>          /**
>           * \fn MediaDevice::isValid()
>           * \brief Query whether the media graph has been populated and 
> is valid
>           * \return true if the media graph is valid, false otherwise
>           */
> 
> It makes sense to me now, but the line above in the para, still seems a 
> bit implicit to bring this meaning there.
> >
> >> + * with entities(), or entities can be looked up by name with getEntityByName()
> >> + * function. The graph can be traversed from entity to entity through pads and
> >> + * links as exposed by the corresponding classes.
> >>    *
> >>    * Media devices can be claimed for exclusive use with acquire(), released with
> >>    * release() and tested with busy(). This mechanism is aimed at pipeline