| Message ID | 20220806190433.59128-2-utkarsh02t@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 >
Quoting Kieran Bingham (2022-08-08 00:19:36) > 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@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? Aha, the next patch answers most of my questions I think ... hotplug and keeping this accessible. I'll check through that tomorrow. > > > > > - /* 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 > >
Hi Kieran thanks for the review. On Mon, 8 Aug, 2022, 04:49 Kieran Bingham, <kieran.bingham@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@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 > > >
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@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@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 > > > > >
Hi Kieran thanks for the review. On Mon, Aug 8, 2022 at 2:15 PM Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > 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@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@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. ? > > I think we can re-use it. And I think the current way is causing a leak :P. > > > > > > > > > > > > > > - /* 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 > > > > > > > >
On Mon, Aug 08, 2022 at 10:49:20PM +0530, Utkarsh Tiwari via libcamera-devel wrote: > On Mon, Aug 8, 2022 at 2:15 PM Kieran Bingham wrote: > > Quoting Utkarsh Tiwari (2022-08-08 07:56:41) > > > On Mon, 8 Aug, 2022, 04:49 Kieran Bingham 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@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); You can simply call the variable "layout". > > > > > + > > > > > + /* 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 = And this can be called "buttonBox" or even just "buttons". There's no need for the "dialog" prefix in the name as all these variables are within the scope of CamSelectDialog. > > > > > + 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_); No need for a space after ':'. > > > > > + 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. I was about to say the same, the implementation should move to cam_select_dialog.cpp. > > > 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. ? > > I think we can re-use it. And I think the current way is causing a leak > :P. Reusing the dialog would be best indeed. Also, why do you use a QPointer ? > > > > > - /* 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', > > > > > ])
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 @@ + +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * 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 +{ + Q_OBJECT +public: + CamSelectDialog(libcamera::CameraManager *cameraManager, QWidget *parent) + : QDialog(parent), cm_(cameraManager) + { + /* 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())); + + /* 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(); + } + +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); - /* 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', ])
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> --- 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