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

[Kieran Bingham]

Quoting Utkarsh Tiwari via libcamera-devel (2022-08-06 20:04:31)
> 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 at gmail.com>
> ---
>  src/qcam/cam_select_dialog.h | 14 ++++++++++++++
>  src/qcam/main_window.cpp     |  6 ++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h
> index c23bad59..ee65eb88 100644
> --- a/src/qcam/cam_select_dialog.h
> +++ b/src/qcam/cam_select_dialog.h
> @@ -56,6 +56,20 @@ public:
>                 return cameraIdComboBox_->currentText().toStdString();
>         }
>  
> +       /* Hotplug / Unplug Support. */
> +       void cameraAdded(libcamera::Camera *camera)
> +       {
> +               cameraIdComboBox_->addItem(QString::fromStdString(camera->id()));
> +       }
> +
> +       void cameraRemoved(libcamera::Camera *camera)
> +       {
> +               int cameraIndex = cameraIdComboBox_->findText(
> +                       QString::fromStdString(camera->id()));
> +

Will this always be guaranteed to find an item? Are there any races
where we might try to remove a camera that hasn't been added?

I expect not, but just curious to be sure what error case might occur.

But it's fine:

https://doc.qt.io/qt-6/qcombobox.html#removeItem:
  void QComboBox::removeItem(int index)

  Removes the item at the given index from the combobox. This will
  update the current index if the index is removed.

  This function does nothing if index is out of range.


So even if the camera isn't found - it just won't do anything.


> +               cameraIdComboBox_->removeItem(cameraIndex);
> +       }
> +
>  private:
>         libcamera::CameraManager *cm_;
>  
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 758e2c94..dd30817d 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -594,6 +594,9 @@ void MainWindow::processHotplug(HotplugEvent *e)
>  
>         if (event == HotplugEvent::HotPlug) {
>                 cameraCombo_->addItem(QString::fromStdString(camera->id()));
> +
> +               if (camSelectDialog_)
> +                       camSelectDialog_->cameraAdded(camera);

This seems reasonable - but we're updating a camSelectDialog_ even if
it's not visible - and I think it gets destroyed and remade when opened?
(I'll have to go back and check) - but this is good for if we do re-use
the dialog anyway.

Anyway, I think this is ok overall.


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

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