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

[Kieran Bingham]

Quoting Utkarsh Tiwari via libcamera-devel (2022-08-09 21:50:41)
> Display the path of the selected capture script in a thinner font.
> 
> Signed-off-by: Utkarsh Tiwari <utkarsh02t at gmail.com>
> ---
>  src/qcam/cam_select_dialog.cpp | 38 +++++++++++++++++++++++++++-------
>  src/qcam/cam_select_dialog.h   |  8 ++++++-
>  src/qcam/main_window.cpp       |  3 ++-
>  3 files changed, 40 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp
> index f3df9970..5a9de08c 100644
> --- a/src/qcam/cam_select_dialog.cpp
> +++ b/src/qcam/cam_select_dialog.cpp
> @@ -20,8 +20,9 @@
>  #include <QString>
>  
>  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)
>  {
>         /* Use a QFormLayout for the dialog. */
>         QFormLayout *layout = new QFormLayout(this);
> @@ -39,14 +40,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;

Creating it again? That's another memory leak.

It might be worth trying your patchs with valgrind or the memleak
detectors that can be enabled with meson.


> +       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 */
> @@ -63,7 +81,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);
>  }
>  
> @@ -138,16 +156,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_);
> +               }
>         }
>  }
>  
> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h
> index 56d90596..84904640 100644
> --- a/src/qcam/cam_select_dialog.h
> +++ b/src/qcam/cam_select_dialog.h
> @@ -18,17 +18,20 @@
>  #include <QDialog>
>  #include <QDialogButtonBox>
>  #include <QFileDialog>
> +#include <QFont>

If this is only used in the .cpp file, include it there. I don't think
it's needed in the header.

>  #include <QFormLayout>
>  #include <QLabel>
>  #include <QPushButton>
>  #include <QString>
> +#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;
>  
> @@ -66,5 +69,8 @@ private:
>         QComboBox *cameraIdComboBox_;
>         QLabel *cameraLocation_;
>         QLabel *cameraModel_;
> +
> +       QVBoxLayout *captureWidgetLayout_;
>         QPushButton *captureScriptButton_;
> +       QLabel *scriptPathLabel_ = new QLabel;


This doesn't match how everthing else is initialised, and I also think
this could simply be a local instance, not a pointer:
	QLabel scriptPath_;


>  };
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index d73fb42a..f2e3c576 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -337,7 +337,8 @@ std::string MainWindow::chooseCamera()
>         bool scriptRunning = script_ != nullptr;
>  
>         /* Construct the selection dialog, unconditionally. */
> -       cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptRunning, this);
> +       cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptRunning,
> +                                                        scriptPath_, this);
>  
>         connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,
>                 this, &MainWindow::stopCaptureScript);
> -- 
> 2.25.1
>