<div dir="ltr">Hi Kieran thanks for the review.<br><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Aug 8, 2022 at 2:12 PM Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Utkarsh Tiwari via libcamera-devel (2022-08-06 20:04:31)<br>
> Currently if there is HotPlug event when the user is on the Camera<br>
> selection dialog, the QComboBox didn't update to reflect the change.<br>
> <br>
> If the QDialog exists then alert it for the Hotplug event. The check<br>
> for QDialog existance is done by QPointer.<br>
> <br>
> Signed-off-by: Utkarsh Tiwari <<a href="mailto:utkarsh02t@gmail.com" target="_blank">utkarsh02t@gmail.com</a>><br>
> ---<br>
>  src/qcam/cam_select_dialog.h | 14 ++++++++++++++<br>
>  src/qcam/main_window.cpp     |  6 ++++++<br>
>  2 files changed, 20 insertions(+)<br>
> <br>
> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h<br>
> index c23bad59..ee65eb88 100644<br>
> --- a/src/qcam/cam_select_dialog.h<br>
> +++ b/src/qcam/cam_select_dialog.h<br>
> @@ -56,6 +56,20 @@ public:<br>
>                 return cameraIdComboBox_->currentText().toStdString();<br>
>         }<br>
>  <br>
> +       /* Hotplug / Unplug Support. */<br>
> +       void cameraAdded(libcamera::Camera *camera)<br>
> +       {<br>
> +               cameraIdComboBox_->addItem(QString::fromStdString(camera->id()));<br>
> +       }<br>
> +<br>
> +       void cameraRemoved(libcamera::Camera *camera)<br>
> +       {<br>
> +               int cameraIndex = cameraIdComboBox_->findText(<br>
> +                       QString::fromStdString(camera->id()));<br>
> +<br>
<br>
Will this always be guaranteed to find an item? Are there any races<br>
where we might try to remove a camera that hasn't been added?<br>
<br>
I expect not, but just curious to be sure what error case might occur.<br>
<br>
But it's fine:<br>
<br>
<a href="https://doc.qt.io/qt-6/qcombobox.html#removeItem" rel="noreferrer" target="_blank">https://doc.qt.io/qt-6/qcombobox.html#removeItem</a>:<br>
  void QComboBox::removeItem(int index)<br>
<br>
  Removes the item at the given index from the combobox. This will<br>
  update the current index if the index is removed.<br>
<br>
  This function does nothing if index is out of range.<br>
<br>
<br>
So even if the camera isn't found - it just won't do anything.<br>
<br>
<br>
> +               cameraIdComboBox_->removeItem(cameraIndex);<br>
> +       }<br>
> +<br>
>  private:<br>
>         libcamera::CameraManager *cm_;<br>
>  <br>
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp<br>
> index 758e2c94..dd30817d 100644<br>
> --- a/src/qcam/main_window.cpp<br>
> +++ b/src/qcam/main_window.cpp<br>
> @@ -594,6 +594,9 @@ void MainWindow::processHotplug(HotplugEvent *e)<br>
>  <br>
>         if (event == HotplugEvent::HotPlug) {<br>
>                 cameraCombo_->addItem(QString::fromStdString(camera->id()));<br>
> +<br>
> +               if (camSelectDialog_)<br>
> +                       camSelectDialog_->cameraAdded(camera);<br>
<br>
This seems reasonable - but we're updating a camSelectDialog_ even if<br>
it's not visible - and I think it gets destroyed and remade when opened?<br>
(I'll have to go back and check) - but this is good for if we do re-use<br>
the dialog anyway.<br></blockquote><div>Yes, the remaking on open is not supposed to be the way. When we</div><div>create just one instance of the CamSelectDialog we need to add the</div><div>camera even if it's not visible, because as the dialog is not destroyed on</div><div>closing the dialog we need to keep track of the camera's hotplug events. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Anyway, I think this is ok overall.<br>
<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
>         } else if (event == HotplugEvent::HotUnplug) {<br>
>                 /* Check if the currently-streaming camera is removed. */<br>
>                 if (camera == camera_.get()) {<br>
> @@ -605,6 +608,9 @@ void MainWindow::processHotplug(HotplugEvent *e)<br>
>  <br>
>                 int camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));<br>
>                 cameraCombo_->removeItem(camIndex);<br>
> +<br>
> +               if (camSelectDialog_)<br>
> +                       camSelectDialog_->cameraRemoved(camera);<br>
>         }<br>
>  }<br>
>  <br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div>