<div dir="ltr"><div dir="auto"><div>Hi Kieran thanks for the review.<br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 8 Aug, 2022, 04:49 Kieran Bingham, <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">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 via libcamera-devel (2022-08-06 20:04:30)<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" target="_blank">utkarsh02t@gmail.com</a>><br>
> ---<br>
> Difference from V1:<br>
> The biggest change here is the introduction of class, inside of<br>
> the class the layout remains same.<br>
> src/qcam/cam_select_dialog.h | 64 ++++++++++++++++++++++++++++++++++++<br>
> src/qcam/main_window.cpp | 22 +++----------<br>
> src/qcam/main_window.h | 4 +++<br>
> src/qcam/meson.build | 1 +<br>
> 4 files changed, 74 insertions(+), 17 deletions(-)<br>
> create mode 100644 src/qcam/cam_select_dialog.h<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..c23bad59<br>
> --- /dev/null<br>
> +++ b/src/qcam/cam_select_dialog.h<br>
> @@ -0,0 +1,64 @@<br>
> +<br>
<br>
Remove the first blank line please.<br>
<br>
> +/* SPDX-License-Identifier: GPL-2.0-or-later */<br>
> +/*<br>
> + * Copyright (C) 2019, Google Inc.<br>
<br>
I think this should be 2022. I'm not sure if copyright goes to you or<br>
Google for GSoC.<br></blockquote></div></div><div dir="auto">Oops copy paste error, It's my copyright.</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>
<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 CamSelectDialog : public QDialog<br>
<br>
It's bikeshedding territory - but CameraSelectDialog sounds better to me<br>
here. (Spell 'Camera' in full).<br>
<br>
It seems Laurent suggested CameraSelectorDialog too.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Fine by me. </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>
<br>
> +{<br>
> + Q_OBJECT<br>
> +public:<br>
> + CamSelectDialog(libcamera::CameraManager *cameraManager, QWidget *parent)<br>
> + : QDialog(parent), cm_(cameraManager)<br>
<br>
Is cm_ used later? I don't mind it being added here if it's needed<br>
later, but it only seems to be used in this construction for the moment.<br>
<br></blockquote><div><br></div><div>Yes, to show the camera info I need to use cm_ to get the camera. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + {<br>
> + /* Use a QFormLayout for the dialog. */<br>
> + QFormLayout *camSelectDialogLayout = 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>
I could imagine we need to add hotplug events in here too.<br>
It could be done on top, but to test it - try removing or adding a<br>
camera while the dialog box is opened.<br>
<br>
As long as it doesn't crash if the camera is removed while the dialog<br>
box is open, I don't mind if the hotplug (adding cameras when they are<br>
plugged in, removing when removed) is added on top. Especially as the<br>
existing dialog didn't support hotplug either.<br>
<br>
<br>
I guess if there's no camera left, the dialog box doesn't have much to<br>
do ;-) (though should stay open in case someone replugs the UVC camera)<br>
<br></blockquote><div><br></div><div>Yes saw your other reply, and yes it's done in patch 2nd.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> + /* Setup the QDialogButton Box */<br>
> + QDialogButtonBox *dialogButtonBox =<br>
> + new QDialogButtonBox(QDialogButtonBox::Ok |<br>
> + QDialogButtonBox::Cancel);<br>
> +<br>
> + connect(dialogButtonBox, &QDialogButtonBox::accepted,<br>
> + this, &QDialog::accept);<br>
> + connect(dialogButtonBox, &QDialogButtonBox::rejected,<br>
> + this, &QDialog::reject);<br>
> +<br>
> + /* Set the layout. */<br>
> + camSelectDialogLayout->addRow("Camera: ", cameraIdComboBox_);<br>
> + camSelectDialogLayout->addWidget(dialogButtonBox);<br>
> + }<br>
> +<br>
> + ~CamSelectDialog() = default;<br>
> +<br>
> + std::string getCameraId()<br>
> + {<br>
> + return cameraIdComboBox_->currentText().toStdString();<br>
> + }<br>
<br>
While there's not a lot of implementation here, I think the code should<br>
be in a .cpp file. I'm not sure where the threshold is for how small a<br>
class can be to be inlined directly in the header, but I think this one<br>
can potentially expand, so probably deserves it's own implementation<br>
file.<br>
<br></blockquote><div>Sure, I would wait on review for the other patches then move everything <br></div><div>to its .cpp. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +<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..758e2c94 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,24 +290,12 @@ void MainWindow::switchCamera(int index)<br>
> <br>
> std::string MainWindow::chooseCamera()<br>
> {<br>
> - QStringList cameras;<br>
> - bool result;<br>
> + camSelectDialog_ = new CamSelectDialog(cm_, this);<br>
<br>
Do we need to keep the dialog in a class variable? or could it be local<br>
and destroyed at the end of this function?<br></blockquote><div>Yes we need it as a class variable for camera hotplugging. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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>
> - /* 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>
> + if (camSelectDialog_->exec() == QDialog::Accepted)<br>
> + return camSelectDialog_->getCameraId();<br>
> + else<br>
> return std::string();<br>
> -<br>
> - return id.toStdString();<br>
> }<br>
> <br>
> int MainWindow::openCamera()<br>
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h<br>
> index fc70920f..6d80b5be 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>
> + QPointer<CamSelectDialog> camSelectDialog_;<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..70615e92 100644<br>
> --- a/src/qcam/meson.build<br>
> +++ b/src/qcam/meson.build<br>
> @@ -26,6 +26,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>
</div>