[libcamera-devel,06/21] qcam: main_window: Replace start and stop actions with a toggle action
diff mbox series

Message ID 20200323142205.28342-7-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • qcam: Bypass format conversion when not required
Related show

Commit Message

Laurent Pinchart March 23, 2020, 2:21 p.m. UTC 
The main window toolbar contains a start button and a stop button. This
allows starting an already started camera (which is currently not
handled and results in an error) or stopping an already stopped camera.
Replace the two actions with a single start/stop toggle action,
preventing UI misuse and reducing confusion.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/qcam/main_window.cpp | 30 ++++++++++++++++++++----------
 src/qcam/main_window.h   |  9 ++++++---
 2 files changed, 26 insertions(+), 13 deletions(-)

Comments

Kieran Bingham March 23, 2020, 3:03 p.m. UTC | #1 
Hi Laurent,

On 23/03/2020 14:21, Laurent Pinchart wrote:
> The main window toolbar contains a start button and a stop button. This
> allows starting an already started camera (which is currently not
> handled and results in an error) or stopping an already stopped camera.
> Replace the two actions with a single start/stop toggle action,
> preventing UI misuse and reducing confusion.

Oh - I was actually using this as a test already :-) - I have used it to
start a running stream, and stop a stopped stream.

It even found a bug in a pipeline already - but perhaps that just
highlights that we need a pipeline validation tool already, so it's not
particularly a requirement of the GUI interface.

> 

Otherwise a question below - but:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/qcam/main_window.cpp | 30 ++++++++++++++++++++----------
>  src/qcam/main_window.h   |  9 ++++++---
>  2 files changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index afd8b9c413f5..86f92360a1a9 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -63,11 +63,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>  	adjustSize();
>  
>  	ret = openCamera();
> -	if (!ret)
> -		ret = startCapture();
> -
>  	if (ret < 0)
>  		quit();
> +
> +	startStopAction_->setChecked(true);
>  }
>  
>  MainWindow::~MainWindow()
> @@ -113,11 +112,10 @@ int MainWindow::createToolbars()
>  
>  	toolbar_->addSeparator();
>  
> -	action = toolbar_->addAction(QIcon(":play-circle.svg"), "start");
> -	connect(action, &QAction::triggered, this, &MainWindow::startCapture);
> -
> -	action = toolbar_->addAction(QIcon(":stop-circle.svg"), "stop");
> -	connect(action, &QAction::triggered, this, &MainWindow::stopCapture);
> +	action = toolbar_->addAction(QIcon(":play-circle.svg"), "Start Capture");
> +	action->setCheckable(true);
> +	connect(action, &QAction::toggled, this, &MainWindow::toggleCapture);
> +	startStopAction_ = action;
>  
>  	action = toolbar_->addAction(QIcon(":save.svg"), "saveAs");
>  	connect(action, &QAction::triggered, this, &MainWindow::saveImageAs);
> @@ -159,12 +157,12 @@ void MainWindow::switchCamera(int index)
>  
>  	std::cout << "Switching to camera " << cam->name() << std::endl;
>  
> -	stopCapture();
> +	startStopAction_->setChecked(false);
>  
>  	camera_->release();
>  	camera_ = cam;
>  
> -	startCapture();
> +	startStopAction_->setChecked(true);
>  }
>  
>  std::string MainWindow::chooseCamera()
> @@ -217,6 +215,17 @@ int MainWindow::openCamera()
>  	return 0;
>  }
>  
> +void MainWindow::toggleCapture(bool start)
> +{
> +	if (start) {
> +		startCapture();
> +		startStopAction_->setText("Stop Capture");
> +	} else {
> +		stopCapture();
> +		startStopAction_->setText("Start Capture");

Shouldn't this update the icon to reflect the change in state somehow?

> +	}
> +}
> +
>  int MainWindow::startCapture()
>  {
>  	int ret;
> @@ -322,6 +331,7 @@ int MainWindow::startCapture()
>  	}
>  
>  	isCapturing_ = true;
> +
>  	return 0;
>  
>  error_disconnect:
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index c623120d5894..3d8d02b44696 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -26,6 +26,7 @@
>  
>  using namespace libcamera;
>  
> +class QAction;
>  class ViewFinder;
>  
>  enum {
> @@ -49,9 +50,7 @@ private Q_SLOTS:
>  	void updateTitle();
>  
>  	void switchCamera(int index);
> -
> -	int startCapture();
> -	void stopCapture();
> +	void toggleCapture(bool start);
>  
>  	void saveImageAs();
>  
> @@ -60,6 +59,9 @@ private:
>  	std::string chooseCamera();
>  	int openCamera();
>  
> +	int startCapture();
> +	void stopCapture();
> +
>  	void requestComplete(Request *request);
>  	void processCapture();
>  	int display(FrameBuffer *buffer);
> @@ -87,6 +89,7 @@ private:
>  	QQueue<FrameBuffer *> doneQueue_;
>  
>  	QToolBar *toolbar_;
> +	QAction *startStopAction_;
>  	ViewFinder *viewfinder_;
>  	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
>  };
>
Laurent Pinchart March 23, 2020, 3:50 p.m. UTC | #2 
Hi Kieran,

On Mon, Mar 23, 2020 at 03:03:51PM +0000, Kieran Bingham wrote:
> On 23/03/2020 14:21, Laurent Pinchart wrote:
> > The main window toolbar contains a start button and a stop button. This
> > allows starting an already started camera (which is currently not
> > handled and results in an error) or stopping an already stopped camera.
> > Replace the two actions with a single start/stop toggle action,
> > preventing UI misuse and reducing confusion.
> 
> Oh - I was actually using this as a test already :-) - I have used it to
> start a running stream, and stop a stopped stream.

That's something that should be caught by unit tests though.

> It even found a bug in a pipeline already - but perhaps that just
> highlights that we need a pipeline validation tool already, so it's not
> particularly a requirement of the GUI interface.

Yes, we also need a pipeline validation tool :-)

> > 
> 
> Otherwise a question below - but:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/qcam/main_window.cpp | 30 ++++++++++++++++++++----------
> >  src/qcam/main_window.h   |  9 ++++++---
> >  2 files changed, 26 insertions(+), 13 deletions(-)
> > 
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index afd8b9c413f5..86f92360a1a9 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -63,11 +63,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> >  	adjustSize();
> >  
> >  	ret = openCamera();
> > -	if (!ret)
> > -		ret = startCapture();
> > -
> >  	if (ret < 0)
> >  		quit();
> > +
> > +	startStopAction_->setChecked(true);
> >  }
> >  
> >  MainWindow::~MainWindow()
> > @@ -113,11 +112,10 @@ int MainWindow::createToolbars()
> >  
> >  	toolbar_->addSeparator();
> >  
> > -	action = toolbar_->addAction(QIcon(":play-circle.svg"), "start");
> > -	connect(action, &QAction::triggered, this, &MainWindow::startCapture);
> > -
> > -	action = toolbar_->addAction(QIcon(":stop-circle.svg"), "stop");
> > -	connect(action, &QAction::triggered, this, &MainWindow::stopCapture);
> > +	action = toolbar_->addAction(QIcon(":play-circle.svg"), "Start Capture");
> > +	action->setCheckable(true);
> > +	connect(action, &QAction::toggled, this, &MainWindow::toggleCapture);
> > +	startStopAction_ = action;
> >  
> >  	action = toolbar_->addAction(QIcon(":save.svg"), "saveAs");
> >  	connect(action, &QAction::triggered, this, &MainWindow::saveImageAs);
> > @@ -159,12 +157,12 @@ void MainWindow::switchCamera(int index)
> >  
> >  	std::cout << "Switching to camera " << cam->name() << std::endl;
> >  
> > -	stopCapture();
> > +	startStopAction_->setChecked(false);
> >  
> >  	camera_->release();
> >  	camera_ = cam;
> >  
> > -	startCapture();
> > +	startStopAction_->setChecked(true);
> >  }
> >  
> >  std::string MainWindow::chooseCamera()
> > @@ -217,6 +215,17 @@ int MainWindow::openCamera()
> >  	return 0;
> >  }
> >  
> > +void MainWindow::toggleCapture(bool start)
> > +{
> > +	if (start) {
> > +		startCapture();
> > +		startStopAction_->setText("Stop Capture");
> > +	} else {
> > +		stopCapture();
> > +		startStopAction_->setText("Start Capture");
> 
> Shouldn't this update the icon to reflect the change in state somehow?

I have mixed feelings, so now that you ask, I'll fix this.

> > +	}
> > +}
> > +
> >  int MainWindow::startCapture()
> >  {
> >  	int ret;
> > @@ -322,6 +331,7 @@ int MainWindow::startCapture()
> >  	}
> >  
> >  	isCapturing_ = true;
> > +
> >  	return 0;
> >  
> >  error_disconnect:
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index c623120d5894..3d8d02b44696 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -26,6 +26,7 @@
> >  
> >  using namespace libcamera;
> >  
> > +class QAction;
> >  class ViewFinder;
> >  
> >  enum {
> > @@ -49,9 +50,7 @@ private Q_SLOTS:
> >  	void updateTitle();
> >  
> >  	void switchCamera(int index);
> > -
> > -	int startCapture();
> > -	void stopCapture();
> > +	void toggleCapture(bool start);
> >  
> >  	void saveImageAs();
> >  
> > @@ -60,6 +59,9 @@ private:
> >  	std::string chooseCamera();
> >  	int openCamera();
> >  
> > +	int startCapture();
> > +	void stopCapture();
> > +
> >  	void requestComplete(Request *request);
> >  	void processCapture();
> >  	int display(FrameBuffer *buffer);
> > @@ -87,6 +89,7 @@ private:
> >  	QQueue<FrameBuffer *> doneQueue_;
> >  
> >  	QToolBar *toolbar_;
> > +	QAction *startStopAction_;
> >  	ViewFinder *viewfinder_;
> >  	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
> >  };

Patch
diff mbox series 

diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
index afd8b9c413f5..86f92360a1a9 100644
--- a/src/qcam/main_window.cpp
+++ b/src/qcam/main_window.cpp
@@ -63,11 +63,10 @@  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
 	adjustSize();
 
 	ret = openCamera();
-	if (!ret)
-		ret = startCapture();
-
 	if (ret < 0)
 		quit();
+
+	startStopAction_->setChecked(true);
 }
 
 MainWindow::~MainWindow()
@@ -113,11 +112,10 @@  int MainWindow::createToolbars()
 
 	toolbar_->addSeparator();
 
-	action = toolbar_->addAction(QIcon(":play-circle.svg"), "start");
-	connect(action, &QAction::triggered, this, &MainWindow::startCapture);
-
-	action = toolbar_->addAction(QIcon(":stop-circle.svg"), "stop");
-	connect(action, &QAction::triggered, this, &MainWindow::stopCapture);
+	action = toolbar_->addAction(QIcon(":play-circle.svg"), "Start Capture");
+	action->setCheckable(true);
+	connect(action, &QAction::toggled, this, &MainWindow::toggleCapture);
+	startStopAction_ = action;
 
 	action = toolbar_->addAction(QIcon(":save.svg"), "saveAs");
 	connect(action, &QAction::triggered, this, &MainWindow::saveImageAs);
@@ -159,12 +157,12 @@  void MainWindow::switchCamera(int index)
 
 	std::cout << "Switching to camera " << cam->name() << std::endl;
 
-	stopCapture();
+	startStopAction_->setChecked(false);
 
 	camera_->release();
 	camera_ = cam;
 
-	startCapture();
+	startStopAction_->setChecked(true);
 }
 
 std::string MainWindow::chooseCamera()
@@ -217,6 +215,17 @@  int MainWindow::openCamera()
 	return 0;
 }
 
+void MainWindow::toggleCapture(bool start)
+{
+	if (start) {
+		startCapture();
+		startStopAction_->setText("Stop Capture");
+	} else {
+		stopCapture();
+		startStopAction_->setText("Start Capture");
+	}
+}
+
 int MainWindow::startCapture()
 {
 	int ret;
@@ -322,6 +331,7 @@  int MainWindow::startCapture()
 	}
 
 	isCapturing_ = true;
+
 	return 0;
 
 error_disconnect:
diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
index c623120d5894..3d8d02b44696 100644
--- a/src/qcam/main_window.h
+++ b/src/qcam/main_window.h
@@ -26,6 +26,7 @@ 
 
 using namespace libcamera;
 
+class QAction;
 class ViewFinder;
 
 enum {
@@ -49,9 +50,7 @@  private Q_SLOTS:
 	void updateTitle();
 
 	void switchCamera(int index);
-
-	int startCapture();
-	void stopCapture();
+	void toggleCapture(bool start);
 
 	void saveImageAs();
 
@@ -60,6 +59,9 @@  private:
 	std::string chooseCamera();
 	int openCamera();
 
+	int startCapture();
+	void stopCapture();
+
 	void requestComplete(Request *request);
 	void processCapture();
 	int display(FrameBuffer *buffer);
@@ -87,6 +89,7 @@  private:
 	QQueue<FrameBuffer *> doneQueue_;
 
 	QToolBar *toolbar_;
+	QAction *startStopAction_;
 	ViewFinder *viewfinder_;
 	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
 };