[libcamera-devel] [PATCH v9 7/7] qcam: Add --script to load capture script

[Laurent Pinchart]

On Wed, Aug 31, 2022 at 10:41:32AM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-31 06:49:38)
> > 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 an critical error
> > QMessageBox and close the application.
> > ---
> >  Differences from v8:
> >     1. Functionally the patch remains same.
> >     2. The difference is that checking for OptCaptureScript is now done
> >     in MainWindow::MainWindow()
> >  src/qcam/main.cpp        |  3 +++
> >  src/qcam/main_window.cpp | 11 +++++++++--
> >  src/qcam/main_window.h   |  1 +
> >  3 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > 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 af992b94..e488b67f 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -145,6 +145,9 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> >         cm_->cameraAdded.connect(this, &MainWindow::addCamera);
> >         cm_->cameraRemoved.connect(this, &MainWindow::removeCamera);
> >  
> > +       if (options_.isSet(OptCaptureScript))
> > +               scriptPath_ = options_[OptCaptureScript].toString();

At this point the script isn't selected yet, it's only a default. I'd
store it in a local variable, passed to the camera selector dialog box
constructor, and let the normal capture script selection mechanism then
set scriptPath_.

Adding the script path parameter to the camera selector dialog box in
this patch instead of 6/7 would also be better, to make 6/7 clearer.

> > +
> >         cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptPath_, this);
> >         connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,
> >                 this, &MainWindow::stopCaptureScript);
> > @@ -157,9 +160,13 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> >         }
> >  
> >         /* Start capture script. */
> > -       if (!scriptPath_.empty())
> > +       if (!scriptPath_.empty()) {
> >                 ret = loadCaptureScript();
> > -
> > +               if (options_.isSet(OptCaptureScript)) {
> 
> Isn't ret supposed to be checked here ?
> 
> > +                       quit();

I don't think quitting is right here. Imagine the following scenerio:

- The user selects a capture script on the command line
- When the camera selection dialog box is opened, the user picks a
  different script
- The script fails to load

Why should we quit in that case, compared to the case where the user
will not have selected a script on the command line ?

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

-- 
Regards,

Laurent Pinchart