[libcamera-devel] [PATCH v2.1 3/4] qcam: MainWindow: Replace cameraCombo_ with camSelectDialog

[Kieran Bingham]

Quoting Utkarsh Tiwari via libcamera-devel (2022-08-07 16:40:02)
> Replace the cameraCombo_ on the toolbar with a QPushButton which
> displays the camSelectDialog. This would allow the user to view
> information about the camera when switching.
> 
> The QPushButton text is set to the camera Id currently in use.
> 
> Signed-off-by: Utkarsh Tiwari <utkarsh02t at gmail.com>
> ---
> Difference from v2 :
>         remove redudant function getCurrentCamera, its job can be done by
>         getCameraId()
>  src/qcam/main_window.cpp | 41 ++++++++++++++++++++--------------------
>  src/qcam/main_window.h   |  5 +++--
>  2 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index dd30817d..98c22b71 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -193,14 +193,11 @@ int MainWindow::createToolbars()
>         connect(action, &QAction::triggered, this, &MainWindow::quit);
>  
>         /* Camera selector. */
> -       cameraCombo_ = new QComboBox();
> -       connect(cameraCombo_, QOverload<int>::of(&QComboBox::activated),
> +       cameraSwitchButton_ = new QPushButton;

The dialog is called camSelectDialog (or cameraSelectDialog), so I'd call this
cameraSelectButton_


> +       connect(cameraSwitchButton_, &QPushButton::clicked,
>                 this, &MainWindow::switchCamera);
>  
> -       for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> -               cameraCombo_->addItem(QString::fromStdString(cam->id()));
> -
> -       toolbar_->addWidget(cameraCombo_);
> +       toolbar_->addWidget(cameraSwitchButton_);
>  
>         toolbar_->addSeparator();
>  
> @@ -260,14 +257,19 @@ void MainWindow::updateTitle()
>   * Camera Selection
>   */
>  
> -void MainWindow::switchCamera(int index)
> +void MainWindow::switchCamera()
>  {
>         /* Get and acquire the new camera. */
> -       const auto &cameras = cm_->cameras();
> -       if (static_cast<unsigned int>(index) >= cameras.size())
> +       std::string newCameraId = chooseCamera();
> +
> +       if (newCameraId.empty())
>                 return;
>  
> -       const std::shared_ptr<Camera> &cam = cameras[index];
> +       if (camera_) {
> +               if (newCameraId == camera_->id())
> +                       return;
> +       }

I think you could shorten this to two lines:

	if (camera_ && camera_->id() == newCameraId)
		return;



> +       const std::shared_ptr<Camera> &cam = cm_->get(newCameraId);
>  
>         if (cam->acquire()) {
>                 qInfo() << "Failed to acquire camera" << cam->id().c_str();
> @@ -292,9 +294,12 @@ std::string MainWindow::chooseCamera()
>  {
>         camSelectDialog_ = new CamSelectDialog(cm_, this);

Aha so this could be as simple as :

	if (!camSelectDialog_)
		camSelectDialog_ = new CamSelectDialog(cm_, this);

to make it re-usable? (It would only construct on the first call).
(in the earlier patch).


Aside from those trivialish comments, I think this is ok so:

With those handled,
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>  
> -       if (camSelectDialog_->exec() == QDialog::Accepted)
> -               return camSelectDialog_->getCameraId();
> -       else
> +       if (camSelectDialog_->exec() == QDialog::Accepted) {
> +               std::string cameraId = camSelectDialog_->getCameraId();
> +               cameraSwitchButton_->setText(QString::fromStdString(cameraId));
> +
> +               return cameraId;
> +       } else
>                 return std::string();
>  }
>  
> @@ -327,8 +332,8 @@ int MainWindow::openCamera()
>                 return -EBUSY;
>         }
>  
> -       /* Set the combo-box entry with the currently selected Camera. */
> -       cameraCombo_->setCurrentText(QString::fromStdString(cameraName));
> +       /* Set the camera switch button with the currently selected Camera id. */
> +       cameraSwitchButton_->setText(QString::fromStdString(cameraName));
>  
>         return 0;
>  }
> @@ -593,8 +598,6 @@ void MainWindow::processHotplug(HotplugEvent *e)
>         HotplugEvent::PlugEvent event = e->hotplugEvent();
>  
>         if (event == HotplugEvent::HotPlug) {
> -               cameraCombo_->addItem(QString::fromStdString(camera->id()));
> -
>                 if (camSelectDialog_)
>                         camSelectDialog_->cameraAdded(camera);
>         } else if (event == HotplugEvent::HotUnplug) {
> @@ -603,12 +606,8 @@ void MainWindow::processHotplug(HotplugEvent *e)
>                         toggleCapture(false);
>                         camera_->release();
>                         camera_.reset();
> -                       cameraCombo_->setCurrentIndex(0);
>                 }
>  
> -               int camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));
> -               cameraCombo_->removeItem(camIndex);
> -
>                 if (camSelectDialog_)
>                         camSelectDialog_->cameraRemoved(camera);
>         }
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 6d80b5be..fe0f5938 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -24,6 +24,7 @@
>  #include <QMutex>
>  #include <QObject>
>  #include <QPointer>
> +#include <QPushButton>
>  #include <QQueue>
>  #include <QTimer>
>  
> @@ -61,7 +62,7 @@ private Q_SLOTS:
>         void quit();
>         void updateTitle();
>  
> -       void switchCamera(int index);
> +       void switchCamera();
>         void toggleCapture(bool start);
>  
>         void saveImageAs();
> @@ -91,7 +92,7 @@ private:
>         /* UI elements */
>         QToolBar *toolbar_;
>         QAction *startStopAction_;
> -       QComboBox *cameraCombo_;
> +       QPushButton *cameraSwitchButton_;
>         QAction *saveRaw_;
>         ViewFinder *viewfinder_;
>  
> -- 
> 2.25.1
>