[libcamera-devel] [PATCH 2/4] qcam: Support Hotplug for Camera Selection Dialog

[Umang Jain]

Hi Utkarsh

Thank you for the patch.

On 8/3/22 23:25, Utkarsh Tiwari via libcamera-devel wrote:
> Currently if there is HotPlug event when the user is on the Camera
> selection dialog, the QComboBox didn't update to reflect the change.
>
> If the cameraIdComboBox_ contains a valid QComboBox then update it to
> reflect the HotPlug change. Check the validity of QComboBox pointer by
> guarding it with a QPointer.
>
> Signed-off-by: Utkarsh Tiwari <utkarsh02t at gmail.com>
> ---
>   src/qcam/main_window.cpp | 23 +++++++++++++++--------
>   src/qcam/main_window.h   |  4 ++++
>   2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 7761a6c6..80a73b68 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -293,18 +293,15 @@ void MainWindow::switchCamera(int index)
>   
>   std::string MainWindow::chooseCamera()
>   {
> -	QStringList cameras;
>   	std::string result;
>   
>   	/* Present a dialog box to pick a camera. */
>   	QDialog *cameraSelectDialog = new QDialog(this);
>   
>   	/* Setup a QComboBox to display camera Ids. */
> +	cameraIdComboBox_ = new QComboBox;
>   	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> -		cameras.push_back(QString::fromStdString(cam->id()));
> -
> -	QComboBox *cameraIdComboBox = new QComboBox;
> -	cameraIdComboBox->addItems(cameras);
> +		cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));
>   
>   	/* Setup QDialogButtonBox. */
>   	QDialogButtonBox *dialogButtonBox = new QDialogButtonBox;
> @@ -313,7 +310,7 @@ std::string MainWindow::chooseCamera()
>   
>   	connect(dialogButtonBox, &QDialogButtonBox::accepted,
>   		this, [&]() {
> -			result = cameraIdComboBox->currentText().toStdString();
> +			result = cameraIdComboBox_->currentText().toStdString();
>   			cameraSelectDialog->accept();
>   		});
>   
> @@ -325,7 +322,7 @@ std::string MainWindow::chooseCamera()
>   
>   	/* Setup the layout for the dialog. */
>   	QFormLayout *cameraSelectLayout = new QFormLayout(cameraSelectDialog);
> -	cameraSelectLayout->addRow("Camera: ", cameraIdComboBox);
> +	cameraSelectLayout->addRow("Camera: ", cameraIdComboBox_);
>   	cameraSelectLayout->addWidget(dialogButtonBox);
>   
>   	cameraSelectDialog->exec();
> @@ -628,7 +625,12 @@ void MainWindow::processHotplug(HotplugEvent *e)
>   	HotplugEvent::PlugEvent event = e->hotplugEvent();
>   
>   	if (event == HotplugEvent::HotPlug) {
> -		cameraCombo_->addItem(QString::fromStdString(camera->id()));
> +		QString cameraId = QString::fromStdString(camera->id());
> +		cameraCombo_->addItem(cameraId);
> +
> +		/* Update cameraIdCombox_ to include the new camera. */
> +		if (cameraIdComboBox_)
> +			cameraIdComboBox_->addItem(cameraId);
>   	} else if (event == HotplugEvent::HotUnplug) {
>   		/* Check if the currently-streaming camera is removed. */
>   		if (camera == camera_.get()) {
> @@ -638,6 +640,11 @@ void MainWindow::processHotplug(HotplugEvent *e)
>   			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 fc70920f..b8122eb9 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -23,7 +23,9 @@
>   #include <QMainWindow>
>   #include <QMutex>
>   #include <QObject>
> +#include <QPointer>
>   #include <QQueue>
> +#include <QStringList>
>   #include <QTimer>
>   
>   #include "../cam/stream_options.h"
> @@ -99,6 +101,8 @@ private:
>   	QString title_;
>   	QTimer titleTimer_;
>   
> +	QPointer<QComboBox> cameraIdComboBox_;


You can simply start with a private cameraIdComboBox_ member in 1/4, 
rather than defining it locally, then making it private here.

> +
>   	/* Options */
>   	const OptionsParser::Options &options_;
>