[libcamera-devel] [PATCH v5 3/3] qcam: Add --script to load capture script

[Laurent Pinchart]

Hi Utkarsh,

Thank you for the patch.

On Wed, Jul 27, 2022 at 01:11:23AM +0530, Utkarsh Tiwari via libcamera-devel wrote:
> Add --script as an individual option to load capture scripts.
> Load the capture script before starting the capture.
> 
> If an invalid capture script has been given, display a QMessageBox and
> print it on the console informing that the script is invalid. Do not
> start the capture.
> 
> Signed-off-by: Utkarsh Tiwari <utkarsh02t at gmail.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  Changes from v4
> 	- Now when we have an invalid script we quit and don't start the
> 	  capture.
>  src/qcam/main.cpp        |  3 +++
>  src/qcam/main_window.cpp | 20 ++++++++++++++++++++
>  src/qcam/main_window.h   |  1 +
>  3 files changed, 24 insertions(+)
> 
> diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp
> index d3f01a85..91166be5 100644
> --- a/src/qcam/main.cpp
> +++ b/src/qcam/main.cpp
> @@ -43,6 +43,9 @@ OptionsParser::Options parseOptions(int argc, char *argv[])
>  			 "Set configuration of a camera stream", "stream", true);
>  	parser.addOption(OptVerbose, OptionNone,
>  			 "Print verbose log messages", "verbose");
> +	parser.addOption(OptCaptureScript, OptionString,
> +			 "Load a capture session configuration script from a file",
> +			 "script", ArgumentRequired, "script");
>  
>  	OptionsParser::Options options = parser.parse(argc, argv);
>  	if (options.isSet(OptHelp))
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 9dc96fbb..7b2bc84b 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -152,6 +152,26 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)

The comment a few lines above should be changed to

	/* Open the camera. */

>  		return;
>  	}
>  

And here,

	/* Open and load the capture script if specified on the command line. */

> +	if (options_.isSet(OptCaptureScript)) {
> +		std::string scriptName = options_[OptCaptureScript].toString();
> +		script_ = std::make_unique<CaptureScript>(camera_, scriptName);

There's one issue here. You create the capture script for the currently
selected camera, which is fine, but only until a different camera is
selected. Then the capture script won't match anymore. We could then
reset the capture script, but what should we do if the user select the
previous camera again ? The behaviour seems confusing to me. I wonder if
a better user experience could be achieved, maybe by redesigning the
camera selection UI to tie it with capture script selection.

Good UI design is difficult :-S

> +		if (!script_->valid()) {
> +			QMessageBox::critical(this, "Invalid Script",
> +					      "Couldn't load the capture script");

I'd skip the message box. If the command line argument is wrong, a
message on the console should be enough.

> +			qInfo() << "Invalid Capture Script";

You can make that a qError().

> +
> +			script_.reset();

That's probably not needed given that you quit() right after.

> +
> +			/* Do not start capture if invalid script. */
> +			quit();
> +			return;
> +		}
> +
> +		/* Show stopping availability. */
> +		scriptExecAction_->setIcon(QIcon(":x-square.svg"));
> +		scriptExecAction_->setText("Stop Script execution");

It would be nice if we could centralize the logic that handles the
button state (and possible the construction of the CaptureScript
object). I'm not sure if there's an easy way to do so that will result
in more readable code.

> +	}
> +

Finally,

	/* Start the camera. */

>  	startStopAction_->setChecked(true);
>  }
>  
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index eb398c1d..59e50b5e 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -43,6 +43,7 @@ enum {
>  	OptRenderer = 'r',
>  	OptStream = 's',
>  	OptVerbose = 'v',
> +	OptCaptureScript = 256,
>  };
>  
>  class MainWindow : public QMainWindow

-- 
Regards,

Laurent Pinchart