[libcamera-devel] [PATCH 1/4] qcam: Use QDialog for selection of cameras

[Laurent Pinchart]

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 at 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()

-- 
Regards,

Laurent Pinchart