| Message ID | 20220809205042.344923-2-utkarsh02t@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Utkarsh Tiwari (2022-08-09 21:50:35) > 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@gmail.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> This hasn't been given yet on this patch. Please take care to only apply tags on patches that are given. > --- > Difference > 1. Renamed dialogButtonBox to buttonBox > 2. Removed spaces > 3. Renamed to just layout > 4. No longer use the QPointer > 5. The dialog is always constructed now > src/qcam/cam_select_dialog.cpp | 51 ++++++++++++++++++++++++++++++++++ > src/qcam/cam_select_dialog.h | 37 ++++++++++++++++++++++++ > src/qcam/main_window.cpp | 39 +++++++++----------------- > src/qcam/main_window.h | 4 +++ > src/qcam/meson.build | 2 ++ > 5 files changed, 107 insertions(+), 26 deletions(-) > create mode 100644 src/qcam/cam_select_dialog.cpp > create mode 100644 src/qcam/cam_select_dialog.h > > diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp > new file mode 100644 > index 00000000..dceaa590 > --- /dev/null > +++ b/src/qcam/cam_select_dialog.cpp > @@ -0,0 +1,51 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2022, Utkarsh Tiwari <utkarsh02t@gmail.com> > + * > + * cam_select_dialog.cpp - qcam - Camera Selection dialog > + */ > + > +#include "cam_select_dialog.h" > + > +#include <string> > + > +#include <libcamera/camera.h> > +#include <libcamera/camera_manager.h> > + > +#include <QComboBox> > +#include <QDialog> > +#include <QDialogButtonBox> > +#include <QFormLayout> > +#include <QString> > + > +CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager, > + QWidget *parent) > + : QDialog(parent), cm_(cameraManager) > +{ > + /* Use a QFormLayout for the dialog. */ > + QFormLayout *layout = new QFormLayout(this); > + > + /* Setup the camera id combo-box. */ > + cameraIdComboBox_ = new QComboBox; > + for (const auto &cam : cm_->cameras()) > + cameraIdComboBox_->addItem(QString::fromStdString(cam->id())); > + > + /* Setup the QDialogButton Box */ > + QDialogButtonBox *buttonBox = > + new QDialogButtonBox(QDialogButtonBox::Ok | > + QDialogButtonBox::Cancel); > + > + connect(buttonBox, &QDialogButtonBox::accepted, > + this, &QDialog::accept); > + connect(buttonBox, &QDialogButtonBox::rejected, > + this, &QDialog::reject); > + > + /* Set the layout. */ > + layout->addRow("Camera:", cameraIdComboBox_); > + layout->addWidget(buttonBox); > +} > + > +std::string CameraSelectorDialog::getCameraId() > +{ > + return cameraIdComboBox_->currentText().toStdString(); > +} > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h > new file mode 100644 > index 00000000..8e54f916 > --- /dev/null > +++ b/src/qcam/cam_select_dialog.h > @@ -0,0 +1,37 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2022, Utkarsh Tiwari <utkarsh02t@gmail.com> > + * > + * 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 CameraSelectorDialog : public QDialog > +{ > + Q_OBJECT > +public: > + CameraSelectorDialog(libcamera::CameraManager *cameraManager, > + QWidget *parent); > + > + ~CameraSelectorDialog() = default; > + > + std::string getCameraId(); > + > +private: > + libcamera::CameraManager *cm_; > + > + /* UI elements. */ > + QComboBox *cameraIdComboBox_; > +}; > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 7433d647..48479f35 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,38 +290,25 @@ void MainWindow::switchCamera(int index) > > std::string MainWindow::chooseCamera() > { > - QStringList cameras; > - bool result; > - > - /* If only one camera is available, use it automatically. */ > - if (cm_->cameras().size() == 1) > - return cm_->cameras()[0]->id(); We've now lost this behaviour that if there is only one camera it will automatically be chosen when the application is started. While that might be fine, as we expect this to grow to support more configuration of the camera before it starts, it should at least be documented in the commit message. With the commit message updated, you can keep the tag: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > - > - /* 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) > - return std::string(); > - > - return id.toStdString(); > -} > - > -int MainWindow::openCamera() > -{ > - std::string cameraName; > + /* Construct the selection dialog, unconditionally. */ > + cameraSelectorDialog_ = new CameraSelectorDialog(cm_, this); > > /* > * Use the camera specified on the command line, if any, or display the > * camera selection dialog box otherwise. > */ > if (options_.isSet(OptCamera)) > - cameraName = static_cast<std::string>(options_[OptCamera]); > + return static_cast<std::string>(options_[OptCamera]); > + > + if (cameraSelectorDialog_->exec() == QDialog::Accepted) > + return cameraSelectorDialog_->getCameraId(); > else > - cameraName = chooseCamera(); > + return std::string(); > +} > + > +int MainWindow::openCamera() > +{ > + std::string cameraName = chooseCamera(); > > if (cameraName == "") > return -EINVAL; > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > index fc70920f..b01d2e59 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_; > > + CameraSelectorDialog *cameraSelectorDialog_; > + > /* Options */ > const OptionsParser::Options &options_; > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build > index c46f4631..61861ea6 100644 > --- a/src/qcam/meson.build > +++ b/src/qcam/meson.build > @@ -18,6 +18,7 @@ qcam_sources = files([ > '../cam/image.cpp', > '../cam/options.cpp', > '../cam/stream_options.cpp', > + 'cam_select_dialog.cpp', > 'format_converter.cpp', > 'main.cpp', > 'main_window.cpp', > @@ -26,6 +27,7 @@ qcam_sources = files([ > ]) > > qcam_moc_headers = files([ > + 'cam_select_dialog.h', > 'main_window.h', > 'viewfinder_qt.h', > ]) > -- > 2.25.1 >
Hi Kieran, thanks for the review. On Wed, 10 Aug, 2022, 03:08 Kieran Bingham, <kieran.bingham@ideasonboard.com> wrote: > Quoting Utkarsh Tiwari (2022-08-09 21:50:35) > > 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@gmail.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > This hasn't been given yet on this patch. Please take care to only apply > tags on patches that are given. Oops, I apologise I mistakenly added it here. > > --- > > Difference > > 1. Renamed dialogButtonBox to buttonBox > > 2. Removed spaces > > 3. Renamed to just layout > > 4. No longer use the QPointer > > 5. The dialog is always constructed now > > src/qcam/cam_select_dialog.cpp | 51 ++++++++++++++++++++++++++++++++++ > > src/qcam/cam_select_dialog.h | 37 ++++++++++++++++++++++++ > > src/qcam/main_window.cpp | 39 +++++++++----------------- > > src/qcam/main_window.h | 4 +++ > > src/qcam/meson.build | 2 ++ > > 5 files changed, 107 insertions(+), 26 deletions(-) > > create mode 100644 src/qcam/cam_select_dialog.cpp > > create mode 100644 src/qcam/cam_select_dialog.h > > > > diff --git a/src/qcam/cam_select_dialog.cpp > b/src/qcam/cam_select_dialog.cpp > > new file mode 100644 > > index 00000000..dceaa590 > > --- /dev/null > > +++ b/src/qcam/cam_select_dialog.cpp > > @@ -0,0 +1,51 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2022, Utkarsh Tiwari <utkarsh02t@gmail.com> > > + * > > + * cam_select_dialog.cpp - qcam - Camera Selection dialog > > + */ > > + > > +#include "cam_select_dialog.h" > > + > > +#include <string> > > + > > +#include <libcamera/camera.h> > > +#include <libcamera/camera_manager.h> > > + > > +#include <QComboBox> > > +#include <QDialog> > > +#include <QDialogButtonBox> > > +#include <QFormLayout> > > +#include <QString> > > + > > +CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager > *cameraManager, > > + QWidget *parent) > > + : QDialog(parent), cm_(cameraManager) > > +{ > > + /* Use a QFormLayout for the dialog. */ > > + QFormLayout *layout = new QFormLayout(this); > > + > > + /* Setup the camera id combo-box. */ > > + cameraIdComboBox_ = new QComboBox; > > + for (const auto &cam : cm_->cameras()) > > + > cameraIdComboBox_->addItem(QString::fromStdString(cam->id())); > > + > > + /* Setup the QDialogButton Box */ > > + QDialogButtonBox *buttonBox = > > + new QDialogButtonBox(QDialogButtonBox::Ok | > > + QDialogButtonBox::Cancel); > > + > > + connect(buttonBox, &QDialogButtonBox::accepted, > > + this, &QDialog::accept); > > + connect(buttonBox, &QDialogButtonBox::rejected, > > + this, &QDialog::reject); > > + > > + /* Set the layout. */ > > + layout->addRow("Camera:", cameraIdComboBox_); > > + layout->addWidget(buttonBox); > > +} > > + > > +std::string CameraSelectorDialog::getCameraId() > > +{ > > + return cameraIdComboBox_->currentText().toStdString(); > > +} > > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h > > new file mode 100644 > > index 00000000..8e54f916 > > --- /dev/null > > +++ b/src/qcam/cam_select_dialog.h > > @@ -0,0 +1,37 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2022, Utkarsh Tiwari <utkarsh02t@gmail.com> > > + * > > + * 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 CameraSelectorDialog : public QDialog > > +{ > > + Q_OBJECT > > +public: > > + CameraSelectorDialog(libcamera::CameraManager *cameraManager, > > + QWidget *parent); > > + > > + ~CameraSelectorDialog() = default; > > + > > + std::string getCameraId(); > > + > > +private: > > + libcamera::CameraManager *cm_; > > + > > + /* UI elements. */ > > + QComboBox *cameraIdComboBox_; > > +}; > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > index 7433d647..48479f35 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,38 +290,25 @@ void MainWindow::switchCamera(int index) > > > > std::string MainWindow::chooseCamera() > > { > > - QStringList cameras; > > - bool result; > > - > > - /* If only one camera is available, use it automatically. */ > > - if (cm_->cameras().size() == 1) > > - return cm_->cameras()[0]->id(); > > We've now lost this behaviour that if there is only one camera it will > automatically be chosen when the application is started. > > While that might be fine, as we expect this to grow to support more > configuration of the camera before it starts, it should at least be > documented in the commit message. > > With the commit message updated, you can keep the tag: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > - > > - /* 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) > > - return std::string(); > > - > > - return id.toStdString(); > > -} > > - > > -int MainWindow::openCamera() > > -{ > > - std::string cameraName; > > + /* Construct the selection dialog, unconditionally. */ > > + cameraSelectorDialog_ = new CameraSelectorDialog(cm_, this); > > > > /* > > * Use the camera specified on the command line, if any, or > display the > > * camera selection dialog box otherwise. > > */ > > if (options_.isSet(OptCamera)) > > - cameraName = > static_cast<std::string>(options_[OptCamera]); > > + return static_cast<std::string>(options_[OptCamera]); > > + > > + if (cameraSelectorDialog_->exec() == QDialog::Accepted) > > + return cameraSelectorDialog_->getCameraId(); > > else > > - cameraName = chooseCamera(); > > + return std::string(); > > +} > > + > > +int MainWindow::openCamera() > > +{ > > + std::string cameraName = chooseCamera(); > > > > if (cameraName == "") > > return -EINVAL; > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > > index fc70920f..b01d2e59 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_; > > > > + CameraSelectorDialog *cameraSelectorDialog_; > > + > > /* Options */ > > const OptionsParser::Options &options_; > > > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build > > index c46f4631..61861ea6 100644 > > --- a/src/qcam/meson.build > > +++ b/src/qcam/meson.build > > @@ -18,6 +18,7 @@ qcam_sources = files([ > > '../cam/image.cpp', > > '../cam/options.cpp', > > '../cam/stream_options.cpp', > > + 'cam_select_dialog.cpp', > > 'format_converter.cpp', > > 'main.cpp', > > 'main_window.cpp', > > @@ -26,6 +27,7 @@ qcam_sources = files([ > > ]) > > > > qcam_moc_headers = files([ > > + 'cam_select_dialog.h', > > 'main_window.h', > > 'viewfinder_qt.h', > > ]) > > -- > > 2.25.1 > > >
diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp new file mode 100644 index 00000000..dceaa590 --- /dev/null +++ b/src/qcam/cam_select_dialog.cpp @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2022, Utkarsh Tiwari <utkarsh02t@gmail.com> + * + * cam_select_dialog.cpp - qcam - Camera Selection dialog + */ + +#include "cam_select_dialog.h" + +#include <string> + +#include <libcamera/camera.h> +#include <libcamera/camera_manager.h> + +#include <QComboBox> +#include <QDialog> +#include <QDialogButtonBox> +#include <QFormLayout> +#include <QString> + +CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager, + QWidget *parent) + : QDialog(parent), cm_(cameraManager) +{ + /* Use a QFormLayout for the dialog. */ + QFormLayout *layout = new QFormLayout(this); + + /* Setup the camera id combo-box. */ + cameraIdComboBox_ = new QComboBox; + for (const auto &cam : cm_->cameras()) + cameraIdComboBox_->addItem(QString::fromStdString(cam->id())); + + /* Setup the QDialogButton Box */ + QDialogButtonBox *buttonBox = + new QDialogButtonBox(QDialogButtonBox::Ok | + QDialogButtonBox::Cancel); + + connect(buttonBox, &QDialogButtonBox::accepted, + this, &QDialog::accept); + connect(buttonBox, &QDialogButtonBox::rejected, + this, &QDialog::reject); + + /* Set the layout. */ + layout->addRow("Camera:", cameraIdComboBox_); + layout->addWidget(buttonBox); +} + +std::string CameraSelectorDialog::getCameraId() +{ + return cameraIdComboBox_->currentText().toStdString(); +} diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h new file mode 100644 index 00000000..8e54f916 --- /dev/null +++ b/src/qcam/cam_select_dialog.h @@ -0,0 +1,37 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2022, Utkarsh Tiwari <utkarsh02t@gmail.com> + * + * 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 CameraSelectorDialog : public QDialog +{ + Q_OBJECT +public: + CameraSelectorDialog(libcamera::CameraManager *cameraManager, + QWidget *parent); + + ~CameraSelectorDialog() = default; + + std::string getCameraId(); + +private: + libcamera::CameraManager *cm_; + + /* UI elements. */ + QComboBox *cameraIdComboBox_; +}; diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index 7433d647..48479f35 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,38 +290,25 @@ void MainWindow::switchCamera(int index) std::string MainWindow::chooseCamera() { - QStringList cameras; - bool result; - - /* 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) - return std::string(); - - return id.toStdString(); -} - -int MainWindow::openCamera() -{ - std::string cameraName; + /* Construct the selection dialog, unconditionally. */ + cameraSelectorDialog_ = new CameraSelectorDialog(cm_, this); /* * Use the camera specified on the command line, if any, or display the * camera selection dialog box otherwise. */ if (options_.isSet(OptCamera)) - cameraName = static_cast<std::string>(options_[OptCamera]); + return static_cast<std::string>(options_[OptCamera]); + + if (cameraSelectorDialog_->exec() == QDialog::Accepted) + return cameraSelectorDialog_->getCameraId(); else - cameraName = chooseCamera(); + return std::string(); +} + +int MainWindow::openCamera() +{ + std::string cameraName = chooseCamera(); if (cameraName == "") return -EINVAL; diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h index fc70920f..b01d2e59 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_; + CameraSelectorDialog *cameraSelectorDialog_; + /* Options */ const OptionsParser::Options &options_; diff --git a/src/qcam/meson.build b/src/qcam/meson.build index c46f4631..61861ea6 100644 --- a/src/qcam/meson.build +++ b/src/qcam/meson.build @@ -18,6 +18,7 @@ qcam_sources = files([ '../cam/image.cpp', '../cam/options.cpp', '../cam/stream_options.cpp', + 'cam_select_dialog.cpp', 'format_converter.cpp', 'main.cpp', 'main_window.cpp', @@ -26,6 +27,7 @@ qcam_sources = files([ ]) qcam_moc_headers = files([ + 'cam_select_dialog.h', 'main_window.h', 'viewfinder_qt.h', ])