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

[Kieran Bingham]

Quoting Utkarsh Tiwari via libcamera-devel (2022-08-12 13:18:35)
> Display the path of the selected capture script in a thinner font.
> 
> 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...


>  {
>         /* 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...

>         }
>  }
>  
> @@ -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);
> -- 
> 2.25.1
>