<div dir="ltr">Hi Kieran thanks for the review.<br><div dir="ltr"><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Aug 8, 2022 at 2:15 PM Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Utkarsh Tiwari (2022-08-08 07:56:41)<br>
> Hi Kieran thanks for the review.<br>
> <br>
> On Mon, 8 Aug, 2022, 04:49 Kieran Bingham, <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> wrote:<br>
> <br>
> > 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" 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>
> ><br>
> Oops copy paste error, It's my copyright.<br>
> <br>
> ><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>
> ><br>
> <br>
> Fine by me.<br>
> <br>
> ><br>
> ><br>
> > > +{<br>
> > > + Q_OBJECT<br>
> > > +public:<br>
> > > + CamSelectDialog(libcamera::CameraManager *cameraManager, QWidget<br>
> > *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>
> ><br>
> Yes, to show the camera info I need to use cm_ to get the camera.<br>
> <br>
> ><br>
> > > + {<br>
> > > + /* Use a QFormLayout for the dialog. */<br>
> > > + QFormLayout *camSelectDialogLayout = new<br>
> > QFormLayout(this);<br>
> > > +<br>
> > > + /* Setup the camera id combo-box. */<br>
> > > + cameraIdComboBox_ = new QComboBox;<br>
> > > + for (const auto &cam : cm_->cameras())<br>
> > > +<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>
> ><br>
> Yes saw your other reply, and yes it's done in patch 2nd.<br>
> <br>
> <br>
> ><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: ",<br>
> > 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>
> > Sure, I would wait on review for the other patches then move everything<br>
> to its .cpp.<br>
> <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..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>
> ><br>
> Yes we need it as a class variable for camera hotplugging.<br>
<br>
This seems to recreate the CamSelectDialog box each time from scratch,<br>
even though we constructed it - and keep it updated with hotplug events.<br>
<br>
Is it possible to re-use it when it's created? or does accepting the<br>
dialog or closing it make it no longer possible to re-use?<br>
<br>
If we can re-use it - we should ... Otherwise - we probably shouldn't<br>
send hotplug events when it's been closed. ?<br>
<br></blockquote><div><br></div><div>I think we can re-use it. And I think the current way is causing a leak</div><div>:P.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
> <br>
> ><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>
> ><br>
</blockquote></div></div>