[libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for selection of cameras at startup
Utkarsh Tiwari
utkarsh02t at gmail.com
Mon Aug 8 08:56:41 CEST 2022
Hi Kieran thanks for the review.
On Mon, 8 Aug, 2022, 04:49 Kieran Bingham, <kieran.bingham at ideasonboard.com>
wrote:
> 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.
>
Oops copy paste error, It's my copyright.
>
>
> > + *
> > + * 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.
>
Fine by me.
>
>
> > +{
> > + 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.
>
>
Yes, to show the camera info I need to use cm_ to get the camera.
>
> > + {
> > + /* 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)
>
>
Yes saw your other reply, and yes it's done in patch 2nd.
>
> > +
> > + /* 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.
>
> Sure, I would wait on review for the other patches then move everything
to its .cpp.
> > +
> > +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?
>
Yes we need it as a class variable for camera hotplugging.
>
> >
> > - /* 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220808/3d2bc8d6/attachment.htm>
More information about the libcamera-devel
mailing list