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

[Utkarsh Tiwari]

Hi, Kieran agreed with everything on this review,
would do the changes.

On Mon, Aug 8, 2022 at 2:26 PM Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220808/da6534e4/attachment.htm>