[libcamera-devel,v8,1/8] qcam: Use QDialog for selection of cameras at startup

17067 diff mbox series

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 QDialogButtonBox to provide buttons for accepting and
cancelling the action.

The CameraSelectorDialog is only initialized the first time when the
MainWindow is created.

From this commit we cease to auto select the camera if only a single
camera is available to libcamera. We would always display the selection
dialog with the exception being that being if the camera is supplied by
the console.

Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
Difference from v7:
	1. Updated the commit message to inform of the behavioural change in
		selecting the first camera.
 src/qcam/cam_select_dialog.cpp | 51 ++++++++++++++++++++++++++++++++++
 src/qcam/cam_select_dialog.h   | 34 +++++++++++++++++++++++
 src/qcam/main_window.cpp       | 44 +++++++++++------------------
 src/qcam/main_window.h         |  3 ++
 src/qcam/meson.build           |  2 ++
 5 files changed, 106 insertions(+), 28 deletions(-)
 create mode 100644 src/qcam/cam_select_dialog.cpp
 create mode 100644 src/qcam/cam_select_dialog.h

Hi Utkrash,

Thank you for the patch.

On Wed, Aug 10, 2022 at 08:33:42PM +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 QDialogButtonBox to provide buttons for accepting and
> cancelling the action.
> 
> The CameraSelectorDialog is only initialized the first time when the
> MainWindow is created.
> 
> From this commit we cease to auto select the camera if only a single
> camera is available to libcamera. We would always display the selection
> dialog with the exception being that being if the camera is supplied by
> the console.

s/by the console/on the command line/

> Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> Difference from v7:
> 	1. Updated the commit message to inform of the behavioural change in
> 		selecting the first camera.
>  src/qcam/cam_select_dialog.cpp | 51 ++++++++++++++++++++++++++++++++++
>  src/qcam/cam_select_dialog.h   | 34 +++++++++++++++++++++++
>  src/qcam/main_window.cpp       | 44 +++++++++++------------------
>  src/qcam/main_window.h         |  3 ++
>  src/qcam/meson.build           |  2 ++
>  5 files changed, 106 insertions(+), 28 deletions(-)
>  create mode 100644 src/qcam/cam_select_dialog.cpp
>  create mode 100644 src/qcam/cam_select_dialog.h
> 
> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp
> new file mode 100644
> index 00000000..dceaa590
> --- /dev/null
> +++ b/src/qcam/cam_select_dialog.cpp
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2022, Utkarsh Tiwari <utkarsh02t@gmail.com>
> + *
> + * cam_select_dialog.cpp - qcam - Camera Selection dialog
> + */
> +
> +#include "cam_select_dialog.h"
> +
> +#include <string>

You don't need to include this header.

> +
> +#include <libcamera/camera.h>
> +#include <libcamera/camera_manager.h>
> +
> +#include <QComboBox>
> +#include <QDialog>

You can drop QDialog, as CameraSelectorDialog inherits from it, it's
guaranteed to be included in cam_select_dialog.h.

> +#include <QDialogButtonBox>
> +#include <QFormLayout>
> +#include <QString>
> +
> +CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,
> +					   QWidget *parent)
> +	: QDialog(parent), cm_(cameraManager)
> +{
> +	/* Use a QFormLayout for the dialog. */
> +	QFormLayout *layout = new QFormLayout(this);
> +
> +	/* Setup the camera id combo-box. */
> +	cameraIdComboBox_ = new QComboBox;
> +	for (const auto &cam : cm_->cameras())
> +		cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));
> +
> +	/* Setup the QDialogButton Box */
> +	QDialogButtonBox *buttonBox =
> +		new QDialogButtonBox(QDialogButtonBox::Ok |
> +				     QDialogButtonBox::Cancel);
> +
> +	connect(buttonBox, &QDialogButtonBox::accepted,
> +		this, &QDialog::accept);
> +	connect(buttonBox, &QDialogButtonBox::rejected,
> +		this, &QDialog::reject);
> +
> +	/* Set the layout. */
> +	layout->addRow("Camera:", cameraIdComboBox_);
> +	layout->addWidget(buttonBox);
> +}
> +
> +std::string CameraSelectorDialog::getCameraId()
> +{
> +	return cameraIdComboBox_->currentText().toStdString();
> +}
> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h
> new file mode 100644
> index 00000000..5544f49a
> --- /dev/null
> +++ b/src/qcam/cam_select_dialog.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2022, Utkarsh Tiwari <utkarsh02t@gmail.com>
> + *
> + * cam_select_dialog.h - qcam - Camera Selection dialog
> + */
> +
> +#pragma once
> +
> +#include <string>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/camera_manager.h>
> +
> +#include <QComboBox>

QComboxBox can be forward-declared (see main_window.h).

> +#include <QDialog>
> +
> +class CameraSelectorDialog : public QDialog
> +{
> +	Q_OBJECT
> +public:
> +	CameraSelectorDialog(libcamera::CameraManager *cameraManager,
> +			     QWidget *parent);
> +
> +	~CameraSelectorDialog() = default;

Defining a function in the class definition, regardless of whether you
define it with a manual implementation or with = default, results in the
function being inlined. This can increase the binary size, and should
thus only be done when needed. Here, you should write

	CameraSelectorDialog(libcamera::CameraManager *cameraManager,
			     QWidget *parent);
	~CameraSelectorDialog();

and in the .cpp file add

CameraSelectorDialog::~CameraSelectorDialog() = default;

> +
> +	std::string getCameraId();
> +
> +private:
> +	libcamera::CameraManager *cm_;
> +
> +	/* UI elements. */
> +	QComboBox *cameraIdComboBox_;
> +};
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 7433d647..e794221a 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -19,7 +19,6 @@
>  #include <QFileDialog>
>  #include <QImage>
>  #include <QImageWriter>
> -#include <QInputDialog>
>  #include <QMutexLocker>
>  #include <QStandardPaths>
>  #include <QStringList>
> @@ -30,6 +29,7 @@
>  
>  #include "../cam/image.h"
>  
> +#include "cam_select_dialog.h"
>  #include "dng_writer.h"
>  #ifndef QT_NO_OPENGL
>  #include "viewfinder_gl.h"
> @@ -97,8 +97,8 @@ private:
>  };
>  
>  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> -	: saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr),
> -	  isCapturing_(false), captureRaw_(false)
> +	: saveRaw_(nullptr), cameraSelectorDialog_(nullptr), options_(options),
> +	  cm_(cm), allocator_(nullptr), isCapturing_(false), captureRaw_(false)
>  {
>  	int ret;
>  
> @@ -290,38 +290,26 @@ 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();
> -
> -	/* Present a dialog box to pick a camera. */
> -	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> -		cameras.append(QString::fromStdString(cam->id()));
> -
> -	QString id = QInputDialog::getItem(this, "Select Camera",
> -					   "Camera:", cameras, 0,
> -					   false, &result);
> -	if (!result)
> -		return std::string();
> -
> -	return id.toStdString();
> -}
> -
> -int MainWindow::openCamera()
> -{
> -	std::string cameraName;
> +	/* Construct the selection dialog, only the first time. */
> +	if (!cameraSelectorDialog_)
> +		cameraSelectorDialog_ = new CameraSelectorDialog(cm_, this);
>  
>  	/*
>  	 * Use the camera specified on the command line, if any, or display the
>  	 * camera selection dialog box otherwise.
>  	 */
>  	if (options_.isSet(OptCamera))
> -		cameraName = static_cast<std::string>(options_[OptCamera]);
> +		return static_cast<std::string>(options_[OptCamera]);
> +
> +	if (cameraSelectorDialog_->exec() == QDialog::Accepted)
> +		return cameraSelectorDialog_->getCameraId();
>  	else
> -		cameraName = chooseCamera();
> +		return std::string();
> +}
> +
> +int MainWindow::openCamera()
> +{
> +	std::string cameraName = chooseCamera();
>  
>  	if (cameraName == "")
>  		return -EINVAL;
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index fc70920f..4ad5e6e9 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -28,6 +28,7 @@
>  
>  #include "../cam/stream_options.h"
>  
> +#include "cam_select_dialog.h"
>  #include "viewfinder.h"
>  
>  class QAction;
> @@ -99,6 +100,8 @@ private:
>  	QString title_;
>  	QTimer titleTimer_;
>  
> +	CameraSelectorDialog *cameraSelectorDialog_;

You can forward-declare CameraSelectorDialog the same way we do for
Image and HotplugEvent.

With those small issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  	/* Options */
>  	const OptionsParser::Options &options_;
>  
> diff --git a/src/qcam/meson.build b/src/qcam/meson.build
> index c46f4631..61861ea6 100644
> --- a/src/qcam/meson.build
> +++ b/src/qcam/meson.build
> @@ -18,6 +18,7 @@ qcam_sources = files([
>      '../cam/image.cpp',
>      '../cam/options.cpp',
>      '../cam/stream_options.cpp',
> +    'cam_select_dialog.cpp',
>      'format_converter.cpp',
>      'main.cpp',
>      'main_window.cpp',
> @@ -26,6 +27,7 @@ qcam_sources = files([
>  ])
>  
>  qcam_moc_headers = files([
> +    'cam_select_dialog.h',
>      'main_window.h',
>      'viewfinder_qt.h',
>  ])