[libcamera-devel,v2.1,3/4] qcam: MainWindow: Replace cameraCombo_ with camSelectDialog
diff mbox series

Message ID 20220807154002.91607-1-utkarsh02t@gmail.com
State Superseded
Headers show
Series
  • Untitled series #3383
Related show

Commit Message

Utkarsh Tiwari Aug. 7, 2022, 3:40 p.m. UTC 
Replace the cameraCombo_ on the toolbar with a QPushButton which
displays the camSelectDialog. This would allow the user to view
information about the camera when switching.

The QPushButton text is set to the camera Id currently in use.

Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
---
Difference from v2 :
	remove redudant function getCurrentCamera, its job can be done by
	getCameraId()
 src/qcam/main_window.cpp | 41 ++++++++++++++++++++--------------------
 src/qcam/main_window.h   |  5 +++--
 2 files changed, 23 insertions(+), 23 deletions(-)

Comments

Kieran Bingham Aug. 8, 2022, 8:56 a.m. UTC | #1 
Quoting Utkarsh Tiwari via libcamera-devel (2022-08-07 16:40:02)
> Replace the cameraCombo_ on the toolbar with a QPushButton which
> displays the camSelectDialog. This would allow the user to view
> information about the camera when switching.
> 
> The QPushButton text is set to the camera Id currently in use.
> 
> Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
> ---
> Difference from v2 :
>         remove redudant function getCurrentCamera, its job can be done by
>         getCameraId()
>  src/qcam/main_window.cpp | 41 ++++++++++++++++++++--------------------
>  src/qcam/main_window.h   |  5 +++--
>  2 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index dd30817d..98c22b71 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -193,14 +193,11 @@ int MainWindow::createToolbars()
>         connect(action, &QAction::triggered, this, &MainWindow::quit);
>  
>         /* Camera selector. */
> -       cameraCombo_ = new QComboBox();
> -       connect(cameraCombo_, QOverload<int>::of(&QComboBox::activated),
> +       cameraSwitchButton_ = new QPushButton;

The dialog is called camSelectDialog (or cameraSelectDialog), so I'd call this
cameraSelectButton_


> +       connect(cameraSwitchButton_, &QPushButton::clicked,
>                 this, &MainWindow::switchCamera);
>  
> -       for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> -               cameraCombo_->addItem(QString::fromStdString(cam->id()));
> -
> -       toolbar_->addWidget(cameraCombo_);
> +       toolbar_->addWidget(cameraSwitchButton_);
>  
>         toolbar_->addSeparator();
>  
> @@ -260,14 +257,19 @@ void MainWindow::updateTitle()
>   * Camera Selection
>   */
>  
> -void MainWindow::switchCamera(int index)
> +void MainWindow::switchCamera()
>  {
>         /* Get and acquire the new camera. */
> -       const auto &cameras = cm_->cameras();
> -       if (static_cast<unsigned int>(index) >= cameras.size())
> +       std::string newCameraId = chooseCamera();
> +
> +       if (newCameraId.empty())
>                 return;
>  
> -       const std::shared_ptr<Camera> &cam = cameras[index];
> +       if (camera_) {
> +               if (newCameraId == camera_->id())
> +                       return;
> +       }

I think you could shorten this to two lines:

	if (camera_ && camera_->id() == newCameraId)
		return;



> +       const std::shared_ptr<Camera> &cam = cm_->get(newCameraId);
>  
>         if (cam->acquire()) {
>                 qInfo() << "Failed to acquire camera" << cam->id().c_str();
> @@ -292,9 +294,12 @@ std::string MainWindow::chooseCamera()
>  {
>         camSelectDialog_ = new CamSelectDialog(cm_, this);

Aha so this could be as simple as :

	if (!camSelectDialog_)
		camSelectDialog_ = new CamSelectDialog(cm_, this);

to make it re-usable? (It would only construct on the first call).
(in the earlier patch).


Aside from those trivialish comments, I think this is ok so:

With those handled,
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  
> -       if (camSelectDialog_->exec() == QDialog::Accepted)
> -               return camSelectDialog_->getCameraId();
> -       else
> +       if (camSelectDialog_->exec() == QDialog::Accepted) {
> +               std::string cameraId = camSelectDialog_->getCameraId();
> +               cameraSwitchButton_->setText(QString::fromStdString(cameraId));
> +
> +               return cameraId;
> +       } else
>                 return std::string();
>  }
>  
> @@ -327,8 +332,8 @@ int MainWindow::openCamera()
>                 return -EBUSY;
>         }
>  
> -       /* Set the combo-box entry with the currently selected Camera. */
> -       cameraCombo_->setCurrentText(QString::fromStdString(cameraName));
> +       /* Set the camera switch button with the currently selected Camera id. */
> +       cameraSwitchButton_->setText(QString::fromStdString(cameraName));
>  
>         return 0;
>  }
> @@ -593,8 +598,6 @@ void MainWindow::processHotplug(HotplugEvent *e)
>         HotplugEvent::PlugEvent event = e->hotplugEvent();
>  
>         if (event == HotplugEvent::HotPlug) {
> -               cameraCombo_->addItem(QString::fromStdString(camera->id()));
> -
>                 if (camSelectDialog_)
>                         camSelectDialog_->cameraAdded(camera);
>         } else if (event == HotplugEvent::HotUnplug) {
> @@ -603,12 +606,8 @@ void MainWindow::processHotplug(HotplugEvent *e)
>                         toggleCapture(false);
>                         camera_->release();
>                         camera_.reset();
> -                       cameraCombo_->setCurrentIndex(0);
>                 }
>  
> -               int camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));
> -               cameraCombo_->removeItem(camIndex);
> -
>                 if (camSelectDialog_)
>                         camSelectDialog_->cameraRemoved(camera);
>         }
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 6d80b5be..fe0f5938 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -24,6 +24,7 @@
>  #include <QMutex>
>  #include <QObject>
>  #include <QPointer>
> +#include <QPushButton>
>  #include <QQueue>
>  #include <QTimer>
>  
> @@ -61,7 +62,7 @@ private Q_SLOTS:
>         void quit();
>         void updateTitle();
>  
> -       void switchCamera(int index);
> +       void switchCamera();
>         void toggleCapture(bool start);
>  
>         void saveImageAs();
> @@ -91,7 +92,7 @@ private:
>         /* UI elements */
>         QToolBar *toolbar_;
>         QAction *startStopAction_;
> -       QComboBox *cameraCombo_;
> +       QPushButton *cameraSwitchButton_;
>         QAction *saveRaw_;
>         ViewFinder *viewfinder_;
>  
> -- 
> 2.25.1
>
Utkarsh Tiwari Aug. 8, 2022, 5:25 p.m. UTC | #2 
Hi, Kieran agreed with everything on this review,
would do the changes.

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

> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-07 16:40:02)
> > Replace the cameraCombo_ on the toolbar with a QPushButton which
> > displays the camSelectDialog. This would allow the user to view
> > information about the camera when switching.
> >
> > The QPushButton text is set to the camera Id currently in use.
> >
> > Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
> > ---
> > Difference from v2 :
> >         remove redudant function getCurrentCamera, its job can be done by
> >         getCameraId()
> >  src/qcam/main_window.cpp | 41 ++++++++++++++++++++--------------------
> >  src/qcam/main_window.h   |  5 +++--
> >  2 files changed, 23 insertions(+), 23 deletions(-)
> >
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index dd30817d..98c22b71 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -193,14 +193,11 @@ int MainWindow::createToolbars()
> >         connect(action, &QAction::triggered, this, &MainWindow::quit);
> >
> >         /* Camera selector. */
> > -       cameraCombo_ = new QComboBox();
> > -       connect(cameraCombo_, QOverload<int>::of(&QComboBox::activated),
> > +       cameraSwitchButton_ = new QPushButton;
>
> The dialog is called camSelectDialog (or cameraSelectDialog), so I'd call
> this
> cameraSelectButton_
>
>
> > +       connect(cameraSwitchButton_, &QPushButton::clicked,
> >                 this, &MainWindow::switchCamera);
> >
> > -       for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> > -               cameraCombo_->addItem(QString::fromStdString(cam->id()));
> > -
> > -       toolbar_->addWidget(cameraCombo_);
> > +       toolbar_->addWidget(cameraSwitchButton_);
> >
> >         toolbar_->addSeparator();
> >
> > @@ -260,14 +257,19 @@ void MainWindow::updateTitle()
> >   * Camera Selection
> >   */
> >
> > -void MainWindow::switchCamera(int index)
> > +void MainWindow::switchCamera()
> >  {
> >         /* Get and acquire the new camera. */
> > -       const auto &cameras = cm_->cameras();
> > -       if (static_cast<unsigned int>(index) >= cameras.size())
> > +       std::string newCameraId = chooseCamera();
> > +
> > +       if (newCameraId.empty())
> >                 return;
> >
> > -       const std::shared_ptr<Camera> &cam = cameras[index];
> > +       if (camera_) {
> > +               if (newCameraId == camera_->id())
> > +                       return;
> > +       }
>
> I think you could shorten this to two lines:
>
>         if (camera_ && camera_->id() == newCameraId)
>                 return;
>
>
>
> > +       const std::shared_ptr<Camera> &cam = cm_->get(newCameraId);
> >
> >         if (cam->acquire()) {
> >                 qInfo() << "Failed to acquire camera" <<
> cam->id().c_str();
> > @@ -292,9 +294,12 @@ std::string MainWindow::chooseCamera()
> >  {
> >         camSelectDialog_ = new CamSelectDialog(cm_, this);
>
> Aha so this could be as simple as :
>
>         if (!camSelectDialog_)
>                 camSelectDialog_ = new CamSelectDialog(cm_, this);
>
> to make it re-usable? (It would only construct on the first call).
> (in the earlier patch).
>
>
> Aside from those trivialish comments, I think this is ok so:
>
> With those handled,
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> >
> > -       if (camSelectDialog_->exec() == QDialog::Accepted)
> > -               return camSelectDialog_->getCameraId();
> > -       else
> > +       if (camSelectDialog_->exec() == QDialog::Accepted) {
> > +               std::string cameraId = camSelectDialog_->getCameraId();
> > +
>  cameraSwitchButton_->setText(QString::fromStdString(cameraId));
> > +
> > +               return cameraId;
> > +       } else
> >                 return std::string();
> >  }
> >
> > @@ -327,8 +332,8 @@ int MainWindow::openCamera()
> >                 return -EBUSY;
> >         }
> >
> > -       /* Set the combo-box entry with the currently selected Camera. */
> > -       cameraCombo_->setCurrentText(QString::fromStdString(cameraName));
> > +       /* Set the camera switch button with the currently selected
> Camera id. */
> > +       cameraSwitchButton_->setText(QString::fromStdString(cameraName));
> >
> >         return 0;
> >  }
> > @@ -593,8 +598,6 @@ void MainWindow::processHotplug(HotplugEvent *e)
> >         HotplugEvent::PlugEvent event = e->hotplugEvent();
> >
> >         if (event == HotplugEvent::HotPlug) {
> > -
>  cameraCombo_->addItem(QString::fromStdString(camera->id()));
> > -
> >                 if (camSelectDialog_)
> >                         camSelectDialog_->cameraAdded(camera);
> >         } else if (event == HotplugEvent::HotUnplug) {
> > @@ -603,12 +606,8 @@ void MainWindow::processHotplug(HotplugEvent *e)
> >                         toggleCapture(false);
> >                         camera_->release();
> >                         camera_.reset();
> > -                       cameraCombo_->setCurrentIndex(0);
> >                 }
> >
> > -               int camIndex =
> cameraCombo_->findText(QString::fromStdString(camera->id()));
> > -               cameraCombo_->removeItem(camIndex);
> > -
> >                 if (camSelectDialog_)
> >                         camSelectDialog_->cameraRemoved(camera);
> >         }
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index 6d80b5be..fe0f5938 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -24,6 +24,7 @@
> >  #include <QMutex>
> >  #include <QObject>
> >  #include <QPointer>
> > +#include <QPushButton>
> >  #include <QQueue>
> >  #include <QTimer>
> >
> > @@ -61,7 +62,7 @@ private Q_SLOTS:
> >         void quit();
> >         void updateTitle();
> >
> > -       void switchCamera(int index);
> > +       void switchCamera();
> >         void toggleCapture(bool start);
> >
> >         void saveImageAs();
> > @@ -91,7 +92,7 @@ private:
> >         /* UI elements */
> >         QToolBar *toolbar_;
> >         QAction *startStopAction_;
> > -       QComboBox *cameraCombo_;
> > +       QPushButton *cameraSwitchButton_;
> >         QAction *saveRaw_;
> >         ViewFinder *viewfinder_;
> >
> > --
> > 2.25.1
> >
>

Patch
diff mbox series 

diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
index dd30817d..98c22b71 100644
--- a/src/qcam/main_window.cpp
+++ b/src/qcam/main_window.cpp
@@ -193,14 +193,11 @@  int MainWindow::createToolbars()
 	connect(action, &QAction::triggered, this, &MainWindow::quit);
 
 	/* Camera selector. */
-	cameraCombo_ = new QComboBox();
-	connect(cameraCombo_, QOverload<int>::of(&QComboBox::activated),
+	cameraSwitchButton_ = new QPushButton;
+	connect(cameraSwitchButton_, &QPushButton::clicked,
 		this, &MainWindow::switchCamera);
 
-	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
-		cameraCombo_->addItem(QString::fromStdString(cam->id()));
-
-	toolbar_->addWidget(cameraCombo_);
+	toolbar_->addWidget(cameraSwitchButton_);
 
 	toolbar_->addSeparator();
 
@@ -260,14 +257,19 @@  void MainWindow::updateTitle()
  * Camera Selection
  */
 
-void MainWindow::switchCamera(int index)
+void MainWindow::switchCamera()
 {
 	/* Get and acquire the new camera. */
-	const auto &cameras = cm_->cameras();
-	if (static_cast<unsigned int>(index) >= cameras.size())
+	std::string newCameraId = chooseCamera();
+
+	if (newCameraId.empty())
 		return;
 
-	const std::shared_ptr<Camera> &cam = cameras[index];
+	if (camera_) {
+		if (newCameraId == camera_->id())
+			return;
+	}
+	const std::shared_ptr<Camera> &cam = cm_->get(newCameraId);
 
 	if (cam->acquire()) {
 		qInfo() << "Failed to acquire camera" << cam->id().c_str();
@@ -292,9 +294,12 @@  std::string MainWindow::chooseCamera()
 {
 	camSelectDialog_ = new CamSelectDialog(cm_, this);
 
-	if (camSelectDialog_->exec() == QDialog::Accepted)
-		return camSelectDialog_->getCameraId();
-	else
+	if (camSelectDialog_->exec() == QDialog::Accepted) {
+		std::string cameraId = camSelectDialog_->getCameraId();
+		cameraSwitchButton_->setText(QString::fromStdString(cameraId));
+
+		return cameraId;
+	} else
 		return std::string();
 }
 
@@ -327,8 +332,8 @@  int MainWindow::openCamera()
 		return -EBUSY;
 	}
 
-	/* Set the combo-box entry with the currently selected Camera. */
-	cameraCombo_->setCurrentText(QString::fromStdString(cameraName));
+	/* Set the camera switch button with the currently selected Camera id. */
+	cameraSwitchButton_->setText(QString::fromStdString(cameraName));
 
 	return 0;
 }
@@ -593,8 +598,6 @@  void MainWindow::processHotplug(HotplugEvent *e)
 	HotplugEvent::PlugEvent event = e->hotplugEvent();
 
 	if (event == HotplugEvent::HotPlug) {
-		cameraCombo_->addItem(QString::fromStdString(camera->id()));
-
 		if (camSelectDialog_)
 			camSelectDialog_->cameraAdded(camera);
 	} else if (event == HotplugEvent::HotUnplug) {
@@ -603,12 +606,8 @@  void MainWindow::processHotplug(HotplugEvent *e)
 			toggleCapture(false);
 			camera_->release();
 			camera_.reset();
-			cameraCombo_->setCurrentIndex(0);
 		}
 
-		int camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));
-		cameraCombo_->removeItem(camIndex);
-
 		if (camSelectDialog_)
 			camSelectDialog_->cameraRemoved(camera);
 	}
diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
index 6d80b5be..fe0f5938 100644
--- a/src/qcam/main_window.h
+++ b/src/qcam/main_window.h
@@ -24,6 +24,7 @@ 
 #include <QMutex>
 #include <QObject>
 #include <QPointer>
+#include <QPushButton>
 #include <QQueue>
 #include <QTimer>
 
@@ -61,7 +62,7 @@  private Q_SLOTS:
 	void quit();
 	void updateTitle();
 
-	void switchCamera(int index);
+	void switchCamera();
 	void toggleCapture(bool start);
 
 	void saveImageAs();
@@ -91,7 +92,7 @@  private:
 	/* UI elements */
 	QToolBar *toolbar_;
 	QAction *startStopAction_;
-	QComboBox *cameraCombo_;
+	QPushButton *cameraSwitchButton_;
 	QAction *saveRaw_;
 	ViewFinder *viewfinder_;