[{"id":24620,"web_url":"https://patchwork.libcamera.org/comment/24620/","msgid":"<166073150764.3403653.9049967065052307094@Monstersaurus>","date":"2022-08-17T10:18:27","subject":"Re: [libcamera-devel] [PATCH v8 2/8] qcam: Support Hotplug for\n\tCamera Selection Dialog","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-10 16:03:43)\n> Currently if there is HotPlug event when the user is on the Camera\n> selection dialog, the QComboBox didn't update to reflect the change.\n> \n> If the QDialog exists then alert it for the Hotplug event. The check\n> for QDialog existance is done by QPointer.\n> \n> Signed-off-by: Utkarsh Tiwari \n> ---\n> Difference from v7:\n> 1. Nothing\n> src/qcam/cam_select_dialog.cpp | 14 ++++++++++++++\n> src/qcam/cam_select_dialog.h | 4 ++++\n> src/qcam/main_window.cpp | 4 ++++\n> 3 files changed, 22 insertions(+)\n> \n> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp\n> index dceaa590..d8982800 100644\n> --- a/src/qcam/cam_select_dialog.cpp\n> +++ b/src/qcam/cam_select_dialog.cpp\n> @@ -49,3 +49,17 @@ std::string CameraSelectorDialog::getCameraId()\n> {\n> return cameraIdComboBox_->currentText().toStdString();\n> }\n> +\n> +/* Hotplug / Unplug Support. */\n> +void CameraSelectorDialog::cameraAdded(libcamera::Camera *camera)\n> +{\n> + cameraIdComboBox_->addItem(QString::fromStdString(camera->id()));\n> +}\n> +\n> +void CameraSelectorDialog::cameraRemoved(libcamera::Camera *camera)\n> +{\n> + int cameraIndex = cameraIdComboBox_->findText(\n> + QString::fromStdString(camera->id()));\n> +\n> + cameraIdComboBox_->removeItem(cameraIndex);\n> +}\n> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> index 5544f49a..04c71fd8 100644\n> --- a/src/qcam/cam_select_dialog.h\n> +++ b/src/qcam/cam_select_dialog.h\n> @@ -26,6 +26,10 @@ public:\n> \n> std::string getCameraId();\n> \n> + /* Hotplug / Unplug Support. */\n> + void cameraAdded(libcamera::Camera *camera);\n> +\n> + void cameraRemoved(libcamera::Camera *camera);\n\nTrivial nits. I'd group these two together, and leave a blank line after\nto keep the private section separated.\n\n\"\"\"\n\tgetCameraId\n\n\t/* ... */\n\tcameraAdded\n\tcameraRemoved\n\nprivate:\n\tCameraManager\n\"\"\"\n\n\nReviewed-by: Kieran Bingham \n\n> private:\n> libcamera::CameraManager *cm_;\n> \n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index e794221a..9ec94708 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -594,6 +594,8 @@ void MainWindow::processHotplug(HotplugEvent *e)\n> \n> if (event == HotplugEvent::HotPlug) {\n> cameraCombo_->addItem(QString::fromStdString(camera->id()));\n> +\n> + cameraSelectorDialog_->cameraAdded(camera);\n> } else if (event == HotplugEvent::HotUnplug) {\n> /* Check if the currently-streaming camera is removed. */\n> if (camera == camera_.get()) {\n> @@ -605,6 +607,8 @@ void MainWindow::processHotplug(HotplugEvent *e)\n> \n> int camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));\n> cameraCombo_->removeItem(camIndex);\n> +\n> + cameraSelectorDialog_->cameraRemoved(camera);\n> }\n> }\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 9E6E6C3272\n\tfor ;\n\tWed, 17 Aug 2022 10:18:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D715761FC0;\n\tWed, 17 Aug 2022 12:18:32 +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 A7CAE61FA8\n\tfor ;\n\tWed, 17 Aug 2022 12:18:30 +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 335D249C;\n\tWed, 17 Aug 2022 12:18:30 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660731512;\n\tbh=o70oQB3MtoFe7CBy/8IU9Ir0JzLhIR7JjQonpVhsRA0=;\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=s1Fhc2vzwZBP8kF9Df1z9UolIAsXpsSoxj40MLKfdjZVT2QgB3/OsNyVcAOtZoCiG\n\t3y0BMwWQJKvc/oQgz6Kz339XsNVCUk/95XRdF+EbgDPbgJRQDmpaX6zUhHWUdiwD48\n\t4Udg33AV7kgWLmqq1rTJN1vkow56TdJYuXTLT4gnVSHveQvPcK0vjLuZkBDfSrhLGN\n\tyDvBQt3zt0WCzzzGQC3WOwRYPJ1o80phe0vV6HRCqw/6iqFnDBBnWqwoGkqMa1kY8W\n\tU6fa+pSOSXiO0iJDfDQGEK1dz3QT3TkWLLCnWVVnidMCNSYdexIZ4CrVKoP4HKmQ8o\n\tn6LqyqmWTRWfg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660731510;\n\tbh=o70oQB3MtoFe7CBy/8IU9Ir0JzLhIR7JjQonpVhsRA0=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=X4kJx3LFoPh8++FPlLibo8WRRFNc0h00GOZkT/Fa2yde4HG/el2tum14Id/DlEpLa\n\tL43ddwk+CaB06xOgm67firyRmFY+8tzASndmYByAXxoXUsbPlaSegB33r0nTyY9h7X\n\t47Vri9j9YAHgYPPn4Rc5qEgyxIfDGx4xPRHFLPr8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"X4kJx3LF\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220810150349.414043-3-utkarsh02t@gmail.com>","References":"<20220810150349.414043-1-utkarsh02t@gmail.com>\n\t<20220810150349.414043-3-utkarsh02t@gmail.com>","To":"Utkarsh Tiwari ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 17 Aug 2022 11:18:27 +0100","Message-ID":"<166073150764.3403653.9049967065052307094@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v8 2/8] qcam: Support Hotplug for\n\tCamera Selection Dialog","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":24739,"web_url":"https://patchwork.libcamera.org/comment/24739/","msgid":"","date":"2022-08-23T01:47:59","subject":"Re: [libcamera-devel] [PATCH v8 2/8] qcam: Support Hotplug for\n\tCamera Selection Dialog","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Utkarsh,\n\nThank you for the patch.\n\nOn Wed, Aug 10, 2022 at 08:33:43PM +0530, Utkarsh Tiwari via libcamera-devel wrote:\n> Currently if there is HotPlug event when the user is on the Camera\n> selection dialog, the QComboBox didn't update to reflect the change.\n\ns/didn't/doesn't/\n\n> If the QDialog exists then alert it for the Hotplug event. The check\n> for QDialog existance is done by QPointer.\n\ns/existance/existence/\n\nBut I think you need to rework this paragraph as there's no QPointer\nanymore.\n\n> Signed-off-by: Utkarsh Tiwari \n> ---\n> Difference from v7:\n> \t1. Nothing\n> src/qcam/cam_select_dialog.cpp | 14 ++++++++++++++\n> src/qcam/cam_select_dialog.h | 4 ++++\n> src/qcam/main_window.cpp | 4 ++++\n> 3 files changed, 22 insertions(+)\n> \n> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp\n> index dceaa590..d8982800 100644\n> --- a/src/qcam/cam_select_dialog.cpp\n> +++ b/src/qcam/cam_select_dialog.cpp\n> @@ -49,3 +49,17 @@ std::string CameraSelectorDialog::getCameraId()\n> {\n> \treturn cameraIdComboBox_->currentText().toStdString();\n> }\n> +\n> +/* Hotplug / Unplug Support. */\n> +void CameraSelectorDialog::cameraAdded(libcamera::Camera *camera)\n> +{\n> +\tcameraIdComboBox_->addItem(QString::fromStdString(camera->id()));\n> +}\n> +\n> +void CameraSelectorDialog::cameraRemoved(libcamera::Camera *camera)\n> +{\n> +\tint cameraIndex = cameraIdComboBox_->findText(\n> +\t\tQString::fromStdString(camera->id()));\n> +\n> +\tcameraIdComboBox_->removeItem(cameraIndex);\n> +}\n\nI think you can optimize this by passing a Qstring cameraId to both\nfunctions, as the caller already has to construct a QString.\n\n> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> index 5544f49a..04c71fd8 100644\n> --- a/src/qcam/cam_select_dialog.h\n> +++ b/src/qcam/cam_select_dialog.h\n> @@ -26,6 +26,10 @@ public:\n> \n> \tstd::string getCameraId();\n> \n> +\t/* Hotplug / Unplug Support. */\n> +\tvoid cameraAdded(libcamera::Camera *camera);\n> +\n> +\tvoid cameraRemoved(libcamera::Camera *camera);\n\nI would name the two fucntions addCamera() and removeCamera().\n\n> private:\n> \tlibcamera::CameraManager *cm_;\n> \n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index e794221a..9ec94708 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -594,6 +594,8 @@ void MainWindow::processHotplug(HotplugEvent *e)\n> \n\nSo here you could do\n\n\tQString cameraId = QString::fromStdString(camera->id());\n\n> \tif (event == HotplugEvent::HotPlug) {\n> \t\tcameraCombo_->addItem(QString::fromStdString(camera->id()));\n> +\n> +\t\tcameraSelectorDialog_->cameraAdded(camera);\n\nand use cameraId for both functions here.\n\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> @@ -605,6 +607,8 @@ void MainWindow::processHotplug(HotplugEvent *e)\n> \n> \t\tint camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));\n> \t\tcameraCombo_->removeItem(camIndex);\n> +\n> +\t\tcameraSelectorDialog_->cameraRemoved(camera);\n\nAnd here too.\n\nWith that, and Kieran's comment addressed,\n\nReviewed-by: Laurent Pinchart \n\n> \t}\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 ED0F4C3272\n\tfor ;\n\tTue, 23 Aug 2022 01:48:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D92E361FC0;\n\tTue, 23 Aug 2022 03:48:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ACA4360E25\n\tfor ;\n\tTue, 23 Aug 2022 03:48:03 +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 16A222B3;\n\tTue, 23 Aug 2022 03:48:03 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661219285;\n\tbh=YA3D/GPIJku7Q0NEE0hfpEgAParuqExfx/ynHkJI5ZA=;\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=tmBYx3mHLejFJUNIW+zACNfQm8u8luKxvy/69tCVBwBDGrhdQrnKPa8Do1RyjPLh7\n\tR9Kv1Qd+V0C4FpSaaIvCNmv0bEZ6M6ji4YvxUbRP/24r86BW9/IPH8io1TRjCOseYO\n\tFboYPAI1v6DwS47rqBk/ONi2HQLEMIKKsKm+Ymjue6yuCefLSFqj3tZl/5R0U7mShA\n\t6uxJvEZQuvROWB5tgsP3uvgT+IiU9E6vSlWVke8p1eWJUc3GhGMgeqr3i+FNCt2JqC\n\tKitcx01unUH+NbgCRrXMWNRgEFMcw5qtEtmO5GuXzlqCJTTKENQ3bg0hKAGjGjBxek\n\tJCnLECuaiDNwQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661219283;\n\tbh=YA3D/GPIJku7Q0NEE0hfpEgAParuqExfx/ynHkJI5ZA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=S+1Jr/jzDneEGdPf2nb115QNeFlWRt2ddF8xwDqY5pDtalFRgKs0knE8u/RmieRj7\n\tHQUJ7sDjJlu8bJTHMPTh5w1cZkTAKXL4hWxHwWN4EJKoNn2zQosPicTyR2f0Nsg5aT\n\tYf4VMJoJ5BPUWLyNX9S0iEFUMvlFh4IPHGA8Igk8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"S+1Jr/jz\"; dkim-atps=neutral","Date":"Tue, 23 Aug 2022 04:47:59 +0300","To":"Utkarsh Tiwari ","Message-ID":"","References":"<20220810150349.414043-1-utkarsh02t@gmail.com>\n\t<20220810150349.414043-3-utkarsh02t@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220810150349.414043-3-utkarsh02t@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v8 2/8] qcam: Support Hotplug for\n\tCamera Selection Dialog","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\" "}}]