<div dir="auto"><div>Hi Kieran, thanks for the review.<br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 10 Aug, 2022, 03:08 Kieran Bingham, <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank" rel="noreferrer">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Quoting Utkarsh Tiwari (2022-08-09 21:50:35)<br>
> Currently we use QInputDialog convenience dialogs to allow the user to<br>
> select a camera. This doesn't allow adding of more information (such as<br>
> camera location, model etc).<br>
> <br>
> Create a QDialog with a QFormLayout that shows a QComboBox with camera<br>
> Ids. Use a QDialogButtonBox to provide buttons for accepting and<br>
> cancelling the action.<br>
> <br>
> Signed-off-by: Utkarsh Tiwari <<a href="mailto:utkarsh02t@gmail.com" rel="noreferrer noreferrer" target="_blank">utkarsh02t@gmail.com</a>><br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" rel="noreferrer noreferrer" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
This hasn't been given yet on this patch. Please take care to only apply<br>
tags on patches that are given.</blockquote></div></div><div dir="auto">Oops, I apologise I mistakenly added it here.</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> ---<br>
> Difference<br>
> 1. Renamed dialogButtonBox to buttonBox<br>
> 2. Removed spaces<br>
> 3. Renamed to just layout<br>
> 4. No longer use the QPointer<br>
> 5. The dialog is always constructed now<br>
> src/qcam/cam_select_dialog.cpp | 51 ++++++++++++++++++++++++++++++++++<br>
> src/qcam/cam_select_dialog.h | 37 ++++++++++++++++++++++++<br>
> src/qcam/main_window.cpp | 39 +++++++++-----------------<br>
> src/qcam/main_window.h | 4 +++<br>
> src/qcam/meson.build | 2 ++<br>
> 5 files changed, 107 insertions(+), 26 deletions(-)<br>
> create mode 100644 src/qcam/cam_select_dialog.cpp<br>
> create mode 100644 src/qcam/cam_select_dialog.h<br>
> <br>
> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp<br>
> new file mode 100644<br>
> index 00000000..dceaa590<br>
> --- /dev/null<br>
> +++ b/src/qcam/cam_select_dialog.cpp<br>
> @@ -0,0 +1,51 @@<br>
> +/* SPDX-License-Identifier: GPL-2.0-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Utkarsh Tiwari <<a href="mailto:utkarsh02t@gmail.com" rel="noreferrer noreferrer" target="_blank">utkarsh02t@gmail.com</a>><br>
> + *<br>
> + * cam_select_dialog.cpp - qcam - Camera Selection dialog<br>
> + */<br>
> +<br>
> +#include "cam_select_dialog.h"<br>
> +<br>
> +#include <string><br>
> +<br>
> +#include <libcamera/camera.h><br>
> +#include <libcamera/camera_manager.h><br>
> +<br>
> +#include <QComboBox><br>
> +#include <QDialog><br>
> +#include <QDialogButtonBox><br>
> +#include <QFormLayout><br>
> +#include <QString><br>
> +<br>
> +CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,<br>
> + QWidget *parent)<br>
> + : QDialog(parent), cm_(cameraManager)<br>
> +{<br>
> + /* Use a QFormLayout for the dialog. */<br>
> + QFormLayout *layout = new QFormLayout(this);<br>
> +<br>
> + /* Setup the camera id combo-box. */<br>
> + cameraIdComboBox_ = new QComboBox;<br>
> + for (const auto &cam : cm_->cameras())<br>
> + cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));<br>
> +<br>
> + /* Setup the QDialogButton Box */<br>
> + QDialogButtonBox *buttonBox =<br>
> + new QDialogButtonBox(QDialogButtonBox::Ok |<br>
> + QDialogButtonBox::Cancel);<br>
> +<br>
> + connect(buttonBox, &QDialogButtonBox::accepted,<br>
> + this, &QDialog::accept);<br>
> + connect(buttonBox, &QDialogButtonBox::rejected,<br>
> + this, &QDialog::reject);<br>
> +<br>
> + /* Set the layout. */<br>
> + layout->addRow("Camera:", cameraIdComboBox_);<br>
> + layout->addWidget(buttonBox);<br>
> +}<br>
> +<br>
> +std::string CameraSelectorDialog::getCameraId()<br>
> +{<br>
> + return cameraIdComboBox_->currentText().toStdString();<br>
> +}<br>
> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h<br>
> new file mode 100644<br>
> index 00000000..8e54f916<br>
> --- /dev/null<br>
> +++ b/src/qcam/cam_select_dialog.h<br>
> @@ -0,0 +1,37 @@<br>
> +/* SPDX-License-Identifier: GPL-2.0-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Utkarsh Tiwari <<a href="mailto:utkarsh02t@gmail.com" rel="noreferrer noreferrer" target="_blank">utkarsh02t@gmail.com</a>><br>
> + *<br>
> + * cam_select_dialog.h - qcam - Camera Selection dialog<br>
> + */<br>
> +<br>
> +#pragma once<br>
> +<br>
> +#include <string><br>
> +<br>
> +#include <libcamera/camera.h><br>
> +#include <libcamera/camera_manager.h><br>
> +<br>
> +#include <QComboBox><br>
> +#include <QDialog><br>
> +#include <QDialogButtonBox><br>
> +#include <QFormLayout><br>
> +#include <QString><br>
> +<br>
> +class CameraSelectorDialog : public QDialog<br>
> +{<br>
> + Q_OBJECT<br>
> +public:<br>
> + CameraSelectorDialog(libcamera::CameraManager *cameraManager,<br>
> + QWidget *parent);<br>
> +<br>
> + ~CameraSelectorDialog() = default;<br>
> +<br>
> + std::string getCameraId();<br>
> +<br>
> +private:<br>
> + libcamera::CameraManager *cm_;<br>
> +<br>
> + /* UI elements. */<br>
> + QComboBox *cameraIdComboBox_;<br>
> +};<br>
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp<br>
> index 7433d647..48479f35 100644<br>
> --- a/src/qcam/main_window.cpp<br>
> +++ b/src/qcam/main_window.cpp<br>
> @@ -19,7 +19,6 @@<br>
> #include <QFileDialog><br>
> #include <QImage><br>
> #include <QImageWriter><br>
> -#include <QInputDialog><br>
> #include <QMutexLocker><br>
> #include <QStandardPaths><br>
> #include <QStringList><br>
> @@ -30,6 +29,7 @@<br>
> <br>
> #include "../cam/image.h"<br>
> <br>
> +#include "cam_select_dialog.h"<br>
> #include "dng_writer.h"<br>
> #ifndef QT_NO_OPENGL<br>
> #include "viewfinder_gl.h"<br>
> @@ -290,38 +290,25 @@ void MainWindow::switchCamera(int index)<br>
> <br>
> std::string MainWindow::chooseCamera()<br>
> {<br>
> - QStringList cameras;<br>
> - bool result;<br>
> -<br>
> - /* If only one camera is available, use it automatically. */<br>
> - if (cm_->cameras().size() == 1)<br>
> - return cm_->cameras()[0]->id();<br>
<br>
We've now lost this behaviour that if there is only one camera it will<br>
automatically be chosen when the application is started.<br>
<br>
While that might be fine, as we expect this to grow to support more<br>
configuration of the camera before it starts, it should at least be<br>
documented in the commit message.<br>
<br>
With the commit message updated, you can keep the tag:<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" rel="noreferrer noreferrer" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
> -<br>
> - /* Present a dialog box to pick a camera. */<br>
> - for (const std::shared_ptr<Camera> &cam : cm_->cameras())<br>
> - cameras.append(QString::fromStdString(cam->id()));<br>
> -<br>
> - QString id = QInputDialog::getItem(this, "Select Camera",<br>
> - "Camera:", cameras, 0,<br>
> - false, &result);<br>
> - if (!result)<br>
> - return std::string();<br>
> -<br>
> - return id.toStdString();<br>
> -}<br>
> -<br>
> -int MainWindow::openCamera()<br>
> -{<br>
> - std::string cameraName;<br>
> + /* Construct the selection dialog, unconditionally. */<br>
> + cameraSelectorDialog_ = new CameraSelectorDialog(cm_, this);<br>
> <br>
> /*<br>
> * Use the camera specified on the command line, if any, or display the<br>
> * camera selection dialog box otherwise.<br>
> */<br>
> if (options_.isSet(OptCamera))<br>
> - cameraName = static_cast<std::string>(options_[OptCamera]);<br>
> + return static_cast<std::string>(options_[OptCamera]);<br>
> +<br>
> + if (cameraSelectorDialog_->exec() == QDialog::Accepted)<br>
> + return cameraSelectorDialog_->getCameraId();<br>
> else<br>
> - cameraName = chooseCamera();<br>
> + return std::string();<br>
> +}<br>
> +<br>
> +int MainWindow::openCamera()<br>
> +{<br>
> + std::string cameraName = chooseCamera();<br>
> <br>
> if (cameraName == "")<br>
> return -EINVAL;<br>
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h<br>
> index fc70920f..b01d2e59 100644<br>
> --- a/src/qcam/main_window.h<br>
> +++ b/src/qcam/main_window.h<br>
> @@ -23,11 +23,13 @@<br>
> #include <QMainWindow><br>
> #include <QMutex><br>
> #include <QObject><br>
> +#include <QPointer><br>
> #include <QQueue><br>
> #include <QTimer><br>
> <br>
> #include "../cam/stream_options.h"<br>
> <br>
> +#include "cam_select_dialog.h"<br>
> #include "viewfinder.h"<br>
> <br>
> class QAction;<br>
> @@ -99,6 +101,8 @@ private:<br>
> QString title_;<br>
> QTimer titleTimer_;<br>
> <br>
> + CameraSelectorDialog *cameraSelectorDialog_;<br>
> +<br>
> /* Options */<br>
> const OptionsParser::Options &options_;<br>
> <br>
> diff --git a/src/qcam/meson.build b/src/qcam/meson.build<br>
> index c46f4631..61861ea6 100644<br>
> --- a/src/qcam/meson.build<br>
> +++ b/src/qcam/meson.build<br>
> @@ -18,6 +18,7 @@ qcam_sources = files([<br>
> '../cam/image.cpp',<br>
> '../cam/options.cpp',<br>
> '../cam/stream_options.cpp',<br>
> + 'cam_select_dialog.cpp',<br>
> 'format_converter.cpp',<br>
> 'main.cpp',<br>
> 'main_window.cpp',<br>
> @@ -26,6 +27,7 @@ qcam_sources = files([<br>
> ])<br>
> <br>
> qcam_moc_headers = files([<br>
> + 'cam_select_dialog.h',<br>
> 'main_window.h',<br>
> 'viewfinder_qt.h',<br>
> ])<br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div></div>