[libcamera-devel,1/4] qcam: Use QDialog for selection of cameras
diff mbox series

Message ID 20220803175517.175332-2-utkarsh02t@gmail.com
State Superseded
Headers show
Series
  • Improve Camera Selection GUI in QCam
Related show

Commit Message

Utkarsh Tiwari Aug. 3, 2022, 5:55 p.m. UTC
Currently we use QInputDialog convenience dialogs to allow the user to
select a camera. This doesn't allow adding of more information (such as
camera location, model etc).

Create a QDialog with a QFormLayout that shows a QComboBox with camera
Ids. Use a QButtonBox to provide buttons for accepting and cancelling the
action.

Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
---
 src/qcam/main_window.cpp | 47 ++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 12 deletions(-)

Comments

Laurent Pinchart Aug. 4, 2022, 3:26 p.m. UTC | #1
Hi Utkarsh,

Thank you for the patch.

On Wed, Aug 03, 2022 at 11:25:14PM +0530, Utkarsh Tiwari via libcamera-devel wrote:
> Currently we use QInputDialog convenience dialogs to allow the user to
> select a camera. This doesn't allow adding of more information (such as
> camera location, model etc).
> 
> Create a QDialog with a QFormLayout that shows a QComboBox with camera
> Ids. Use a QButtonBox to provide buttons for accepting and cancelling the
> action.
> 
> Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
> ---
>  src/qcam/main_window.cpp | 47 ++++++++++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 7433d647..7761a6c6 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -16,7 +16,10 @@
>  
>  #include <QComboBox>
>  #include <QCoreApplication>
> +#include <QDialog>
> +#include <QDialogButtonBox>
>  #include <QFileDialog>
> +#include <QFormLayout>
>  #include <QImage>
>  #include <QImageWriter>
>  #include <QInputDialog>

I think you can now drop this header.

> @@ -291,23 +294,43 @@ void MainWindow::switchCamera(int index)
>  std::string MainWindow::chooseCamera()
>  {
>  	QStringList cameras;
> -	bool result;
> -
> -	/* If only one camera is available, use it automatically. */
> -	if (cm_->cameras().size() == 1)
> -		return cm_->cameras()[0]->id();
> +	std::string result;
>  
>  	/* Present a dialog box to pick a camera. */
> +	QDialog *cameraSelectDialog = new QDialog(this);
> +
> +	/* Setup a QComboBox to display camera Ids. */
>  	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> -		cameras.append(QString::fromStdString(cam->id()));
> +		cameras.push_back(QString::fromStdString(cam->id()));

Is there a specific reason for switching from append() to push_back() ?

> +
> +	QComboBox *cameraIdComboBox = new QComboBox;
> +	cameraIdComboBox->addItems(cameras);
> +
> +	/* Setup QDialogButtonBox. */
> +	QDialogButtonBox *dialogButtonBox = new QDialogButtonBox;
> +	dialogButtonBox->addButton(QDialogButtonBox::Cancel);
> +	dialogButtonBox->addButton(QDialogButtonBox::Ok);

There's a convenience API you can use for this:

	QDialogButtonBox *dialogButtonBox =
		new QDialogButtonBox(QDialogButtonBox::Ok |
				     QDialogButtonBox::Cancel);

> +
> +	connect(dialogButtonBox, &QDialogButtonBox::accepted,
> +		this, [&]() {
> +			result = cameraIdComboBox->currentText().toStdString();
> +			cameraSelectDialog->accept();
> +		});
> +
> +	connect(dialogButtonBox, &QDialogButtonBox::rejected,
> +		this, [&]() {
> +			result = std::string();

This isn't needed as result is initialized to an empty string.

> +			cameraSelectDialog->reject();
> +		});

Which means that you can just

	connect(dialogButtonBox, &QDialogButtonBox::rejected,
	        cameraSelectDialog, &QDialog::reject);

But overall, given that the next patches make this code more complex,
you should create a CameraSelectorDialog class that inherits from
QDialog, and implement all the camera selection process in there.

> +
> +	/* Setup the layout for the dialog. */
> +	QFormLayout *cameraSelectLayout = new QFormLayout(cameraSelectDialog);
> +	cameraSelectLayout->addRow("Camera: ", cameraIdComboBox);
> +	cameraSelectLayout->addWidget(dialogButtonBox);
>  
> -	QString id = QInputDialog::getItem(this, "Select Camera",
> -					   "Camera:", cameras, 0,
> -					   false, &result);
> -	if (!result)
> -		return std::string();
> +	cameraSelectDialog->exec();
>  
> -	return id.toStdString();
> +	return result;
>  }
>  
>  int MainWindow::openCamera()

Patch
diff mbox series

diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
index 7433d647..7761a6c6 100644
--- a/src/qcam/main_window.cpp
+++ b/src/qcam/main_window.cpp
@@ -16,7 +16,10 @@ 
 
 #include <QComboBox>
 #include <QCoreApplication>
+#include <QDialog>
+#include <QDialogButtonBox>
 #include <QFileDialog>
+#include <QFormLayout>
 #include <QImage>
 #include <QImageWriter>
 #include <QInputDialog>
@@ -291,23 +294,43 @@  void MainWindow::switchCamera(int index)
 std::string MainWindow::chooseCamera()
 {
 	QStringList cameras;
-	bool result;
-
-	/* If only one camera is available, use it automatically. */
-	if (cm_->cameras().size() == 1)
-		return cm_->cameras()[0]->id();
+	std::string result;
 
 	/* Present a dialog box to pick a camera. */
+	QDialog *cameraSelectDialog = new QDialog(this);
+
+	/* Setup a QComboBox to display camera Ids. */
 	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
-		cameras.append(QString::fromStdString(cam->id()));
+		cameras.push_back(QString::fromStdString(cam->id()));
+
+	QComboBox *cameraIdComboBox = new QComboBox;
+	cameraIdComboBox->addItems(cameras);
+
+	/* Setup QDialogButtonBox. */
+	QDialogButtonBox *dialogButtonBox = new QDialogButtonBox;
+	dialogButtonBox->addButton(QDialogButtonBox::Cancel);
+	dialogButtonBox->addButton(QDialogButtonBox::Ok);
+
+	connect(dialogButtonBox, &QDialogButtonBox::accepted,
+		this, [&]() {
+			result = cameraIdComboBox->currentText().toStdString();
+			cameraSelectDialog->accept();
+		});
+
+	connect(dialogButtonBox, &QDialogButtonBox::rejected,
+		this, [&]() {
+			result = std::string();
+			cameraSelectDialog->reject();
+		});
+
+	/* Setup the layout for the dialog. */
+	QFormLayout *cameraSelectLayout = new QFormLayout(cameraSelectDialog);
+	cameraSelectLayout->addRow("Camera: ", cameraIdComboBox);
+	cameraSelectLayout->addWidget(dialogButtonBox);
 
-	QString id = QInputDialog::getItem(this, "Select Camera",
-					   "Camera:", cameras, 0,
-					   false, &result);
-	if (!result)
-		return std::string();
+	cameraSelectDialog->exec();
 
-	return id.toStdString();
+	return result;
 }
 
 int MainWindow::openCamera()