[libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for selection of cameras at startup
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Aug 8 10:45:32 CEST 2022
Quoting Utkarsh Tiwari (2022-08-08 07:56:41)
> 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.
This seems to recreate the CamSelectDialog box each time from scratch,
even though we constructed it - and keep it updated with hotplug events.
Is it possible to re-use it when it's created? or does accepting the
dialog or closing it make it no longer possible to re-use?
If we can re-use it - we should ... Otherwise - we probably shouldn't
send hotplug events when it's been closed. ?
>
> >
> > >
> > > - /* 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
> > >
> >
More information about the libcamera-devel
mailing list