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

[Umang Jain]

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 at 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 at 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_;
>