[libcamera-devel] [PATCH v8.1 7/8] qcam: CamSelectDialog: Display Capture script path

[Laurent Pinchart]

On Mon, Aug 22, 2022 at 11:35:03PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-12 13:18:35)
> > Display the path of the selected capture script in a thinner font.

The rationale for this should be indicated in the commit message. I
suspect this is because the path can be long. This would be fixed by my
UI proposal in patch 6/8, by displaying the file name without the path
in the button.

> > Signed-off-by: Utkarsh Tiwari <utkarsh02t at gmail.com>
> > ---
> > Difference from v8:
> >         1. scriptPathLabel_ now removed when informScriptReset() is invoked.
> > Difference from v7:
> >         1. The scriptPathLabel_ has a fixed parent now captureWidget
> >  src/qcam/cam_select_dialog.cpp | 40 ++++++++++++++++++++++++++++------
> >  src/qcam/cam_select_dialog.h   |  7 +++++-
> >  src/qcam/main_window.cpp       |  3 ++-
> >  3 files changed, 41 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp
> > index 58cbfc28..44fa3ce6 100644
> > --- a/src/qcam/cam_select_dialog.cpp
> > +++ b/src/qcam/cam_select_dialog.cpp
> > @@ -19,10 +19,12 @@
> >  #include <QFileDialog>
> >  #include <QFormLayout>
> >  #include <QString>
> > +#include <QVBoxLayout>
> >  
> >  CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,
> > -                                          bool isScriptRunning, QWidget *parent)
> > -       : QDialog(parent), cm_(cameraManager), isScriptRunning_(isScriptRunning)
> > +                                          bool isScriptRunning, std::string scriptPath, QWidget *parent)
> > +       : QDialog(parent), cm_(cameraManager),
> > +         isScriptRunning_(isScriptRunning), scriptPath_(scriptPath)
> 
> I feel suspicous that we need to store copies of these state variables
> that are (/were?) owned by the MainWindow and now are stored here...

And this change is unrelated to this patch, it seems to be done to
support 8/8, and should move there.

> >  {
> >         /* Use a QFormLayout for the dialog. */
> >         QFormLayout *layout = new QFormLayout(this);
> > @@ -40,14 +42,31 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag
> >         connect(cameraIdComboBox_, &QComboBox::currentTextChanged,
> >                 this, &CameraSelectorDialog::handleCameraChange);
> >  
> > +       /* Setup widget for capture script button. */
> > +       QWidget *captureWidget = new QWidget;
> > +       captureWidgetLayout_ = new QVBoxLayout(captureWidget);
> > +       captureWidgetLayout_->setMargin(0);
> > +
> >         captureScriptButton_ = new QPushButton;
> >         connect(captureScriptButton_, &QPushButton::clicked,
> >                 this, &CameraSelectorDialog::handleCaptureScriptButton);
> > +       captureWidgetLayout_->addWidget(captureScriptButton_);
> > +
> > +       /* Use a thinner font to indicate script info. */
> > +       QFont smallFont;
> > +       smallFont.setWeight(QFont::Thin);
> > +
> > +       scriptPathLabel_ = new QLabel(captureWidget);
> > +       scriptPathLabel_->setFont(smallFont);
> > +       scriptPathLabel_->setWordWrap(true);
> >  
> >         /* Display the action that would be performed when button is clicked. */
> > -       if (isScriptRunning_)
> > +       if (isScriptRunning_) {
> >                 captureScriptButton_->setText("Stop");
> > -       else
> > +
> > +               scriptPathLabel_->setText(QString::fromStdString(scriptPath_));
> > +               captureWidgetLayout_->addWidget(scriptPathLabel_);
> > +       } else
> >                 captureScriptButton_->setText("Open");
> >  
> >         /* Setup the QDialogButton Box */
> > @@ -64,7 +83,7 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag
> >         layout->addRow("Camera:", cameraIdComboBox_);
> >         layout->addRow("Location:", cameraLocation_);
> >         layout->addRow("Model:", cameraModel_);
> > -       layout->addRow("Capture Script:", captureScriptButton_);
> > +       layout->addRow("Capture Script:", captureWidget);
> >         layout->addWidget(buttonBox);
> >  }
> >  
> > @@ -139,16 +158,22 @@ void CameraSelectorDialog::handleCaptureScriptButton()
> >                 Q_EMIT stopCaptureScript();
> >                 isScriptRunning_ = false;
> >                 captureScriptButton_->setText("Open");
> > +
> > +               captureWidgetLayout_->removeWidget(scriptPathLabel_);
> >         } else {
> >                 selectedScriptPath_ = QFileDialog::getOpenFileName(this,
> >                                                                    "Run Capture Script", QDir::currentPath(),
> >                                                                    "Capture Script (*.yaml)")
> >                                               .toStdString();
> >  
> > -               if (!selectedScriptPath_.empty())
> > +               if (!selectedScriptPath_.empty()) {
> >                         captureScriptButton_->setText("Loaded");
> > -               else
> > +                       scriptPathLabel_->setText(QString::fromStdString(selectedScriptPath_));
> > +                       captureWidgetLayout_->addWidget(scriptPathLabel_);
> > +               } else {
> >                         captureScriptButton_->setText("Open");
> > +                       captureWidgetLayout_->removeWidget(scriptPathLabel_);
> > +               }
> 
> This adding and removing of the label seems a bit awkward too.
> 
> I think most dialog boxes would present a consistent view, and have the
> fields enabled or disabled for entry when available. Or always visible
> presenting an empty box.
> 
> As this is displaying a filename, I'd expect some sort of text entry box
> to be visible (always) in the layout that represents the path of the
> capture script (or empty when there is none).
> 
> I think this would all be squashed in the previous patch too...

I expect this patch to be heavily modified, or possibly dropped, with
the UI changes from 6/8.

> >         }
> >  }
> >  
> > @@ -170,6 +195,7 @@ void CameraSelectorDialog::informScriptReset()
> >         isScriptRunning_ = false;
> >         scriptPath_.clear();
> >         selectedScriptPath_.clear();
> > +       captureWidgetLayout_->removeWidget(scriptPathLabel_);
> >         captureScriptButton_->setText("Open");
> >  }
> >  
> > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h
> > index bbdf897e..72dfbb14 100644
> > --- a/src/qcam/cam_select_dialog.h
> > +++ b/src/qcam/cam_select_dialog.h
> > @@ -18,13 +18,15 @@
> >  #include <QDialog>
> >  #include <QLabel>
> >  #include <QPushButton>
> > +#include <QVBoxLayout>
> > +#include <QWidget>
> >  
> >  class CameraSelectorDialog : public QDialog
> >  {
> >         Q_OBJECT
> >  public:
> >         CameraSelectorDialog(libcamera::CameraManager *cameraManager,
> > -                            bool isScriptRunning, QWidget *parent);
> > +                            bool isScriptRunning, std::string scriptPath, QWidget *parent);
> >  
> >         ~CameraSelectorDialog() = default;
> >  
> > @@ -62,5 +64,8 @@ private:
> >         QComboBox *cameraIdComboBox_;
> >         QLabel *cameraLocation_;
> >         QLabel *cameraModel_;
> > +
> > +       QVBoxLayout *captureWidgetLayout_;
> >         QPushButton *captureScriptButton_;
> > +       QLabel *scriptPathLabel_;
> >  };
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 3c7c3173..753e1af9 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -339,7 +339,8 @@ std::string MainWindow::chooseCamera()
> >  
> >         /* Construct the selection dialog, only the first time. */
> >         if (!cameraSelectorDialog_)
> > -               cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptRunning, this);
> > +               cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptRunning,
> > +                                                                scriptPath_, this);
> >  
> >         connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,
> >                 this, &MainWindow::stopCaptureScript);

-- 
Regards,

Laurent Pinchart