[{"id":24496,"web_url":"https://patchwork.libcamera.org/comment/24496/","msgid":"<166008314844.2423137.2848268962276089078@Monstersaurus>","date":"2022-08-09T22:12:28","subject":"Re: [libcamera-devel] [PATCH v7 7/8] qcam: CamSelectDialog: Display\n\tCapture script path","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Utkarsh Tiwari via libcamera-devel (2022-08-09 21:50:41)\n> Display the path of the selected capture script in a thinner font.\n> \n> Signed-off-by: Utkarsh Tiwari \n> ---\n> src/qcam/cam_select_dialog.cpp | 38 +++++++++++++++++++++++++++-------\n> src/qcam/cam_select_dialog.h | 8 ++++++-\n> src/qcam/main_window.cpp | 3 ++-\n> 3 files changed, 40 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp\n> index f3df9970..5a9de08c 100644\n> --- a/src/qcam/cam_select_dialog.cpp\n> +++ b/src/qcam/cam_select_dialog.cpp\n> @@ -20,8 +20,9 @@\n> #include \n> \n> CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> - bool isScriptRunning, QWidget *parent)\n> - : QDialog(parent), cm_(cameraManager), isScriptRunning_(isScriptRunning)\n> + bool isScriptRunning, std::string scriptPath, QWidget *parent)\n> + : QDialog(parent), cm_(cameraManager),\n> + isScriptRunning_(isScriptRunning), scriptPath_(scriptPath)\n> {\n> /* Use a QFormLayout for the dialog. */\n> QFormLayout *layout = new QFormLayout(this);\n> @@ -39,14 +40,31 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> connect(cameraIdComboBox_, &QComboBox::currentTextChanged,\n> this, &CameraSelectorDialog::handleCameraChange);\n> \n> + /* Setup widget for capture script button. */\n> + QWidget *captureWidget = new QWidget;\n> + captureWidgetLayout_ = new QVBoxLayout(captureWidget);\n> + captureWidgetLayout_->setMargin(0);\n> +\n> captureScriptButton_ = new QPushButton;\n> connect(captureScriptButton_, &QPushButton::clicked,\n> this, &CameraSelectorDialog::handleCaptureScriptButton);\n> + captureWidgetLayout_->addWidget(captureScriptButton_);\n> +\n> + /* Use a thinner font to indicate script info. */\n> + QFont smallFont;\n> + smallFont.setWeight(QFont::Thin);\n> +\n> + scriptPathLabel_ = new QLabel;\n\nCreating it again? That's another memory leak.\n\nIt might be worth trying your patchs with valgrind or the memleak\ndetectors that can be enabled with meson.\n\n\n> + scriptPathLabel_->setFont(smallFont);\n> + scriptPathLabel_->setWordWrap(true);\n> \n> /* Display the action that would be performed when button is clicked. */\n> - if (isScriptRunning_)\n> + if (isScriptRunning_) {\n> captureScriptButton_->setText(\"Stop\");\n> - else\n> +\n> + scriptPathLabel_->setText(QString::fromStdString(scriptPath_));\n> + captureWidgetLayout_->addWidget(scriptPathLabel_);\n> + } else\n> captureScriptButton_->setText(\"Open\");\n> \n> /* Setup the QDialogButton Box */\n> @@ -63,7 +81,7 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> layout->addRow(\"Camera:\", cameraIdComboBox_);\n> layout->addRow(\"Location:\", cameraLocation_);\n> layout->addRow(\"Model:\", cameraModel_);\n> - layout->addRow(\"Capture Script:\", captureScriptButton_);\n> + layout->addRow(\"Capture Script:\", captureWidget);\n> layout->addWidget(buttonBox);\n> }\n> \n> @@ -138,16 +156,22 @@ void CameraSelectorDialog::handleCaptureScriptButton()\n> Q_EMIT stopCaptureScript();\n> isScriptRunning_ = false;\n> captureScriptButton_->setText(\"Open\");\n> +\n> + captureWidgetLayout_->removeWidget(scriptPathLabel_);\n> } else {\n> selectedScriptPath_ = QFileDialog::getOpenFileName(this,\n> \"Run Capture Script\", QDir::currentPath(),\n> \"Capture Script (*.yaml)\")\n> .toStdString();\n> \n> - if (!selectedScriptPath_.empty())\n> + if (!selectedScriptPath_.empty()) {\n> captureScriptButton_->setText(\"Loaded\");\n> - else\n> + scriptPathLabel_->setText(QString::fromStdString(selectedScriptPath_));\n> + captureWidgetLayout_->addWidget(scriptPathLabel_);\n> + } else {\n> captureScriptButton_->setText(\"Open\");\n> + captureWidgetLayout_->removeWidget(scriptPathLabel_);\n> + }\n> }\n> }\n> \n> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> index 56d90596..84904640 100644\n> --- a/src/qcam/cam_select_dialog.h\n> +++ b/src/qcam/cam_select_dialog.h\n> @@ -18,17 +18,20 @@\n> #include \n> #include \n> #include \n> +#include \n\nIf this is only used in the .cpp file, include it there. I don't think\nit's needed in the header.\n\n> #include \n> #include \n> #include \n> #include \n> +#include \n> +#include \n> \n> class CameraSelectorDialog : public QDialog\n> {\n> Q_OBJECT\n> public:\n> CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> - bool isScriptRunning, QWidget *parent);\n> + bool isScriptRunning, std::string scriptPath, QWidget *parent);\n> \n> ~CameraSelectorDialog() = default;\n> \n> @@ -66,5 +69,8 @@ private:\n> QComboBox *cameraIdComboBox_;\n> QLabel *cameraLocation_;\n> QLabel *cameraModel_;\n> +\n> + QVBoxLayout *captureWidgetLayout_;\n> QPushButton *captureScriptButton_;\n> + QLabel *scriptPathLabel_ = new QLabel;\n\n\nThis doesn't match how everthing else is initialised, and I also think\nthis could simply be a local instance, not a pointer:\n\tQLabel scriptPath_;\n\n\n> };\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index d73fb42a..f2e3c576 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -337,7 +337,8 @@ std::string MainWindow::chooseCamera()\n> bool scriptRunning = script_ != nullptr;\n> \n> /* Construct the selection dialog, unconditionally. */\n> - cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptRunning, this);\n> + cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptRunning,\n> + scriptPath_, this);\n> \n> connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,\n> this, &MainWindow::stopCaptureScript);\n> -- \n> 2.25.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AF94EC3272\n\tfor ;\n\tTue, 9 Aug 2022 22:12:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A0ED6332B;\n\tWed, 10 Aug 2022 00:12:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C06A463315\n\tfor ;\n\tWed, 10 Aug 2022 00:12:31 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4DCE2481;\n\tWed, 10 Aug 2022 00:12:30 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660083154;\n\tbh=sFhqcVRStbYrjIoeEgLVDbF/1nR3QtmRZ2CPhz+o7qU=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=fTR3OVP1fwJGh21gYO9tBAuv+QYaXgy4E5u1reBkzPb6rSbcIMWZb9vRgGBIYCKs3\n\tv2HUDk+dKPP9VKniNpF8nc801qLmCjGVY3VzCTtRLvOjLOaVQ0wyiKnQ/ZyjjZxO2P\n\tyy8H9K33VnWe2KA7UcZRxTJx1GBI4/ofrt3IlouIRfvXOuUX+33sqgB1/h41opeseR\n\tPPFZR2i9CdhCwdmc+9NVPfOjYvDeTNCmS5XmWn41joAiUa4PPREBSIVew+WujJmW3n\n\tW0xifPEAgAKMsESePiFte6THekLlW9xTCIXoeC8uZhiFtZ8p/26gzTcMtq0/P3L/lS\n\tMK0pdTyvecEWA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660083151;\n\tbh=sFhqcVRStbYrjIoeEgLVDbF/1nR3QtmRZ2CPhz+o7qU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=g2rArDXFRK/fY5Vx+M5q2zbme3TnwAWPrpENvhjwod4IALq7QG9+yyHLi6EG9LiaG\n\twyl3Dpi4hl3wES+hSEM2kIhLum7LC2MXgmuK/6HkUwDfCr21yrKCz2AbdqKMhMAMwa\n\tBayLCVP/WZSsPZLbC5RPhC2CboMbnnYoDhaVWup4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"g2rArDXF\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220809205042.344923-8-utkarsh02t@gmail.com>","References":"<20220809205042.344923-1-utkarsh02t@gmail.com>\n\t<20220809205042.344923-8-utkarsh02t@gmail.com>","To":"Utkarsh Tiwari ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 09 Aug 2022 23:12:28 +0100","Message-ID":"<166008314844.2423137.2848268962276089078@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v7 7/8] qcam: CamSelectDialog: Display\n\tCapture script path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24499,"web_url":"https://patchwork.libcamera.org/comment/24499/","msgid":"","date":"2022-08-10T04:26:58","subject":"Re: [libcamera-devel] [PATCH v7 7/8] qcam: CamSelectDialog: Display\n\tCapture script path","submitter":{"id":114,"url":"https://patchwork.libcamera.org/api/people/114/","name":"Utkarsh Tiwari","email":"utkarsh02t@gmail.com"},"content":"Hi Kieran, thanks for the review.\n\nOn Wed, Aug 10, 2022 at 3:42 AM Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-09 21:50:41)\n> > Display the path of the selected capture script in a thinner font.\n> >\n> > Signed-off-by: Utkarsh Tiwari \n> > ---\n> > src/qcam/cam_select_dialog.cpp | 38 +++++++++++++++++++++++++++-------\n> > src/qcam/cam_select_dialog.h | 8 ++++++-\n> > src/qcam/main_window.cpp | 3 ++-\n> > 3 files changed, 40 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/src/qcam/cam_select_dialog.cpp\n> b/src/qcam/cam_select_dialog.cpp\n> > index f3df9970..5a9de08c 100644\n> > --- a/src/qcam/cam_select_dialog.cpp\n> > +++ b/src/qcam/cam_select_dialog.cpp\n> > @@ -20,8 +20,9 @@\n> > #include \n> >\n> > CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager\n> *cameraManager,\n> > - bool isScriptRunning, QWidget\n> *parent)\n> > - : QDialog(parent), cm_(cameraManager),\n> isScriptRunning_(isScriptRunning)\n> > + bool isScriptRunning,\n> std::string scriptPath, QWidget *parent)\n> > + : QDialog(parent), cm_(cameraManager),\n> > + isScriptRunning_(isScriptRunning), scriptPath_(scriptPath)\n> > {\n> > /* Use a QFormLayout for the dialog. */\n> > QFormLayout *layout = new QFormLayout(this);\n> > @@ -39,14 +40,31 @@\n> CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager\n> *cameraManag\n> > connect(cameraIdComboBox_, &QComboBox::currentTextChanged,\n> > this, &CameraSelectorDialog::handleCameraChange);\n> >\n> > + /* Setup widget for capture script button. */\n> > + QWidget *captureWidget = new QWidget;\n> > + captureWidgetLayout_ = new QVBoxLayout(captureWidget);\n> > + captureWidgetLayout_->setMargin(0);\n> > +\n> > captureScriptButton_ = new QPushButton;\n> > connect(captureScriptButton_, &QPushButton::clicked,\n> > this, &CameraSelectorDialog::handleCaptureScriptButton);\n> > + captureWidgetLayout_->addWidget(captureScriptButton_);\n> > +\n> > + /* Use a thinner font to indicate script info. */\n> > + QFont smallFont;\n> > + smallFont.setWeight(QFont::Thin);\n> > +\n> > + scriptPathLabel_ = new QLabel;\n>\n> Creating it again? That's another memory leak.\n>\n> Oops this should be the only one, the one below is an remnant of\nrebases.\n\n> It might be worth trying your patchs with valgrind or the memleak\n> detectors that can be enabled with meson.\n>\n\nYes, I would figure out how to run them. I have never used them before\nbut I don't think they would be able to find these leaks because these\nobjects *are* deleted when the window is closed as the parent of the\nthese QObjects is still the QDialog and when it dies it deletes its\nchildren.\n\n>\n> > + scriptPathLabel_->setFont(smallFont);\n> > + scriptPathLabel_->setWordWrap(true);\n> >\n> > /* Display the action that would be performed when button is\n> clicked. */\n> > - if (isScriptRunning_)\n> > + if (isScriptRunning_) {\n> > captureScriptButton_->setText(\"Stop\");\n> > - else\n> > +\n> > +\n> scriptPathLabel_->setText(QString::fromStdString(scriptPath_));\n> > + captureWidgetLayout_->addWidget(scriptPathLabel_);\n> > + } else\n> > captureScriptButton_->setText(\"Open\");\n> >\n> > /* Setup the QDialogButton Box */\n> > @@ -63,7 +81,7 @@\n> CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager\n> *cameraManag\n> > layout->addRow(\"Camera:\", cameraIdComboBox_);\n> > layout->addRow(\"Location:\", cameraLocation_);\n> > layout->addRow(\"Model:\", cameraModel_);\n> > - layout->addRow(\"Capture Script:\", captureScriptButton_);\n> > + layout->addRow(\"Capture Script:\", captureWidget);\n> > layout->addWidget(buttonBox);\n> > }\n> >\n> > @@ -138,16 +156,22 @@ void\n> CameraSelectorDialog::handleCaptureScriptButton()\n> > Q_EMIT stopCaptureScript();\n> > isScriptRunning_ = false;\n> > captureScriptButton_->setText(\"Open\");\n> > +\n> > + captureWidgetLayout_->removeWidget(scriptPathLabel_);\n> > } else {\n> > selectedScriptPath_ = QFileDialog::getOpenFileName(this,\n> > \"Run\n> Capture Script\", QDir::currentPath(),\n> >\n> \"Capture Script (*.yaml)\")\n> > .toStdString();\n> >\n> > - if (!selectedScriptPath_.empty())\n> > + if (!selectedScriptPath_.empty()) {\n> > captureScriptButton_->setText(\"Loaded\");\n> > - else\n> > +\n> scriptPathLabel_->setText(QString::fromStdString(selectedScriptPath_));\n> > +\n> captureWidgetLayout_->addWidget(scriptPathLabel_);\n> > + } else {\n> > captureScriptButton_->setText(\"Open\");\n> > +\n> captureWidgetLayout_->removeWidget(scriptPathLabel_);\n> > + }\n> > }\n> > }\n> >\n> > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> > index 56d90596..84904640 100644\n> > --- a/src/qcam/cam_select_dialog.h\n> > +++ b/src/qcam/cam_select_dialog.h\n> > @@ -18,17 +18,20 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n>\n> If this is only used in the .cpp file, include it there. I don't think\n> it's needed in the header.\n>\nAgreed.\n\n>\n> > #include \n> > #include \n> > #include \n> > #include \n> > +#include \n> > +#include \n> >\n> > class CameraSelectorDialog : public QDialog\n> > {\n> > Q_OBJECT\n> > public:\n> > CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> > - bool isScriptRunning, QWidget *parent);\n> > + bool isScriptRunning, std::string\n> scriptPath, QWidget *parent);\n> >\n> > ~CameraSelectorDialog() = default;\n> >\n> > @@ -66,5 +69,8 @@ private:\n> > QComboBox *cameraIdComboBox_;\n> > QLabel *cameraLocation_;\n> > QLabel *cameraModel_;\n> > +\n> > + QVBoxLayout *captureWidgetLayout_;\n> > QPushButton *captureScriptButton_;\n> > + QLabel *scriptPathLabel_ = new QLabel;\n>\n>\n> This doesn't match how everthing else is initialised, and I also think\n> this could simply be a local instance, not a pointer:\n> QLabel scriptPath_;\n>\n> Ah yes. this should just be QLabel *scriptPath_ = new QLabel;\nI still think we should use pointers and allocate them on the heap.\nIf we go the stack based approach we would have to think in two phases\n1. The GUI layout\n2. The stack layout (Such that parent doesn't die before child)\nCurrently we can limit the member variables to only the important ones\nFor e.g captureWidget the important thing here is its layout and the\nactual widget's symbol just lives locally in the constructor.\n\nThe QObject trees do take care of deleting everything if parents are\nset right. And at all places (even in this buggy recreation ) whenever we\nare creating them and adding them to its layout, the object deletion\ngets handled.\n\n>\n> > };\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index d73fb42a..f2e3c576 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -337,7 +337,8 @@ std::string MainWindow::chooseCamera()\n> > bool scriptRunning = script_ != nullptr;\n> >\n> > /* Construct the selection dialog, unconditionally. */\n> > - cameraSelectorDialog_ = new CameraSelectorDialog(cm_,\n> scriptRunning, this);\n> > + cameraSelectorDialog_ = new CameraSelectorDialog(cm_,\n> scriptRunning,\n> > + scriptPath_,\n> this);\n> >\n> > connect(cameraSelectorDialog_,\n> &CameraSelectorDialog::stopCaptureScript,\n> > this, &MainWindow::stopCaptureScript);\n> > --\n> > 2.25.1\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 17AFFBE173\n\tfor ;\n\tWed, 10 Aug 2022 04:27:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7826563326;\n\tWed, 10 Aug 2022 06:27:15 +0200 (CEST)","from mail-pj1-x1032.google.com (mail-pj1-x1032.google.com\n\t[IPv6:2607:f8b0:4864:20::1032])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BFF4B61FA9\n\tfor ;\n\tWed, 10 Aug 2022 06:27:13 +0200 (CEST)","by mail-pj1-x1032.google.com with SMTP id b4so13605472pji.4\n\tfor ;\n\tTue, 09 Aug 2022 21:27:13 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660105635;\n\tbh=rH+0tFDrrhhubkEAjoTRAuKLy0ApmXpSRR5OdXFTAR4=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=cYAIi1/RYBnudviPtFgrSV/JJqFFroT6+ZFiHViCosNkJEHxVS/2pGo50CuQ8q7Jl\n\tvKBqtzXQo9wHi6nXX1zxAr7Gr5kxVGmI5ymvClyPAwvO7pGd1GYVFAaUO5M30fPa03\n\tD43E6izZsSwbe8fLb4buawshxBMpyMfSnrqfW7eNPwCMOjVvuYuZLYH/hFJfGbKlwu\n\tNIuWLcSni4BUMOnn/38LXBjdfOrf5SeOK8BqPC2DN+83lbZMV8cG5mpzQKGTo1knAD\n\t83hOwIA8a0bGP9Nq3+Kypqb/FMaUhunqBtefRybkbbx82GxILJqZ5hhqtmlsg28MYC\n\tZkeg/CtBWlvog==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc;\n\tbh=0qnTNq9RzvDsJO9TzOT3ENZ//Sgx37vlrTRi41XKHlE=;\n\tb=iyPL1oS36X+55vm8VLWPUdfVyaATL4H9qmiiHbbvQS5pXuW/D2JOllGqJO/S217stg\n\tPHItJ1fXeAvi/7QZ9iKnp6CUmBuKb2pLS8viqDHEFQu2qtrLEThWLLGyfb1F8RNLKbBr\n\tZbtRg4cuOkPblA3ISPkAKeanP443KjCp67XAvANScE/Du6nIiv7oZYOhsFXOn93u4N6U\n\t+jqj/y5Sku2lPZDU9+wrbSq+hsePZ8IMYi8lGmDaF+lQKqIEJ67or3lo99kWEjDjyjPT\n\tP0KJweIOhJnb+74tVVzV88SR4jvB1vu34D0kesWZ1oCFiu4/RfaqlgVrkwT3AdVzbSRz\n\t5Lng=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"iyPL1oS3\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc;\n\tbh=0qnTNq9RzvDsJO9TzOT3ENZ//Sgx37vlrTRi41XKHlE=;\n\tb=1zO8QVmHPNmbcymDYc5EJPFwo+1b0iQjmuPq2Z7SfqmA1IKOafVsb0EYoYJShrLwCI\n\tK+4D7JqJGT4s2DJ+GGWkM0NJyy3D2Yo9Ss9LATenOhamIAQZjr1Vf+hUZBapcWg44tLa\n\t4oNokp6311DxYjnSYAYNdcAY6egLq2CYKAnWMe5JPOLSB5QG9nAIPsQfDqnFsUVRB6Xw\n\tZix5ySFoeLsSmvK9sszxP8z3qnnuxKxIlllmxsvtlE5EPnABeq3Px23mQhCgIBDnLGI3\n\tRacQOywjIsdNzH6H8diYttK93HnECu1qcShrtpG2cg6fq2kOrEDnI+QLr+GfgPpgj4jM\n\tja5A==","X-Gm-Message-State":"ACgBeo2x7KSTGmMfNq2MtujDRSMxW3Q1shAT+sP9wNx57d7OIId32U4N\n\tOJiYRpQTGf4ExgmPFcnMbGr+gRJO9kgRF3Wu8gA=","X-Google-Smtp-Source":"AA6agR6oGSpNLcGotnetN2vMkP3Mju18RaG9VOV7/lbMc4oanoZqnXyIOtNbVzhFpwBvxIswkRUyZxtspPJ/mrY0TLs=","X-Received":"by 2002:a17:902:70c7:b0:170:9030:2665 with SMTP id\n\tl7-20020a17090270c700b0017090302665mr17118203plt.73.1660105632063;\n\tTue, 09 Aug 2022 21:27:12 -0700 (PDT)","MIME-Version":"1.0","References":"<20220809205042.344923-1-utkarsh02t@gmail.com>\n\t<20220809205042.344923-8-utkarsh02t@gmail.com>\n\t<166008314844.2423137.2848268962276089078@Monstersaurus>","In-Reply-To":"<166008314844.2423137.2848268962276089078@Monstersaurus>","Date":"Wed, 10 Aug 2022 09:56:58 +0530","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"0000000000007d262805e5db7690\"","Subject":"Re: [libcamera-devel] [PATCH v7 7/8] qcam: CamSelectDialog: Display\n\tCapture script path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Utkarsh Tiwari via libcamera-devel\n\t","Reply-To":"Utkarsh Tiwari ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24519,"web_url":"https://patchwork.libcamera.org/comment/24519/","msgid":"<166013469337.15821.8184643417692810964@Monstersaurus>","date":"2022-08-10T12:31:33","subject":"Re: [libcamera-devel] [PATCH v7 7/8] qcam: CamSelectDialog: Display\n\tCapture script path","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Utkarsh Tiwari (2022-08-10 05:26:58)\n> Hi Kieran, thanks for the review.\n> \n> On Wed, Aug 10, 2022 at 3:42 AM Kieran Bingham <\n> kieran.bingham@ideasonboard.com> wrote:\n> \n> > Quoting Utkarsh Tiwari via libcamera-devel (2022-08-09 21:50:41)\n> > > Display the path of the selected capture script in a thinner font.\n> > >\n> > > Signed-off-by: Utkarsh Tiwari \n> > > ---\n> > > src/qcam/cam_select_dialog.cpp | 38 +++++++++++++++++++++++++++-------\n> > > src/qcam/cam_select_dialog.h | 8 ++++++-\n> > > src/qcam/main_window.cpp | 3 ++-\n> > > 3 files changed, 40 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/src/qcam/cam_select_dialog.cpp\n> > b/src/qcam/cam_select_dialog.cpp\n> > > index f3df9970..5a9de08c 100644\n> > > --- a/src/qcam/cam_select_dialog.cpp\n> > > +++ b/src/qcam/cam_select_dialog.cpp\n> > > @@ -20,8 +20,9 @@\n> > > #include \n> > >\n> > > CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager\n> > *cameraManager,\n> > > - bool isScriptRunning, QWidget\n> > *parent)\n> > > - : QDialog(parent), cm_(cameraManager),\n> > isScriptRunning_(isScriptRunning)\n> > > + bool isScriptRunning,\n> > std::string scriptPath, QWidget *parent)\n> > > + : QDialog(parent), cm_(cameraManager),\n> > > + isScriptRunning_(isScriptRunning), scriptPath_(scriptPath)\n> > > {\n> > > /* Use a QFormLayout for the dialog. */\n> > > QFormLayout *layout = new QFormLayout(this);\n> > > @@ -39,14 +40,31 @@\n> > CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager\n> > *cameraManag\n> > > connect(cameraIdComboBox_, &QComboBox::currentTextChanged,\n> > > this, &CameraSelectorDialog::handleCameraChange);\n> > >\n> > > + /* Setup widget for capture script button. */\n> > > + QWidget *captureWidget = new QWidget;\n> > > + captureWidgetLayout_ = new QVBoxLayout(captureWidget);\n> > > + captureWidgetLayout_->setMargin(0);\n> > > +\n> > > captureScriptButton_ = new QPushButton;\n> > > connect(captureScriptButton_, &QPushButton::clicked,\n> > > this, &CameraSelectorDialog::handleCaptureScriptButton);\n> > > + captureWidgetLayout_->addWidget(captureScriptButton_);\n> > > +\n> > > + /* Use a thinner font to indicate script info. */\n> > > + QFont smallFont;\n> > > + smallFont.setWeight(QFont::Thin);\n> > > +\n> > > + scriptPathLabel_ = new QLabel;\n> >\n> > Creating it again? That's another memory leak.\n> >\n> > Oops this should be the only one, the one below is an remnant of\n> rebases.\n> \n> > It might be worth trying your patchs with valgrind or the memleak\n> > detectors that can be enabled with meson.\n> >\n> \n> Yes, I would figure out how to run them. I have never used them before\n> but I don't think they would be able to find these leaks because these\n> objects *are* deleted when the window is closed as the parent of the\n> these QObjects is still the QDialog and when it dies it deletes its\n> children.\n> \n> >\n> > > + scriptPathLabel_->setFont(smallFont);\n> > > + scriptPathLabel_->setWordWrap(true);\n> > >\n> > > /* Display the action that would be performed when button is\n> > clicked. */\n> > > - if (isScriptRunning_)\n> > > + if (isScriptRunning_) {\n> > > captureScriptButton_->setText(\"Stop\");\n> > > - else\n> > > +\n> > > +\n> > scriptPathLabel_->setText(QString::fromStdString(scriptPath_));\n> > > + captureWidgetLayout_->addWidget(scriptPathLabel_);\n> > > + } else\n> > > captureScriptButton_->setText(\"Open\");\n> > >\n> > > /* Setup the QDialogButton Box */\n> > > @@ -63,7 +81,7 @@\n> > CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager\n> > *cameraManag\n> > > layout->addRow(\"Camera:\", cameraIdComboBox_);\n> > > layout->addRow(\"Location:\", cameraLocation_);\n> > > layout->addRow(\"Model:\", cameraModel_);\n> > > - layout->addRow(\"Capture Script:\", captureScriptButton_);\n> > > + layout->addRow(\"Capture Script:\", captureWidget);\n> > > layout->addWidget(buttonBox);\n> > > }\n> > >\n> > > @@ -138,16 +156,22 @@ void\n> > CameraSelectorDialog::handleCaptureScriptButton()\n> > > Q_EMIT stopCaptureScript();\n> > > isScriptRunning_ = false;\n> > > captureScriptButton_->setText(\"Open\");\n> > > +\n> > > + captureWidgetLayout_->removeWidget(scriptPathLabel_);\n> > > } else {\n> > > selectedScriptPath_ = QFileDialog::getOpenFileName(this,\n> > > \"Run\n> > Capture Script\", QDir::currentPath(),\n> > >\n> > \"Capture Script (*.yaml)\")\n> > > .toStdString();\n> > >\n> > > - if (!selectedScriptPath_.empty())\n> > > + if (!selectedScriptPath_.empty()) {\n> > > captureScriptButton_->setText(\"Loaded\");\n> > > - else\n> > > +\n> > scriptPathLabel_->setText(QString::fromStdString(selectedScriptPath_));\n> > > +\n> > captureWidgetLayout_->addWidget(scriptPathLabel_);\n> > > + } else {\n> > > captureScriptButton_->setText(\"Open\");\n> > > +\n> > captureWidgetLayout_->removeWidget(scriptPathLabel_);\n> > > + }\n> > > }\n> > > }\n> > >\n> > > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> > > index 56d90596..84904640 100644\n> > > --- a/src/qcam/cam_select_dialog.h\n> > > +++ b/src/qcam/cam_select_dialog.h\n> > > @@ -18,17 +18,20 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> >\n> > If this is only used in the .cpp file, include it there. I don't think\n> > it's needed in the header.\n> >\n> Agreed.\n> \n> >\n> > > #include \n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > +#include \n> > >\n> > > class CameraSelectorDialog : public QDialog\n> > > {\n> > > Q_OBJECT\n> > > public:\n> > > CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> > > - bool isScriptRunning, QWidget *parent);\n> > > + bool isScriptRunning, std::string\n> > scriptPath, QWidget *parent);\n> > >\n> > > ~CameraSelectorDialog() = default;\n> > >\n> > > @@ -66,5 +69,8 @@ private:\n> > > QComboBox *cameraIdComboBox_;\n> > > QLabel *cameraLocation_;\n> > > QLabel *cameraModel_;\n> > > +\n> > > + QVBoxLayout *captureWidgetLayout_;\n> > > QPushButton *captureScriptButton_;\n> > > + QLabel *scriptPathLabel_ = new QLabel;\n> >\n> >\n> > This doesn't match how everthing else is initialised, and I also think\n> > this could simply be a local instance, not a pointer:\n> > QLabel scriptPath_;\n> >\n> > Ah yes. this should just be QLabel *scriptPath_ = new QLabel;\n> I still think we should use pointers and allocate them on the heap.\n> If we go the stack based approach we would have to think in two phases\n> 1. The GUI layout\n> 2. The stack layout (Such that parent doesn't die before child)\n\nThis isn't about stack vs heap - if the QLabel is a direct member of\nCameraSelectorDialog, then when it is allocated, so too will the QLabel.\nIt becomes 'part' of the CameraSelectorDialog, so it will be allocated\nat the same time (as part of the same allocation).\n\n\n> Currently we can limit the member variables to only the important ones\n> For e.g captureWidget the important thing here is its layout and the\n> actual widget's symbol just lives locally in the constructor.\n> \n> The QObject trees do take care of deleting everything if parents are\n> set right. And at all places (even in this buggy recreation ) whenever we\n> are creating them and adding them to its layout, the object deletion\n> gets handled.\n\nOk - I don't know enough about how QT is handling pointer ownership\nhere, so as long as it works and something is cleaning up correctly.\n\n> \n> >\n> > > };\n> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > > index d73fb42a..f2e3c576 100644\n> > > --- a/src/qcam/main_window.cpp\n> > > +++ b/src/qcam/main_window.cpp\n> > > @@ -337,7 +337,8 @@ std::string MainWindow::chooseCamera()\n> > > bool scriptRunning = script_ != nullptr;\n> > >\n> > > /* Construct the selection dialog, unconditionally. */\n> > > - cameraSelectorDialog_ = new CameraSelectorDialog(cm_,\n> > scriptRunning, this);\n> > > + cameraSelectorDialog_ = new CameraSelectorDialog(cm_,\n> > scriptRunning,\n> > > + scriptPath_,\n> > this);\n> > >\n> > > connect(cameraSelectorDialog_,\n> > &CameraSelectorDialog::stopCaptureScript,\n> > > this, &MainWindow::stopCaptureScript);\n> > > --\n> > > 2.25.1\n> > >\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 846C8BE173\n\tfor ;\n\tWed, 10 Aug 2022 12:31:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D37C763328;\n\tWed, 10 Aug 2022 14:31:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3148D600EA\n\tfor ;\n\tWed, 10 Aug 2022 14:31:36 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B0A533F1;\n\tWed, 10 Aug 2022 14:31:35 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660134697;\n\tbh=vCipS7PRJaf4v5alj4WGB/lp6yOYCDpE3Ab0ZXgjH8w=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=LFHbEgB5QGeb+baaGGvBMcLuABw/rroO51knDFUU3/lUhg9z1p3T0KppX45kWTwPZ\n\t3VfcaQBVbvpOlt7DhRePqbYCDfCB/nm+U3QoEoGEPydqb7UsvVYeBQVqiLvDqv8x+R\n\tA14sverrZ3mUWko9yIJTXYynA0+rA+zABuQJn2C6bpNAZZu0wgq/d3WDSjxkzpVfML\n\tKvf58MfE328RPExogzgbvbWL/tFE6GEQrmSNzcTSNb5tiRPXFiUBniTcCleP4GLz1Z\n\tJUiTiGzHiF95/JQKaXdVhdcZ+uaw2XnZqLcYDKItmoDureLEW8BGORRNcdePMEONVJ\n\tv4+aPko6N2wBw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660134695;\n\tbh=vCipS7PRJaf4v5alj4WGB/lp6yOYCDpE3Ab0ZXgjH8w=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ZzKgx36j/IohFv0xNzJgKYV0Jn5aS0ETc/vXGVyW1mV7RHyEhkPWRfpu2ixc50Zmo\n\tNDHFQ20ZWK+TTY7IJ2zWHZEwAK9rdoSJFeIILuqNy+Xe8xCrIUVCSzo0vMTcdZBrR7\n\td4G3A1D0udVh5wn4smeUHbRM9ZUk/6s8Ff5IKaI4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ZzKgx36j\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220809205042.344923-1-utkarsh02t@gmail.com>\n\t<20220809205042.344923-8-utkarsh02t@gmail.com>\n\t<166008314844.2423137.2848268962276089078@Monstersaurus>\n\t","To":"Utkarsh Tiwari ","Date":"Wed, 10 Aug 2022 13:31:33 +0100","Message-ID":"<166013469337.15821.8184643417692810964@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v7 7/8] qcam: CamSelectDialog: Display\n\tCapture script path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]