[libcamera-devel,4/4] qcam: MainWindow: Replace cameraCombo_ with cameraSelectDialog

16959 diff mbox series

Replace the cameraCombo_ on the toolbar with a QPushButton which
displays the cameraSelectDialog. 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@gmail.com>
---
 src/qcam/main_window.cpp | 47 ++++++++++++++++++++++------------------
 src/qcam/main_window.h   |  5 +++--
 2 files changed, 29 insertions(+), 23 deletions(-)

Hi Utkarsh,

Thank you for the patch.

On 8/3/22 23:25, Utkarsh Tiwari via libcamera-devel wrote:
> Replace the cameraCombo_ on the toolbar with a QPushButton which
> displays the cameraSelectDialog. 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@gmail.com>
> ---
>   src/qcam/main_window.cpp | 47 ++++++++++++++++++++++------------------
>   src/qcam/main_window.h   |  5 +++--
>   2 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 81fa3e60..2a9dcc1e 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -198,14 +198,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();
>   
> @@ -265,14 +262,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 cameraId = chooseCamera();


Can this be renamed to better clarify that the string belongs to a 
(newly) chosen camera?

> +	if (cameraId.empty())
>   		return;
>   
> -	const std::shared_ptr<Camera> &cam = cameras[index];
> +	/* Don't try the current camera. */


Are you missing something here, I find the comment a bit confusing. Is 
this similar,

         /* Is user selects the current camera, return */

> +	if (cameraId == camera_->id())
> +		return;
> +
> +	const std::shared_ptr<Camera> &cam = cm_->get(cameraId);
>   
>   	if (cam->acquire()) {
>   		qInfo() << "Failed to acquire camera" << cam->id().c_str();
> @@ -290,6 +291,9 @@ void MainWindow::switchCamera(int index)
>   	camera_->release();
>   	camera_ = cam;
>   
> +	/* Set the QPushButton text with current camera. */
> +	cameraSelectButton_->setText(QString::fromStdString(camera_->id()));
> +
>   	startStopAction_->setChecked(true);
>   }
>   
> @@ -356,10 +360,16 @@ std::string MainWindow::chooseCamera()
>   	 * When the Qdialog starts, the QComboBox would have the first
>   	 * camera's Id as its currentText.
>   	 */
> -	if (cm_->cameras().size())
> -		currentCamera = cm_->cameras()[0];
> -	else
> -		currentCamera = nullptr;
> +	if (!isCapturing_) {
> +		if (cm_->cameras().size())
> +			currentCamera = cm_->cameras()[0];
> +		else
> +			currentCamera = nullptr;


same comment from previous patch on nullptr

Other than that,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> +	} else {
> +		int cameraIndex = cameraIdComboBox_->findText(QString::fromStdString(camera_->id()));
> +		cameraIdComboBox_->setCurrentIndex(cameraIndex);
> +		currentCamera = camera_;
> +	}
>   
>   	if (currentCamera)
>   		updateCameraInfo(currentCamera);
> @@ -428,8 +438,8 @@ int MainWindow::openCamera()
>   		return -EBUSY;
>   	}
>   
> -	/* Set the combo-box entry with the currently selected Camera. */
> -	cameraCombo_->setCurrentText(QString::fromStdString(cameraName));
> +	/* Set the QPushButton text with the currently selected Camera. */
> +	cameraSelectButton_->setText(QString::fromStdString(cameraName));
>   
>   	return 0;
>   }
> @@ -695,7 +705,6 @@ void MainWindow::processHotplug(HotplugEvent *e)
>   
>   	if (event == HotplugEvent::HotPlug) {
>   		QString cameraId = QString::fromStdString(camera->id());
> -		cameraCombo_->addItem(cameraId);
>   
>   		/* Update cameraIdCombox_ to include the new camera. */
>   		if (cameraIdComboBox_)
> @@ -706,16 +715,12 @@ void MainWindow::processHotplug(HotplugEvent *e)
>   			toggleCapture(false);
>   			camera_->release();
>   			camera_.reset();
> -			cameraCombo_->setCurrentIndex(0);
>   		}
>   
>   		if (cameraIdComboBox_) {
>   			int cameraIdIndex = cameraIdComboBox_->findText(QString::fromStdString(camera_->id()));
>   			cameraIdComboBox_->removeItem(cameraIdIndex);
>   		}
> -
> -		int camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));
> -		cameraCombo_->removeItem(camIndex);
>   	}
>   }
>   
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 3a1c6156..b15f9409 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -25,6 +25,7 @@
>   #include <QMutex>
>   #include <QObject>
>   #include <QPointer>
> +#include <QPushButton>
>   #include <QQueue>
>   #include <QStringList>
>   #include <QTimer>
> @@ -62,7 +63,7 @@ private Q_SLOTS:
>   	void quit();
>   	void updateTitle();
>   
> -	void switchCamera(int index);
> +	void switchCamera();
>   	void toggleCapture(bool start);
>   
>   	void saveImageAs();
> @@ -96,7 +97,7 @@ private:
>   	/* UI elements */
>   	QToolBar *toolbar_;
>   	QAction *startStopAction_;
> -	QComboBox *cameraCombo_;
> +	QPushButton *cameraSelectButton_;
>   	QAction *saveRaw_;
>   	ViewFinder *viewfinder_;
>