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

16995 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>
---
 src/qcam/cam_select_dialog.h | 14 ++++++++++++++
 src/qcam/main_window.cpp     |  6 ++++++
 2 files changed, 20 insertions(+)

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

Hi Kieran thanks for the review.


On Mon, Aug 8, 2022 at 2:12 PM Kieran Bingham <
kieran.bingham@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@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@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
> >
>

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