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

[Laurent Pinchart]

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 at ideasonboard.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at 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_;
> >  };

-- 
Regards,

Laurent Pinchart