[libcamera-devel,v8,3/8] qcam: MainWindow: Replace cameraCombo_ with CameraSelectorDialog

17069 diff mbox series

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@gmail.com>
Reviewed-by: Kieran Bingham <kieran.bingham@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(-)

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@gmail.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@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@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_;