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

[Kieran Bingham]

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?

>  
> -       /* 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
>