[libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for selection of cameras at startup

[Kieran Bingham]

Quoting Kieran Bingham (2022-08-08 00:19:36)
> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-06 20:04:30)
> > Currently we use QInputDialog convenience dialogs to allow the user to
> > select a camera. This doesn't allow adding of more information (such as
> > camera location, model etc).
> > 
> > Create a QDialog with a QFormLayout that shows a QComboBox with camera
> > Ids. Use a QDialogButtonBox to provide buttons for accepting and
> > cancelling the action.
> > 
> > Signed-off-by: Utkarsh Tiwari <utkarsh02t at gmail.com>
> > ---
> > Difference from V1:
> > The biggest change here is the introduction of class,  inside of
> > the class the layout remains same.
> >  src/qcam/cam_select_dialog.h | 64 ++++++++++++++++++++++++++++++++++++
> >  src/qcam/main_window.cpp     | 22 +++----------
> >  src/qcam/main_window.h       |  4 +++
> >  src/qcam/meson.build         |  1 +
> >  4 files changed, 74 insertions(+), 17 deletions(-)
> >  create mode 100644 src/qcam/cam_select_dialog.h
> > 
> > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h
> > new file mode 100644
> > index 00000000..c23bad59
> > --- /dev/null
> > +++ b/src/qcam/cam_select_dialog.h
> > @@ -0,0 +1,64 @@
> > +
> 
> Remove the first blank line please.
> 
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> 
> I think this should be 2022. I'm not sure if copyright goes to you or
> Google for GSoC.
> 
> 
> > + *
> > + * cam_select_dialog.h - qcam - Camera Selection dialog
> > + */
> > +
> > +#pragma once
> > +
> > +#include <string>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/camera_manager.h>
> > +
> > +#include <QComboBox>
> > +#include <QDialog>
> > +#include <QDialogButtonBox>
> > +#include <QFormLayout>
> > +#include <QString>
> > +
> > +class CamSelectDialog : public QDialog
> 
> It's bikeshedding territory - but CameraSelectDialog sounds better to me
> here. (Spell 'Camera' in full).
> 
> It seems Laurent suggested CameraSelectorDialog too.
> 
> 
> > +{
> > +       Q_OBJECT
> > +public:
> > +       CamSelectDialog(libcamera::CameraManager *cameraManager, QWidget *parent)
> > +               : QDialog(parent), cm_(cameraManager)
> 
> Is cm_ used later? I don't mind it being added here if it's needed
> later, but it only seems to be used in this construction for the moment.
> 
> 
> > +       {
> > +               /* Use a QFormLayout for the dialog. */
> > +               QFormLayout *camSelectDialogLayout = new QFormLayout(this);
> > +
> > +               /* Setup the camera id combo-box. */
> > +               cameraIdComboBox_ = new QComboBox;
> > +               for (const auto &cam : cm_->cameras())
> > +                       cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));
> 
> I could imagine we need to add hotplug events in here too.
> It could be done on top, but to test it - try removing or adding a
> camera while the dialog box is opened.
> 
> As long as it doesn't crash if the camera is removed while the dialog
> box is open, I don't mind if the hotplug (adding cameras when they are
> plugged in, removing when removed) is added on top. Especially as the
> existing dialog didn't support hotplug either.
> 
> 
> I guess if there's no camera left, the dialog box doesn't have much to
> do ;-) (though should stay open in case someone replugs the UVC camera)
> 
> 
> > +
> > +               /* Setup the QDialogButton Box */
> > +               QDialogButtonBox *dialogButtonBox =
> > +                       new QDialogButtonBox(QDialogButtonBox::Ok |
> > +                                            QDialogButtonBox::Cancel);
> > +
> > +               connect(dialogButtonBox, &QDialogButtonBox::accepted,
> > +                       this, &QDialog::accept);
> > +               connect(dialogButtonBox, &QDialogButtonBox::rejected,
> > +                       this, &QDialog::reject);
> > +
> > +               /* Set the layout. */
> > +               camSelectDialogLayout->addRow("Camera: ", cameraIdComboBox_);
> > +               camSelectDialogLayout->addWidget(dialogButtonBox);
> > +       }
> > +
> > +       ~CamSelectDialog() = default;
> > +
> > +       std::string getCameraId()
> > +       {
> > +               return cameraIdComboBox_->currentText().toStdString();
> > +       }
> 
> While there's not a lot of implementation here, I think the code should
> be in a .cpp file. I'm not sure where the threshold is for how small a
> class can be to be inlined directly in the header, but I think this one
> can potentially expand, so probably deserves it's own implementation
> file.
> 
> > +
> > +private:
> > +       libcamera::CameraManager *cm_;
> > +
> > +       /* UI elements. */
> > +       QComboBox *cameraIdComboBox_;
> > +};
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 7433d647..758e2c94 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -19,7 +19,6 @@
> >  #include <QFileDialog>
> >  #include <QImage>
> >  #include <QImageWriter>
> > -#include <QInputDialog>
> >  #include <QMutexLocker>
> >  #include <QStandardPaths>
> >  #include <QStringList>
> > @@ -30,6 +29,7 @@
> >  
> >  #include "../cam/image.h"
> >  
> > +#include "cam_select_dialog.h"
> >  #include "dng_writer.h"
> >  #ifndef QT_NO_OPENGL
> >  #include "viewfinder_gl.h"
> > @@ -290,24 +290,12 @@ void MainWindow::switchCamera(int index)
> >  
> >  std::string MainWindow::chooseCamera()
> >  {
> > -       QStringList cameras;
> > -       bool result;
> > +       camSelectDialog_ = new CamSelectDialog(cm_, this);
> 
> Do we need to keep the dialog in a class variable? or could it be local
> and destroyed at the end of this function?

Aha, the next patch answers most of my questions I think ... hotplug and
keeping this accessible.

I'll check through that tomorrow.


> 
> >  
> > -       /* If only one camera is available, use it automatically. */
> > -       if (cm_->cameras().size() == 1)
> > -               return cm_->cameras()[0]->id();
> > -
> > -       /* Present a dialog box to pick a camera. */
> > -       for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> > -               cameras.append(QString::fromStdString(cam->id()));
> > -
> > -       QString id = QInputDialog::getItem(this, "Select Camera",
> > -                                          "Camera:", cameras, 0,
> > -                                          false, &result);
> > -       if (!result)
> > +       if (camSelectDialog_->exec() == QDialog::Accepted)
> > +               return camSelectDialog_->getCameraId();
> > +       else
> >                 return std::string();
> > -
> > -       return id.toStdString();
> >  }
> >  
> >  int MainWindow::openCamera()
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index fc70920f..6d80b5be 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -23,11 +23,13 @@
> >  #include <QMainWindow>
> >  #include <QMutex>
> >  #include <QObject>
> > +#include <QPointer>
> >  #include <QQueue>
> >  #include <QTimer>
> >  
> >  #include "../cam/stream_options.h"
> >  
> > +#include "cam_select_dialog.h"
> >  #include "viewfinder.h"
> >  
> >  class QAction;
> > @@ -99,6 +101,8 @@ private:
> >         QString title_;
> >         QTimer titleTimer_;
> >  
> > +       QPointer<CamSelectDialog> camSelectDialog_;
> > +
> >         /* Options */
> >         const OptionsParser::Options &options_;
> >  
> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build
> > index c46f4631..70615e92 100644
> > --- a/src/qcam/meson.build
> > +++ b/src/qcam/meson.build
> > @@ -26,6 +26,7 @@ qcam_sources = files([
> >  ])
> >  
> >  qcam_moc_headers = files([
> > +    'cam_select_dialog.h',
> >      'main_window.h',
> >      'viewfinder_qt.h',
> >  ])
> > -- 
> > 2.25.1
> >