<div dir="ltr"><div>Hi, Kieran agreed with everything on this review, <br></div><div>would do the changes.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Aug 8, 2022 at 2:26 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 via libcamera-devel (2022-08-07 16:40:02)<br>
> Replace the cameraCombo_ on the toolbar with a QPushButton which<br>
> displays the camSelectDialog. This would allow the user to view<br>
> information about the camera when switching.<br>
> <br>
> The QPushButton text is set to the camera Id currently in use.<br>
> <br>
> Signed-off-by: Utkarsh Tiwari <<a href="mailto:utkarsh02t@gmail.com" target="_blank">utkarsh02t@gmail.com</a>><br>
> ---<br>
> Difference from v2 :<br>
> remove redudant function getCurrentCamera, its job can be done by<br>
> getCameraId()<br>
> src/qcam/main_window.cpp | 41 ++++++++++++++++++++--------------------<br>
> src/qcam/main_window.h | 5 +++--<br>
> 2 files changed, 23 insertions(+), 23 deletions(-)<br>
> <br>
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp<br>
> index dd30817d..98c22b71 100644<br>
> --- a/src/qcam/main_window.cpp<br>
> +++ b/src/qcam/main_window.cpp<br>
> @@ -193,14 +193,11 @@ int MainWindow::createToolbars()<br>
> connect(action, &QAction::triggered, this, &MainWindow::quit);<br>
> <br>
> /* Camera selector. */<br>
> - cameraCombo_ = new QComboBox();<br>
> - connect(cameraCombo_, QOverload<int>::of(&QComboBox::activated),<br>
> + cameraSwitchButton_ = new QPushButton;<br>
<br>
The dialog is called camSelectDialog (or cameraSelectDialog), so I'd call this<br>
cameraSelectButton_<br>
<br>
<br>
> + connect(cameraSwitchButton_, &QPushButton::clicked,<br>
> this, &MainWindow::switchCamera);<br>
> <br>
> - for (const std::shared_ptr<Camera> &cam : cm_->cameras())<br>
> - cameraCombo_->addItem(QString::fromStdString(cam->id()));<br>
> -<br>
> - toolbar_->addWidget(cameraCombo_);<br>
> + toolbar_->addWidget(cameraSwitchButton_);<br>
> <br>
> toolbar_->addSeparator();<br>
> <br>
> @@ -260,14 +257,19 @@ void MainWindow::updateTitle()<br>
> * Camera Selection<br>
> */<br>
> <br>
> -void MainWindow::switchCamera(int index)<br>
> +void MainWindow::switchCamera()<br>
> {<br>
> /* Get and acquire the new camera. */<br>
> - const auto &cameras = cm_->cameras();<br>
> - if (static_cast<unsigned int>(index) >= cameras.size())<br>
> + std::string newCameraId = chooseCamera();<br>
> +<br>
> + if (newCameraId.empty())<br>
> return;<br>
> <br>
> - const std::shared_ptr<Camera> &cam = cameras[index];<br>
> + if (camera_) {<br>
> + if (newCameraId == camera_->id())<br>
> + return;<br>
> + }<br>
<br>
I think you could shorten this to two lines:<br>
<br>
if (camera_ && camera_->id() == newCameraId)<br>
return;<br>
<br>
<br>
<br>
> + const std::shared_ptr<Camera> &cam = cm_->get(newCameraId);<br>
> <br>
> if (cam->acquire()) {<br>
> qInfo() << "Failed to acquire camera" << cam->id().c_str();<br>
> @@ -292,9 +294,12 @@ std::string MainWindow::chooseCamera()<br>
> {<br>
> camSelectDialog_ = new CamSelectDialog(cm_, this);<br>
<br>
Aha so this could be as simple as :<br>
<br>
if (!camSelectDialog_)<br>
camSelectDialog_ = new CamSelectDialog(cm_, this);<br>
<br>
to make it re-usable? (It would only construct on the first call).<br>
(in the earlier patch).<br>
<br>
<br>
Aside from those trivialish comments, I think this is ok so:<br>
<br>
With those handled,<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
> <br>
> - if (camSelectDialog_->exec() == QDialog::Accepted)<br>
> - return camSelectDialog_->getCameraId();<br>
> - else<br>
> + if (camSelectDialog_->exec() == QDialog::Accepted) {<br>
> + std::string cameraId = camSelectDialog_->getCameraId();<br>
> + cameraSwitchButton_->setText(QString::fromStdString(cameraId));<br>
> +<br>
> + return cameraId;<br>
> + } else<br>
> return std::string();<br>
> }<br>
> <br>
> @@ -327,8 +332,8 @@ int MainWindow::openCamera()<br>
> return -EBUSY;<br>
> }<br>
> <br>
> - /* Set the combo-box entry with the currently selected Camera. */<br>
> - cameraCombo_->setCurrentText(QString::fromStdString(cameraName));<br>
> + /* Set the camera switch button with the currently selected Camera id. */<br>
> + cameraSwitchButton_->setText(QString::fromStdString(cameraName));<br>
> <br>
> return 0;<br>
> }<br>
> @@ -593,8 +598,6 @@ void MainWindow::processHotplug(HotplugEvent *e)<br>
> HotplugEvent::PlugEvent event = e->hotplugEvent();<br>
> <br>
> if (event == HotplugEvent::HotPlug) {<br>
> - cameraCombo_->addItem(QString::fromStdString(camera->id()));<br>
> -<br>
> if (camSelectDialog_)<br>
> camSelectDialog_->cameraAdded(camera);<br>
> } else if (event == HotplugEvent::HotUnplug) {<br>
> @@ -603,12 +606,8 @@ void MainWindow::processHotplug(HotplugEvent *e)<br>
> toggleCapture(false);<br>
> camera_->release();<br>
> camera_.reset();<br>
> - cameraCombo_->setCurrentIndex(0);<br>
> }<br>
> <br>
> - int camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));<br>
> - cameraCombo_->removeItem(camIndex);<br>
> -<br>
> if (camSelectDialog_)<br>
> camSelectDialog_->cameraRemoved(camera);<br>
> }<br>
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h<br>
> index 6d80b5be..fe0f5938 100644<br>
> --- a/src/qcam/main_window.h<br>
> +++ b/src/qcam/main_window.h<br>
> @@ -24,6 +24,7 @@<br>
> #include <QMutex><br>
> #include <QObject><br>
> #include <QPointer><br>
> +#include <QPushButton><br>
> #include <QQueue><br>
> #include <QTimer><br>
> <br>
> @@ -61,7 +62,7 @@ private Q_SLOTS:<br>
> void quit();<br>
> void updateTitle();<br>
> <br>
> - void switchCamera(int index);<br>
> + void switchCamera();<br>
> void toggleCapture(bool start);<br>
> <br>
> void saveImageAs();<br>
> @@ -91,7 +92,7 @@ private:<br>
> /* UI elements */<br>
> QToolBar *toolbar_;<br>
> QAction *startStopAction_;<br>
> - QComboBox *cameraCombo_;<br>
> + QPushButton *cameraSwitchButton_;<br>
> QAction *saveRaw_;<br>
> ViewFinder *viewfinder_;<br>
> <br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div>