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

[Utkarsh Tiwari]

Hi Kieran thanks for the review.


On Mon, Aug 8, 2022 at 2:12 PM Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> 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.
>
Yes, the remaking on open is not supposed to be the way. When we
create just one instance of the CamSelectDialog we need to add the
camera even if it's not visible, because as the dialog is not destroyed on
closing the dialog we need to keep track of the camera's hotplug events.

>
> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220808/65226092/attachment.htm>