[{"id":22326,"web_url":"https://patchwork.libcamera.org/comment/22326/","msgid":"<71f2b046-6302-7ee4-6706-fee31d205ee5@ideasonboard.com>","date":"2022-03-21T06:33:47","subject":"Re: [libcamera-devel] [RFC PATCH] Add CameraName to the MainWindow","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Utkarsh\n\nThank you for the patch.\n\nOn 3/19/22 19:55, Utkarsh Tiwari via libcamera-devel wrote:\n> Showing Camera Id on the main window is quite non-intuitive for the user\n>\n> Implementation: A duplicate function exists in cam/main.cpp . There must be a better way to do this instead of duplication of code\n> Signed-off-by: Utkarsh Tiwari \n> ---\n> src/qcam/main_window.cpp | 61 +++++++++++++++++++++++++++++++++-------\n> src/qcam/main_window.h | 2 +-\n> 2 files changed, 52 insertions(+), 11 deletions(-)\n>\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index dd0e51f5..35e737c7 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -12,8 +12,10 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> +\nspurious new line\n> #include \n> #include \n> #include \n> @@ -197,7 +199,7 @@ int MainWindow::createToolbars()\n> \t\tthis, &MainWindow::switchCamera);\n> \n> \tfor (const std::shared_ptr &cam : cm_->cameras())\n> -\t\tcameraCombo_->addItem(QString::fromStdString(cam->id()));\n> +\t\tcameraCombo_->addItem(QString::fromStdString(cameraName(cam.get())));\n> \n> \ttoolbar_->addWidget(cameraCombo_);\n> \n> @@ -287,6 +289,45 @@ void MainWindow::switchCamera(int index)\n> \tstartStopAction_->setChecked(true);\n> }\n> \n> +std::string MainWindow::cameraName(const libcamera::Camera *camera)\n> +{\n> +\tconst ControlList &props = camera->properties();\n> +\tbool addModel = true;\n> +\tstd::string name;\n> +\n> +\t/*\n> +\t * Construct the name from the camera location, model and ID. The model\n> +\t * is only used if the location isn't present or is set to External.\n> +\t */\n> +\tif (props.contains(properties::Location)) {\n> +\t\tswitch (props.get(properties::Location)) {\n> +\t\tcase properties::CameraLocationFront:\n> +\t\t\taddModel = false;\n> +\t\t\tname = \"Internal front camera \";\n> +\t\t\tbreak;\n> +\t\tcase properties::CameraLocationBack:\n> +\t\t\taddModel = false;\n> +\t\t\tname = \"Internal back camera \";\n> +\t\t\tbreak;\n> +\t\tcase properties::CameraLocationExternal:\n> +\t\t\tname = \"External camera \";\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (addModel && props.contains(properties::Model)) {\n> +\t\t/*\n> +\t\t * If the camera location is not availble use the camera model\n> +\t\t * to build the camera name.\n> +\t\t */\n> +\t\tname = \"'\" + props.get(properties::Model) + \"' \";\n> +\t}\n> +\n> +\tname += \"(\" + camera->id() + \")\";\n> +\n> +\treturn name;\n> +}\n> +\n\n\nI get it that this is copied over from cam/main.cpp but since we are \ntrying to keep the camera names construction in-sync across applications \n(cam and qcam), have you thought or discussed the possibility that this \nhelper can go in libcamera::Camera class itself ? For e.g. as\n\nCamera::name()\n\nOR\n\nCamera::displayName()\n\nas a helper provided by libcamera. Applications not happy with this \nhelper (given out by libcamera) can still construct/append their own \nquirks based on id(), location-property and so on.. as it is done right \nnow anyway.\n\nI don't know, this might require some discussion but would simplify \nthings a bit.\n\n> std::string MainWindow::chooseCamera()\n> {\n> \tQStringList cameras;\n> @@ -311,24 +352,24 @@ std::string MainWindow::chooseCamera()\n> \n> int MainWindow::openCamera()\n> {\n> -\tstd::string cameraName;\n> +\tstd::string cameraId;\n> \n> \t/*\n> \t * Use the camera specified on the command line, if any, or display the\n> \t * camera selection dialog box otherwise.\n> \t */\n> \tif (options_.isSet(OptCamera))\n> -\t\tcameraName = static_cast(options_[OptCamera]);\n> +\t\tcameraId = static_cast(options_[OptCamera]);\n> \telse\n> -\t\tcameraName = chooseCamera();\n> +\t\tcameraId = chooseCamera();\n> \n> -\tif (cameraName == \"\")\n> +\tif (cameraId == \"\")\n> \t\treturn -EINVAL;\n> \n> \t/* Get and acquire the camera. */\n> -\tcamera_ = cm_->get(cameraName);\n> +\tcamera_ = cm_->get(cameraId);\n> \tif (!camera_) {\n> -\t\tqInfo() << \"Camera\" << cameraName.c_str() << \"not found\";\n> +\t\tqInfo() << \"Camera\" << cameraId.c_str() << \"not found\";\n> \t\treturn -ENODEV;\n> \t}\n\n\nThis hunk is a variable rename so should be separated out in a different \npatch I think.\n\n> \n> @@ -339,7 +380,7 @@ int MainWindow::openCamera()\n> \t}\n> \n> \t/* Set the combo-box entry with the currently selected Camera. */\n> -\tcameraCombo_->setCurrentText(QString::fromStdString(cameraName));\n> +\tcameraCombo_->setCurrentText(QString::fromStdString(cameraName(camera_.get())));\n> \n> \treturn 0;\n> }\n> @@ -604,7 +645,7 @@ void MainWindow::processHotplug(HotplugEvent *e)\n> \tHotplugEvent::PlugEvent event = e->hotplugEvent();\n> \n> \tif (event == HotplugEvent::HotPlug) {\n> -\t\tcameraCombo_->addItem(QString::fromStdString(camera->id()));\n> +\t\tcameraCombo_->addItem(QString::fromStdString(cameraName(camera)));\n> \t} else if (event == HotplugEvent::HotUnplug) {\n> \t\t/* Check if the currently-streaming camera is removed. */\n> \t\tif (camera == camera_.get()) {\n> @@ -614,7 +655,7 @@ void MainWindow::processHotplug(HotplugEvent *e)\n> \t\t\tcameraCombo_->setCurrentIndex(0);\n> \t\t}\n> \n> -\t\tint camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));\n> +\t\tint camIndex = cameraCombo_->findText(QString::fromStdString(cameraName(camera)));\n> \t\tcameraCombo_->removeItem(camIndex);\n> \t}\n> }\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index 3fbe872c..b31c584f 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -70,7 +70,7 @@ private Q_SLOTS:\n> \n> private:\n> \tint createToolbars();\n> -\nSpurious deletion of newline. It should be retained\n> +\tstd::string cameraName(const libcamera::Camera *camera);\n> \tstd::string chooseCamera();\n> \tint openCamera();\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 BF7D7BD80A\n\tfor ;\n\tMon, 21 Mar 2022 06:33:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 98A40604DC;\n\tMon, 21 Mar 2022 07:33:56 +0100 (CET)","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 0A86560136\n\tfor ;\n\tMon, 21 Mar 2022 07:33:55 +0100 (CET)","from [192.168.1.106] (unknown [103.238.109.171])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 748EC291;\n\tMon, 21 Mar 2022 07:33:53 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647844436;\n\tbh=Q7mQeRuEZNJIL8paS/NmDAEtguIVMQBauwgRB3y3VQ8=;\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:\n\tFrom;\n\tb=2IQMeSpDsAHk7IoPLWnX1U8htuhMXM3lREvgOLpm6ToIKEQuloXrG+ruUxInkUTuX\n\tJbTHzTsuGnfmsrqVf3S7ROcWorWdha/VfeD19eFzBBQCa4G+UqV0eUDRsRld4F3LK/\n\t4MjMQKQI4BGhnvE8BVnZBnq2bNNgExFxmQDIe7hDsFxTLJAQNyezd1YRQYoKpbIfjT\n\tVJyXKOxx0US/G4ALjcfQO+XWBalATGZFJFzuL9XrOfl/GzfzBt3avPSvgzWRKKedE6\n\t7xu3mN9BYs2mb6LiVyV8ssnfOJ7Q0IA5Qt6/FDmaSTD/wB8WZdaeZniTo9ZmuptCuB\n\ttY8NsA2Qhs4AA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647844434;\n\tbh=Q7mQeRuEZNJIL8paS/NmDAEtguIVMQBauwgRB3y3VQ8=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=a2PY0JATjjjpGimizPJR41AUvgyZOQ8ab6Qayq8mqmtYjFLZxD4jx8OVLRw7Y9gRG\n\tEzdkHJomGjlEtuiVht7x6d12/J3G+/BSHUuCyMla3k1Tw60v0xXo+IT31yix68RP7T\n\tacG5rkJ/ngVBbKJUw3E0WJPO3si6FRN/8raKDvi8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"a2PY0JAT\"; dkim-atps=neutral","Message-ID":"<71f2b046-6302-7ee4-6706-fee31d205ee5@ideasonboard.com>","Date":"Mon, 21 Mar 2022 12:03:47 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Utkarsh Tiwari ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220319142545.40433-1-utkarsh02t@gmail.com>","In-Reply-To":"<20220319142545.40433-1-utkarsh02t@gmail.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC PATCH] Add CameraName to the MainWindow","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22335,"web_url":"https://patchwork.libcamera.org/comment/22335/","msgid":"<164786785673.1484799.15677927061388446928@Monstersaurus>","date":"2022-03-21T13:04:16","subject":"Re: [libcamera-devel] [RFC PATCH] Add CameraName to the MainWindow","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain via libcamera-devel (2022-03-21 06:33:47)\n> Hi Utkarsh\n> \n> Thank you for the patch.\n> \n> On 3/19/22 19:55, Utkarsh Tiwari via libcamera-devel wrote:\n> > Showing Camera Id on the main window is quite non-intuitive for the user\n> >\n> > Implementation: A duplicate function exists in cam/main.cpp . There must be a better way to do this instead of duplication of code\n\nA cleaner commit message could look something like:\n\n\"\"\"\nqcam: Display a human readable name in camera selection\n\nShowing the Camera ID on the main window and when selecting a camera is\nnot very intuitive for the user.\n\nCamera IDs are unique references for the camera and include internal\ndetails that do not present an easy indicator for which camera it\nrepresents.\n\nConstruct a Camera Name based on the camera properties by duplicating\nthe existing naming scheme from cam to present more understandable\ndevice name to users of Qcam.\n\"\"\"\n\nWe try to make sure the commit title clearly references which part of\nlibcamera is being changed, (in this case 'qcam:'), and a commit message\nshould try to clearly state the problem it's solving, and how it solves\nit.\n\n\n> > Signed-off-by: Utkarsh Tiwari \n> > ---\n> > src/qcam/main_window.cpp | 61 +++++++++++++++++++++++++++++++++-------\n> > src/qcam/main_window.h | 2 +-\n> > 2 files changed, 52 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index dd0e51f5..35e737c7 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -12,8 +12,10 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > #include \n> > \n> > +\n> spurious new line\n> > #include \n> > #include \n> > #include \n> > @@ -197,7 +199,7 @@ int MainWindow::createToolbars()\n> > this, &MainWindow::switchCamera);\n> > \n> > for (const std::shared_ptr &cam : cm_->cameras())\n> > - cameraCombo_->addItem(QString::fromStdString(cam->id()));\n> > + cameraCombo_->addItem(QString::fromStdString(cameraName(cam.get())));\n\nI think if MainWindow::cameraName() took a shared_ptr:\n\t\n std::string MainWindow::cameraName(const std::shared_ptr &camera)\n\nthen this doesn't have to '.get()' the ptr, it would just pass 'cam',\nand the cameraName function wouldn't have to otherwise change.\n\n\n> > \n> > toolbar_->addWidget(cameraCombo_);\n> > \n> > @@ -287,6 +289,45 @@ void MainWindow::switchCamera(int index)\n> > startStopAction_->setChecked(true);\n> > }\n> > \n> > +std::string MainWindow::cameraName(const libcamera::Camera *camera)\n> > +{\n> > + const ControlList &props = camera->properties();\n> > + bool addModel = true;\n> > + std::string name;\n> > +\n> > + /*\n> > + * Construct the name from the camera location, model and ID. The model\n> > + * is only used if the location isn't present or is set to External.\n> > + */\n> > + if (props.contains(properties::Location)) {\n> > + switch (props.get(properties::Location)) {\n> > + case properties::CameraLocationFront:\n> > + addModel = false;\n> > + name = \"Internal front camera \";\n> > + break;\n> > + case properties::CameraLocationBack:\n> > + addModel = false;\n> > + name = \"Internal back camera \";\n> > + break;\n> > + case properties::CameraLocationExternal:\n> > + name = \"External camera \";\n> > + break;\n> > + }\n> > + }\n> > +\n> > + if (addModel && props.contains(properties::Model)) {\n> > + /*\n> > + * If the camera location is not availble use the camera model\n> > + * to build the camera name.\n> > + */\n> > + name = \"'\" + props.get(properties::Model) + \"' \";\n> > + }\n> > +\n> > + name += \"(\" + camera->id() + \")\";\n> > +\n> > + return name;\n> > +}\n> > +\n> \n> \n> I get it that this is copied over from cam/main.cpp but since we are \n> trying to keep the camera names construction in-sync across applications \n> (cam and qcam), have you thought or discussed the possibility that this \n> helper can go in libcamera::Camera class itself ? For e.g. as\n> \n> Camera::name()\n> \n> OR\n> \n> Camera::displayName()\n> \n> as a helper provided by libcamera. Applications not happy with this \n> helper (given out by libcamera) can still construct/append their own \n> quirks based on id(), location-property and so on.. as it is done right \n> now anyway.\n> \n> I don't know, this might require some discussion but would simplify \n> things a bit.\n\nDefining how to name a camera is supposed to be up to the application,\nnot libcamera, and there has indeed been some previous discussions about\nthis.\n\nI wonder if providing a helper that does what we expect is useful\nthough, and doesn't prevent applications from deciding to copy/or change\nthat themselves as long as all the properties used are public anyway.\n\nBut I think that means that duplicating the code is acceptable for now.\n\n\n> > std::string MainWindow::chooseCamera()\n> > {\n> > QStringList cameras;\n> > @@ -311,24 +352,24 @@ std::string MainWindow::chooseCamera()\n> > \n> > int MainWindow::openCamera()\n> > {\n> > - std::string cameraName;\n> > + std::string cameraId;\n> > \n> > /*\n> > * Use the camera specified on the command line, if any, or display the\n> > * camera selection dialog box otherwise.\n> > */\n> > if (options_.isSet(OptCamera))\n> > - cameraName = static_cast(options_[OptCamera]);\n> > + cameraId = static_cast(options_[OptCamera]);\n> > else\n> > - cameraName = chooseCamera();\n> > + cameraId = chooseCamera();\n> > \n> > - if (cameraName == \"\")\n> > + if (cameraId == \"\")\n> > return -EINVAL;\n> > \n> > /* Get and acquire the camera. */\n> > - camera_ = cm_->get(cameraName);\n> > + camera_ = cm_->get(cameraId);\n> > if (!camera_) {\n> > - qInfo() << \"Camera\" << cameraName.c_str() << \"not found\";\n> > + qInfo() << \"Camera\" << cameraId.c_str() << \"not found\";\n> > return -ENODEV;\n> > }\n> \n> \n> This hunk is a variable rename so should be separated out in a different \n> patch I think.\n\nMaybe not essential, but certainly would be possible and cleaner I think\nto have this rename done as a first patch.\n\n> \n> > \n> > @@ -339,7 +380,7 @@ int MainWindow::openCamera()\n> > }\n> > \n> > /* Set the combo-box entry with the currently selected Camera. */\n> > - cameraCombo_->setCurrentText(QString::fromStdString(cameraName));\n> > + cameraCombo_->setCurrentText(QString::fromStdString(cameraName(camera_.get())));\n> > \n> > return 0;\n> > }\n> > @@ -604,7 +645,7 @@ void MainWindow::processHotplug(HotplugEvent *e)\n> > HotplugEvent::PlugEvent event = e->hotplugEvent();\n> > \n> > if (event == HotplugEvent::HotPlug) {\n> > - cameraCombo_->addItem(QString::fromStdString(camera->id()));\n> > + cameraCombo_->addItem(QString::fromStdString(cameraName(camera)));\n> > } else if (event == HotplugEvent::HotUnplug) {\n> > /* Check if the currently-streaming camera is removed. */\n> > if (camera == camera_.get()) {\n> > @@ -614,7 +655,7 @@ void MainWindow::processHotplug(HotplugEvent *e)\n> > cameraCombo_->setCurrentIndex(0);\n> > }\n> > \n> > - int camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));\n> > + int camIndex = cameraCombo_->findText(QString::fromStdString(cameraName(camera)));\n> > cameraCombo_->removeItem(camIndex);\n> > }\n> > }\n> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > index 3fbe872c..b31c584f 100644\n> > --- a/src/qcam/main_window.h\n> > +++ b/src/qcam/main_window.h\n> > @@ -70,7 +70,7 @@ private Q_SLOTS:\n> > \n> > private:\n> > int createToolbars();\n> > -\n> Spurious deletion of newline. It should be retained\n> > + std::string cameraName(const libcamera::Camera *camera);\n> > std::string chooseCamera();\n> > int openCamera();\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 C9431BD80A\n\tfor ;\n\tMon, 21 Mar 2022 13:04:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 08919604E7;\n\tMon, 21 Mar 2022 14:04:21 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5568F604C6\n\tfor ;\n\tMon, 21 Mar 2022 14:04:20 +0100 (CET)","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 DBD9A291;\n\tMon, 21 Mar 2022 14:04:19 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647867861;\n\tbh=t++24N7pTqXNIkPrVmreCk29DPCnVeKzqw7L9A27t1M=;\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=c4nqcKU3tN/MREgNcZwx+2fgOxrYfSrQ9iBGBRetC0jEmCmf74LjBga6wCcwlXGPv\n\tN0OYPuwqQr1kvyYi/PWq9t0OZjZinXgM7JG8jRuXdgqmeNKQEfy4/sny6TCyEzvNl/\n\tmwM9c65IYiLryxtI4b+qu7Wytld8wB2a2HoqF2IOSn1at3ZlrcFGkUEVL3Mgwr9kBz\n\t9U1l5dxLfpwa1UM+gHDh4z/laY4JribDcid8QtFqms/ebG+0S9M582YyRjA1GTlyQ2\n\t25cocR+w2DZjVeYwbXjw/PQN+N7SrnSb67PpvqEhjq3V7tb+h3L4QfLqWV1h4ANHss\n\tP13mq7Tgqm/kQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647867859;\n\tbh=t++24N7pTqXNIkPrVmreCk29DPCnVeKzqw7L9A27t1M=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=wesRCmNUF7GYCPhnMJKuVf+jX+QCLZntWOBONW2y41C3oODzXeWPTmrHQXQWGcq6N\n\tdIo+PoX1JsDwJVpiA83G16SSc7FQu/y+2G9X7b94ykyyPDyGvloYgURsRMe0l2+Q/k\n\tg1Y2GB9/mFerjrtgCuB+1eYjEQPplZqFEdDLqc4A="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wesRCmNU\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<71f2b046-6302-7ee4-6706-fee31d205ee5@ideasonboard.com>","References":"<20220319142545.40433-1-utkarsh02t@gmail.com>\n\t<71f2b046-6302-7ee4-6706-fee31d205ee5@ideasonboard.com>","To":"Umang Jain ,\n\tUtkarsh Tiwari ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 21 Mar 2022 13:04:16 +0000","Message-ID":"<164786785673.1484799.15677927061388446928@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [RFC PATCH] Add CameraName to the MainWindow","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":22347,"web_url":"https://patchwork.libcamera.org/comment/22347/","msgid":"<07eeddb0-1add-0170-e707-c3ee9e90d508@ideasonboard.com>","date":"2022-03-21T14:23:15","subject":"Re: [libcamera-devel] [RFC PATCH] Add CameraName to the MainWindow","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 3/21/22 18:34, Kieran Bingham wrote:\n> Quoting Umang Jain via libcamera-devel (2022-03-21 06:33:47)\n>> Hi Utkarsh\n>>\n>> Thank you for the patch.\n>>\n>> On 3/19/22 19:55, Utkarsh Tiwari via libcamera-devel wrote:\n>>> Showing Camera Id on the main window is quite non-intuitive for the user\n>>>\n>>> Implementation: A duplicate function exists in cam/main.cpp . There must be a better way to do this instead of duplication of code\n> A cleaner commit message could look something like:\n>\n> \"\"\"\n> qcam: Display a human readable name in camera selection\n>\n> Showing the Camera ID on the main window and when selecting a camera is\n> not very intuitive for the user.\n>\n> Camera IDs are unique references for the camera and include internal\n> details that do not present an easy indicator for which camera it\n> represents.\n>\n> Construct a Camera Name based on the camera properties by duplicating\n> the existing naming scheme from cam to present more understandable\n> device name to users of Qcam.\n> \"\"\"\n>\n> We try to make sure the commit title clearly references which part of\n> libcamera is being changed, (in this case 'qcam:'), and a commit message\n> should try to clearly state the problem it's solving, and how it solves\n> it.\n>\n>\n>>> Signed-off-by: Utkarsh Tiwari \n>>> ---\n>>> src/qcam/main_window.cpp | 61 +++++++++++++++++++++++++++++++++-------\n>>> src/qcam/main_window.h | 2 +-\n>>> 2 files changed, 52 insertions(+), 11 deletions(-)\n>>>\n>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n>>> index dd0e51f5..35e737c7 100644\n>>> --- a/src/qcam/main_window.cpp\n>>> +++ b/src/qcam/main_window.cpp\n>>> @@ -12,8 +12,10 @@\n>>> #include \n>>> \n>>> #include \n>>> +#include \n>>> #include \n>>> \n>>> +\n>> spurious new line\n>>> #include \n>>> #include \n>>> #include \n>>> @@ -197,7 +199,7 @@ int MainWindow::createToolbars()\n>>> this, &MainWindow::switchCamera);\n>>> \n>>> for (const std::shared_ptr &cam : cm_->cameras())\n>>> - cameraCombo_->addItem(QString::fromStdString(cam->id()));\n>>> + cameraCombo_->addItem(QString::fromStdString(cameraName(cam.get())));\n> I think if MainWindow::cameraName() took a shared_ptr:\n> \t\n> std::string MainWindow::cameraName(const std::shared_ptr &camera)\n>\n> then this doesn't have to '.get()' the ptr, it would just pass 'cam',\n> and the cameraName function wouldn't have to otherwise change.\n>\n>\n>>> \n>>> toolbar_->addWidget(cameraCombo_);\n>>> \n>>> @@ -287,6 +289,45 @@ void MainWindow::switchCamera(int index)\n>>> startStopAction_->setChecked(true);\n>>> }\n>>> \n>>> +std::string MainWindow::cameraName(const libcamera::Camera *camera)\n>>> +{\n>>> + const ControlList &props = camera->properties();\n>>> + bool addModel = true;\n>>> + std::string name;\n>>> +\n>>> + /*\n>>> + * Construct the name from the camera location, model and ID. The model\n>>> + * is only used if the location isn't present or is set to External.\n>>> + */\n>>> + if (props.contains(properties::Location)) {\n>>> + switch (props.get(properties::Location)) {\n>>> + case properties::CameraLocationFront:\n>>> + addModel = false;\n>>> + name = \"Internal front camera \";\n>>> + break;\n>>> + case properties::CameraLocationBack:\n>>> + addModel = false;\n>>> + name = \"Internal back camera \";\n>>> + break;\n>>> + case properties::CameraLocationExternal:\n>>> + name = \"External camera \";\n>>> + break;\n>>> + }\n>>> + }\n>>> +\n>>> + if (addModel && props.contains(properties::Model)) {\n>>> + /*\n>>> + * If the camera location is not availble use the camera model\n>>> + * to build the camera name.\n>>> + */\n>>> + name = \"'\" + props.get(properties::Model) + \"' \";\n>>> + }\n>>> +\n>>> + name += \"(\" + camera->id() + \")\";\n>>> +\n>>> + return name;\n>>> +}\n>>> +\n>>\n>> I get it that this is copied over from cam/main.cpp but since we are\n>> trying to keep the camera names construction in-sync across applications\n>> (cam and qcam), have you thought or discussed the possibility that this\n>> helper can go in libcamera::Camera class itself ? For e.g. as\n>>\n>> Camera::name()\n>>\n>> OR\n>>\n>> Camera::displayName()\n>>\n>> as a helper provided by libcamera. Applications not happy with this\n>> helper (given out by libcamera) can still construct/append their own\n>> quirks based on id(), location-property and so on.. as it is done right\n>> now anyway.\n>>\n>> I don't know, this might require some discussion but would simplify\n>> things a bit.\n> Defining how to name a camera is supposed to be up to the application,\n> not libcamera, and there has indeed been some previous discussions about\n> this.\n\n\nThe previous discussions were around a uniquely-defined camera-id \ninstead of camera-name as far as I remember.\n\n>\n> I wonder if providing a helper that does what we expect is useful\n> though, and doesn't prevent applications from deciding to copy/or change\n> that themselves as long as all the properties used are public anyway.\n\n\nPrecisely. Applications are free to append/structure the camera-name as \ntheir will - but a helper from libcamera might help as a base reference \non what we might think as the best option.\n\n>\n> But I think that means that duplicating the code is acceptable for now.\n>\n>\n>>> std::string MainWindow::chooseCamera()\n>>> {\n>>> QStringList cameras;\n>>> @@ -311,24 +352,24 @@ std::string MainWindow::chooseCamera()\n>>> \n>>> int MainWindow::openCamera()\n>>> {\n>>> - std::string cameraName;\n>>> + std::string cameraId;\n>>> \n>>> /*\n>>> * Use the camera specified on the command line, if any, or display the\n>>> * camera selection dialog box otherwise.\n>>> */\n>>> if (options_.isSet(OptCamera))\n>>> - cameraName = static_cast(options_[OptCamera]);\n>>> + cameraId = static_cast(options_[OptCamera]);\n>>> else\n>>> - cameraName = chooseCamera();\n>>> + cameraId = chooseCamera();\n>>> \n>>> - if (cameraName == \"\")\n>>> + if (cameraId == \"\")\n>>> return -EINVAL;\n>>> \n>>> /* Get and acquire the camera. */\n>>> - camera_ = cm_->get(cameraName);\n>>> + camera_ = cm_->get(cameraId);\n>>> if (!camera_) {\n>>> - qInfo() << \"Camera\" << cameraName.c_str() << \"not found\";\n>>> + qInfo() << \"Camera\" << cameraId.c_str() << \"not found\";\n>>> return -ENODEV;\n>>> }\n>>\n>> This hunk is a variable rename so should be separated out in a different\n>> patch I think.\n> Maybe not essential, but certainly would be possible and cleaner I think\n> to have this rename done as a first patch.\n>\n>>> \n>>> @@ -339,7 +380,7 @@ int MainWindow::openCamera()\n>>> }\n>>> \n>>> /* Set the combo-box entry with the currently selected Camera. */\n>>> - cameraCombo_->setCurrentText(QString::fromStdString(cameraName));\n>>> + cameraCombo_->setCurrentText(QString::fromStdString(cameraName(camera_.get())));\n>>> \n>>> return 0;\n>>> }\n>>> @@ -604,7 +645,7 @@ void MainWindow::processHotplug(HotplugEvent *e)\n>>> HotplugEvent::PlugEvent event = e->hotplugEvent();\n>>> \n>>> if (event == HotplugEvent::HotPlug) {\n>>> - cameraCombo_->addItem(QString::fromStdString(camera->id()));\n>>> + cameraCombo_->addItem(QString::fromStdString(cameraName(camera)));\n>>> } else if (event == HotplugEvent::HotUnplug) {\n>>> /* Check if the currently-streaming camera is removed. */\n>>> if (camera == camera_.get()) {\n>>> @@ -614,7 +655,7 @@ void MainWindow::processHotplug(HotplugEvent *e)\n>>> cameraCombo_->setCurrentIndex(0);\n>>> }\n>>> \n>>> - int camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));\n>>> + int camIndex = cameraCombo_->findText(QString::fromStdString(cameraName(camera)));\n>>> cameraCombo_->removeItem(camIndex);\n>>> }\n>>> }\n>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n>>> index 3fbe872c..b31c584f 100644\n>>> --- a/src/qcam/main_window.h\n>>> +++ b/src/qcam/main_window.h\n>>> @@ -70,7 +70,7 @@ private Q_SLOTS:\n>>> \n>>> private:\n>>> int createToolbars();\n>>> -\n>> Spurious deletion of newline. It should be retained\n>>> + std::string cameraName(const libcamera::Camera *camera);\n>>> std::string chooseCamera();\n>>> int openCamera();\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 E9921C0F1B\n\tfor ;\n\tMon, 21 Mar 2022 14:23:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 59B6D604DC;\n\tMon, 21 Mar 2022 15:23:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C7898604C6\n\tfor ;\n\tMon, 21 Mar 2022 15:23:22 +0100 (CET)","from [192.168.1.106] (unknown [103.238.109.171])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1AEB6291;\n\tMon, 21 Mar 2022 15:23:20 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647872604;\n\tbh=WIivCua+8roSbCo44PwgYHlFnf/XPefuFnJJTmuWOJg=;\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:\n\tFrom;\n\tb=zJna6B3gmrIvsDwWjQ4SOZ1TZifzfDOe3uwbtp0R8VMtjTz9aHprqijy3EDC9r4M1\n\tse/YZ79hZ5f4C/6No0aw9csOylk/AabtIu9lFcii8B6OWSL1u9le3b6VuFUL/BhgTi\n\tdz4rIFEZWzbb09vYZ4Z9hk+uwqhaALkDYVsc5wx/zPnJMhRQKLcMtE1seDHRkxPjpx\n\tgeuJ8Rec8HE5qUlq0HiXac77sIJm11SAftBwm2VspXZ/dJZolrv606E4KfG2C+I7Nw\n\tYHTpSQwiY+cR9ID6gYwqofZW+eezF4XB//K9k6lIgncRAgfF0pUygKGaBCTvd+oM5/\n\tsc1wnQulW/cpw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647872602;\n\tbh=WIivCua+8roSbCo44PwgYHlFnf/XPefuFnJJTmuWOJg=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=oa6lv9vNME7ipdtqQIuLMO0jioTy5gBGqpY38ANYe7ZeVJ4ggYnZxaOIkMFkSj9ck\n\tKgx5JPnc7zspCqjYv24O5fXNqNPtabuuI0C8I8oZWAasae6OvRWHfKclgqEjBlicmU\n\t0XybFKhzLWFAgl3+08ZBckMfcS5PTmcWu0DaQG8E="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"oa6lv9vN\"; dkim-atps=neutral","Message-ID":"<07eeddb0-1add-0170-e707-c3ee9e90d508@ideasonboard.com>","Date":"Mon, 21 Mar 2022 19:53:15 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tUtkarsh Tiwari ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220319142545.40433-1-utkarsh02t@gmail.com>\n\t<71f2b046-6302-7ee4-6706-fee31d205ee5@ideasonboard.com>\n\t<164786785673.1484799.15677927061388446928@Monstersaurus>","In-Reply-To":"<164786785673.1484799.15677927061388446928@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC PATCH] Add CameraName to the MainWindow","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]