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

17095 diff mbox series

Display the path of the selected capture script in a thinner font.

Signed-off-by: Utkarsh Tiwari <utkarsh02t@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(-)

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@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
>

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