[libcamera-devel] [PATCH v7 7/8] qcam: CamSelectDialog: Display Capture script path
Utkarsh Tiwari
utkarsh02t at gmail.com
Wed Aug 10 06:26:58 CEST 2022
Hi Kieran, thanks for the review.
On Wed, Aug 10, 2022 at 3:42 AM Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> 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.
>
> Oops this should be the only one, the one below is an remnant of
rebases.
> It might be worth trying your patchs with valgrind or the memleak
> detectors that can be enabled with meson.
>
Yes, I would figure out how to run them. I have never used them before
but I don't think they would be able to find these leaks because these
objects *are* deleted when the window is closed as the parent of the
these QObjects is still the QDialog and when it dies it deletes its
children.
>
> > + 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.
>
Agreed.
>
> > #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_;
>
> Ah yes. this should just be QLabel *scriptPath_ = new QLabel;
I still think we should use pointers and allocate them on the heap.
If we go the stack based approach we would have to think in two phases
1. The GUI layout
2. The stack layout (Such that parent doesn't die before child)
Currently we can limit the member variables to only the important ones
For e.g captureWidget the important thing here is its layout and the
actual widget's symbol just lives locally in the constructor.
The QObject trees do take care of deleting everything if parents are
set right. And at all places (even in this buggy recreation ) whenever we
are creating them and adding them to its layout, the object deletion
gets handled.
>
> > };
> > 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220810/a0e4c42a/attachment.htm>
More information about the libcamera-devel
mailing list