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

[Laurent Pinchart]

On Mon, Aug 08, 2022 at 10:54:03PM +0530, Utkarsh Tiwari via libcamera-devel wrote:
> On Mon, Aug 8, 2022 at 2:12 PM Kieran Bingham 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.

Ah that why you use a QPointer. After reworking patch 1/4 to reuse the
dialog, I think you can drop the QPointer, as the dialog should always
be there.

> > > 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:

Side note, you may want to read the Qt5 documentation instead, as that's
what we're currently using. There could be behavioural differences.

> >   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);
> > >         }
> > >  }
> > >

-- 
Regards,

Laurent Pinchart