[libcamera-devel] [PATCH v2.1 4/4] qcam: CamSelectDialog: Display Location and Model propety of camera

[Laurent Pinchart]

On Mon, Aug 08, 2022 at 10:05:54AM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-07 16:41:38)
> > The camera selection dialog currently only displays the camera Id.
> > Display the camera location and camera model if available.
> > 
> 
>  \o/ This bit is great!
> 
> > Signed-off-by: Utkarsh Tiwari <utkarsh02t at gmail.com>
> > ---
> > Difference from v2:
> >         Use getCameraId() instead of getCurrentCamera()
> >  src/qcam/cam_select_dialog.h | 54 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> > 
> > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h
> > index ee65eb88..6ba61cff 100644
> > --- a/src/qcam/cam_select_dialog.h
> > +++ b/src/qcam/cam_select_dialog.h
> > @@ -12,11 +12,14 @@
> >  
> >  #include <libcamera/camera.h>
> >  #include <libcamera/camera_manager.h>
> > +#include <libcamera/controls.h>
> > +#include <libcamera/property_ids.h>
> >  
> >  #include <QComboBox>
> >  #include <QDialog>
> >  #include <QDialogButtonBox>
> >  #include <QFormLayout>
> > +#include <QLabel>
> >  #include <QString>
> >  
> >  class CamSelectDialog : public QDialog
> > @@ -34,6 +37,17 @@ public:
> >                 for (const auto &cam : cm_->cameras())
> >                         cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));
> >  
> > +               /* Set camera information labels. */
> > +               cameraLocation_ = new QLabel;
> > +               cameraModel_ = new QLabel;
> > +
> > +               updateCamInfo(cm_->get(getCameraId()));

This would be best placed at the very end of the constructor, when the
dialog layout is fully constructed.

> > +               connect(cameraIdComboBox_, &QComboBox::currentTextChanged,
> > +                       this, [&]() {
> > +                               updateCamInfo(cm_->get(
> > +                                       cameraIdComboBox_->currentText().toStdString()));
> > +                       });

Can you add a private member function instead of using a lambda function
? That will be more efficient.

> > +
> >                 /* Setup the QDialogButton Box */
> >                 QDialogButtonBox *dialogButtonBox =
> >                         new QDialogButtonBox(QDialogButtonBox::Ok |
> > @@ -46,6 +60,8 @@ public:
> >  
> >                 /* Set the layout. */
> >                 camSelectDialogLayout->addRow("Camera: ", cameraIdComboBox_);
> > +               camSelectDialogLayout->addRow("Location: ", cameraLocation_);
> > +               camSelectDialogLayout->addRow("Model: ", cameraModel_);
> >                 camSelectDialogLayout->addWidget(dialogButtonBox);
> >         }
> >  
> > @@ -70,9 +86,47 @@ public:
> >                 cameraIdComboBox_->removeItem(cameraIndex);
> >         }
> >  
> > +       /* Camera Information */
> > +       void updateCamInfo(const std::shared_ptr<libcamera::Camera> &camera)
> > +       {
> > +               if (camera == nullptr)

		if (!camera)

> > +                       return;
> > +
> > +               const libcamera::ControlList &cameraProperties = camera->properties();
> > +
> > +               const auto &location =
> > +                       cameraProperties.get(libcamera::properties::Location);
> > +               if (location) {
> > +                       switch (*location) {
> > +                       case libcamera::properties::CameraLocationFront:
> > +                               cameraLocation_->setText("Internal front camera ");
> 
> Do these need the space at the end? or could those be removed?
> 
> 
> > +                               break;
> > +                       case libcamera::properties::CameraLocationBack:
> > +                               cameraLocation_->setText("Internal back camera ");
> > +                               break;
> > +                       case libcamera::properties::CameraLocationExternal:
> > +                               cameraLocation_->setText("External camera ");
> > +                               break;
> > +                       default:
> > +                               cameraLocation_->setText(QString());

"Unknown" may be better than an empty string.

> > +                       }
> > +               } else {
> > +                       cameraLocation_->setText(QString());

Same here.

> > +               }
> > +
> > +               const auto &model = cameraProperties.get(libcamera::properties::Model);
> 
> I think with the new 'optional' controls this could read something like
> (not tested/checked)
> 		 const std::string model = cameraProperties
> 		 				.get(libcamera::properties::Model)
> 						.value_or(utils::defopt)
> 
> To save having to be conditional on the if (model) below...

That's nicer. It could also be writte .value_or("") as constructing an
empty string is cheap.

Similarly to the location, maybe "Unknown" would be better too ?

> But it all looks functional, and I've seen the output so:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > +
> > +               if (model)
> > +                       cameraModel_->setText(QString::fromStdString(*model));
> > +               else
> > +                       cameraModel_->setText(QString());
> > +       }
> > +
> >  private:
> >         libcamera::CameraManager *cm_;
> >  
> >         /* UI elements. */
> >         QComboBox *cameraIdComboBox_;
> > +       QLabel *cameraLocation_;
> > +       QLabel *cameraModel_;
> >  };

-- 
Regards,

Laurent Pinchart