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

[Laurent Pinchart]

Hi Utkarsh,

Thank you for the patch.

On Wed, Aug 10, 2022 at 08:33:43PM +0530, 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.

s/didn't/doesn't/

> If the QDialog exists then alert it for the Hotplug event. The check
> for QDialog existance is done by QPointer.

s/existance/existence/

But I think you need to rework this paragraph as there's no QPointer
anymore.

> Signed-off-by: Utkarsh Tiwari <utkarsh02t at gmail.com>
> ---
> Difference from v7:
> 	1. Nothing
>  src/qcam/cam_select_dialog.cpp | 14 ++++++++++++++
>  src/qcam/cam_select_dialog.h   |  4 ++++
>  src/qcam/main_window.cpp       |  4 ++++
>  3 files changed, 22 insertions(+)
> 
> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp
> index dceaa590..d8982800 100644
> --- a/src/qcam/cam_select_dialog.cpp
> +++ b/src/qcam/cam_select_dialog.cpp
> @@ -49,3 +49,17 @@ std::string CameraSelectorDialog::getCameraId()
>  {
>  	return cameraIdComboBox_->currentText().toStdString();
>  }
> +
> +/* Hotplug / Unplug Support. */
> +void CameraSelectorDialog::cameraAdded(libcamera::Camera *camera)
> +{
> +	cameraIdComboBox_->addItem(QString::fromStdString(camera->id()));
> +}
> +
> +void CameraSelectorDialog::cameraRemoved(libcamera::Camera *camera)
> +{
> +	int cameraIndex = cameraIdComboBox_->findText(
> +		QString::fromStdString(camera->id()));
> +
> +	cameraIdComboBox_->removeItem(cameraIndex);
> +}

I think you can optimize this by passing a Qstring cameraId to both
functions, as the caller already has to construct a QString.

> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h
> index 5544f49a..04c71fd8 100644
> --- a/src/qcam/cam_select_dialog.h
> +++ b/src/qcam/cam_select_dialog.h
> @@ -26,6 +26,10 @@ public:
>  
>  	std::string getCameraId();
>  
> +	/* Hotplug / Unplug Support. */
> +	void cameraAdded(libcamera::Camera *camera);
> +
> +	void cameraRemoved(libcamera::Camera *camera);

I would name the two fucntions addCamera() and removeCamera().

>  private:
>  	libcamera::CameraManager *cm_;
>  
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index e794221a..9ec94708 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -594,6 +594,8 @@ void MainWindow::processHotplug(HotplugEvent *e)
>  

So here you could do

	QString cameraId = QString::fromStdString(camera->id());

>  	if (event == HotplugEvent::HotPlug) {
>  		cameraCombo_->addItem(QString::fromStdString(camera->id()));
> +
> +		cameraSelectorDialog_->cameraAdded(camera);

and use cameraId for both functions here.

>  	} else if (event == HotplugEvent::HotUnplug) {
>  		/* Check if the currently-streaming camera is removed. */
>  		if (camera == camera_.get()) {
> @@ -605,6 +607,8 @@ void MainWindow::processHotplug(HotplugEvent *e)
>  
>  		int camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));
>  		cameraCombo_->removeItem(camIndex);
> +
> +		cameraSelectorDialog_->cameraRemoved(camera);

And here too.

With that, and Kieran's comment addressed,

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

>  	}
>  }
>  

-- 
Regards,

Laurent Pinchart