[libcamera-devel] [PATCH v7 7/8] qcam: CamSelectDialog: Display Capture script path
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Aug 10 14:31:33 CEST 2022
Quoting Utkarsh Tiwari (2022-08-10 05:26:58)
> 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)
This isn't about stack vs heap - if the QLabel is a direct member of
CameraSelectorDialog, then when it is allocated, so too will the QLabel.
It becomes 'part' of the CameraSelectorDialog, so it will be allocated
at the same time (as part of the same allocation).
> 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.
Ok - I don't know enough about how QT is handling pointer ownership
here, so as long as it works and something is cleaning up correctly.
>
> >
> > > };
> > > 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
> > >
> >
More information about the libcamera-devel
mailing list