[libcamera-devel] [PATCH v8 3/8] qcam: MainWindow: Replace cameraCombo_ with CameraSelectorDialog
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Aug 23 04:02:44 CEST 2022
Hi Utkarsh,
Thank you for the patch.
On Wed, Aug 10, 2022 at 08:33:44PM +0530, Utkarsh Tiwari via libcamera-devel wrote:
> Replace the cameraCombo_ on the toolbar with a QPushButton which
> displays the CameraSelectorDialog. 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>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> Difference from v7:
> 1. Moved firstCameraSelect_ from 8/8 to here because if we were previously
> supplied camera id on the cmdline we always used that and sdisabled the
> showing of the dialog
> src/qcam/main_window.cpp | 51 ++++++++++++++++++++--------------------
> src/qcam/main_window.h | 6 +++--
> 2 files changed, 30 insertions(+), 27 deletions(-)
>
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 9ec94708..d1b8d2dd 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -98,7 +98,8 @@ private:
>
> MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> : saveRaw_(nullptr), cameraSelectorDialog_(nullptr), options_(options),
> - cm_(cm), allocator_(nullptr), isCapturing_(false), captureRaw_(false)
> + cm_(cm), allocator_(nullptr), isCapturing_(false), captureRaw_(false),
> + firstCameraSelect_(true)
> {
> int ret;
>
> @@ -193,14 +194,11 @@ int MainWindow::createToolbars()
> connect(action, &QAction::triggered, this, &MainWindow::quit);
>
> /* Camera selector. */
> - cameraCombo_ = new QComboBox();
> - connect(cameraCombo_, QOverload<int>::of(&QComboBox::activated),
> + cameraSelectButton_ = new QPushButton;
> + connect(cameraSelectButton_, &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(cameraSelectButton_);
>
> toolbar_->addSeparator();
>
> @@ -260,14 +258,18 @@ 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_ && newCameraId == camera_->id())
> + return;
> +
> + const std::shared_ptr<Camera> &cam = cm_->get(newCameraId);
>
> if (cam->acquire()) {
> qInfo() << "Failed to acquire camera" << cam->id().c_str();
> @@ -298,12 +300,17 @@ std::string MainWindow::chooseCamera()
> * Use the camera specified on the command line, if any, or display the
> * camera selection dialog box otherwise.
> */
> - if (options_.isSet(OptCamera))
> + if (firstCameraSelect_ && options_.isSet(OptCamera)) {
> + firstCameraSelect_ = false;
> return static_cast<std::string>(options_[OptCamera]);
> + }
How about moving this to MainWindow::openCamera() (in patch 1/8) ? You
could then drop the firstCameraSelect_ variable.
>
> - if (cameraSelectorDialog_->exec() == QDialog::Accepted)
> - return cameraSelectorDialog_->getCameraId();
> - else
> + if (cameraSelectorDialog_->exec() == QDialog::Accepted) {
> + std::string cameraId = cameraSelectorDialog_->getCameraId();
> + cameraSelectButton_->setText(QString::fromStdString(cameraId));
> +
> + return cameraId;
> + } else
> return std::string();
You need curly braces around this too as per our coding style. Another
option would be
if (cameraSelectorDialog_->exec() != QDialog::Accepted)
return std::string();
std::string cameraId = cameraSelectorDialog_->getCameraId();
cameraSelectButton_->setText(QString::fromStdString(cameraId));
return cameraId;
which I think looks nicer, but it's up to you.
This being said, I would move the setText() call to
MainWindow::switchCamera() as you need a setText() in
MainWindow::openCamera() to handle the first open.
> }
>
> @@ -327,8 +334,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. */
> + cameraSelectButton_->setText(QString::fromStdString(cameraName));
>
> return 0;
> }
> @@ -592,22 +599,16 @@ void MainWindow::processHotplug(HotplugEvent *e)
> Camera *camera = e->camera();
> HotplugEvent::PlugEvent event = e->hotplugEvent();
>
> - if (event == HotplugEvent::HotPlug) {
> - cameraCombo_->addItem(QString::fromStdString(camera->id()));
> -
> + if (event == HotplugEvent::HotPlug)
> cameraSelectorDialog_->cameraAdded(camera);
Please keep the curly braces, we require then for all branches of an if,
or for none of them.
> - } else if (event == HotplugEvent::HotUnplug) {
> + else if (event == HotplugEvent::HotUnplug) {
> /* Check if the currently-streaming camera is removed. */
> if (camera == camera_.get()) {
> toggleCapture(false);
> camera_->release();
> camera_.reset();
> - cameraCombo_->setCurrentIndex(0);
> }
>
> - int camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));
> - cameraCombo_->removeItem(camIndex);
> -
> cameraSelectorDialog_->cameraRemoved(camera);
> }
> }
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 4ad5e6e9..f9ea8bd3 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -23,6 +23,7 @@
> #include <QMainWindow>
> #include <QMutex>
> #include <QObject>
> +#include <QPushButton>
You can also drop the QComboBox above.
With those changes,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> #include <QQueue>
> #include <QTimer>
>
> @@ -60,7 +61,7 @@ private Q_SLOTS:
> void quit();
> void updateTitle();
>
> - void switchCamera(int index);
> + void switchCamera();
> void toggleCapture(bool start);
>
> void saveImageAs();
> @@ -90,7 +91,7 @@ private:
> /* UI elements */
> QToolBar *toolbar_;
> QAction *startStopAction_;
> - QComboBox *cameraCombo_;
> + QPushButton *cameraSelectButton_;
> QAction *saveRaw_;
> ViewFinder *viewfinder_;
>
> @@ -116,6 +117,7 @@ private:
> /* Capture state, buffers queue and statistics */
> bool isCapturing_;
> bool captureRaw_;
> + bool firstCameraSelect_;
> libcamera::Stream *vfStream_;
> libcamera::Stream *rawStream_;
> std::map<const libcamera::Stream *, QQueue<libcamera::FrameBuffer *>> freeBuffers_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list