[{"id":24417,"web_url":"https://patchwork.libcamera.org/comment/24417/","msgid":"<165994955498.15821.2804559049372976841@Monstersaurus>","date":"2022-08-08T09:05:54","subject":"Re: [libcamera-devel] [PATCH v2.1 4/4] qcam: CamSelectDialog:\n\tDisplay Location and Model propety of camera","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-07 16:41:38)\n> The camera selection dialog currently only displays the camera Id.\n> Display the camera location and camera model if available.\n> \n\n \\o/ This bit is great!\n\n> Signed-off-by: Utkarsh Tiwari \n> ---\n> Difference from v2:\n> Use getCameraId() instead of getCurrentCamera()\n> src/qcam/cam_select_dialog.h | 54 ++++++++++++++++++++++++++++++++++++\n> 1 file changed, 54 insertions(+)\n> \n> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> index ee65eb88..6ba61cff 100644\n> --- a/src/qcam/cam_select_dialog.h\n> +++ b/src/qcam/cam_select_dialog.h\n> @@ -12,11 +12,14 @@\n> \n> #include \n> #include \n> +#include \n> +#include \n> \n> #include \n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> class CamSelectDialog : public QDialog\n> @@ -34,6 +37,17 @@ public:\n> for (const auto &cam : cm_->cameras())\n> cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));\n> \n> + /* Set camera information labels. */\n> + cameraLocation_ = new QLabel;\n> + cameraModel_ = new QLabel;\n> +\n> + updateCamInfo(cm_->get(getCameraId()));\n> + connect(cameraIdComboBox_, &QComboBox::currentTextChanged,\n> + this, [&]() {\n> + updateCamInfo(cm_->get(\n> + cameraIdComboBox_->currentText().toStdString()));\n> + });\n> +\n> /* Setup the QDialogButton Box */\n> QDialogButtonBox *dialogButtonBox =\n> new QDialogButtonBox(QDialogButtonBox::Ok |\n> @@ -46,6 +60,8 @@ public:\n> \n> /* Set the layout. */\n> camSelectDialogLayout->addRow(\"Camera: \", cameraIdComboBox_);\n> + camSelectDialogLayout->addRow(\"Location: \", cameraLocation_);\n> + camSelectDialogLayout->addRow(\"Model: \", cameraModel_);\n> camSelectDialogLayout->addWidget(dialogButtonBox);\n> }\n> \n> @@ -70,9 +86,47 @@ public:\n> cameraIdComboBox_->removeItem(cameraIndex);\n> }\n> \n> + /* Camera Information */\n> + void updateCamInfo(const std::shared_ptr &camera)\n> + {\n> + if (camera == nullptr)\n> + return;\n> +\n> + const libcamera::ControlList &cameraProperties = camera->properties();\n> +\n> + const auto &location =\n> + cameraProperties.get(libcamera::properties::Location);\n> + if (location) {\n> + switch (*location) {\n> + case libcamera::properties::CameraLocationFront:\n> + cameraLocation_->setText(\"Internal front camera \");\n\nDo these need the space at the end? or could those be removed?\n\n\n> + break;\n> + case libcamera::properties::CameraLocationBack:\n> + cameraLocation_->setText(\"Internal back camera \");\n> + break;\n> + case libcamera::properties::CameraLocationExternal:\n> + cameraLocation_->setText(\"External camera \");\n> + break;\n> + default:\n> + cameraLocation_->setText(QString());\n> + }\n> + } else {\n> + cameraLocation_->setText(QString());\n> + }\n> +\n> + const auto &model = cameraProperties.get(libcamera::properties::Model);\n\nI think with the new 'optional' controls this could read something like\n(not tested/checked)\n\t\t const std::string model = cameraProperties\n\t\t \t\t\t\t.get(libcamera::properties::Model)\n\t\t\t\t\t\t.value_or(utils::defopt)\n\nTo save having to be conditional on the if (model) below...\n\nBut it all looks functional, and I've seen the output so:\n\n\nReviewed-by: Kieran Bingham \n\n\n> +\n> + if (model)\n> + cameraModel_->setText(QString::fromStdString(*model));\n> + else\n> + cameraModel_->setText(QString());\n> + }\n> +\n> private:\n> libcamera::CameraManager *cm_;\n> \n> /* UI elements. */\n> QComboBox *cameraIdComboBox_;\n> + QLabel *cameraLocation_;\n> + QLabel *cameraModel_;\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 55A06C3272\n\tfor ;\n\tMon, 8 Aug 2022 09:05:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79AA56332B;\n\tMon, 8 Aug 2022 11:05:58 +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 A71EF63326\n\tfor ;\n\tMon, 8 Aug 2022 11:05:57 +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 3D1A5481;\n\tMon, 8 Aug 2022 11:05:57 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659949558;\n\tbh=13IXBggUibbPunbwt5yCEjKfLbX9w/U3QNDLsP1uqHc=;\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=2uN0q5Lhnv7uHoqAOQxs38RlMfXVYZDwtL5iVlKX9WchA+0zxGFaVj7MTwBYqdaFa\n\tjocYimfIyLdVReRhLYHCnBmzwREv8f2gAiNO7NVUWpQ6VHN00XCnMZotf64ggrqqbt\n\t0J//c111c35cDh3cqxTzabfdMTHSTj3p7hno6OtGO3SWjOD4c57LjU7D30JMVVnT8X\n\tSOrfQEKCeIIZR+07dz9nxj2uQ81MLd3twyry9lVYYMIPxdtdvDsyCY9wzMHfpWSsXC\n\tgiRs2RI4cRgbyat2DukRnyIk4XIszB+3z1XR/eqKj7M1LaFeMKRCE0NgDlaK4L7occ\n\tVZajLO7daMETQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659949557;\n\tbh=13IXBggUibbPunbwt5yCEjKfLbX9w/U3QNDLsP1uqHc=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=aQwY0JZSnENs13yZuS1Xm1KGjmtbUzQu+HvwbSyHzKbxy6OUTw4qw6QOkM/6y1GvL\n\tTeDqan9LOOiYtLhNh9c/3j4bh9VG0cKuk4T8MXvkWkfE/aOXb349ml36efXqooOmuB\n\t541sVFz/akAbNPouTlduH0GMWLmFAp7Fe/ZFoG6w="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"aQwY0JZS\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220807154138.91960-1-utkarsh02t@gmail.com>","References":"<20220806190433.59128-5-utkarsh02t@gmail.com>\n\t<20220807154138.91960-1-utkarsh02t@gmail.com>","To":"Utkarsh Tiwari ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 08 Aug 2022 10:05:54 +0100","Message-ID":"<165994955498.15821.2804559049372976841@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2.1 4/4] qcam: CamSelectDialog:\n\tDisplay Location and Model propety of camera","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":24442,"web_url":"https://patchwork.libcamera.org/comment/24442/","msgid":"","date":"2022-08-08T20:54:44","subject":"Re: [libcamera-devel] [PATCH v2.1 4/4] qcam: CamSelectDialog:\n\tDisplay Location and Model propety of camera","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:05:54AM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-07 16:41:38)\n> > The camera selection dialog currently only displays the camera Id.\n> > Display the camera location and camera model if available.\n> > \n> \n> \\o/ This bit is great!\n> \n> > Signed-off-by: Utkarsh Tiwari \n> > ---\n> > Difference from v2:\n> > Use getCameraId() instead of getCurrentCamera()\n> > src/qcam/cam_select_dialog.h | 54 ++++++++++++++++++++++++++++++++++++\n> > 1 file changed, 54 insertions(+)\n> > \n> > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> > index ee65eb88..6ba61cff 100644\n> > --- a/src/qcam/cam_select_dialog.h\n> > +++ b/src/qcam/cam_select_dialog.h\n> > @@ -12,11 +12,14 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > +#include \n> > \n> > #include \n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > \n> > class CamSelectDialog : public QDialog\n> > @@ -34,6 +37,17 @@ public:\n> > for (const auto &cam : cm_->cameras())\n> > cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));\n> > \n> > + /* Set camera information labels. */\n> > + cameraLocation_ = new QLabel;\n> > + cameraModel_ = new QLabel;\n> > +\n> > + updateCamInfo(cm_->get(getCameraId()));\n\nThis would be best placed at the very end of the constructor, when the\ndialog layout is fully constructed.\n\n> > + connect(cameraIdComboBox_, &QComboBox::currentTextChanged,\n> > + this, [&]() {\n> > + updateCamInfo(cm_->get(\n> > + cameraIdComboBox_->currentText().toStdString()));\n> > + });\n\nCan you add a private member function instead of using a lambda function\n? That will be more efficient.\n\n> > +\n> > /* Setup the QDialogButton Box */\n> > QDialogButtonBox *dialogButtonBox =\n> > new QDialogButtonBox(QDialogButtonBox::Ok |\n> > @@ -46,6 +60,8 @@ public:\n> > \n> > /* Set the layout. */\n> > camSelectDialogLayout->addRow(\"Camera: \", cameraIdComboBox_);\n> > + camSelectDialogLayout->addRow(\"Location: \", cameraLocation_);\n> > + camSelectDialogLayout->addRow(\"Model: \", cameraModel_);\n> > camSelectDialogLayout->addWidget(dialogButtonBox);\n> > }\n> > \n> > @@ -70,9 +86,47 @@ public:\n> > cameraIdComboBox_->removeItem(cameraIndex);\n> > }\n> > \n> > + /* Camera Information */\n> > + void updateCamInfo(const std::shared_ptr &camera)\n> > + {\n> > + if (camera == nullptr)\n\n\t\tif (!camera)\n\n> > + return;\n> > +\n> > + const libcamera::ControlList &cameraProperties = camera->properties();\n> > +\n> > + const auto &location =\n> > + cameraProperties.get(libcamera::properties::Location);\n> > + if (location) {\n> > + switch (*location) {\n> > + case libcamera::properties::CameraLocationFront:\n> > + cameraLocation_->setText(\"Internal front camera \");\n> \n> Do these need the space at the end? or could those be removed?\n> \n> \n> > + break;\n> > + case libcamera::properties::CameraLocationBack:\n> > + cameraLocation_->setText(\"Internal back camera \");\n> > + break;\n> > + case libcamera::properties::CameraLocationExternal:\n> > + cameraLocation_->setText(\"External camera \");\n> > + break;\n> > + default:\n> > + cameraLocation_->setText(QString());\n\n\"Unknown\" may be better than an empty string.\n\n> > + }\n> > + } else {\n> > + cameraLocation_->setText(QString());\n\nSame here.\n\n> > + }\n> > +\n> > + const auto &model = cameraProperties.get(libcamera::properties::Model);\n> \n> I think with the new 'optional' controls this could read something like\n> (not tested/checked)\n> \t\t const std::string model = cameraProperties\n> \t\t \t\t\t\t.get(libcamera::properties::Model)\n> \t\t\t\t\t\t.value_or(utils::defopt)\n> \n> To save having to be conditional on the if (model) below...\n\nThat's nicer. It could also be writte .value_or(\"\") as constructing an\nempty string is cheap.\n\nSimilarly to the location, maybe \"Unknown\" would be better too ?\n\n> But it all looks functional, and I've seen the output so:\n> \n> Reviewed-by: Kieran Bingham \n> \n> > +\n> > + if (model)\n> > + cameraModel_->setText(QString::fromStdString(*model));\n> > + else\n> > + cameraModel_->setText(QString());\n> > + }\n> > +\n> > private:\n> > libcamera::CameraManager *cm_;\n> > \n> > /* UI elements. */\n> > QComboBox *cameraIdComboBox_;\n> > + QLabel *cameraLocation_;\n> > + QLabel *cameraModel_;\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 E9613C3272\n\tfor ;\n\tMon, 8 Aug 2022 20:54:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 49C996332B;\n\tMon, 8 Aug 2022 22:54:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 06FC263315\n\tfor ;\n\tMon, 8 Aug 2022 22:54:56 +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 60DB2481;\n\tMon, 8 Aug 2022 22:54:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659992097;\n\tbh=PhPHu/0jZ2Q5KP6fjPMIBdaFJVQiMCxPzaJHssE7FE4=;\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=gmJMHm6ULdibVRRqtavASTNBCSw2FpNo5qbu4LwUFKHntRcRP+IurIV6Ej2cAani5\n\tADnP23D6ZKB5lBC99EV9ujBNcngMBrIo4o/b6f2dliPv+HF0V8260v3XsAn9rNKapU\n\tiLT9fGhWQ/9bUgKUBkBoysxcOtjv4lgc8xdt3/XKL1N6qpSRa7G+UGERQ0gIbkM3eA\n\tag+uOPk/zzrpOoMIOd/e5NfMiZS7yWyX0/TTv/Bc6PtRaHD0Kc/j4JKCnO7VhywiZb\n\tEAKCTa2j8UM3Wv4qIire+T/rTCZV2gyldf/0BEHKp06ETpr/dOR1yN+geyS3cVH/ph\n\t3BkmpCiOMdfVQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659992095;\n\tbh=PhPHu/0jZ2Q5KP6fjPMIBdaFJVQiMCxPzaJHssE7FE4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jTon3jtl3g34i9nWQshhh2gqjHDMDgAVWT8M/OSCgIKZ4jGgks7iwghl/riV/leEy\n\tj0Rgr2Ou7id5RCumu4cZ58gaLIqOazILgPmGnF7xSub9h7Dub0P6JkqJ2Op3C3i7jd\n\tcOS52uTyXUFxfh9dE928vo6XwIvCTS0RIKwYx4LY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jTon3jtl\"; dkim-atps=neutral","Date":"Mon, 8 Aug 2022 23:54:44 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220806190433.59128-5-utkarsh02t@gmail.com>\n\t<20220807154138.91960-1-utkarsh02t@gmail.com>\n\t<165994955498.15821.2804559049372976841@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<165994955498.15821.2804559049372976841@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v2.1 4/4] qcam: CamSelectDialog:\n\tDisplay Location and Model propety of camera","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\" "}}]