<div dir="ltr"><div dir="ltr">Hi Kieran, thanks for the review.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Aug 10, 2022 at 3:42 AM Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Utkarsh Tiwari via libcamera-devel (2022-08-09 21:50:41)<br>
> Display the path of the selected capture script in a thinner font.<br>
> <br>
> Signed-off-by: Utkarsh Tiwari <<a href="mailto:utkarsh02t@gmail.com" target="_blank">utkarsh02t@gmail.com</a>><br>
> ---<br>
> src/qcam/cam_select_dialog.cpp | 38 +++++++++++++++++++++++++++-------<br>
> src/qcam/cam_select_dialog.h | 8 ++++++-<br>
> src/qcam/main_window.cpp | 3 ++-<br>
> 3 files changed, 40 insertions(+), 9 deletions(-)<br>
> <br>
> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp<br>
> index f3df9970..5a9de08c 100644<br>
> --- a/src/qcam/cam_select_dialog.cpp<br>
> +++ b/src/qcam/cam_select_dialog.cpp<br>
> @@ -20,8 +20,9 @@<br>
> #include <QString><br>
> <br>
> CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,<br>
> - bool isScriptRunning, QWidget *parent)<br>
> - : QDialog(parent), cm_(cameraManager), isScriptRunning_(isScriptRunning)<br>
> + bool isScriptRunning, std::string scriptPath, QWidget *parent)<br>
> + : QDialog(parent), cm_(cameraManager),<br>
> + isScriptRunning_(isScriptRunning), scriptPath_(scriptPath)<br>
> {<br>
> /* Use a QFormLayout for the dialog. */<br>
> QFormLayout *layout = new QFormLayout(this);<br>
> @@ -39,14 +40,31 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag<br>
> connect(cameraIdComboBox_, &QComboBox::currentTextChanged,<br>
> this, &CameraSelectorDialog::handleCameraChange);<br>
> <br>
> + /* Setup widget for capture script button. */<br>
> + QWidget *captureWidget = new QWidget;<br>
> + captureWidgetLayout_ = new QVBoxLayout(captureWidget);<br>
> + captureWidgetLayout_->setMargin(0);<br>
> +<br>
> captureScriptButton_ = new QPushButton;<br>
> connect(captureScriptButton_, &QPushButton::clicked,<br>
> this, &CameraSelectorDialog::handleCaptureScriptButton);<br>
> + captureWidgetLayout_->addWidget(captureScriptButton_);<br>
> +<br>
> + /* Use a thinner font to indicate script info. */<br>
> + QFont smallFont;<br>
> + smallFont.setWeight(QFont::Thin);<br>
> +<br>
> + scriptPathLabel_ = new QLabel;<br>
<br>
Creating it again? That's another memory leak.<br>
<br></blockquote><div>Oops this should be the only one, the one below is an remnant of <br></div><div>rebases. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
It might be worth trying your patchs with valgrind or the memleak<br>
detectors that can be enabled with meson.<br>
</blockquote><div>Yes, I would figure out how to run them. I have never used them before</div><div>but I don't think they would be able to find these leaks because these</div><div>objects *are* deleted when the window is closed as the parent of the</div><div>these QObjects is still the QDialog and when it dies it deletes its children.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + scriptPathLabel_->setFont(smallFont);<br>
> + scriptPathLabel_->setWordWrap(true);<br>
> <br>
> /* Display the action that would be performed when button is clicked. */<br>
> - if (isScriptRunning_)<br>
> + if (isScriptRunning_) {<br>
> captureScriptButton_->setText("Stop");<br>
> - else<br>
> +<br>
> + scriptPathLabel_->setText(QString::fromStdString(scriptPath_));<br>
> + captureWidgetLayout_->addWidget(scriptPathLabel_);<br>
> + } else<br>
> captureScriptButton_->setText("Open");<br>
> <br>
> /* Setup the QDialogButton Box */<br>
> @@ -63,7 +81,7 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag<br>
> layout->addRow("Camera:", cameraIdComboBox_);<br>
> layout->addRow("Location:", cameraLocation_);<br>
> layout->addRow("Model:", cameraModel_);<br>
> - layout->addRow("Capture Script:", captureScriptButton_);<br>
> + layout->addRow("Capture Script:", captureWidget);<br>
> layout->addWidget(buttonBox);<br>
> }<br>
> <br>
> @@ -138,16 +156,22 @@ void CameraSelectorDialog::handleCaptureScriptButton()<br>
> Q_EMIT stopCaptureScript();<br>
> isScriptRunning_ = false;<br>
> captureScriptButton_->setText("Open");<br>
> +<br>
> + captureWidgetLayout_->removeWidget(scriptPathLabel_);<br>
> } else {<br>
> selectedScriptPath_ = QFileDialog::getOpenFileName(this,<br>
> "Run Capture Script", QDir::currentPath(),<br>
> "Capture Script (*.yaml)")<br>
> .toStdString();<br>
> <br>
> - if (!selectedScriptPath_.empty())<br>
> + if (!selectedScriptPath_.empty()) {<br>
> captureScriptButton_->setText("Loaded");<br>
> - else<br>
> + scriptPathLabel_->setText(QString::fromStdString(selectedScriptPath_));<br>
> + captureWidgetLayout_->addWidget(scriptPathLabel_);<br>
> + } else {<br>
> captureScriptButton_->setText("Open");<br>
> + captureWidgetLayout_->removeWidget(scriptPathLabel_);<br>
> + }<br>
> }<br>
> }<br>
> <br>
> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h<br>
> index 56d90596..84904640 100644<br>
> --- a/src/qcam/cam_select_dialog.h<br>
> +++ b/src/qcam/cam_select_dialog.h<br>
> @@ -18,17 +18,20 @@<br>
> #include <QDialog><br>
> #include <QDialogButtonBox><br>
> #include <QFileDialog><br>
> +#include <QFont><br>
<br>
If this is only used in the .cpp file, include it there. I don't think<br>
it's needed in the header.<br></blockquote><div>Agreed. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> #include <QFormLayout><br>
> #include <QLabel><br>
> #include <QPushButton><br>
> #include <QString><br>
> +#include <QVBoxLayout><br>
> +#include <QWidget><br>
> <br>
> class CameraSelectorDialog : public QDialog<br>
> {<br>
> Q_OBJECT<br>
> public:<br>
> CameraSelectorDialog(libcamera::CameraManager *cameraManager,<br>
> - bool isScriptRunning, QWidget *parent);<br>
> + bool isScriptRunning, std::string scriptPath, QWidget *parent);<br>
> <br>
> ~CameraSelectorDialog() = default;<br>
> <br>
> @@ -66,5 +69,8 @@ private:<br>
> QComboBox *cameraIdComboBox_;<br>
> QLabel *cameraLocation_;<br>
> QLabel *cameraModel_;<br>
> +<br>
> + QVBoxLayout *captureWidgetLayout_;<br>
> QPushButton *captureScriptButton_;<br>
> + QLabel *scriptPathLabel_ = new QLabel;<br>
<br>
<br>
This doesn't match how everthing else is initialised, and I also think<br>
this could simply be a local instance, not a pointer:<br>
QLabel scriptPath_;<br>
<br></blockquote><div>Ah yes. this should just be QLabel *scriptPath_ = new QLabel;</div><div>I still think we should use pointers and allocate them on the heap.</div><div>If we go the stack based approach we would have to think in two phases</div><div>1. The GUI layout</div><div>2. The stack layout (Such that parent doesn't die before child)</div><div>Currently we can limit the member variables to only the important ones</div><div>For e.g captureWidget the important thing here is its layout and the<br></div><div>actual widget's symbol just lives locally in the constructor. <br></div><div><br></div><div>The QObject trees do take care of deleting everything if parents are</div><div>set right. And at all places (even in this buggy recreation ) whenever we</div><div>are creating them and adding them to its layout, the object deletion</div><div>gets handled.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> };<br>
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp<br>
> index d73fb42a..f2e3c576 100644<br>
> --- a/src/qcam/main_window.cpp<br>
> +++ b/src/qcam/main_window.cpp<br>
> @@ -337,7 +337,8 @@ std::string MainWindow::chooseCamera()<br>
> bool scriptRunning = script_ != nullptr;<br>
> <br>
> /* Construct the selection dialog, unconditionally. */<br>
> - cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptRunning, this);<br>
> + cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptRunning,<br>
> + scriptPath_, this);<br>
> <br>
> connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,<br>
> this, &MainWindow::stopCaptureScript);<br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div>