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

17068 diff mbox series

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 QDialog exists then alert it for the Hotplug event. The check
for QDialog existance is done by QPointer.

Signed-off-by: Utkarsh Tiwari <utkarsh02t@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(+)

Quoting Utkarsh Tiwari via libcamera-devel (2022-08-10 16:03:43)
> 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 QDialog exists then alert it for the Hotplug event. The check
> for QDialog existance is done by QPointer.
> 
> Signed-off-by: Utkarsh Tiwari <utkarsh02t@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);
> +}
> 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);

Trivial nits. I'd group these two together, and leave a blank line after
to keep the private section separated.

"""
	getCameraId

	/* ... */
	cameraAdded
	cameraRemoved

private:
	CameraManager
"""


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  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)
>  
>         if (event == HotplugEvent::HotPlug) {
>                 cameraCombo_->addItem(QString::fromStdString(camera->id()));
> +
> +               cameraSelectorDialog_->cameraAdded(camera);
>         } 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);
>         }
>  }
>  
> -- 
> 2.25.1
>

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@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@ideasonboard.com>

>  	}
>  }
>