[{"id":24403,"web_url":"https://patchwork.libcamera.org/comment/24403/","msgid":"<165991437612.1706285.3866162190018711245@Monstersaurus>","date":"2022-08-07T23:19:36","subject":"Re: [libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for\n\tselection of cameras at startup","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-06 20:04:30)\n> Currently we use QInputDialog convenience dialogs to allow the user to\n> select a camera. This doesn't allow adding of more information (such as\n> camera location, model etc).\n> \n> Create a QDialog with a QFormLayout that shows a QComboBox with camera\n> Ids. Use a QDialogButtonBox to provide buttons for accepting and\n> cancelling the action.\n> \n> Signed-off-by: Utkarsh Tiwari \n> ---\n> Difference from V1:\n> The biggest change here is the introduction of class, inside of\n> the class the layout remains same.\n> src/qcam/cam_select_dialog.h | 64 ++++++++++++++++++++++++++++++++++++\n> src/qcam/main_window.cpp | 22 +++----------\n> src/qcam/main_window.h | 4 +++\n> src/qcam/meson.build | 1 +\n> 4 files changed, 74 insertions(+), 17 deletions(-)\n> create mode 100644 src/qcam/cam_select_dialog.h\n> \n> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> new file mode 100644\n> index 00000000..c23bad59\n> --- /dev/null\n> +++ b/src/qcam/cam_select_dialog.h\n> @@ -0,0 +1,64 @@\n> +\n\nRemove the first blank line please.\n\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n\nI think this should be 2022. I'm not sure if copyright goes to you or\nGoogle for GSoC.\n\n\n> + *\n> + * cam_select_dialog.h - qcam - Camera Selection dialog\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +class CamSelectDialog : public QDialog\n\nIt's bikeshedding territory - but CameraSelectDialog sounds better to me\nhere. (Spell 'Camera' in full).\n\nIt seems Laurent suggested CameraSelectorDialog too.\n\n\n> +{\n> + Q_OBJECT\n> +public:\n> + CamSelectDialog(libcamera::CameraManager *cameraManager, QWidget *parent)\n> + : QDialog(parent), cm_(cameraManager)\n\nIs cm_ used later? I don't mind it being added here if it's needed\nlater, but it only seems to be used in this construction for the moment.\n\n\n> + {\n> + /* Use a QFormLayout for the dialog. */\n> + QFormLayout *camSelectDialogLayout = new QFormLayout(this);\n> +\n> + /* Setup the camera id combo-box. */\n> + cameraIdComboBox_ = new QComboBox;\n> + for (const auto &cam : cm_->cameras())\n> + cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));\n\nI could imagine we need to add hotplug events in here too.\nIt could be done on top, but to test it - try removing or adding a\ncamera while the dialog box is opened.\n\nAs long as it doesn't crash if the camera is removed while the dialog\nbox is open, I don't mind if the hotplug (adding cameras when they are\nplugged in, removing when removed) is added on top. Especially as the\nexisting dialog didn't support hotplug either.\n\n\nI guess if there's no camera left, the dialog box doesn't have much to\ndo ;-) (though should stay open in case someone replugs the UVC camera)\n\n\n> +\n> + /* Setup the QDialogButton Box */\n> + QDialogButtonBox *dialogButtonBox =\n> + new QDialogButtonBox(QDialogButtonBox::Ok |\n> + QDialogButtonBox::Cancel);\n> +\n> + connect(dialogButtonBox, &QDialogButtonBox::accepted,\n> + this, &QDialog::accept);\n> + connect(dialogButtonBox, &QDialogButtonBox::rejected,\n> + this, &QDialog::reject);\n> +\n> + /* Set the layout. */\n> + camSelectDialogLayout->addRow(\"Camera: \", cameraIdComboBox_);\n> + camSelectDialogLayout->addWidget(dialogButtonBox);\n> + }\n> +\n> + ~CamSelectDialog() = default;\n> +\n> + std::string getCameraId()\n> + {\n> + return cameraIdComboBox_->currentText().toStdString();\n> + }\n\nWhile there's not a lot of implementation here, I think the code should\nbe in a .cpp file. I'm not sure where the threshold is for how small a\nclass can be to be inlined directly in the header, but I think this one\ncan potentially expand, so probably deserves it's own implementation\nfile.\n\n> +\n> +private:\n> + libcamera::CameraManager *cm_;\n> +\n> + /* UI elements. */\n> + QComboBox *cameraIdComboBox_;\n> +};\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 7433d647..758e2c94 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -19,7 +19,6 @@\n> #include \n> #include \n> #include \n> -#include \n> #include \n> #include \n> #include \n> @@ -30,6 +29,7 @@\n> \n> #include \"../cam/image.h\"\n> \n> +#include \"cam_select_dialog.h\"\n> #include \"dng_writer.h\"\n> #ifndef QT_NO_OPENGL\n> #include \"viewfinder_gl.h\"\n> @@ -290,24 +290,12 @@ void MainWindow::switchCamera(int index)\n> \n> std::string MainWindow::chooseCamera()\n> {\n> - QStringList cameras;\n> - bool result;\n> + camSelectDialog_ = new CamSelectDialog(cm_, this);\n\nDo we need to keep the dialog in a class variable? or could it be local\nand destroyed at the end of this function?\n\n> \n> - /* If only one camera is available, use it automatically. */\n> - if (cm_->cameras().size() == 1)\n> - return cm_->cameras()[0]->id();\n> -\n> - /* Present a dialog box to pick a camera. */\n> - for (const std::shared_ptr &cam : cm_->cameras())\n> - cameras.append(QString::fromStdString(cam->id()));\n> -\n> - QString id = QInputDialog::getItem(this, \"Select Camera\",\n> - \"Camera:\", cameras, 0,\n> - false, &result);\n> - if (!result)\n> + if (camSelectDialog_->exec() == QDialog::Accepted)\n> + return camSelectDialog_->getCameraId();\n> + else\n> return std::string();\n> -\n> - return id.toStdString();\n> }\n> \n> int MainWindow::openCamera()\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index fc70920f..6d80b5be 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -23,11 +23,13 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> #include \"../cam/stream_options.h\"\n> \n> +#include \"cam_select_dialog.h\"\n> #include \"viewfinder.h\"\n> \n> class QAction;\n> @@ -99,6 +101,8 @@ private:\n> QString title_;\n> QTimer titleTimer_;\n> \n> + QPointer camSelectDialog_;\n> +\n> /* Options */\n> const OptionsParser::Options &options_;\n> \n> diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> index c46f4631..70615e92 100644\n> --- a/src/qcam/meson.build\n> +++ b/src/qcam/meson.build\n> @@ -26,6 +26,7 @@ qcam_sources = files([\n> ])\n> \n> qcam_moc_headers = files([\n> + 'cam_select_dialog.h',\n> 'main_window.h',\n> 'viewfinder_qt.h',\n> ])\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 62344C3272\n\tfor ;\n\tSun, 7 Aug 2022 23:19:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3FDF36332B;\n\tMon, 8 Aug 2022 01:19:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE8D3603EA\n\tfor ;\n\tMon, 8 Aug 2022 01:19:38 +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 3F8F056D;\n\tMon, 8 Aug 2022 01:19:38 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659914380;\n\tbh=89+4iELbwdgLKrA77yt0woZ1pEpDIQpjnbaOU47OTJk=;\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=DOgDTfvF3NzLBwCKwaAZaW16qS20fQVKqfe3/U4aUJCKCk5q4bmcdVT4XWhQf6Zrp\n\t8TTtDSV7pNEk8cpCP/vhUBfMI6QyZqO4F4+8+mrU2HdZpRc0EYj2QrhMImFTR10h1z\n\tvKJ7ox9A7srUggBdjvTCSvlKUPIJ9BfCIVQzrtGLuaJF4ZX++YFZ3KG7SdLg9hTOlS\n\tRrOWz/MqPQ4Eaurl5q55t78NbkiRlFbU+PLafS047x4aT2J/DzSMHHi2CfNXXYZ7U1\n\tmfYeHop3cqjs2H8+bdzJDHQeqnDFK4KAh+y8tUnn+bJVr2qv21OOWLyCOhkVtdu8jV\n\thAitdq3WclPbQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659914378;\n\tbh=89+4iELbwdgLKrA77yt0woZ1pEpDIQpjnbaOU47OTJk=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=PykK5bD3CykTHO5tLfd2Gdt4jVQHIAfHNE2GKSvMOoy/5cmjIuSwFnbhQLzzWaTEj\n\tN8JCnMehk9XvrkNTEW24R1j+PAAysbIAZ7iQ1ZX1pzizqTD9ZU9PYyyXG7iI8ZduR2\n\th5mMFitgT0yHqiE2wTLdVU9V3OWxDyECmVnXCKiU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"PykK5bD3\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220806190433.59128-2-utkarsh02t@gmail.com>","References":"<20220806190433.59128-1-utkarsh02t@gmail.com>\n\t<20220806190433.59128-2-utkarsh02t@gmail.com>","To":"Utkarsh Tiwari ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 08 Aug 2022 00:19:36 +0100","Message-ID":"<165991437612.1706285.3866162190018711245@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for\n\tselection of cameras at startup","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":24404,"web_url":"https://patchwork.libcamera.org/comment/24404/","msgid":"<165991445896.1706285.5549292569645190696@Monstersaurus>","date":"2022-08-07T23:20:58","subject":"Re: [libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for\n\tselection of cameras at startup","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2022-08-08 00:19:36)\n> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-06 20:04:30)\n> > Currently we use QInputDialog convenience dialogs to allow the user to\n> > select a camera. This doesn't allow adding of more information (such as\n> > camera location, model etc).\n> > \n> > Create a QDialog with a QFormLayout that shows a QComboBox with camera\n> > Ids. Use a QDialogButtonBox to provide buttons for accepting and\n> > cancelling the action.\n> > \n> > Signed-off-by: Utkarsh Tiwari \n> > ---\n> > Difference from V1:\n> > The biggest change here is the introduction of class, inside of\n> > the class the layout remains same.\n> > src/qcam/cam_select_dialog.h | 64 ++++++++++++++++++++++++++++++++++++\n> > src/qcam/main_window.cpp | 22 +++----------\n> > src/qcam/main_window.h | 4 +++\n> > src/qcam/meson.build | 1 +\n> > 4 files changed, 74 insertions(+), 17 deletions(-)\n> > create mode 100644 src/qcam/cam_select_dialog.h\n> > \n> > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> > new file mode 100644\n> > index 00000000..c23bad59\n> > --- /dev/null\n> > +++ b/src/qcam/cam_select_dialog.h\n> > @@ -0,0 +1,64 @@\n> > +\n> \n> Remove the first blank line please.\n> \n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> \n> I think this should be 2022. I'm not sure if copyright goes to you or\n> Google for GSoC.\n> \n> \n> > + *\n> > + * cam_select_dialog.h - qcam - Camera Selection dialog\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +class CamSelectDialog : public QDialog\n> \n> It's bikeshedding territory - but CameraSelectDialog sounds better to me\n> here. (Spell 'Camera' in full).\n> \n> It seems Laurent suggested CameraSelectorDialog too.\n> \n> \n> > +{\n> > + Q_OBJECT\n> > +public:\n> > + CamSelectDialog(libcamera::CameraManager *cameraManager, QWidget *parent)\n> > + : QDialog(parent), cm_(cameraManager)\n> \n> Is cm_ used later? I don't mind it being added here if it's needed\n> later, but it only seems to be used in this construction for the moment.\n> \n> \n> > + {\n> > + /* Use a QFormLayout for the dialog. */\n> > + QFormLayout *camSelectDialogLayout = new QFormLayout(this);\n> > +\n> > + /* Setup the camera id combo-box. */\n> > + cameraIdComboBox_ = new QComboBox;\n> > + for (const auto &cam : cm_->cameras())\n> > + cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));\n> \n> I could imagine we need to add hotplug events in here too.\n> It could be done on top, but to test it - try removing or adding a\n> camera while the dialog box is opened.\n> \n> As long as it doesn't crash if the camera is removed while the dialog\n> box is open, I don't mind if the hotplug (adding cameras when they are\n> plugged in, removing when removed) is added on top. Especially as the\n> existing dialog didn't support hotplug either.\n> \n> \n> I guess if there's no camera left, the dialog box doesn't have much to\n> do ;-) (though should stay open in case someone replugs the UVC camera)\n> \n> \n> > +\n> > + /* Setup the QDialogButton Box */\n> > + QDialogButtonBox *dialogButtonBox =\n> > + new QDialogButtonBox(QDialogButtonBox::Ok |\n> > + QDialogButtonBox::Cancel);\n> > +\n> > + connect(dialogButtonBox, &QDialogButtonBox::accepted,\n> > + this, &QDialog::accept);\n> > + connect(dialogButtonBox, &QDialogButtonBox::rejected,\n> > + this, &QDialog::reject);\n> > +\n> > + /* Set the layout. */\n> > + camSelectDialogLayout->addRow(\"Camera: \", cameraIdComboBox_);\n> > + camSelectDialogLayout->addWidget(dialogButtonBox);\n> > + }\n> > +\n> > + ~CamSelectDialog() = default;\n> > +\n> > + std::string getCameraId()\n> > + {\n> > + return cameraIdComboBox_->currentText().toStdString();\n> > + }\n> \n> While there's not a lot of implementation here, I think the code should\n> be in a .cpp file. I'm not sure where the threshold is for how small a\n> class can be to be inlined directly in the header, but I think this one\n> can potentially expand, so probably deserves it's own implementation\n> file.\n> \n> > +\n> > +private:\n> > + libcamera::CameraManager *cm_;\n> > +\n> > + /* UI elements. */\n> > + QComboBox *cameraIdComboBox_;\n> > +};\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index 7433d647..758e2c94 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -19,7 +19,6 @@\n> > #include \n> > #include \n> > #include \n> > -#include \n> > #include \n> > #include \n> > #include \n> > @@ -30,6 +29,7 @@\n> > \n> > #include \"../cam/image.h\"\n> > \n> > +#include \"cam_select_dialog.h\"\n> > #include \"dng_writer.h\"\n> > #ifndef QT_NO_OPENGL\n> > #include \"viewfinder_gl.h\"\n> > @@ -290,24 +290,12 @@ void MainWindow::switchCamera(int index)\n> > \n> > std::string MainWindow::chooseCamera()\n> > {\n> > - QStringList cameras;\n> > - bool result;\n> > + camSelectDialog_ = new CamSelectDialog(cm_, this);\n> \n> Do we need to keep the dialog in a class variable? or could it be local\n> and destroyed at the end of this function?\n\nAha, the next patch answers most of my questions I think ... hotplug and\nkeeping this accessible.\n\nI'll check through that tomorrow.\n\n\n> \n> > \n> > - /* If only one camera is available, use it automatically. */\n> > - if (cm_->cameras().size() == 1)\n> > - return cm_->cameras()[0]->id();\n> > -\n> > - /* Present a dialog box to pick a camera. */\n> > - for (const std::shared_ptr &cam : cm_->cameras())\n> > - cameras.append(QString::fromStdString(cam->id()));\n> > -\n> > - QString id = QInputDialog::getItem(this, \"Select Camera\",\n> > - \"Camera:\", cameras, 0,\n> > - false, &result);\n> > - if (!result)\n> > + if (camSelectDialog_->exec() == QDialog::Accepted)\n> > + return camSelectDialog_->getCameraId();\n> > + else\n> > return std::string();\n> > -\n> > - return id.toStdString();\n> > }\n> > \n> > int MainWindow::openCamera()\n> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > index fc70920f..6d80b5be 100644\n> > --- a/src/qcam/main_window.h\n> > +++ b/src/qcam/main_window.h\n> > @@ -23,11 +23,13 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > \n> > #include \"../cam/stream_options.h\"\n> > \n> > +#include \"cam_select_dialog.h\"\n> > #include \"viewfinder.h\"\n> > \n> > class QAction;\n> > @@ -99,6 +101,8 @@ private:\n> > QString title_;\n> > QTimer titleTimer_;\n> > \n> > + QPointer camSelectDialog_;\n> > +\n> > /* Options */\n> > const OptionsParser::Options &options_;\n> > \n> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > index c46f4631..70615e92 100644\n> > --- a/src/qcam/meson.build\n> > +++ b/src/qcam/meson.build\n> > @@ -26,6 +26,7 @@ qcam_sources = files([\n> > ])\n> > \n> > qcam_moc_headers = files([\n> > + 'cam_select_dialog.h',\n> > 'main_window.h',\n> > 'viewfinder_qt.h',\n> > ])\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 63E18C3272\n\tfor ;\n\tSun, 7 Aug 2022 23:21:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28CCF63327;\n\tMon, 8 Aug 2022 01:21:03 +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 09CC9603EA\n\tfor ;\n\tMon, 8 Aug 2022 01:21:02 +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 B475E56D;\n\tMon, 8 Aug 2022 01:21:01 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659914463;\n\tbh=ILAeEaMT9XQPWCYXsfzLWfskioE1VKLBlfBsBtKpe0Y=;\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=FuNjK6JQa8lkZTUsx7CsnTgWAXqWdWbaK4TnRQgDwppb+TQU6WgaefSQlhoRY9fAf\n\t6jAcubFTLd0Q8zoLP5oRgY0hXAgIjkOVdcR2KodxcHgeNo3sXsUaSTGSMrPaSyjkn+\n\tg1UKJ/R7/I5Rfczx/TY8KY/A65bFQrgWBqQIFeCE555XRofPHpX7zQLRFe+/NRXJ+v\n\tRY2vwdwqdoeNrIcUHBOdPZQkeXSy5uHIOUQPDmfkhqZX0U/yfLwYgDVjGz1x3Oq65N\n\t9hAb5RpM88EeaoYeV13OW5lmlmqS4C9zoyvGNKx7vhIVG9D58cZmRITvwNMuDVOeL1\n\tMj02peL5s9nZQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659914461;\n\tbh=ILAeEaMT9XQPWCYXsfzLWfskioE1VKLBlfBsBtKpe0Y=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=wXa5oexCt+wgSeOGa/JmfeIEo8eQJuV3SXOW0hTRCda4K9ns4d6VWn7MnluZQ36bE\n\t0wQLxhP5zOaLkium3oZe0r2OXjqiVXqglRUUPeKIBfW8a03SnwErNkabOsRb8cWETZ\n\tklGSGl1/FBpwqspUmpnxDQhB4QOAKVyZXvHObH/I="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wXa5oexC\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<165991437612.1706285.3866162190018711245@Monstersaurus>","References":"<20220806190433.59128-1-utkarsh02t@gmail.com>\n\t<20220806190433.59128-2-utkarsh02t@gmail.com>\n\t<165991437612.1706285.3866162190018711245@Monstersaurus>","To":"Utkarsh Tiwari ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 08 Aug 2022 00:20:58 +0100","Message-ID":"<165991445896.1706285.5549292569645190696@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for\n\tselection of cameras at startup","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":24406,"web_url":"https://patchwork.libcamera.org/comment/24406/","msgid":"","date":"2022-08-08T06:56:41","subject":"Re: [libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for\n\tselection of cameras at startup","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 Mon, 8 Aug, 2022, 04:49 Kieran Bingham, \nwrote:\n\n> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-06 20:04:30)\n> > Currently we use QInputDialog convenience dialogs to allow the user to\n> > select a camera. This doesn't allow adding of more information (such as\n> > camera location, model etc).\n> >\n> > Create a QDialog with a QFormLayout that shows a QComboBox with camera\n> > Ids. Use a QDialogButtonBox to provide buttons for accepting and\n> > cancelling the action.\n> >\n> > Signed-off-by: Utkarsh Tiwari \n> > ---\n> > Difference from V1:\n> > The biggest change here is the introduction of class, inside of\n> > the class the layout remains same.\n> > src/qcam/cam_select_dialog.h | 64 ++++++++++++++++++++++++++++++++++++\n> > src/qcam/main_window.cpp | 22 +++----------\n> > src/qcam/main_window.h | 4 +++\n> > src/qcam/meson.build | 1 +\n> > 4 files changed, 74 insertions(+), 17 deletions(-)\n> > create mode 100644 src/qcam/cam_select_dialog.h\n> >\n> > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> > new file mode 100644\n> > index 00000000..c23bad59\n> > --- /dev/null\n> > +++ b/src/qcam/cam_select_dialog.h\n> > @@ -0,0 +1,64 @@\n> > +\n>\n> Remove the first blank line please.\n>\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n>\n> I think this should be 2022. I'm not sure if copyright goes to you or\n> Google for GSoC.\n>\nOops copy paste error, It's my copyright.\n\n>\n>\n> > + *\n> > + * cam_select_dialog.h - qcam - Camera Selection dialog\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +class CamSelectDialog : public QDialog\n>\n> It's bikeshedding territory - but CameraSelectDialog sounds better to me\n> here. (Spell 'Camera' in full).\n>\n> It seems Laurent suggested CameraSelectorDialog too.\n>\n\nFine by me.\n\n>\n>\n> > +{\n> > + Q_OBJECT\n> > +public:\n> > + CamSelectDialog(libcamera::CameraManager *cameraManager, QWidget\n> *parent)\n> > + : QDialog(parent), cm_(cameraManager)\n>\n> Is cm_ used later? I don't mind it being added here if it's needed\n> later, but it only seems to be used in this construction for the moment.\n>\n>\nYes, to show the camera info I need to use cm_ to get the camera.\n\n>\n> > + {\n> > + /* Use a QFormLayout for the dialog. */\n> > + QFormLayout *camSelectDialogLayout = new\n> QFormLayout(this);\n> > +\n> > + /* Setup the camera id combo-box. */\n> > + cameraIdComboBox_ = new QComboBox;\n> > + for (const auto &cam : cm_->cameras())\n> > +\n> cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));\n>\n> I could imagine we need to add hotplug events in here too.\n> It could be done on top, but to test it - try removing or adding a\n> camera while the dialog box is opened.\n>\n> As long as it doesn't crash if the camera is removed while the dialog\n> box is open, I don't mind if the hotplug (adding cameras when they are\n> plugged in, removing when removed) is added on top. Especially as the\n> existing dialog didn't support hotplug either.\n>\n>\n> I guess if there's no camera left, the dialog box doesn't have much to\n> do ;-) (though should stay open in case someone replugs the UVC camera)\n>\n>\nYes saw your other reply, and yes it's done in patch 2nd.\n\n\n>\n> > +\n> > + /* Setup the QDialogButton Box */\n> > + QDialogButtonBox *dialogButtonBox =\n> > + new QDialogButtonBox(QDialogButtonBox::Ok |\n> > + QDialogButtonBox::Cancel);\n> > +\n> > + connect(dialogButtonBox, &QDialogButtonBox::accepted,\n> > + this, &QDialog::accept);\n> > + connect(dialogButtonBox, &QDialogButtonBox::rejected,\n> > + this, &QDialog::reject);\n> > +\n> > + /* Set the layout. */\n> > + camSelectDialogLayout->addRow(\"Camera: \",\n> cameraIdComboBox_);\n> > + camSelectDialogLayout->addWidget(dialogButtonBox);\n> > + }\n> > +\n> > + ~CamSelectDialog() = default;\n> > +\n> > + std::string getCameraId()\n> > + {\n> > + return cameraIdComboBox_->currentText().toStdString();\n> > + }\n>\n> While there's not a lot of implementation here, I think the code should\n> be in a .cpp file. I'm not sure where the threshold is for how small a\n> class can be to be inlined directly in the header, but I think this one\n> can potentially expand, so probably deserves it's own implementation\n> file.\n>\n> Sure, I would wait on review for the other patches then move everything\nto its .cpp.\n\n> > +\n> > +private:\n> > + libcamera::CameraManager *cm_;\n> > +\n> > + /* UI elements. */\n> > + QComboBox *cameraIdComboBox_;\n> > +};\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index 7433d647..758e2c94 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -19,7 +19,6 @@\n> > #include \n> > #include \n> > #include \n> > -#include \n> > #include \n> > #include \n> > #include \n> > @@ -30,6 +29,7 @@\n> >\n> > #include \"../cam/image.h\"\n> >\n> > +#include \"cam_select_dialog.h\"\n> > #include \"dng_writer.h\"\n> > #ifndef QT_NO_OPENGL\n> > #include \"viewfinder_gl.h\"\n> > @@ -290,24 +290,12 @@ void MainWindow::switchCamera(int index)\n> >\n> > std::string MainWindow::chooseCamera()\n> > {\n> > - QStringList cameras;\n> > - bool result;\n> > + camSelectDialog_ = new CamSelectDialog(cm_, this);\n>\n> Do we need to keep the dialog in a class variable? or could it be local\n> and destroyed at the end of this function?\n>\nYes we need it as a class variable for camera hotplugging.\n\n>\n> >\n> > - /* If only one camera is available, use it automatically. */\n> > - if (cm_->cameras().size() == 1)\n> > - return cm_->cameras()[0]->id();\n> > -\n> > - /* Present a dialog box to pick a camera. */\n> > - for (const std::shared_ptr &cam : cm_->cameras())\n> > - cameras.append(QString::fromStdString(cam->id()));\n> > -\n> > - QString id = QInputDialog::getItem(this, \"Select Camera\",\n> > - \"Camera:\", cameras, 0,\n> > - false, &result);\n> > - if (!result)\n> > + if (camSelectDialog_->exec() == QDialog::Accepted)\n> > + return camSelectDialog_->getCameraId();\n> > + else\n> > return std::string();\n> > -\n> > - return id.toStdString();\n> > }\n> >\n> > int MainWindow::openCamera()\n> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > index fc70920f..6d80b5be 100644\n> > --- a/src/qcam/main_window.h\n> > +++ b/src/qcam/main_window.h\n> > @@ -23,11 +23,13 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > #include \"../cam/stream_options.h\"\n> >\n> > +#include \"cam_select_dialog.h\"\n> > #include \"viewfinder.h\"\n> >\n> > class QAction;\n> > @@ -99,6 +101,8 @@ private:\n> > QString title_;\n> > QTimer titleTimer_;\n> >\n> > + QPointer camSelectDialog_;\n> > +\n> > /* Options */\n> > const OptionsParser::Options &options_;\n> >\n> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > index c46f4631..70615e92 100644\n> > --- a/src/qcam/meson.build\n> > +++ b/src/qcam/meson.build\n> > @@ -26,6 +26,7 @@ qcam_sources = files([\n> > ])\n> >\n> > qcam_moc_headers = files([\n> > + 'cam_select_dialog.h',\n> > 'main_window.h',\n> > 'viewfinder_qt.h',\n> > ])\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 634E6C3272\n\tfor ;\n\tMon, 8 Aug 2022 06:56:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 250336332C;\n\tMon, 8 Aug 2022 08:56:59 +0200 (CEST)","from mail-pg1-x530.google.com (mail-pg1-x530.google.com\n\t[IPv6:2607:f8b0:4864:20::530])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E5CC603EA\n\tfor ;\n\tMon, 8 Aug 2022 08:56:57 +0200 (CEST)","by mail-pg1-x530.google.com with SMTP id 12so7814804pga.1\n\tfor ;\n\tSun, 07 Aug 2022 23:56:57 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659941819;\n\tbh=pAdIYS23LMsyFFrydLCc1hC27acktsLpT0wsGDbc9CI=;\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=LgIXTEiCWuW5pxB48Q9lJMPIYrXv82HRCaWPMuFKyxuuzKXZSraYSaxYnkPcl1bTA\n\tvQ2jqDYEQlXnWDC7Py//KGl763SefTh/XKzcRlvF0PylnfIU4Btfo3OuZW3j4vois1\n\tg1eIkSwtfWWKqShaZEj0vv+cj3Z2KX4rb5XbEI3RtQfg8tMb/S8VkO0ghCTrxXEiim\n\teWmbVNcaqO4jjdU263c8WBZgcumuaQSjGsn9dDROERG/4rgZEntLVdqKAEEBNhZpsk\n\tOGFgCXB83Mgm8LNqloFd7kF66CLgGUp3ncb5hlpFbfk1YuHF3rqaPbaRnwf4EGmJ2d\n\tEpB3aCxpRXS2w==","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=WafSMhXp9fTlayII9AFY0yINFtxxVfDr9hipE38be6E=;\n\tb=SP6Dj+IFjIwZ+i0iDJ6TKRLuAX1wMHVuMJ/nM6k+QDSkcataAdYxg09ovg7BefYVrO\n\tv7v44+JxWv2N/yMwTh5c+vRb2OIfv/5jvmydUq7YykdxEHhD7QJKVu6N3wOlvNbIQmfx\n\tE7OMJNcfUtKHJuDQiX0yIus8HfnaztSF5n65emLyWZj66lTVK+HnwRT5fa5YESRsNW53\n\tSvb7l6WegyCP8XXW0Sf2FJoHmHkrm20uexA7iacfACwcxpscW5YsigCGKLm5hLmava/r\n\tjgf0IF3jF5haEqdSbp8TovxLo49mTGme7UgjlcLQBFg4vsnSHkMyVg4P9Dyv19G8Oxal\n\t3e0Q=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"SP6Dj+IF\"; 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=WafSMhXp9fTlayII9AFY0yINFtxxVfDr9hipE38be6E=;\n\tb=mdQ64Ysqrw4CO7HjhoNiou0ycmDmEQqep3+lYsX1slXWhgN/wSO61wB2QnMlBgYJhW\n\t/i/+AnsNoQ7VBm/AKBgS0ZC1FzbHmHrJnFCpD48vlBRExUVrPhIOH+661manBgJBVYYG\n\tDmmBtkTl6BTAYa//QmZ3i5wirasEzqNYzlAKcVcYve+MHSdOGr816yAFy1hTCL9J3mI7\n\tfpsZ1ADLv4mTtpdRh/XxNe4g54u495hSFCGA9tb3x/3Kdwgpp2kkBpoDJvWGNTuLRoK4\n\tmTkaMVvhc7x+P2vbkXmDTQae0LltJNW3FPzlJSffcc70EOZlVtBbdiZyYX6Q+3ybE/wh\n\tpr9g==","X-Gm-Message-State":"ACgBeo10f6LdbMCCvxPgPL26FML1dvELUnBqavUstyVX9T1tlx+xWhnL\n\tr926uwMtjImL17rDWE8yudV/STVHBis9DtA6huo=","X-Google-Smtp-Source":"AA6agR4NII3pvPz2VgKYH2YuI7ORfdZ9BZowVzjswaj2YHoxzJpYALudLJ4edoSJGo/NuKgwd4qtAxUBd7ddshwjaWY=","X-Received":"by 2002:a05:6a00:15c3:b0:52e:677b:7026 with SMTP id\n\to3-20020a056a0015c300b0052e677b7026mr17505208pfu.75.1659941815350;\n\tSun, 07 Aug 2022 23:56:55 -0700 (PDT)","MIME-Version":"1.0","References":"<20220806190433.59128-1-utkarsh02t@gmail.com>\n\t<20220806190433.59128-2-utkarsh02t@gmail.com>\n\t<165991437612.1706285.3866162190018711245@Monstersaurus>","In-Reply-To":"<165991437612.1706285.3866162190018711245@Monstersaurus>","Date":"Mon, 8 Aug 2022 12:26:41 +0530","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000407b4705e5b5529e\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for\n\tselection of cameras at startup","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":24414,"web_url":"https://patchwork.libcamera.org/comment/24414/","msgid":"<165994833230.15821.3357093096308907252@Monstersaurus>","date":"2022-08-08T08:45:32","subject":"Re: [libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for\n\tselection of cameras at startup","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-08 07:56:41)\n> Hi Kieran thanks for the review.\n> \n> On Mon, 8 Aug, 2022, 04:49 Kieran Bingham, \n> wrote:\n> \n> > Quoting Utkarsh Tiwari via libcamera-devel (2022-08-06 20:04:30)\n> > > Currently we use QInputDialog convenience dialogs to allow the user to\n> > > select a camera. This doesn't allow adding of more information (such as\n> > > camera location, model etc).\n> > >\n> > > Create a QDialog with a QFormLayout that shows a QComboBox with camera\n> > > Ids. Use a QDialogButtonBox to provide buttons for accepting and\n> > > cancelling the action.\n> > >\n> > > Signed-off-by: Utkarsh Tiwari \n> > > ---\n> > > Difference from V1:\n> > > The biggest change here is the introduction of class, inside of\n> > > the class the layout remains same.\n> > > src/qcam/cam_select_dialog.h | 64 ++++++++++++++++++++++++++++++++++++\n> > > src/qcam/main_window.cpp | 22 +++----------\n> > > src/qcam/main_window.h | 4 +++\n> > > src/qcam/meson.build | 1 +\n> > > 4 files changed, 74 insertions(+), 17 deletions(-)\n> > > create mode 100644 src/qcam/cam_select_dialog.h\n> > >\n> > > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> > > new file mode 100644\n> > > index 00000000..c23bad59\n> > > --- /dev/null\n> > > +++ b/src/qcam/cam_select_dialog.h\n> > > @@ -0,0 +1,64 @@\n> > > +\n> >\n> > Remove the first blank line please.\n> >\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> >\n> > I think this should be 2022. I'm not sure if copyright goes to you or\n> > Google for GSoC.\n> >\n> Oops copy paste error, It's my copyright.\n> \n> >\n> >\n> > > + *\n> > > + * cam_select_dialog.h - qcam - Camera Selection dialog\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +class CamSelectDialog : public QDialog\n> >\n> > It's bikeshedding territory - but CameraSelectDialog sounds better to me\n> > here. (Spell 'Camera' in full).\n> >\n> > It seems Laurent suggested CameraSelectorDialog too.\n> >\n> \n> Fine by me.\n> \n> >\n> >\n> > > +{\n> > > + Q_OBJECT\n> > > +public:\n> > > + CamSelectDialog(libcamera::CameraManager *cameraManager, QWidget\n> > *parent)\n> > > + : QDialog(parent), cm_(cameraManager)\n> >\n> > Is cm_ used later? I don't mind it being added here if it's needed\n> > later, but it only seems to be used in this construction for the moment.\n> >\n> >\n> Yes, to show the camera info I need to use cm_ to get the camera.\n> \n> >\n> > > + {\n> > > + /* Use a QFormLayout for the dialog. */\n> > > + QFormLayout *camSelectDialogLayout = new\n> > QFormLayout(this);\n> > > +\n> > > + /* Setup the camera id combo-box. */\n> > > + cameraIdComboBox_ = new QComboBox;\n> > > + for (const auto &cam : cm_->cameras())\n> > > +\n> > cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));\n> >\n> > I could imagine we need to add hotplug events in here too.\n> > It could be done on top, but to test it - try removing or adding a\n> > camera while the dialog box is opened.\n> >\n> > As long as it doesn't crash if the camera is removed while the dialog\n> > box is open, I don't mind if the hotplug (adding cameras when they are\n> > plugged in, removing when removed) is added on top. Especially as the\n> > existing dialog didn't support hotplug either.\n> >\n> >\n> > I guess if there's no camera left, the dialog box doesn't have much to\n> > do ;-) (though should stay open in case someone replugs the UVC camera)\n> >\n> >\n> Yes saw your other reply, and yes it's done in patch 2nd.\n> \n> \n> >\n> > > +\n> > > + /* Setup the QDialogButton Box */\n> > > + QDialogButtonBox *dialogButtonBox =\n> > > + new QDialogButtonBox(QDialogButtonBox::Ok |\n> > > + QDialogButtonBox::Cancel);\n> > > +\n> > > + connect(dialogButtonBox, &QDialogButtonBox::accepted,\n> > > + this, &QDialog::accept);\n> > > + connect(dialogButtonBox, &QDialogButtonBox::rejected,\n> > > + this, &QDialog::reject);\n> > > +\n> > > + /* Set the layout. */\n> > > + camSelectDialogLayout->addRow(\"Camera: \",\n> > cameraIdComboBox_);\n> > > + camSelectDialogLayout->addWidget(dialogButtonBox);\n> > > + }\n> > > +\n> > > + ~CamSelectDialog() = default;\n> > > +\n> > > + std::string getCameraId()\n> > > + {\n> > > + return cameraIdComboBox_->currentText().toStdString();\n> > > + }\n> >\n> > While there's not a lot of implementation here, I think the code should\n> > be in a .cpp file. I'm not sure where the threshold is for how small a\n> > class can be to be inlined directly in the header, but I think this one\n> > can potentially expand, so probably deserves it's own implementation\n> > file.\n> >\n> > Sure, I would wait on review for the other patches then move everything\n> to its .cpp.\n> \n> > > +\n> > > +private:\n> > > + libcamera::CameraManager *cm_;\n> > > +\n> > > + /* UI elements. */\n> > > + QComboBox *cameraIdComboBox_;\n> > > +};\n> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > > index 7433d647..758e2c94 100644\n> > > --- a/src/qcam/main_window.cpp\n> > > +++ b/src/qcam/main_window.cpp\n> > > @@ -19,7 +19,6 @@\n> > > #include \n> > > #include \n> > > #include \n> > > -#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -30,6 +29,7 @@\n> > >\n> > > #include \"../cam/image.h\"\n> > >\n> > > +#include \"cam_select_dialog.h\"\n> > > #include \"dng_writer.h\"\n> > > #ifndef QT_NO_OPENGL\n> > > #include \"viewfinder_gl.h\"\n> > > @@ -290,24 +290,12 @@ void MainWindow::switchCamera(int index)\n> > >\n> > > std::string MainWindow::chooseCamera()\n> > > {\n> > > - QStringList cameras;\n> > > - bool result;\n> > > + camSelectDialog_ = new CamSelectDialog(cm_, this);\n> >\n> > Do we need to keep the dialog in a class variable? or could it be local\n> > and destroyed at the end of this function?\n> >\n> Yes we need it as a class variable for camera hotplugging.\n\nThis seems to recreate the CamSelectDialog box each time from scratch,\neven though we constructed it - and keep it updated with hotplug events.\n\nIs it possible to re-use it when it's created? or does accepting the\ndialog or closing it make it no longer possible to re-use?\n\nIf we can re-use it - we should ... Otherwise - we probably shouldn't\nsend hotplug events when it's been closed. ?\n\n\n\n> \n> >\n> > >\n> > > - /* If only one camera is available, use it automatically. */\n> > > - if (cm_->cameras().size() == 1)\n> > > - return cm_->cameras()[0]->id();\n> > > -\n> > > - /* Present a dialog box to pick a camera. */\n> > > - for (const std::shared_ptr &cam : cm_->cameras())\n> > > - cameras.append(QString::fromStdString(cam->id()));\n> > > -\n> > > - QString id = QInputDialog::getItem(this, \"Select Camera\",\n> > > - \"Camera:\", cameras, 0,\n> > > - false, &result);\n> > > - if (!result)\n> > > + if (camSelectDialog_->exec() == QDialog::Accepted)\n> > > + return camSelectDialog_->getCameraId();\n> > > + else\n> > > return std::string();\n> > > -\n> > > - return id.toStdString();\n> > > }\n> > >\n> > > int MainWindow::openCamera()\n> > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > > index fc70920f..6d80b5be 100644\n> > > --- a/src/qcam/main_window.h\n> > > +++ b/src/qcam/main_window.h\n> > > @@ -23,11 +23,13 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > #include \"../cam/stream_options.h\"\n> > >\n> > > +#include \"cam_select_dialog.h\"\n> > > #include \"viewfinder.h\"\n> > >\n> > > class QAction;\n> > > @@ -99,6 +101,8 @@ private:\n> > > QString title_;\n> > > QTimer titleTimer_;\n> > >\n> > > + QPointer camSelectDialog_;\n> > > +\n> > > /* Options */\n> > > const OptionsParser::Options &options_;\n> > >\n> > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > > index c46f4631..70615e92 100644\n> > > --- a/src/qcam/meson.build\n> > > +++ b/src/qcam/meson.build\n> > > @@ -26,6 +26,7 @@ qcam_sources = files([\n> > > ])\n> > >\n> > > qcam_moc_headers = files([\n> > > + 'cam_select_dialog.h',\n> > > 'main_window.h',\n> > > 'viewfinder_qt.h',\n> > > ])\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 3DCA2BE173\n\tfor ;\n\tMon, 8 Aug 2022 08:45:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A992E63327;\n\tMon, 8 Aug 2022 10:45:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2668563326\n\tfor ;\n\tMon, 8 Aug 2022 10:45:35 +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 B0C39481;\n\tMon, 8 Aug 2022 10:45:34 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659948336;\n\tbh=lsUilKHFIQL5BnTaiwEyH9rvyx1Tdn5kA8eWmoUY61g=;\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=0XK15SsaghW46KD/EesqrD9lpIlE9HGOrUGf3pY1CAsyqbCh3y8F0rXRVqxEpMC94\n\t4uMZ4sEVDAvjWx96it7q9APoazGE8iXYCP5J1MicF2bVeMwLtSzZWJu7jWU+gjcx+h\n\tbmuxltRefpJYjuwQ05XwqLO7P7HkdM0oEGvb/pKxKr5Z9p3ntSdM0zMBx9Uuvzrz9d\n\tDtVVNM3zOhSzvGHbQPYcwbDRvrmfKPf614RYH+1md9KlJxhhBp9iQv6NTYBF8sHPHX\n\tGQAhFn45XGd3zFVwyhEpl40g8wbU9x1gCc4SkbiFnfPDv8DmcM1pzBLRVZc2pTA1iW\n\tMeycZQKkbfBFg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659948334;\n\tbh=lsUilKHFIQL5BnTaiwEyH9rvyx1Tdn5kA8eWmoUY61g=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=MV9Aw84unhTKMvOLJIEiDTnTmBdnnTq094QQVQpLXxlBaLNmN1bD7lTPbyO6CPpao\n\tKZ1tSP3DXhZPWtdQ2TXdH8ss2XzH+Hkx/o6YzgwPGWVwKye+7rqbQR34xVCP/45oRg\n\twWndnkdLjD7pPU5f/x4N4HzNqvs41PngEMbxcm0U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"MV9Aw84u\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220806190433.59128-1-utkarsh02t@gmail.com>\n\t<20220806190433.59128-2-utkarsh02t@gmail.com>\n\t<165991437612.1706285.3866162190018711245@Monstersaurus>\n\t","To":"Utkarsh Tiwari ","Date":"Mon, 08 Aug 2022 09:45:32 +0100","Message-ID":"<165994833230.15821.3357093096308907252@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for\n\tselection of cameras at startup","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\" "}},{"id":24434,"web_url":"https://patchwork.libcamera.org/comment/24434/","msgid":"","date":"2022-08-08T17:19:20","subject":"Re: [libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for\n\tselection of cameras at startup","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 Mon, Aug 8, 2022 at 2:15 PM Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Utkarsh Tiwari (2022-08-08 07:56:41)\n> > Hi Kieran thanks for the review.\n> >\n> > On Mon, 8 Aug, 2022, 04:49 Kieran Bingham, <\n> kieran.bingham@ideasonboard.com>\n> > wrote:\n> >\n> > > Quoting Utkarsh Tiwari via libcamera-devel (2022-08-06 20:04:30)\n> > > > Currently we use QInputDialog convenience dialogs to allow the user\n> to\n> > > > select a camera. This doesn't allow adding of more information (such\n> as\n> > > > camera location, model etc).\n> > > >\n> > > > Create a QDialog with a QFormLayout that shows a QComboBox with\n> camera\n> > > > Ids. Use a QDialogButtonBox to provide buttons for accepting and\n> > > > cancelling the action.\n> > > >\n> > > > Signed-off-by: Utkarsh Tiwari \n> > > > ---\n> > > > Difference from V1:\n> > > > The biggest change here is the introduction of class, inside of\n> > > > the class the layout remains same.\n> > > > src/qcam/cam_select_dialog.h | 64\n> ++++++++++++++++++++++++++++++++++++\n> > > > src/qcam/main_window.cpp | 22 +++----------\n> > > > src/qcam/main_window.h | 4 +++\n> > > > src/qcam/meson.build | 1 +\n> > > > 4 files changed, 74 insertions(+), 17 deletions(-)\n> > > > create mode 100644 src/qcam/cam_select_dialog.h\n> > > >\n> > > > diff --git a/src/qcam/cam_select_dialog.h\n> b/src/qcam/cam_select_dialog.h\n> > > > new file mode 100644\n> > > > index 00000000..c23bad59\n> > > > --- /dev/null\n> > > > +++ b/src/qcam/cam_select_dialog.h\n> > > > @@ -0,0 +1,64 @@\n> > > > +\n> > >\n> > > Remove the first blank line please.\n> > >\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2019, Google Inc.\n> > >\n> > > I think this should be 2022. I'm not sure if copyright goes to you or\n> > > Google for GSoC.\n> > >\n> > Oops copy paste error, It's my copyright.\n> >\n> > >\n> > >\n> > > > + *\n> > > > + * cam_select_dialog.h - qcam - Camera Selection dialog\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +class CamSelectDialog : public QDialog\n> > >\n> > > It's bikeshedding territory - but CameraSelectDialog sounds better to\n> me\n> > > here. (Spell 'Camera' in full).\n> > >\n> > > It seems Laurent suggested CameraSelectorDialog too.\n> > >\n> >\n> > Fine by me.\n> >\n> > >\n> > >\n> > > > +{\n> > > > + Q_OBJECT\n> > > > +public:\n> > > > + CamSelectDialog(libcamera::CameraManager *cameraManager,\n> QWidget\n> > > *parent)\n> > > > + : QDialog(parent), cm_(cameraManager)\n> > >\n> > > Is cm_ used later? I don't mind it being added here if it's needed\n> > > later, but it only seems to be used in this construction for the\n> moment.\n> > >\n> > >\n> > Yes, to show the camera info I need to use cm_ to get the camera.\n> >\n> > >\n> > > > + {\n> > > > + /* Use a QFormLayout for the dialog. */\n> > > > + QFormLayout *camSelectDialogLayout = new\n> > > QFormLayout(this);\n> > > > +\n> > > > + /* Setup the camera id combo-box. */\n> > > > + cameraIdComboBox_ = new QComboBox;\n> > > > + for (const auto &cam : cm_->cameras())\n> > > > +\n> > > cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));\n> > >\n> > > I could imagine we need to add hotplug events in here too.\n> > > It could be done on top, but to test it - try removing or adding a\n> > > camera while the dialog box is opened.\n> > >\n> > > As long as it doesn't crash if the camera is removed while the dialog\n> > > box is open, I don't mind if the hotplug (adding cameras when they are\n> > > plugged in, removing when removed) is added on top. Especially as the\n> > > existing dialog didn't support hotplug either.\n> > >\n> > >\n> > > I guess if there's no camera left, the dialog box doesn't have much to\n> > > do ;-) (though should stay open in case someone replugs the UVC camera)\n> > >\n> > >\n> > Yes saw your other reply, and yes it's done in patch 2nd.\n> >\n> >\n> > >\n> > > > +\n> > > > + /* Setup the QDialogButton Box */\n> > > > + QDialogButtonBox *dialogButtonBox =\n> > > > + new QDialogButtonBox(QDialogButtonBox::Ok |\n> > > > +\n> QDialogButtonBox::Cancel);\n> > > > +\n> > > > + connect(dialogButtonBox, &QDialogButtonBox::accepted,\n> > > > + this, &QDialog::accept);\n> > > > + connect(dialogButtonBox, &QDialogButtonBox::rejected,\n> > > > + this, &QDialog::reject);\n> > > > +\n> > > > + /* Set the layout. */\n> > > > + camSelectDialogLayout->addRow(\"Camera: \",\n> > > cameraIdComboBox_);\n> > > > + camSelectDialogLayout->addWidget(dialogButtonBox);\n> > > > + }\n> > > > +\n> > > > + ~CamSelectDialog() = default;\n> > > > +\n> > > > + std::string getCameraId()\n> > > > + {\n> > > > + return\n> cameraIdComboBox_->currentText().toStdString();\n> > > > + }\n> > >\n> > > While there's not a lot of implementation here, I think the code should\n> > > be in a .cpp file. I'm not sure where the threshold is for how small a\n> > > class can be to be inlined directly in the header, but I think this one\n> > > can potentially expand, so probably deserves it's own implementation\n> > > file.\n> > >\n> > > Sure, I would wait on review for the other patches then move everything\n> > to its .cpp.\n> >\n> > > > +\n> > > > +private:\n> > > > + libcamera::CameraManager *cm_;\n> > > > +\n> > > > + /* UI elements. */\n> > > > + QComboBox *cameraIdComboBox_;\n> > > > +};\n> > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > > > index 7433d647..758e2c94 100644\n> > > > --- a/src/qcam/main_window.cpp\n> > > > +++ b/src/qcam/main_window.cpp\n> > > > @@ -19,7 +19,6 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > -#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -30,6 +29,7 @@\n> > > >\n> > > > #include \"../cam/image.h\"\n> > > >\n> > > > +#include \"cam_select_dialog.h\"\n> > > > #include \"dng_writer.h\"\n> > > > #ifndef QT_NO_OPENGL\n> > > > #include \"viewfinder_gl.h\"\n> > > > @@ -290,24 +290,12 @@ void MainWindow::switchCamera(int index)\n> > > >\n> > > > std::string MainWindow::chooseCamera()\n> > > > {\n> > > > - QStringList cameras;\n> > > > - bool result;\n> > > > + camSelectDialog_ = new CamSelectDialog(cm_, this);\n> > >\n> > > Do we need to keep the dialog in a class variable? or could it be local\n> > > and destroyed at the end of this function?\n> > >\n> > Yes we need it as a class variable for camera hotplugging.\n>\n> This seems to recreate the CamSelectDialog box each time from scratch,\n> even though we constructed it - and keep it updated with hotplug events.\n>\n> Is it possible to re-use it when it's created? or does accepting the\n> dialog or closing it make it no longer possible to re-use?\n>\n> If we can re-use it - we should ... Otherwise - we probably shouldn't\n> send hotplug events when it's been closed. ?\n>\n>\nI think we can re-use it. And I think the current way is causing a leak\n:P.\n\n\n>\n> >\n> > >\n> > > >\n> > > > - /* If only one camera is available, use it automatically. */\n> > > > - if (cm_->cameras().size() == 1)\n> > > > - return cm_->cameras()[0]->id();\n> > > > -\n> > > > - /* Present a dialog box to pick a camera. */\n> > > > - for (const std::shared_ptr &cam : cm_->cameras())\n> > > > - cameras.append(QString::fromStdString(cam->id()));\n> > > > -\n> > > > - QString id = QInputDialog::getItem(this, \"Select Camera\",\n> > > > - \"Camera:\", cameras, 0,\n> > > > - false, &result);\n> > > > - if (!result)\n> > > > + if (camSelectDialog_->exec() == QDialog::Accepted)\n> > > > + return camSelectDialog_->getCameraId();\n> > > > + else\n> > > > return std::string();\n> > > > -\n> > > > - return id.toStdString();\n> > > > }\n> > > >\n> > > > int MainWindow::openCamera()\n> > > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > > > index fc70920f..6d80b5be 100644\n> > > > --- a/src/qcam/main_window.h\n> > > > +++ b/src/qcam/main_window.h\n> > > > @@ -23,11 +23,13 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > #include \"../cam/stream_options.h\"\n> > > >\n> > > > +#include \"cam_select_dialog.h\"\n> > > > #include \"viewfinder.h\"\n> > > >\n> > > > class QAction;\n> > > > @@ -99,6 +101,8 @@ private:\n> > > > QString title_;\n> > > > QTimer titleTimer_;\n> > > >\n> > > > + QPointer camSelectDialog_;\n> > > > +\n> > > > /* Options */\n> > > > const OptionsParser::Options &options_;\n> > > >\n> > > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > > > index c46f4631..70615e92 100644\n> > > > --- a/src/qcam/meson.build\n> > > > +++ b/src/qcam/meson.build\n> > > > @@ -26,6 +26,7 @@ qcam_sources = files([\n> > > > ])\n> > > >\n> > > > qcam_moc_headers = files([\n> > > > + 'cam_select_dialog.h',\n> > > > 'main_window.h',\n> > > > 'viewfinder_qt.h',\n> > > > ])\n> > > > --\n> > > > 2.25.1\n> > > >\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 954F4C3272\n\tfor ;\n\tMon, 8 Aug 2022 17:19:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE0336332B;\n\tMon, 8 Aug 2022 19:19:35 +0200 (CEST)","from mail-pj1-x102d.google.com (mail-pj1-x102d.google.com\n\t[IPv6:2607:f8b0:4864:20::102d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 79AF663315\n\tfor ;\n\tMon, 8 Aug 2022 19:19:34 +0200 (CEST)","by mail-pj1-x102d.google.com with SMTP id\n\to3-20020a17090a0a0300b001f7649cd317so2094271pjo.0\n\tfor ;\n\tMon, 08 Aug 2022 10:19:34 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659979175;\n\tbh=vKzqx7yLLGMXEn6zOxt4ajsW94q1UamLRmy7B6KqV2o=;\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=h6i1i+nAelXb5i1XItKC2544Ym3eNOe35uwK7uJacw2dlOUDA4bssyxrzZpG7W3ss\n\t3kBNijgAQ7lsKjbuy+az82YrBBvHcmm++5MpVOyp4VHrkHFZga7lylMn037tdjjgkW\n\t8qmxg7cCR9gARw96mqujJmBpjHuxFqPGw3bZ8N+x89POlKfq6QangTpd5YhGA7CXOf\n\tODY/mCSiqw12U4lNUHjH6BXpPPWNUprh9HQZvMeCnq56zPjj3QqsoM7bqmlhxcIFkF\n\tqXbWRi6KVyww0ZnDqDCkfekjQxIpenqLyxBNUW1slDt0T4KXeE5IYjIRKT0d7u+LdG\n\tRX41T6FTB1+Bg==","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=0sJ8cgGkrIpPJhwTKETqFdWyaEK0TjMw1xkQK/H9gv4=;\n\tb=JHvYZmHCXbD4kJvQUAIw+YGvZxyXnFK93EYINkLHmU2BQIXdESJo84YCqiLLmx398s\n\t1T31QOzZXu1vJJSlhtZIXJJv9VyXjdSbbOC3eYRY0FOzlz50jdP9vzccfMLRLMpztCsQ\n\tZpvoczSE26DCSs6O1RLQ2wFKBiu1NfZC2s96HINHnv6HiSG4xee8ew+PMhl+CCLiWGLh\n\t1lCBvi/T6wuQy9PrggkDY+JRorxqxh0yB6R4wlxKzMgiTQhbDx5uOl+1zVEtZK+vnbvP\n\tJneCK6xQJyHT1Ax1YpzE3zvM920jzoeIknDPmNRYBCBMLqJcxNoxaLoMNPUzoNk5Pv19\n\tMWZQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"JHvYZmHC\"; 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=0sJ8cgGkrIpPJhwTKETqFdWyaEK0TjMw1xkQK/H9gv4=;\n\tb=jWUlakqY0Nm7J158bw4h4u8KWmMBXN9dqyzRyKTXihYvNYqOQyzpc9r/LeQbHbpaKa\n\t2U5KOHhC/5RNmWOxxErHLqTNkbwY7JBLe3WCRdgK2TB697RhlW7m/SRsLxAIM/icyV4o\n\tVWyIMqaGrqQ+9zS5qBER64tlmJjtiL9F8hHbA+bjxPdonXAmEDVL3rrjrKZ1pFW+iYKY\n\tD2YsAG3ldyiNM+RSo67zjLkT71usIPxMg5fjLzhwEYQXA0wg1e4ce45zSdkYhx8Mk2/X\n\tFsc/cg9Rp9yMIYY0ztfCIuywYEaheCfTlnl+Hbf3NMtlHWFUmcMk2Od9m2oIOwGUkG2r\n\tKXBg==","X-Gm-Message-State":"ACgBeo078PJY5Ky1wnQfb5RAinwn4Owt7O8qJGEh8aIJC6/WAvLmM8P5\n\tFBaEZqxZlNFreeM5G0WcvheolpZ74yexjb3Z2VWkCPKK","X-Google-Smtp-Source":"AA6agR6EYdZdK55aHBPaHA+6EPWAxGSEbaty/WTq4UhWdex5Z+3qrBUTQV0LSWVKT0FEN25GS0bmFO0Lq+GjSlGyXbQ=","X-Received":"by 2002:a17:90b:4ccb:b0:1f5:20b4:fc9e with SMTP id\n\tnd11-20020a17090b4ccb00b001f520b4fc9emr22005792pjb.69.1659979172685;\n\tMon, 08 Aug 2022 10:19:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20220806190433.59128-1-utkarsh02t@gmail.com>\n\t<20220806190433.59128-2-utkarsh02t@gmail.com>\n\t<165991437612.1706285.3866162190018711245@Monstersaurus>\n\t\n\t<165994833230.15821.3357093096308907252@Monstersaurus>","In-Reply-To":"<165994833230.15821.3357093096308907252@Monstersaurus>","Date":"Mon, 8 Aug 2022 22:49:20 +0530","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000ec2c2905e5be043a\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for\n\tselection of cameras at startup","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":24439,"web_url":"https://patchwork.libcamera.org/comment/24439/","msgid":"","date":"2022-08-08T20:38:21","subject":"Re: [libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for\n\tselection of cameras at startup","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Aug 08, 2022 at 10:49:20PM +0530, Utkarsh Tiwari via libcamera-devel wrote:\n> On Mon, Aug 8, 2022 at 2:15 PM Kieran Bingham wrote:\n> > Quoting Utkarsh Tiwari (2022-08-08 07:56:41)\n> > > On Mon, 8 Aug, 2022, 04:49 Kieran Bingham wrote:\n> > > > Quoting Utkarsh Tiwari via libcamera-devel (2022-08-06 20:04:30)\n> > > > > Currently we use QInputDialog convenience dialogs to allow the user to\n> > > > > select a camera. This doesn't allow adding of more information (such as\n> > > > > camera location, model etc).\n> > > > >\n> > > > > Create a QDialog with a QFormLayout that shows a QComboBox with camera\n> > > > > Ids. Use a QDialogButtonBox to provide buttons for accepting and\n> > > > > cancelling the action.\n> > > > >\n> > > > > Signed-off-by: Utkarsh Tiwari \n> > > > > ---\n> > > > > Difference from V1:\n> > > > > The biggest change here is the introduction of class, inside of\n> > > > > the class the layout remains same.\n> > > > > src/qcam/cam_select_dialog.h | 64 ++++++++++++++++++++++++++++++++++++\n> > > > > src/qcam/main_window.cpp | 22 +++----------\n> > > > > src/qcam/main_window.h | 4 +++\n> > > > > src/qcam/meson.build | 1 +\n> > > > > 4 files changed, 74 insertions(+), 17 deletions(-)\n> > > > > create mode 100644 src/qcam/cam_select_dialog.h\n> > > > >\n> > > > > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> > > > > new file mode 100644\n> > > > > index 00000000..c23bad59\n> > > > > --- /dev/null\n> > > > > +++ b/src/qcam/cam_select_dialog.h\n> > > > > @@ -0,0 +1,64 @@\n> > > > > +\n> > > >\n> > > > Remove the first blank line please.\n> > > >\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2019, Google Inc.\n> > > >\n> > > > I think this should be 2022. I'm not sure if copyright goes to you or\n> > > > Google for GSoC.\n> > > \n> > > Oops copy paste error, It's my copyright.\n> > >\n> > > > > + *\n> > > > > + * cam_select_dialog.h - qcam - Camera Selection dialog\n> > > > > + */\n> > > > > +\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +class CamSelectDialog : public QDialog\n> > > >\n> > > > It's bikeshedding territory - but CameraSelectDialog sounds better to me\n> > > > here. (Spell 'Camera' in full).\n> > > >\n> > > > It seems Laurent suggested CameraSelectorDialog too.\n> > >\n> > > Fine by me.\n> > >\n> > > > > +{\n> > > > > + Q_OBJECT\n> > > > > +public:\n> > > > > + CamSelectDialog(libcamera::CameraManager *cameraManager, QWidget *parent)\n> > > > > + : QDialog(parent), cm_(cameraManager)\n> > > >\n> > > > Is cm_ used later? I don't mind it being added here if it's needed\n> > > > later, but it only seems to be used in this construction for the moment.\n> > > \n> > > Yes, to show the camera info I need to use cm_ to get the camera.\n> > >\n> > > > > + {\n> > > > > + /* Use a QFormLayout for the dialog. */\n> > > > > + QFormLayout *camSelectDialogLayout = new QFormLayout(this);\n\nYou can simply call the variable \"layout\".\n\n> > > > > +\n> > > > > + /* Setup the camera id combo-box. */\n> > > > > + cameraIdComboBox_ = new QComboBox;\n> > > > > + for (const auto &cam : cm_->cameras())\n> > > > > + cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));\n> > > >\n> > > > I could imagine we need to add hotplug events in here too.\n> > > > It could be done on top, but to test it - try removing or adding a\n> > > > camera while the dialog box is opened.\n> > > >\n> > > > As long as it doesn't crash if the camera is removed while the dialog\n> > > > box is open, I don't mind if the hotplug (adding cameras when they are\n> > > > plugged in, removing when removed) is added on top. Especially as the\n> > > > existing dialog didn't support hotplug either.\n> > > >\n> > > > I guess if there's no camera left, the dialog box doesn't have much to\n> > > > do ;-) (though should stay open in case someone replugs the UVC camera)\n> > > \n> > > Yes saw your other reply, and yes it's done in patch 2nd.\n> > >\n> > > > > +\n> > > > > + /* Setup the QDialogButton Box */\n> > > > > + QDialogButtonBox *dialogButtonBox =\n\nAnd this can be called \"buttonBox\" or even just \"buttons\". There's no\nneed for the \"dialog\" prefix in the name as all these variables are\nwithin the scope of CamSelectDialog.\n\n> > > > > + new QDialogButtonBox(QDialogButtonBox::Ok |\n> > > > > + QDialogButtonBox::Cancel);\n> > > > > +\n> > > > > + connect(dialogButtonBox, &QDialogButtonBox::accepted,\n> > > > > + this, &QDialog::accept);\n> > > > > + connect(dialogButtonBox, &QDialogButtonBox::rejected,\n> > > > > + this, &QDialog::reject);\n> > > > > +\n> > > > > + /* Set the layout. */\n> > > > > + camSelectDialogLayout->addRow(\"Camera: \", cameraIdComboBox_);\n\nNo need for a space after ':'.\n\n> > > > > + camSelectDialogLayout->addWidget(dialogButtonBox);\n> > > > > + }\n> > > > > +\n> > > > > + ~CamSelectDialog() = default;\n> > > > > +\n> > > > > + std::string getCameraId()\n> > > > > + {\n> > > > > + return cameraIdComboBox_->currentText().toStdString();\n> > > > > + }\n> > > >\n> > > > While there's not a lot of implementation here, I think the code should\n> > > > be in a .cpp file. I'm not sure where the threshold is for how small a\n> > > > class can be to be inlined directly in the header, but I think this one\n> > > > can potentially expand, so probably deserves it's own implementation\n> > > > file.\n\nI was about to say the same, the implementation should move to\ncam_select_dialog.cpp.\n\n> > > Sure, I would wait on review for the other patches then move everything\n> > > to its .cpp.\n> > >\n> > > > > +\n> > > > > +private:\n> > > > > + libcamera::CameraManager *cm_;\n> > > > > +\n> > > > > + /* UI elements. */\n> > > > > + QComboBox *cameraIdComboBox_;\n> > > > > +};\n> > > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > > > > index 7433d647..758e2c94 100644\n> > > > > --- a/src/qcam/main_window.cpp\n> > > > > +++ b/src/qcam/main_window.cpp\n> > > > > @@ -19,7 +19,6 @@\n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > -#include \n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -30,6 +29,7 @@\n> > > > >\n> > > > > #include \"../cam/image.h\"\n> > > > >\n> > > > > +#include \"cam_select_dialog.h\"\n> > > > > #include \"dng_writer.h\"\n> > > > > #ifndef QT_NO_OPENGL\n> > > > > #include \"viewfinder_gl.h\"\n> > > > > @@ -290,24 +290,12 @@ void MainWindow::switchCamera(int index)\n> > > > >\n> > > > > std::string MainWindow::chooseCamera()\n> > > > > {\n> > > > > - QStringList cameras;\n> > > > > - bool result;\n> > > > > + camSelectDialog_ = new CamSelectDialog(cm_, this);\n> > > >\n> > > > Do we need to keep the dialog in a class variable? or could it be local\n> > > > and destroyed at the end of this function?\n> > > \n> > > Yes we need it as a class variable for camera hotplugging.\n> >\n> > This seems to recreate the CamSelectDialog box each time from scratch,\n> > even though we constructed it - and keep it updated with hotplug events.\n> >\n> > Is it possible to re-use it when it's created? or does accepting the\n> > dialog or closing it make it no longer possible to re-use?\n> >\n> > If we can re-use it - we should ... Otherwise - we probably shouldn't\n> > send hotplug events when it's been closed. ?\n> \n> I think we can re-use it. And I think the current way is causing a leak\n> :P.\n\nReusing the dialog would be best indeed. Also, why do you use a QPointer\n?\n\n> > > > > - /* If only one camera is available, use it automatically. */\n> > > > > - if (cm_->cameras().size() == 1)\n> > > > > - return cm_->cameras()[0]->id();\n> > > > > -\n> > > > > - /* Present a dialog box to pick a camera. */\n> > > > > - for (const std::shared_ptr &cam : cm_->cameras())\n> > > > > - cameras.append(QString::fromStdString(cam->id()));\n> > > > > -\n> > > > > - QString id = QInputDialog::getItem(this, \"Select Camera\",\n> > > > > - \"Camera:\", cameras, 0,\n> > > > > - false, &result);\n> > > > > - if (!result)\n> > > > > + if (camSelectDialog_->exec() == QDialog::Accepted)\n> > > > > + return camSelectDialog_->getCameraId();\n> > > > > + else\n> > > > > return std::string();\n> > > > > -\n> > > > > - return id.toStdString();\n> > > > > }\n> > > > >\n> > > > > int MainWindow::openCamera()\n> > > > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > > > > index fc70920f..6d80b5be 100644\n> > > > > --- a/src/qcam/main_window.h\n> > > > > +++ b/src/qcam/main_window.h\n> > > > > @@ -23,11 +23,13 @@\n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > >\n> > > > > #include \"../cam/stream_options.h\"\n> > > > >\n> > > > > +#include \"cam_select_dialog.h\"\n> > > > > #include \"viewfinder.h\"\n> > > > >\n> > > > > class QAction;\n> > > > > @@ -99,6 +101,8 @@ private:\n> > > > > QString title_;\n> > > > > QTimer titleTimer_;\n> > > > >\n> > > > > + QPointer camSelectDialog_;\n> > > > > +\n> > > > > /* Options */\n> > > > > const OptionsParser::Options &options_;\n> > > > >\n> > > > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > > > > index c46f4631..70615e92 100644\n> > > > > --- a/src/qcam/meson.build\n> > > > > +++ b/src/qcam/meson.build\n> > > > > @@ -26,6 +26,7 @@ qcam_sources = files([\n> > > > > ])\n> > > > >\n> > > > > qcam_moc_headers = files([\n> > > > > + 'cam_select_dialog.h',\n> > > > > 'main_window.h',\n> > > > > 'viewfinder_qt.h',\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 1A295C3272\n\tfor ;\n\tMon, 8 Aug 2022 20:38:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D2CE063327;\n\tMon, 8 Aug 2022 22:38:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F1F5A63315\n\tfor ;\n\tMon, 8 Aug 2022 22:38:32 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5F1D5481;\n\tMon, 8 Aug 2022 22:38:32 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659991114;\n\tbh=+kzWM0E01s9uUaqvQGwhiEcOLuCrbsuV9vnit/Iv2jc=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=efe0/S+RteeUNgAlHgBS5oybNJRZEOFNTtw7Xpa9uyXKoxK83zwaiDHUXrXUinVzs\n\tuERBBYZYGvY5NBE68hoeqsOtY3PF9YDnS2cBEyku/gNu1Roa/Xtqpm4oNJS7oI5Y2j\n\tTJaShuRTHEPN4yNjQ7GQJ59guTwsuRk9Ta94GfVxoDWo9jgJTLve80TaFhYnhrYSrc\n\tntAUMU0XFgPiiNhahETTFUTlL1CrV2idE2n7SE19CXfMKJSmG1NUzZySQOD37xLRrP\n\t8i/2hrCihDoVPAmvVMTX4sJlfO/qvSZjKzZwPIA4qmXlCbwOOrboi5prFwofduhYap\n\t9cuFYHYr4tuug==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659991112;\n\tbh=+kzWM0E01s9uUaqvQGwhiEcOLuCrbsuV9vnit/Iv2jc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=G44+5IXvUUXkhQB5ujcXJgR6NbrZgS4Lzy6FXEw/Dwwc8W/r/s/tBid8SQDPNetaN\n\tPtHqE9p4jLGMIO/nad/nhepGIa5yIGNkGAj+4sVf61IkO1TXqDjA5Yree0UvscAsGX\n\tRLs0pKuJvlOUkSceAdKlV16zW8aXgMi4OqTIli/s="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"G44+5IXv\"; dkim-atps=neutral","Date":"Mon, 8 Aug 2022 23:38:21 +0300","To":"Utkarsh Tiwari ","Message-ID":"","References":"<20220806190433.59128-1-utkarsh02t@gmail.com>\n\t<20220806190433.59128-2-utkarsh02t@gmail.com>\n\t<165991437612.1706285.3866162190018711245@Monstersaurus>\n\t\n\t<165994833230.15821.3357093096308907252@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for\n\tselection of cameras at startup","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]