[libcamera-devel] [PATCH v2.1 3/4] qcam: MainWindow: Replace cameraCombo_ with camSelectDialog
Utkarsh Tiwari
utkarsh02t at gmail.com
Mon Aug 8 19:25:05 CEST 2022
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>
More information about the libcamera-devel
mailing list