[{"id":3628,"web_url":"https://patchwork.libcamera.org/comment/3628/","msgid":"<20200206232852.GI7611@pendragon.ideasonboard.com>","date":"2020-02-06T23:28:52","subject":"Re: [libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and\n\tcamera switching","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Thu, Feb 06, 2020 at 03:05:01PM +0000, Kieran Bingham wrote:\n> Implement a quit button, and a list of cameras.\n> \n> Selecting a different camera from the Toolbar will stop the current\n> stream, and start streaming the chosen camera device if it can be\n> acquired.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/qcam/main_window.cpp | 60 ++++++++++++++++++++++++++++++++++++++++\n> src/qcam/main_window.h | 4 +++\n> 2 files changed, 64 insertions(+)\n> \n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index b51a16de199d..1c7260f32d0a 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -13,6 +13,8 @@\n> #include \n> #include \n> #include \n> +#include \n> +#include \n> \n> #include \n> #include \n> @@ -27,6 +29,8 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> {\n> \tint ret;\n> \n> +\tcreateToolbars(cm);\n> +\n> \ttitle_ = \"QCam \" + QString::fromStdString(CameraManager::version());\n> \tsetWindowTitle(title_);\n> \tconnect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));\n> @@ -53,6 +57,31 @@ MainWindow::~MainWindow()\n> \t}\n> }\n> \n> +int MainWindow::createToolbars(CameraManager *cm)\n> +{\n> +\tQAction *action;\n> +\n> +\ttoolbar_ = addToolBar(\"\");\n\nYou should give a name to the toolbar, otherwise the context menu will\nhave an empty entry. You can call it \"Main\" or anything similar to start\nwith.\n\n> +\n> +\taction = toolbar_->addAction(\"Quit\");\n> +\tconnect(action, &QAction::triggered, this, &MainWindow::quit);\n> +\n> +\tQAction *cameraAction = new QAction(\"&Cameras\", this);\n> +\ttoolbar_->addAction(cameraAction);\n> +\n> +\tQToolButton *cameraButton = dynamic_cast(toolbar_->widgetForAction(cameraAction));\n> +\n> +\tcameraButton->setPopupMode(QToolButton::InstantPopup);\n\nAny way we could remove the \"Camera\" entry from the popup menu ? It\nseems we may have to insert a manually-created QComboBox. My initial\nexpriment resulted in the following code. Feel free to fold it in,\nmodify it, or ignore it if you think it's not a good idea.\n\ndiff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\nindex 0e994b1e9197..f68b7abccda6 100644\n--- a/src/qcam/main_window.cpp\n+++ b/src/qcam/main_window.cpp\n@@ -10,6 +10,7 @@\n #include \n #include \n\n+#include \n #include \n #include \n #include \n@@ -29,11 +30,11 @@\n using namespace libcamera;\n\n MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n-\t: options_(options), allocator_(nullptr), isCapturing_(false)\n+\t: cm_(cm), options_(options), allocator_(nullptr), isCapturing_(false)\n {\n \tint ret;\n\n-\tcreateToolbars(cm);\n+\tcreateToolbars();\n\n \ttitle_ = \"QCam \" + QString::fromStdString(CameraManager::version());\n \tsetWindowTitle(title_);\n@@ -43,7 +44,7 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n \tsetCentralWidget(viewfinder_);\n \tadjustSize();\n\n-\tret = openCamera(cm);\n+\tret = openCamera();\n \tif (!ret) {\n \t\tret = startCapture();\n \t}\n@@ -61,7 +62,7 @@ MainWindow::~MainWindow()\n \t}\n }\n\n-int MainWindow::createToolbars(CameraManager *cm)\n+int MainWindow::createToolbars()\n {\n \tQAction *action;\n\n@@ -70,18 +71,14 @@ int MainWindow::createToolbars(CameraManager *cm)\n \taction = toolbar_->addAction(QIcon(\":x-circle.svg\"), \"Quit\");\n \tconnect(action, &QAction::triggered, this, &MainWindow::quit);\n\n-\tQAction *cameraAction = new QAction(\"&Cameras\", this);\n-\ttoolbar_->addAction(cameraAction);\n+\tQComboBox *cameraCombo = new QComboBox();\n+\tconnect(cameraCombo, QOverload::of(&QComboBox::activated),\n+\t\tthis, &MainWindow::setCamera);\n\n-\tQToolButton *cameraButton = dynamic_cast(toolbar_->widgetForAction(cameraAction));\n+\tfor (const std::shared_ptr &cam : cm_->cameras())\n+\t\tcameraCombo->addItem(QString::fromStdString(cam->name()));\n\n-\tcameraButton->setPopupMode(QToolButton::InstantPopup);\n-\n-\tfor (const std::shared_ptr &cam : cm->cameras()) {\n-\t\taction = new QAction(QString::fromStdString(cam->name()));\n-\t\tcameraButton->addAction(action);\n-\t\tconnect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });\n-\t}\n+\ttoolbar_->addWidget(cameraCombo);\n\n \taction = toolbar_->addAction(QIcon(\":play-circle.svg\"), \"start\");\n \tconnect(action, &QAction::triggered, this, &MainWindow::startCapture);\n@@ -117,13 +114,19 @@ void MainWindow::updateTitle()\n \tsetWindowTitle(title_ + \" : \" + QString::number(fps, 'f', 2) + \" fps\");\n }\n\n-int MainWindow::setCamera(const std::shared_ptr &cam)\n+void MainWindow::setCamera(int index)\n {\n+\tconst auto &cameras = cm_->cameras();\n+\tif (static_cast(index) >= cameras.size())\n+\t\treturn;\n+\n+\tstd::shared_ptr cam = cameras[index];\n+\n \tstd::cout << \"Chose \" << cam->name() << std::endl;\n\n \tif (cam->acquire()) {\n \t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n-\t\treturn -EBUSY;\n+\t\treturn;\n \t}\n\n \tstd::cout << \"Switching to camera \" << cam->name() << std::endl;\n@@ -144,19 +147,17 @@ int MainWindow::setCamera(const std::shared_ptr &cam)\n \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n\n \tstartCapture();\n-\n-\treturn 0;\n }\n\n-std::string MainWindow::chooseCamera(CameraManager *cm)\n+std::string MainWindow::chooseCamera()\n {\n \tQStringList cameras;\n \tbool result;\n\n-\tif (cm->cameras().size() == 1)\n-\t\treturn cm->cameras()[0]->name();\n+\tif (cm_->cameras().size() == 1)\n+\t\treturn cm_->cameras()[0]->name();\n\n-\tfor (const std::shared_ptr &cam : cm->cameras())\n+\tfor (const std::shared_ptr &cam : cm_->cameras())\n \t\tcameras.append(QString::fromStdString(cam->name()));\n\n \tQString name = QInputDialog::getItem(this, \"Select Camera\",\n@@ -168,19 +169,19 @@ std::string MainWindow::chooseCamera(CameraManager *cm)\n \treturn name.toStdString();\n }\n\n-int MainWindow::openCamera(CameraManager *cm)\n+int MainWindow::openCamera()\n {\n \tstd::string cameraName;\n\n \tif (options_.isSet(OptCamera))\n \t\tcameraName = static_cast(options_[OptCamera]);\n \telse\n-\t\tcameraName = chooseCamera(cm);\n+\t\tcameraName = chooseCamera();\n\n \tif (cameraName == \"\")\n \t\treturn -EINVAL;\n\n-\tcamera_ = cm->get(cameraName);\n+\tcamera_ = cm_->get(cameraName);\n \tif (!camera_) {\n \t\tstd::cout << \"Camera \" << cameraName << \" not found\"\n \t\t\t << std::endl;\ndiff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\nindex fc85b6a46491..49af0f6ad934 100644\n--- a/src/qcam/main_window.h\n+++ b/src/qcam/main_window.h\n@@ -44,19 +44,21 @@ private Q_SLOTS:\n \tvoid quit();\n \tvoid updateTitle();\n\n-\tint setCamera(const std::shared_ptr &cam);\n+\tvoid setCamera(int index);\n\n \tint startCapture();\n \tvoid stopCapture();\n \tvoid saveImage();\n\n private:\n-\tint createToolbars(CameraManager *cm);\n-\tstd::string chooseCamera(CameraManager *cm);\n-\tint openCamera(CameraManager *cm);\n+\tint createToolbars();\n+\tstd::string chooseCamera();\n+\tint openCamera();\n \tvoid requestComplete(Request *request);\n \tint display(FrameBuffer *buffer);\n\n+\tCameraManager *cm_;\n+\n \tQString title_;\n \tQTimer titleTimer_;\n\n> +\n> +\tfor (const std::shared_ptr &cam : cm->cameras()) {\n> +\t\taction = new QAction(QString::fromStdString(cam->name()));\n> +\t\tcameraButton->addAction(action);\n> +\t\tconnect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });\n\nAre you aware that this will create local copies of the\nstd::shared_ptr for each camera ? Probably not a big deal.\n\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> void MainWindow::quit()\n> {\n> \tQTimer::singleShot(0, QCoreApplication::instance(),\n> @@ -72,6 +101,37 @@ void MainWindow::updateTitle()\n> \tsetWindowTitle(title_ + \" : \" + QString::number(fps, 'f', 2) + \" fps\");\n> }\n> \n> +int MainWindow::setCamera(const std::shared_ptr &cam)\n\nYou can make this a void function, the return value of slots is ignored\nwhen connected to signals.\n\n> +{\n> +\tstd::cout << \"Chose \" << cam->name() << std::endl;\n> +\n> +\tif (cam->acquire()) {\n> +\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> +\t\treturn -EBUSY;\n> +\t}\n> +\n> +\tstd::cout << \"Switching to camera \" << cam->name() << std::endl;\n> +\n> +\tstopCapture();\n> +\tcamera_->release();\n> +\n> +\t/*\n> +\t * If we don't disconnect this signal, it will persist (and get\n> +\t * re-added and thus duplicated later if we ever switch back to an\n> +\t * previously streamed camera). This causes all sorts of pain.\n> +\t *\n> +\t * Perhaps releasing a camera should disconnect all (public?) connected\n> +\t * signals forcefully!\n\nI'm not sure that would be a good idea, implicit actions are usually\nconfusing.\n\n> +\t */\n> +\tcamera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);\n> +\tcamera_ = cam;\n> +\tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n\nHow about connecting the signal in startCapture() and disconnecting it\nin stopCapture() ? It will avoid the duplicated connect in openCamera().\n\n> +\n> +\tstartCapture();\n> +\n> +\treturn 0;\n> +}\n> +\n> std::string MainWindow::chooseCamera(CameraManager *cm)\n> {\n> \tQStringList cameras;\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index a11443b30b37..f7c96fdd5c30 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -44,7 +44,10 @@ private Q_SLOTS:\n> \tvoid quit();\n> \tvoid updateTitle();\n> \n> +\tint setCamera(const std::shared_ptr &cam);\n> +\n> private:\n> +\tint createToolbars(CameraManager *cm);\n> \tstd::string chooseCamera(CameraManager *cm);\n> \tint openCamera(CameraManager *cm);\n> \n> @@ -71,6 +74,7 @@ private:\n> \tuint32_t previousFrames_;\n> \tuint32_t framesCaptured_;\n> \n> +\tQToolBar *toolbar_;\n> \tViewFinder *viewfinder_;\n> \tstd::map> mappedBuffers_;\n> };","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B107F607C1\n\tfor ;\n\tFri, 7 Feb 2020 00:29:08 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 265379CE;\n\tFri, 7 Feb 2020 00:29:08 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581031748;\n\tbh=J8hJFHO+c28tGL9GIz+eV9StGFBid4mJoOc6xFZDq5g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IJb9ywRLaTjfvp6dpsBxbwwFLbRt1skWP4A+e4nywqLFwD04UHef6icc0fg9YJMBU\n\tf43BrIntVaF5cupTd6TqTSL1oDy7Zw3bp743ph+VVSHcHt8IkbUEz/WPLXWd33V7iW\n\tZAFhI9o2ZQT/WWFHJPDEI0I4q/Ie9a00ODqRQ3DI=","Date":"Fri, 7 Feb 2020 01:28:52 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20200206232852.GI7611@pendragon.ideasonboard.com>","References":"<20200206150504.24204-1-kieran.bingham@ideasonboard.com>\n\t<20200206150504.24204-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200206150504.24204-4-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and\n\tcamera switching","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","X-List-Received-Date":"Thu, 06 Feb 2020 23:29:08 -0000"}},{"id":3633,"web_url":"https://patchwork.libcamera.org/comment/3633/","msgid":"<4336eb2e-1301-2fa8-12a2-3615e489ba74@ideasonboard.com>","date":"2020-02-07T10:02:23","subject":"Re: [libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and\n\tcamera switching","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 06/02/2020 23:28, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Thu, Feb 06, 2020 at 03:05:01PM +0000, Kieran Bingham wrote:\n>> Implement a quit button, and a list of cameras.\n>>\n>> Selecting a different camera from the Toolbar will stop the current\n>> stream, and start streaming the chosen camera device if it can be\n>> acquired.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/qcam/main_window.cpp | 60 ++++++++++++++++++++++++++++++++++++++++\n>> src/qcam/main_window.h | 4 +++\n>> 2 files changed, 64 insertions(+)\n>>\n>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n>> index b51a16de199d..1c7260f32d0a 100644\n>> --- a/src/qcam/main_window.cpp\n>> +++ b/src/qcam/main_window.cpp\n>> @@ -13,6 +13,8 @@\n>> #include \n>> #include \n>> #include \n>> +#include \n>> +#include \n>> \n>> #include \n>> #include \n>> @@ -27,6 +29,8 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n>> {\n>> \tint ret;\n>> \n>> +\tcreateToolbars(cm);\n>> +\n>> \ttitle_ = \"QCam \" + QString::fromStdString(CameraManager::version());\n>> \tsetWindowTitle(title_);\n>> \tconnect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));\n>> @@ -53,6 +57,31 @@ MainWindow::~MainWindow()\n>> \t}\n>> }\n>> \n>> +int MainWindow::createToolbars(CameraManager *cm)\n>> +{\n>> +\tQAction *action;\n>> +\n>> +\ttoolbar_ = addToolBar(\"\");\n> \n> You should give a name to the toolbar, otherwise the context menu will\n> have an empty entry. You can call it \"Main\" or anything similar to start\n> with.\n> \n\nWhich context menu?\n\nI'm not sure I understand the need. I mean, I don't care, I can add the\nname - I just can't see /why/ a toolbar needs a name :-)\n\nUgh ... I see right clicking on the toolbar lets you hide it and then\nyou can't get it back again. So /that's/ the context menu ...\n\nShould the toolbar be 'permanant'? Or removable - and if removable, how\nwould we expect to get it back. Keyboard shortcut?\n\n\nPerhaps removable is useful to be able to simplify the view - but as\nthis is just a test tool - I don't see the benefit yet.\n\nI'll try to look at disabling the context menu or making the main\ntoolbar permanent.\n\n\n>> +\n>> +\taction = toolbar_->addAction(\"Quit\");\n>> +\tconnect(action, &QAction::triggered, this, &MainWindow::quit);\n>> +\n>> +\tQAction *cameraAction = new QAction(\"&Cameras\", this);\n>> +\ttoolbar_->addAction(cameraAction);\n>> +\n>> +\tQToolButton *cameraButton = dynamic_cast(toolbar_->widgetForAction(cameraAction));\n>> +\n>> +\tcameraButton->setPopupMode(QToolButton::InstantPopup);\n> \n> Any way we could remove the \"Camera\" entry from the popup menu ? It\n> seems we may have to insert a manually-created QComboBox. My initial\n> expriment resulted in the following code. Feel free to fold it in,\n> modify it, or ignore it if you think it's not a good idea.\n> \n\nI would actually like this entry to show the current camera in the toolbar.\n\nAnd yes the duplicated entry is annoying.\n\nBut I haven't got as far as dealing with such UX issues.\nI was focussing on getting the core code to work.\n\nI'll work through your code and see what how it integrates.\n\n\nHrm ... one part I don't like about the below is selecting a camera by\nindex. That seems quite fragile once we have hotplug support ?\n\n\n\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 0e994b1e9197..f68b7abccda6 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -29,11 +30,11 @@\n> using namespace libcamera;\n> \n> MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> -\t: options_(options), allocator_(nullptr), isCapturing_(false)\n> +\t: cm_(cm), options_(options), allocator_(nullptr), isCapturing_(false)\n> {\n> \tint ret;\n> \n> -\tcreateToolbars(cm);\n> +\tcreateToolbars();\n> \n> \ttitle_ = \"QCam \" + QString::fromStdString(CameraManager::version());\n> \tsetWindowTitle(title_);\n> @@ -43,7 +44,7 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> \tsetCentralWidget(viewfinder_);\n> \tadjustSize();\n> \n> -\tret = openCamera(cm);\n> +\tret = openCamera();\n> \tif (!ret) {\n> \t\tret = startCapture();\n> \t}\n> @@ -61,7 +62,7 @@ MainWindow::~MainWindow()\n> \t}\n> }\n> \n> -int MainWindow::createToolbars(CameraManager *cm)\n> +int MainWindow::createToolbars()\n> {\n> \tQAction *action;\n> \n> @@ -70,18 +71,14 @@ int MainWindow::createToolbars(CameraManager *cm)\n> \taction = toolbar_->addAction(QIcon(\":x-circle.svg\"), \"Quit\");\n> \tconnect(action, &QAction::triggered, this, &MainWindow::quit);\n> \n> -\tQAction *cameraAction = new QAction(\"&Cameras\", this);\n> -\ttoolbar_->addAction(cameraAction);\n> +\tQComboBox *cameraCombo = new QComboBox();\n> +\tconnect(cameraCombo, QOverload::of(&QComboBox::activated),\n> +\t\tthis, &MainWindow::setCamera);\n> \n> -\tQToolButton *cameraButton = dynamic_cast(toolbar_->widgetForAction(cameraAction));\n> +\tfor (const std::shared_ptr &cam : cm_->cameras())\n> +\t\tcameraCombo->addItem(QString::fromStdString(cam->name()));\n> \n> -\tcameraButton->setPopupMode(QToolButton::InstantPopup);\n> -\n> -\tfor (const std::shared_ptr &cam : cm->cameras()) {\n> -\t\taction = new QAction(QString::fromStdString(cam->name()));\n> -\t\tcameraButton->addAction(action);\n> -\t\tconnect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });\n> -\t}\n> +\ttoolbar_->addWidget(cameraCombo);\n> \n> \taction = toolbar_->addAction(QIcon(\":play-circle.svg\"), \"start\");\n> \tconnect(action, &QAction::triggered, this, &MainWindow::startCapture);\n> @@ -117,13 +114,19 @@ void MainWindow::updateTitle()\n> \tsetWindowTitle(title_ + \" : \" + QString::number(fps, 'f', 2) + \" fps\");\n> }\n> \n> -int MainWindow::setCamera(const std::shared_ptr &cam)\n> +void MainWindow::setCamera(int index)\n> {\n> +\tconst auto &cameras = cm_->cameras();\n> +\tif (static_cast(index) >= cameras.size())\n> +\t\treturn;\n> +\n> +\tstd::shared_ptr cam = cameras[index];\n> +\n> \tstd::cout << \"Chose \" << cam->name() << std::endl;\n> \n\nI'll remove this debug print and print the camera name if it fails to\nacquire.\n\n\n> \tif (cam->acquire()) {\n> \t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> -\t\treturn -EBUSY;\n> +\t\treturn;\n> \t}\n> \n> \tstd::cout << \"Switching to camera \" << cam->name() << std::endl;\n> @@ -144,19 +147,17 @@ int MainWindow::setCamera(const std::shared_ptr &cam)\n> \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n> \n> \tstartCapture();\n> -\n> -\treturn 0;\n> }\n> \n> -std::string MainWindow::chooseCamera(CameraManager *cm)\n> +std::string MainWindow::chooseCamera()\n> {\n> \tQStringList cameras;\n> \tbool result;\n> \n> -\tif (cm->cameras().size() == 1)\n> -\t\treturn cm->cameras()[0]->name();\n> +\tif (cm_->cameras().size() == 1)\n> +\t\treturn cm_->cameras()[0]->name();\n> \n> -\tfor (const std::shared_ptr &cam : cm->cameras())\n> +\tfor (const std::shared_ptr &cam : cm_->cameras())\n> \t\tcameras.append(QString::fromStdString(cam->name()));\n> \n> \tQString name = QInputDialog::getItem(this, \"Select Camera\",\n> @@ -168,19 +169,19 @@ std::string MainWindow::chooseCamera(CameraManager *cm)\n> \treturn name.toStdString();\n> }\n> \n> -int MainWindow::openCamera(CameraManager *cm)\n> +int MainWindow::openCamera()\n> {\n> \tstd::string cameraName;\n> \n> \tif (options_.isSet(OptCamera))\n> \t\tcameraName = static_cast(options_[OptCamera]);\n> \telse\n> -\t\tcameraName = chooseCamera(cm);\n> +\t\tcameraName = chooseCamera();\n> \n> \tif (cameraName == \"\")\n> \t\treturn -EINVAL;\n> \n> -\tcamera_ = cm->get(cameraName);\n> +\tcamera_ = cm_->get(cameraName);\n> \tif (!camera_) {\n> \t\tstd::cout << \"Camera \" << cameraName << \" not found\"\n> \t\t\t << std::endl;\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index fc85b6a46491..49af0f6ad934 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -44,19 +44,21 @@ private Q_SLOTS:\n> \tvoid quit();\n> \tvoid updateTitle();\n> \n> -\tint setCamera(const std::shared_ptr &cam);\n> +\tvoid setCamera(int index);\n> \n> \tint startCapture();\n> \tvoid stopCapture();\n> \tvoid saveImage();\n> \n> private:\n> -\tint createToolbars(CameraManager *cm);\n> -\tstd::string chooseCamera(CameraManager *cm);\n> -\tint openCamera(CameraManager *cm);\n> +\tint createToolbars();\n> +\tstd::string chooseCamera();\n> +\tint openCamera();\n> \tvoid requestComplete(Request *request);\n> \tint display(FrameBuffer *buffer);\n> \n> +\tCameraManager *cm_;\n> +\n> \tQString title_;\n> \tQTimer titleTimer_;\n> \n>> +\n>> +\tfor (const std::shared_ptr &cam : cm->cameras()) {\n>> +\t\taction = new QAction(QString::fromStdString(cam->name()));\n>> +\t\tcameraButton->addAction(action);\n>> +\t\tconnect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });\n> \n> Are you aware that this will create local copies of the\n> std::shared_ptr for each camera ? Probably not a big deal.\n\nYes, I thought that was needed to make sure they remain in scope. And as\nthey are shared_ptr that should be fine right?\n\nThe alternative is to pass the QAction, and get the camera name from\nthere, or pass the cam->name() itself and then get the camera by name.\n\nI thought as we already have the 'Camera' this would be better - but I\ncan change if it's a problem.\n\n\nI see in your implementation you move to an index, but I fear this would\ncause problems with hotplug support. But maybe that will be painful\nanyway, and we'll have to reconstruct the camera list to update anytime\nthe camera list changes regardless.\n\n\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> void MainWindow::quit()\n>> {\n>> \tQTimer::singleShot(0, QCoreApplication::instance(),\n>> @@ -72,6 +101,37 @@ void MainWindow::updateTitle()\n>> \tsetWindowTitle(title_ + \" : \" + QString::number(fps, 'f', 2) + \" fps\");\n>> }\n>> \n>> +int MainWindow::setCamera(const std::shared_ptr &cam)\n> \n> You can make this a void function, the return value of slots is ignored\n> when connected to signals.\n\nAh indeed, I started out with this as a void, I must have changed it to\nreturn int when I pulled over the code with the -EBUSY.\n\nBut we can simply ignore that return value indeed, no action will occur.\n\nLater it would be nice if we had a status bar to report specific\nmessages through the UI. But that's a 'later' task.\n\nOr maybe a workshop style activity ... ;-)\n\n\n> \n>> +{\n>> +\tstd::cout << \"Chose \" << cam->name() << std::endl;\n>> +\n>> +\tif (cam->acquire()) {\n>> +\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n>> +\t\treturn -EBUSY;\n>> +\t}\n>> +\n>> +\tstd::cout << \"Switching to camera \" << cam->name() << std::endl;\n>> +\n>> +\tstopCapture();\n>> +\tcamera_->release();\n>> +\n>> +\t/*\n>> +\t * If we don't disconnect this signal, it will persist (and get\n>> +\t * re-added and thus duplicated later if we ever switch back to an\n>> +\t * previously streamed camera). This causes all sorts of pain.\n>> +\t *\n>> +\t * Perhaps releasing a camera should disconnect all (public?) connected\n>> +\t * signals forcefully!\n> \n> I'm not sure that would be a good idea, implicit actions are usually\n> confusing.\n> \n>> +\t */\n>> +\tcamera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);\n>> +\tcamera_ = cam;\n>> +\tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n> \n> How about connecting the signal in startCapture() and disconnecting it\n> in stopCapture() ? It will avoid the duplicated connect in openCamera().\n\nAha - that's much better and clearly obvious :)\n\nI'll update to do so.\n\n\n>> +\n>> +\tstartCapture();\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> std::string MainWindow::chooseCamera(CameraManager *cm)\n>> {\n>> \tQStringList cameras;\n>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n>> index a11443b30b37..f7c96fdd5c30 100644\n>> --- a/src/qcam/main_window.h\n>> +++ b/src/qcam/main_window.h\n>> @@ -44,7 +44,10 @@ private Q_SLOTS:\n>> \tvoid quit();\n>> \tvoid updateTitle();\n>> \n>> +\tint setCamera(const std::shared_ptr &cam);\n>> +\n>> private:\n>> +\tint createToolbars(CameraManager *cm);\n>> \tstd::string chooseCamera(CameraManager *cm);\n>> \tint openCamera(CameraManager *cm);\n>> \n>> @@ -71,6 +74,7 @@ private:\n>> \tuint32_t previousFrames_;\n>> \tuint32_t framesCaptured_;\n>> \n>> +\tQToolBar *toolbar_;\n>> \tViewFinder *viewfinder_;\n>> \tstd::map> mappedBuffers_;\n>> };\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A8CF60440\n\tfor ;\n\tFri, 7 Feb 2020 11:02:26 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DEC5252F;\n\tFri, 7 Feb 2020 11:02:25 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581069746;\n\tbh=Pn++m/YfqjgVkAqU7hrzl3FtGVgFCm1UxND6cGuBbsg=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=WFzHeC9TJznG/ZO9aEK8NkD8Q8bJZCKO60bzwVr7s+TcP3obuXNZsmnwfmh76WpNG\n\tBFe2GXu7lNWXFcCa34p5C9GTeK0eijIBc9jLNXjAXIkCHJ8bPzMBtxW6J10QzailOy\n\t0fqVRQLh85YpfKof/8AmbZ4sIQ9OuezsLZ4luUBw=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20200206150504.24204-1-kieran.bingham@ideasonboard.com>\n\t<20200206150504.24204-4-kieran.bingham@ideasonboard.com>\n\t<20200206232852.GI7611@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<4336eb2e-1301-2fa8-12a2-3615e489ba74@ideasonboard.com>","Date":"Fri, 7 Feb 2020 10:02:23 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200206232852.GI7611@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and\n\tcamera switching","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","X-List-Received-Date":"Fri, 07 Feb 2020 10:02:26 -0000"}},{"id":3634,"web_url":"https://patchwork.libcamera.org/comment/3634/","msgid":"<20200207101319.GA4726@pendragon.ideasonboard.com>","date":"2020-02-07T10:13:19","subject":"Re: [libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and\n\tcamera switching","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Feb 07, 2020 at 10:02:23AM +0000, Kieran Bingham wrote:\n> On 06/02/2020 23:28, Laurent Pinchart wrote:\n> > On Thu, Feb 06, 2020 at 03:05:01PM +0000, Kieran Bingham wrote:\n> >> Implement a quit button, and a list of cameras.\n> >>\n> >> Selecting a different camera from the Toolbar will stop the current\n> >> stream, and start streaming the chosen camera device if it can be\n> >> acquired.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> src/qcam/main_window.cpp | 60 ++++++++++++++++++++++++++++++++++++++++\n> >> src/qcam/main_window.h | 4 +++\n> >> 2 files changed, 64 insertions(+)\n> >>\n> >> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> >> index b51a16de199d..1c7260f32d0a 100644\n> >> --- a/src/qcam/main_window.cpp\n> >> +++ b/src/qcam/main_window.cpp\n> >> @@ -13,6 +13,8 @@\n> >> #include \n> >> #include \n> >> #include \n> >> +#include \n> >> +#include \n> >> \n> >> #include \n> >> #include \n> >> @@ -27,6 +29,8 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> >> {\n> >> \tint ret;\n> >> \n> >> +\tcreateToolbars(cm);\n> >> +\n> >> \ttitle_ = \"QCam \" + QString::fromStdString(CameraManager::version());\n> >> \tsetWindowTitle(title_);\n> >> \tconnect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));\n> >> @@ -53,6 +57,31 @@ MainWindow::~MainWindow()\n> >> \t}\n> >> }\n> >> \n> >> +int MainWindow::createToolbars(CameraManager *cm)\n> >> +{\n> >> +\tQAction *action;\n> >> +\n> >> +\ttoolbar_ = addToolBar(\"\");\n> > \n> > You should give a name to the toolbar, otherwise the context menu will\n> > have an empty entry. You can call it \"Main\" or anything similar to start\n> > with.\n> \n> Which context menu?\n> \n> I'm not sure I understand the need. I mean, I don't care, I can add the\n> name - I just can't see /why/ a toolbar needs a name :-)\n> \n> Ugh ... I see right clicking on the toolbar lets you hide it and then\n> you can't get it back again. So /that's/ the context menu ...\n> \n> Should the toolbar be 'permanant'? Or removable - and if removable, how\n> would we expect to get it back. Keyboard shortcut?\n\nIf you can make it permanent I think that would be best.\n\n> Perhaps removable is useful to be able to simplify the view - but as\n> this is just a test tool - I don't see the benefit yet.\n> \n> I'll try to look at disabling the context menu or making the main\n> toolbar permanent.\n> \n> >> +\n> >> +\taction = toolbar_->addAction(\"Quit\");\n> >> +\tconnect(action, &QAction::triggered, this, &MainWindow::quit);\n> >> +\n> >> +\tQAction *cameraAction = new QAction(\"&Cameras\", this);\n> >> +\ttoolbar_->addAction(cameraAction);\n> >> +\n> >> +\tQToolButton *cameraButton = dynamic_cast(toolbar_->widgetForAction(cameraAction));\n> >> +\n> >> +\tcameraButton->setPopupMode(QToolButton::InstantPopup);\n> > \n> > Any way we could remove the \"Camera\" entry from the popup menu ? It\n> > seems we may have to insert a manually-created QComboBox. My initial\n> > expriment resulted in the following code. Feel free to fold it in,\n> > modify it, or ignore it if you think it's not a good idea.\n> \n> I would actually like this entry to show the current camera in the toolbar.\n> \n> And yes the duplicated entry is annoying.\n> \n> But I haven't got as far as dealing with such UX issues.\n> I was focussing on getting the core code to work.\n> \n> I'll work through your code and see what how it integrates.\n> \n> \n> Hrm ... one part I don't like about the below is selecting a camera by\n> index. That seems quite fragile once we have hotplug support ?\n\nAgreed, but once we have hotplug support we'll have to remove and add\nentries from the combo box or popup menu, so refactoring will be needed\nanyway. I think hotplug support will also require stable names/IDs, so\nwe should then have the tool we need to do the job.\n\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index 0e994b1e9197..f68b7abccda6 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -10,6 +10,7 @@\n> > #include \n> > #include \n> > \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -29,11 +30,11 @@\n> > using namespace libcamera;\n> > \n> > MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> > -\t: options_(options), allocator_(nullptr), isCapturing_(false)\n> > +\t: cm_(cm), options_(options), allocator_(nullptr), isCapturing_(false)\n> > {\n> > \tint ret;\n> > \n> > -\tcreateToolbars(cm);\n> > +\tcreateToolbars();\n> > \n> > \ttitle_ = \"QCam \" + QString::fromStdString(CameraManager::version());\n> > \tsetWindowTitle(title_);\n> > @@ -43,7 +44,7 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> > \tsetCentralWidget(viewfinder_);\n> > \tadjustSize();\n> > \n> > -\tret = openCamera(cm);\n> > +\tret = openCamera();\n> > \tif (!ret) {\n> > \t\tret = startCapture();\n> > \t}\n> > @@ -61,7 +62,7 @@ MainWindow::~MainWindow()\n> > \t}\n> > }\n> > \n> > -int MainWindow::createToolbars(CameraManager *cm)\n> > +int MainWindow::createToolbars()\n> > {\n> > \tQAction *action;\n> > \n> > @@ -70,18 +71,14 @@ int MainWindow::createToolbars(CameraManager *cm)\n> > \taction = toolbar_->addAction(QIcon(\":x-circle.svg\"), \"Quit\");\n> > \tconnect(action, &QAction::triggered, this, &MainWindow::quit);\n> > \n> > -\tQAction *cameraAction = new QAction(\"&Cameras\", this);\n> > -\ttoolbar_->addAction(cameraAction);\n> > +\tQComboBox *cameraCombo = new QComboBox();\n> > +\tconnect(cameraCombo, QOverload::of(&QComboBox::activated),\n> > +\t\tthis, &MainWindow::setCamera);\n> > \n> > -\tQToolButton *cameraButton = dynamic_cast(toolbar_->widgetForAction(cameraAction));\n> > +\tfor (const std::shared_ptr &cam : cm_->cameras())\n> > +\t\tcameraCombo->addItem(QString::fromStdString(cam->name()));\n> > \n> > -\tcameraButton->setPopupMode(QToolButton::InstantPopup);\n> > -\n> > -\tfor (const std::shared_ptr &cam : cm->cameras()) {\n> > -\t\taction = new QAction(QString::fromStdString(cam->name()));\n> > -\t\tcameraButton->addAction(action);\n> > -\t\tconnect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });\n> > -\t}\n> > +\ttoolbar_->addWidget(cameraCombo);\n> > \n> > \taction = toolbar_->addAction(QIcon(\":play-circle.svg\"), \"start\");\n> > \tconnect(action, &QAction::triggered, this, &MainWindow::startCapture);\n> > @@ -117,13 +114,19 @@ void MainWindow::updateTitle()\n> > \tsetWindowTitle(title_ + \" : \" + QString::number(fps, 'f', 2) + \" fps\");\n> > }\n> > \n> > -int MainWindow::setCamera(const std::shared_ptr &cam)\n> > +void MainWindow::setCamera(int index)\n> > {\n> > +\tconst auto &cameras = cm_->cameras();\n> > +\tif (static_cast(index) >= cameras.size())\n> > +\t\treturn;\n> > +\n> > +\tstd::shared_ptr cam = cameras[index];\n> > +\n> > \tstd::cout << \"Chose \" << cam->name() << std::endl;\n> \n> I'll remove this debug print and print the camera name if it fails to\n> acquire.\n> \n> > \tif (cam->acquire()) {\n> > \t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> > -\t\treturn -EBUSY;\n> > +\t\treturn;\n> > \t}\n> > \n> > \tstd::cout << \"Switching to camera \" << cam->name() << std::endl;\n> > @@ -144,19 +147,17 @@ int MainWindow::setCamera(const std::shared_ptr &cam)\n> > \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n> > \n> > \tstartCapture();\n> > -\n> > -\treturn 0;\n> > }\n> > \n> > -std::string MainWindow::chooseCamera(CameraManager *cm)\n> > +std::string MainWindow::chooseCamera()\n> > {\n> > \tQStringList cameras;\n> > \tbool result;\n> > \n> > -\tif (cm->cameras().size() == 1)\n> > -\t\treturn cm->cameras()[0]->name();\n> > +\tif (cm_->cameras().size() == 1)\n> > +\t\treturn cm_->cameras()[0]->name();\n> > \n> > -\tfor (const std::shared_ptr &cam : cm->cameras())\n> > +\tfor (const std::shared_ptr &cam : cm_->cameras())\n> > \t\tcameras.append(QString::fromStdString(cam->name()));\n> > \n> > \tQString name = QInputDialog::getItem(this, \"Select Camera\",\n> > @@ -168,19 +169,19 @@ std::string MainWindow::chooseCamera(CameraManager *cm)\n> > \treturn name.toStdString();\n> > }\n> > \n> > -int MainWindow::openCamera(CameraManager *cm)\n> > +int MainWindow::openCamera()\n> > {\n> > \tstd::string cameraName;\n> > \n> > \tif (options_.isSet(OptCamera))\n> > \t\tcameraName = static_cast(options_[OptCamera]);\n> > \telse\n> > -\t\tcameraName = chooseCamera(cm);\n> > +\t\tcameraName = chooseCamera();\n> > \n> > \tif (cameraName == \"\")\n> > \t\treturn -EINVAL;\n> > \n> > -\tcamera_ = cm->get(cameraName);\n> > +\tcamera_ = cm_->get(cameraName);\n> > \tif (!camera_) {\n> > \t\tstd::cout << \"Camera \" << cameraName << \" not found\"\n> > \t\t\t << std::endl;\n> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > index fc85b6a46491..49af0f6ad934 100644\n> > --- a/src/qcam/main_window.h\n> > +++ b/src/qcam/main_window.h\n> > @@ -44,19 +44,21 @@ private Q_SLOTS:\n> > \tvoid quit();\n> > \tvoid updateTitle();\n> > \n> > -\tint setCamera(const std::shared_ptr &cam);\n> > +\tvoid setCamera(int index);\n> > \n> > \tint startCapture();\n> > \tvoid stopCapture();\n> > \tvoid saveImage();\n> > \n> > private:\n> > -\tint createToolbars(CameraManager *cm);\n> > -\tstd::string chooseCamera(CameraManager *cm);\n> > -\tint openCamera(CameraManager *cm);\n> > +\tint createToolbars();\n> > +\tstd::string chooseCamera();\n> > +\tint openCamera();\n> > \tvoid requestComplete(Request *request);\n> > \tint display(FrameBuffer *buffer);\n> > \n> > +\tCameraManager *cm_;\n> > +\n> > \tQString title_;\n> > \tQTimer titleTimer_;\n> > \n> >> +\n> >> +\tfor (const std::shared_ptr &cam : cm->cameras()) {\n> >> +\t\taction = new QAction(QString::fromStdString(cam->name()));\n> >> +\t\tcameraButton->addAction(action);\n> >> +\t\tconnect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });\n> > \n> > Are you aware that this will create local copies of the\n> > std::shared_ptr for each camera ? Probably not a big deal.\n> \n> Yes, I thought that was needed to make sure they remain in scope. And as\n> they are shared_ptr that should be fine right?\n\nI think it's OK, I just wanted to point it out as lambda capture can\nsometimes be confusing.\n\n> The alternative is to pass the QAction, and get the camera name from\n> there, or pass the cam->name() itself and then get the camera by name.\n> \n> I thought as we already have the 'Camera' this would be better - but I\n> can change if it's a problem.\n> \n> I see in your implementation you move to an index, but I fear this would\n> cause problems with hotplug support. But maybe that will be painful\n> anyway, and we'll have to reconstruct the camera list to update anytime\n> the camera list changes regardless.\n\nThe reason I move to an index is becase the activated signal only gives\nan index. There's a way to associate a QVariant to a combo box entry,\nmaybe we can store the shared_ptr in the QVariant, but I haven't tried\nthat.\n\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> void MainWindow::quit()\n> >> {\n> >> \tQTimer::singleShot(0, QCoreApplication::instance(),\n> >> @@ -72,6 +101,37 @@ void MainWindow::updateTitle()\n> >> \tsetWindowTitle(title_ + \" : \" + QString::number(fps, 'f', 2) + \" fps\");\n> >> }\n> >> \n> >> +int MainWindow::setCamera(const std::shared_ptr &cam)\n> > \n> > You can make this a void function, the return value of slots is ignored\n> > when connected to signals.\n> \n> Ah indeed, I started out with this as a void, I must have changed it to\n> return int when I pulled over the code with the -EBUSY.\n> \n> But we can simply ignore that return value indeed, no action will occur.\n> \n> Later it would be nice if we had a status bar to report specific\n> messages through the UI. But that's a 'later' task.\n> \n> Or maybe a workshop style activity ... ;-)\n> \n> >> +{\n> >> +\tstd::cout << \"Chose \" << cam->name() << std::endl;\n> >> +\n> >> +\tif (cam->acquire()) {\n> >> +\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> >> +\t\treturn -EBUSY;\n> >> +\t}\n> >> +\n> >> +\tstd::cout << \"Switching to camera \" << cam->name() << std::endl;\n> >> +\n> >> +\tstopCapture();\n> >> +\tcamera_->release();\n> >> +\n> >> +\t/*\n> >> +\t * If we don't disconnect this signal, it will persist (and get\n> >> +\t * re-added and thus duplicated later if we ever switch back to an\n> >> +\t * previously streamed camera). This causes all sorts of pain.\n> >> +\t *\n> >> +\t * Perhaps releasing a camera should disconnect all (public?) connected\n> >> +\t * signals forcefully!\n> > \n> > I'm not sure that would be a good idea, implicit actions are usually\n> > confusing.\n> > \n> >> +\t */\n> >> +\tcamera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);\n> >> +\tcamera_ = cam;\n> >> +\tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n> > \n> > How about connecting the signal in startCapture() and disconnecting it\n> > in stopCapture() ? It will avoid the duplicated connect in openCamera().\n> \n> Aha - that's much better and clearly obvious :)\n> \n> I'll update to do so.\n> \n> >> +\n> >> +\tstartCapture();\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> std::string MainWindow::chooseCamera(CameraManager *cm)\n> >> {\n> >> \tQStringList cameras;\n> >> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> >> index a11443b30b37..f7c96fdd5c30 100644\n> >> --- a/src/qcam/main_window.h\n> >> +++ b/src/qcam/main_window.h\n> >> @@ -44,7 +44,10 @@ private Q_SLOTS:\n> >> \tvoid quit();\n> >> \tvoid updateTitle();\n> >> \n> >> +\tint setCamera(const std::shared_ptr &cam);\n> >> +\n> >> private:\n> >> +\tint createToolbars(CameraManager *cm);\n> >> \tstd::string chooseCamera(CameraManager *cm);\n> >> \tint openCamera(CameraManager *cm);\n> >> \n> >> @@ -71,6 +74,7 @@ private:\n> >> \tuint32_t previousFrames_;\n> >> \tuint32_t framesCaptured_;\n> >> \n> >> +\tQToolBar *toolbar_;\n> >> \tViewFinder *viewfinder_;\n> >> \tstd::map> mappedBuffers_;\n> >> };","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AD8D960440\n\tfor ;\n\tFri, 7 Feb 2020 11:13:36 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BCD2552F;\n\tFri, 7 Feb 2020 11:13:35 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581070416;\n\tbh=ns6dWOSYvqM6aJ957F3QxM2qzvb262SnZeLhLrBm1rY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sF/BjRm4hi2fwiOiQDkNvLAHa1ZKuHU9ppK7BJMKzPmgGdHWB2NCm6oFcjNrtYial\n\tWUCn7EZyrWDolvRln+fhngLkdAg6BsXOjxTkRYQIy3u5WXgZzTL6mkmB+hAejtepy1\n\tR2cAuUf93igCOu6BCYe5RHGa+xRm7v6ReVnRRZI8=","Date":"Fri, 7 Feb 2020 12:13:19 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20200207101319.GA4726@pendragon.ideasonboard.com>","References":"<20200206150504.24204-1-kieran.bingham@ideasonboard.com>\n\t<20200206150504.24204-4-kieran.bingham@ideasonboard.com>\n\t<20200206232852.GI7611@pendragon.ideasonboard.com>\n\t<4336eb2e-1301-2fa8-12a2-3615e489ba74@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<4336eb2e-1301-2fa8-12a2-3615e489ba74@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and\n\tcamera switching","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","X-List-Received-Date":"Fri, 07 Feb 2020 10:13:36 -0000"}},{"id":3635,"web_url":"https://patchwork.libcamera.org/comment/3635/","msgid":"","date":"2020-02-07T10:21:48","subject":"Re: [libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and\n\tcamera switching","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 07/02/2020 10:13, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Fri, Feb 07, 2020 at 10:02:23AM +0000, Kieran Bingham wrote:\n>> On 06/02/2020 23:28, Laurent Pinchart wrote:\n>>> On Thu, Feb 06, 2020 at 03:05:01PM +0000, Kieran Bingham wrote:\n>>>> Implement a quit button, and a list of cameras.\n>>>>\n>>>> Selecting a different camera from the Toolbar will stop the current\n>>>> stream, and start streaming the chosen camera device if it can be\n>>>> acquired.\n>>>>\n>>>> Signed-off-by: Kieran Bingham \n>>>> ---\n>>>> src/qcam/main_window.cpp | 60 ++++++++++++++++++++++++++++++++++++++++\n>>>> src/qcam/main_window.h | 4 +++\n>>>> 2 files changed, 64 insertions(+)\n>>>>\n>>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n>>>> index b51a16de199d..1c7260f32d0a 100644\n>>>> --- a/src/qcam/main_window.cpp\n>>>> +++ b/src/qcam/main_window.cpp\n>>>> @@ -13,6 +13,8 @@\n>>>> #include \n>>>> #include \n>>>> #include \n>>>> +#include \n>>>> +#include \n>>>> \n>>>> #include \n>>>> #include \n>>>> @@ -27,6 +29,8 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n>>>> {\n>>>> \tint ret;\n>>>> \n>>>> +\tcreateToolbars(cm);\n>>>> +\n>>>> \ttitle_ = \"QCam \" + QString::fromStdString(CameraManager::version());\n>>>> \tsetWindowTitle(title_);\n>>>> \tconnect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));\n>>>> @@ -53,6 +57,31 @@ MainWindow::~MainWindow()\n>>>> \t}\n>>>> }\n>>>> \n>>>> +int MainWindow::createToolbars(CameraManager *cm)\n>>>> +{\n>>>> +\tQAction *action;\n>>>> +\n>>>> +\ttoolbar_ = addToolBar(\"\");\n>>>\n>>> You should give a name to the toolbar, otherwise the context menu will\n>>> have an empty entry. You can call it \"Main\" or anything similar to start\n>>> with.\n>>\n>> Which context menu?\n>>\n>> I'm not sure I understand the need. I mean, I don't care, I can add the\n>> name - I just can't see /why/ a toolbar needs a name :-)\n>>\n>> Ugh ... I see right clicking on the toolbar lets you hide it and then\n>> you can't get it back again. So /that's/ the context menu ...\n>>\n>> Should the toolbar be 'permanant'? Or removable - and if removable, how\n>> would we expect to get it back. Keyboard shortcut?\n> \n> If you can make it permanent I think that would be best.\n\nAgreed.\n\n>> Perhaps removable is useful to be able to simplify the view - but as\n>> this is just a test tool - I don't see the benefit yet.\n>>\n>> I'll try to look at disabling the context menu or making the main\n>> toolbar permanent.\n>>\n>>>> +\n>>>> +\taction = toolbar_->addAction(\"Quit\");\n>>>> +\tconnect(action, &QAction::triggered, this, &MainWindow::quit);\n>>>> +\n>>>> +\tQAction *cameraAction = new QAction(\"&Cameras\", this);\n>>>> +\ttoolbar_->addAction(cameraAction);\n>>>> +\n>>>> +\tQToolButton *cameraButton = dynamic_cast(toolbar_->widgetForAction(cameraAction));\n>>>> +\n>>>> +\tcameraButton->setPopupMode(QToolButton::InstantPopup);\n>>>\n>>> Any way we could remove the \"Camera\" entry from the popup menu ? It\n>>> seems we may have to insert a manually-created QComboBox. My initial\n>>> expriment resulted in the following code. Feel free to fold it in,\n>>> modify it, or ignore it if you think it's not a good idea.\n>>\n>> I would actually like this entry to show the current camera in the toolbar.\n>>\n>> And yes the duplicated entry is annoying.\n>>\n>> But I haven't got as far as dealing with such UX issues.\n>> I was focussing on getting the core code to work.\n>>\n>> I'll work through your code and see what how it integrates.\n>>\n>>\n>> Hrm ... one part I don't like about the below is selecting a camera by\n>> index. That seems quite fragile once we have hotplug support ?\n> \n> Agreed, but once we have hotplug support we'll have to remove and add\n> entries from the combo box or popup menu, so refactoring will be needed\n> anyway. I think hotplug support will also require stable names/IDs, so\n> we should then have the tool we need to do the job.\n\nI think my point is - we already have one at this level. A shared_ptr to\nthe Camera instance. That will always point to the same camera, and\nnever change.\n\nAnd in the event that the camera is removed, then that Camera will be\nmarked disconnected, so it will fail to stream - but the 'object' will\nbe safely preserved until there are no users left?\n\n\n> \n>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n>>> index 0e994b1e9197..f68b7abccda6 100644\n>>> --- a/src/qcam/main_window.cpp\n>>> +++ b/src/qcam/main_window.cpp\n>>> @@ -10,6 +10,7 @@\n>>> #include \n>>> #include \n>>>\n>>> +#include \n>>> #include \n>>> #include \n>>> #include \n>>> @@ -29,11 +30,11 @@\n>>> using namespace libcamera;\n>>>\n>>> MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n>>> -\t: options_(options), allocator_(nullptr), isCapturing_(false)\n>>> +\t: cm_(cm), options_(options), allocator_(nullptr), isCapturing_(false)\n>>> {\n>>> \tint ret;\n>>>\n>>> -\tcreateToolbars(cm);\n>>> +\tcreateToolbars();\n>>>\n>>> \ttitle_ = \"QCam \" + QString::fromStdString(CameraManager::version());\n>>> \tsetWindowTitle(title_);\n>>> @@ -43,7 +44,7 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n>>> \tsetCentralWidget(viewfinder_);\n>>> \tadjustSize();\n>>>\n>>> -\tret = openCamera(cm);\n>>> +\tret = openCamera();\n>>> \tif (!ret) {\n>>> \t\tret = startCapture();\n>>> \t}\n>>> @@ -61,7 +62,7 @@ MainWindow::~MainWindow()\n>>> \t}\n>>> }\n>>>\n>>> -int MainWindow::createToolbars(CameraManager *cm)\n>>> +int MainWindow::createToolbars()\n>>> {\n>>> \tQAction *action;\n>>>\n>>> @@ -70,18 +71,14 @@ int MainWindow::createToolbars(CameraManager *cm)\n>>> \taction = toolbar_->addAction(QIcon(\":x-circle.svg\"), \"Quit\");\n>>> \tconnect(action, &QAction::triggered, this, &MainWindow::quit);\n>>>\n>>> -\tQAction *cameraAction = new QAction(\"&Cameras\", this);\n>>> -\ttoolbar_->addAction(cameraAction);\n>>> +\tQComboBox *cameraCombo = new QComboBox();\n>>> +\tconnect(cameraCombo, QOverload::of(&QComboBox::activated),\n>>> +\t\tthis, &MainWindow::setCamera);\n>>>\n>>> -\tQToolButton *cameraButton = dynamic_cast(toolbar_->widgetForAction(cameraAction));\n>>> +\tfor (const std::shared_ptr &cam : cm_->cameras())\n>>> +\t\tcameraCombo->addItem(QString::fromStdString(cam->name()));\n>>>\n>>> -\tcameraButton->setPopupMode(QToolButton::InstantPopup);\n>>> -\n>>> -\tfor (const std::shared_ptr &cam : cm->cameras()) {\n>>> -\t\taction = new QAction(QString::fromStdString(cam->name()));\n>>> -\t\tcameraButton->addAction(action);\n>>> -\t\tconnect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });\n>>> -\t}\n>>> +\ttoolbar_->addWidget(cameraCombo);\n>>>\n>>> \taction = toolbar_->addAction(QIcon(\":play-circle.svg\"), \"start\");\n>>> \tconnect(action, &QAction::triggered, this, &MainWindow::startCapture);\n>>> @@ -117,13 +114,19 @@ void MainWindow::updateTitle()\n>>> \tsetWindowTitle(title_ + \" : \" + QString::number(fps, 'f', 2) + \" fps\");\n>>> }\n>>>\n>>> -int MainWindow::setCamera(const std::shared_ptr &cam)\n>>> +void MainWindow::setCamera(int index)\n>>> {\n>>> +\tconst auto &cameras = cm_->cameras();\n>>> +\tif (static_cast(index) >= cameras.size())\n>>> +\t\treturn;\n>>> +\n>>> +\tstd::shared_ptr cam = cameras[index];\n>>> +\n>>> \tstd::cout << \"Chose \" << cam->name() << std::endl;\n>>\n>> I'll remove this debug print and print the camera name if it fails to\n>> acquire.\n>>\n>>> \tif (cam->acquire()) {\n>>> \t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n>>> -\t\treturn -EBUSY;\n>>> +\t\treturn;\n>>> \t}\n>>>\n>>> \tstd::cout << \"Switching to camera \" << cam->name() << std::endl;\n>>> @@ -144,19 +147,17 @@ int MainWindow::setCamera(const std::shared_ptr &cam)\n>>> \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n>>>\n>>> \tstartCapture();\n>>> -\n>>> -\treturn 0;\n>>> }\n>>>\n>>> -std::string MainWindow::chooseCamera(CameraManager *cm)\n>>> +std::string MainWindow::chooseCamera()\n>>> {\n>>> \tQStringList cameras;\n>>> \tbool result;\n>>>\n>>> -\tif (cm->cameras().size() == 1)\n>>> -\t\treturn cm->cameras()[0]->name();\n>>> +\tif (cm_->cameras().size() == 1)\n>>> +\t\treturn cm_->cameras()[0]->name();\n>>>\n>>> -\tfor (const std::shared_ptr &cam : cm->cameras())\n>>> +\tfor (const std::shared_ptr &cam : cm_->cameras())\n>>> \t\tcameras.append(QString::fromStdString(cam->name()));\n>>>\n>>> \tQString name = QInputDialog::getItem(this, \"Select Camera\",\n>>> @@ -168,19 +169,19 @@ std::string MainWindow::chooseCamera(CameraManager *cm)\n>>> \treturn name.toStdString();\n>>> }\n>>>\n>>> -int MainWindow::openCamera(CameraManager *cm)\n>>> +int MainWindow::openCamera()\n>>> {\n>>> \tstd::string cameraName;\n>>>\n>>> \tif (options_.isSet(OptCamera))\n>>> \t\tcameraName = static_cast(options_[OptCamera]);\n>>> \telse\n>>> -\t\tcameraName = chooseCamera(cm);\n>>> +\t\tcameraName = chooseCamera();\n>>>\n>>> \tif (cameraName == \"\")\n>>> \t\treturn -EINVAL;\n>>>\n>>> -\tcamera_ = cm->get(cameraName);\n>>> +\tcamera_ = cm_->get(cameraName);\n>>> \tif (!camera_) {\n>>> \t\tstd::cout << \"Camera \" << cameraName << \" not found\"\n>>> \t\t\t << std::endl;\n>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n>>> index fc85b6a46491..49af0f6ad934 100644\n>>> --- a/src/qcam/main_window.h\n>>> +++ b/src/qcam/main_window.h\n>>> @@ -44,19 +44,21 @@ private Q_SLOTS:\n>>> \tvoid quit();\n>>> \tvoid updateTitle();\n>>>\n>>> -\tint setCamera(const std::shared_ptr &cam);\n>>> +\tvoid setCamera(int index);\n>>>\n>>> \tint startCapture();\n>>> \tvoid stopCapture();\n>>> \tvoid saveImage();\n>>>\n>>> private:\n>>> -\tint createToolbars(CameraManager *cm);\n>>> -\tstd::string chooseCamera(CameraManager *cm);\n>>> -\tint openCamera(CameraManager *cm);\n>>> +\tint createToolbars();\n>>> +\tstd::string chooseCamera();\n>>> +\tint openCamera();\n>>> \tvoid requestComplete(Request *request);\n>>> \tint display(FrameBuffer *buffer);\n>>>\n>>> +\tCameraManager *cm_;\n>>> +\n>>> \tQString title_;\n>>> \tQTimer titleTimer_;\n>>>\n>>>> +\n>>>> +\tfor (const std::shared_ptr &cam : cm->cameras()) {\n>>>> +\t\taction = new QAction(QString::fromStdString(cam->name()));\n>>>> +\t\tcameraButton->addAction(action);\n>>>> +\t\tconnect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });\n>>>\n>>> Are you aware that this will create local copies of the\n>>> std::shared_ptr for each camera ? Probably not a big deal.\n>>\n>> Yes, I thought that was needed to make sure they remain in scope. And as\n>> they are shared_ptr that should be fine right?\n> \n> I think it's OK, I just wanted to point it out as lambda capture can\n> sometimes be confusing.\n\nIndeed, but the alternatives felt uglier to code.\nEssentially I'm only using a lambda to give convenient means of passing\nparameters through the signal event.\n\nIf there's a 'neat' way to do so natively in SIGNAL/SLOT I'm open to\nmore suggestions, but when I looked into it, it seemed required to just\nmake an extra function which dealt with it, and at that point - a lambda\ndoes that inline.\n\n\n>> The alternative is to pass the QAction, and get the camera name from\n>> there, or pass the cam->name() itself and then get the camera by name.\n>>\n>> I thought as we already have the 'Camera' this would be better - but I\n>> can change if it's a problem.\n>>\n>> I see in your implementation you move to an index, but I fear this would\n>> cause problems with hotplug support. But maybe that will be painful\n>> anyway, and we'll have to reconstruct the camera list to update anytime\n>> the camera list changes regardless.\n> \n> The reason I move to an index is becase the activated signal only gives\n> an index. There's a way to associate a QVariant to a combo box entry,\n> maybe we can store the shared_ptr in the QVariant, but I haven't tried\n> that.\n\nIf you've got the QAction* you can get the camera name from\nQAction->text() I think...\n\nOh - but perhaps you don't even have that. Perhaps I'm conflating what\nwas my first approach :-)\n\n\n>>>> +\t}\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> void MainWindow::quit()\n>>>> {\n>>>> \tQTimer::singleShot(0, QCoreApplication::instance(),\n>>>> @@ -72,6 +101,37 @@ void MainWindow::updateTitle()\n>>>> \tsetWindowTitle(title_ + \" : \" + QString::number(fps, 'f', 2) + \" fps\");\n>>>> }\n>>>> \n>>>> +int MainWindow::setCamera(const std::shared_ptr &cam)\n>>>\n>>> You can make this a void function, the return value of slots is ignored\n>>> when connected to signals.\n>>\n>> Ah indeed, I started out with this as a void, I must have changed it to\n>> return int when I pulled over the code with the -EBUSY.\n>>\n>> But we can simply ignore that return value indeed, no action will occur.\n>>\n>> Later it would be nice if we had a status bar to report specific\n>> messages through the UI. But that's a 'later' task.\n>>\n>> Or maybe a workshop style activity ... ;-)\n>>\n>>>> +{\n>>>> +\tstd::cout << \"Chose \" << cam->name() << std::endl;\n>>>> +\n>>>> +\tif (cam->acquire()) {\n>>>> +\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n>>>> +\t\treturn -EBUSY;\n>>>> +\t}\n>>>> +\n>>>> +\tstd::cout << \"Switching to camera \" << cam->name() << std::endl;\n>>>> +\n>>>> +\tstopCapture();\n>>>> +\tcamera_->release();\n>>>> +\n>>>> +\t/*\n>>>> +\t * If we don't disconnect this signal, it will persist (and get\n>>>> +\t * re-added and thus duplicated later if we ever switch back to an\n>>>> +\t * previously streamed camera). This causes all sorts of pain.\n>>>> +\t *\n>>>> +\t * Perhaps releasing a camera should disconnect all (public?) connected\n>>>> +\t * signals forcefully!\n>>>\n>>> I'm not sure that would be a good idea, implicit actions are usually\n>>> confusing.\n>>>\n>>>> +\t */\n>>>> +\tcamera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);\n>>>> +\tcamera_ = cam;\n>>>> +\tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n>>>\n>>> How about connecting the signal in startCapture() and disconnecting it\n>>> in stopCapture() ? It will avoid the duplicated connect in openCamera().\n>>\n>> Aha - that's much better and clearly obvious :)\n>>\n>> I'll update to do so.\n>>\n>>>> +\n>>>> +\tstartCapture();\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> std::string MainWindow::chooseCamera(CameraManager *cm)\n>>>> {\n>>>> \tQStringList cameras;\n>>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n>>>> index a11443b30b37..f7c96fdd5c30 100644\n>>>> --- a/src/qcam/main_window.h\n>>>> +++ b/src/qcam/main_window.h\n>>>> @@ -44,7 +44,10 @@ private Q_SLOTS:\n>>>> \tvoid quit();\n>>>> \tvoid updateTitle();\n>>>> \n>>>> +\tint setCamera(const std::shared_ptr &cam);\n>>>> +\n>>>> private:\n>>>> +\tint createToolbars(CameraManager *cm);\n>>>> \tstd::string chooseCamera(CameraManager *cm);\n>>>> \tint openCamera(CameraManager *cm);\n>>>> \n>>>> @@ -71,6 +74,7 @@ private:\n>>>> \tuint32_t previousFrames_;\n>>>> \tuint32_t framesCaptured_;\n>>>> \n>>>> +\tQToolBar *toolbar_;\n>>>> \tViewFinder *viewfinder_;\n>>>> \tstd::map> mappedBuffers_;\n>>>> };\n>","headers":{"Return-Path":"","Received":["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 5886260440\n\tfor ;\n\tFri, 7 Feb 2020 11:21:52 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 738C552F;\n\tFri, 7 Feb 2020 11:21:51 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581070911;\n\tbh=hIxC8ErsMJTokRRf/OtZfszPiaNwL6XZ0eNeAfhrJ64=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=TvnQzv39XexebPr2rGsacNQui6cmtpRgvsmkXNIDm9O7X3uN2pDwBOrVdK5VeUeYH\n\t/JT5LQU/LZJSsNAcYMz1nrEnijEhMae/WH/ubkvvBmwhmUbflBkQDksr+HqBzapFzU\n\tooFpCxRcNGByf82S55b7heGTMb8D2K+KRpHS+86g=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20200206150504.24204-1-kieran.bingham@ideasonboard.com>\n\t<20200206150504.24204-4-kieran.bingham@ideasonboard.com>\n\t<20200206232852.GI7611@pendragon.ideasonboard.com>\n\t<4336eb2e-1301-2fa8-12a2-3615e489ba74@ideasonboard.com>\n\t<20200207101319.GA4726@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Fri, 7 Feb 2020 10:21:48 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200207101319.GA4726@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and\n\tcamera switching","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","X-List-Received-Date":"Fri, 07 Feb 2020 10:21:52 -0000"}},{"id":3636,"web_url":"https://patchwork.libcamera.org/comment/3636/","msgid":"<20200207103916.GC4726@pendragon.ideasonboard.com>","date":"2020-02-07T10:39:16","subject":"Re: [libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and\n\tcamera switching","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Feb 07, 2020 at 10:21:48AM +0000, Kieran Bingham wrote:\n> On 07/02/2020 10:13, Laurent Pinchart wrote:\n> > On Fri, Feb 07, 2020 at 10:02:23AM +0000, Kieran Bingham wrote:\n> >> On 06/02/2020 23:28, Laurent Pinchart wrote:\n> >>> On Thu, Feb 06, 2020 at 03:05:01PM +0000, Kieran Bingham wrote:\n> >>>> Implement a quit button, and a list of cameras.\n> >>>>\n> >>>> Selecting a different camera from the Toolbar will stop the current\n> >>>> stream, and start streaming the chosen camera device if it can be\n> >>>> acquired.\n> >>>>\n> >>>> Signed-off-by: Kieran Bingham \n> >>>> ---\n> >>>> src/qcam/main_window.cpp | 60 ++++++++++++++++++++++++++++++++++++++++\n> >>>> src/qcam/main_window.h | 4 +++\n> >>>> 2 files changed, 64 insertions(+)\n> >>>>\n> >>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> >>>> index b51a16de199d..1c7260f32d0a 100644\n> >>>> --- a/src/qcam/main_window.cpp\n> >>>> +++ b/src/qcam/main_window.cpp\n> >>>> @@ -13,6 +13,8 @@\n> >>>> #include \n> >>>> #include \n> >>>> #include \n> >>>> +#include \n> >>>> +#include \n> >>>> \n> >>>> #include \n> >>>> #include \n> >>>> @@ -27,6 +29,8 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> >>>> {\n> >>>> \tint ret;\n> >>>> \n> >>>> +\tcreateToolbars(cm);\n> >>>> +\n> >>>> \ttitle_ = \"QCam \" + QString::fromStdString(CameraManager::version());\n> >>>> \tsetWindowTitle(title_);\n> >>>> \tconnect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));\n> >>>> @@ -53,6 +57,31 @@ MainWindow::~MainWindow()\n> >>>> \t}\n> >>>> }\n> >>>> \n> >>>> +int MainWindow::createToolbars(CameraManager *cm)\n> >>>> +{\n> >>>> +\tQAction *action;\n> >>>> +\n> >>>> +\ttoolbar_ = addToolBar(\"\");\n> >>>\n> >>> You should give a name to the toolbar, otherwise the context menu will\n> >>> have an empty entry. You can call it \"Main\" or anything similar to start\n> >>> with.\n> >>\n> >> Which context menu?\n> >>\n> >> I'm not sure I understand the need. I mean, I don't care, I can add the\n> >> name - I just can't see /why/ a toolbar needs a name :-)\n> >>\n> >> Ugh ... I see right clicking on the toolbar lets you hide it and then\n> >> you can't get it back again. So /that's/ the context menu ...\n> >>\n> >> Should the toolbar be 'permanant'? Or removable - and if removable, how\n> >> would we expect to get it back. Keyboard shortcut?\n> > \n> > If you can make it permanent I think that would be best.\n> \n> Agreed.\n> \n> >> Perhaps removable is useful to be able to simplify the view - but as\n> >> this is just a test tool - I don't see the benefit yet.\n> >>\n> >> I'll try to look at disabling the context menu or making the main\n> >> toolbar permanent.\n> >>\n> >>>> +\n> >>>> +\taction = toolbar_->addAction(\"Quit\");\n> >>>> +\tconnect(action, &QAction::triggered, this, &MainWindow::quit);\n> >>>> +\n> >>>> +\tQAction *cameraAction = new QAction(\"&Cameras\", this);\n> >>>> +\ttoolbar_->addAction(cameraAction);\n> >>>> +\n> >>>> +\tQToolButton *cameraButton = dynamic_cast(toolbar_->widgetForAction(cameraAction));\n> >>>> +\n> >>>> +\tcameraButton->setPopupMode(QToolButton::InstantPopup);\n> >>>\n> >>> Any way we could remove the \"Camera\" entry from the popup menu ? It\n> >>> seems we may have to insert a manually-created QComboBox. My initial\n> >>> expriment resulted in the following code. Feel free to fold it in,\n> >>> modify it, or ignore it if you think it's not a good idea.\n> >>\n> >> I would actually like this entry to show the current camera in the toolbar.\n> >>\n> >> And yes the duplicated entry is annoying.\n> >>\n> >> But I haven't got as far as dealing with such UX issues.\n> >> I was focussing on getting the core code to work.\n> >>\n> >> I'll work through your code and see what how it integrates.\n> >>\n> >>\n> >> Hrm ... one part I don't like about the below is selecting a camera by\n> >> index. That seems quite fragile once we have hotplug support ?\n> > \n> > Agreed, but once we have hotplug support we'll have to remove and add\n> > entries from the combo box or popup menu, so refactoring will be needed\n> > anyway. I think hotplug support will also require stable names/IDs, so\n> > we should then have the tool we need to do the job.\n> \n> I think my point is - we already have one at this level. A shared_ptr to\n> the Camera instance. That will always point to the same camera, and\n> never change.\n> \n> And in the event that the camera is removed, then that Camera will be\n> marked disconnected, so it will fail to stream - but the 'object' will\n> be safely preserved until there are no users left?\n\nCorrect, but I think we'll have to grey it out in the menu to make sure\nit can't be selected, remove it when appropriate, and handle all other\nkinds of GUI niceties. As the position in the cameras array won't be a\nstable key anymore, we'll have to lookup combo box entries through a\ndifferent mean, requiring some form of stable and unique ID. That will\nresult in quite a bit of refactoring, that's why I'm not concerned about\nusing the index of now.\n\n> >>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> >>> index 0e994b1e9197..f68b7abccda6 100644\n> >>> --- a/src/qcam/main_window.cpp\n> >>> +++ b/src/qcam/main_window.cpp\n> >>> @@ -10,6 +10,7 @@\n> >>> #include \n> >>> #include \n> >>>\n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> #include \n> >>> @@ -29,11 +30,11 @@\n> >>> using namespace libcamera;\n> >>>\n> >>> MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> >>> -\t: options_(options), allocator_(nullptr), isCapturing_(false)\n> >>> +\t: cm_(cm), options_(options), allocator_(nullptr), isCapturing_(false)\n> >>> {\n> >>> \tint ret;\n> >>>\n> >>> -\tcreateToolbars(cm);\n> >>> +\tcreateToolbars();\n> >>>\n> >>> \ttitle_ = \"QCam \" + QString::fromStdString(CameraManager::version());\n> >>> \tsetWindowTitle(title_);\n> >>> @@ -43,7 +44,7 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> >>> \tsetCentralWidget(viewfinder_);\n> >>> \tadjustSize();\n> >>>\n> >>> -\tret = openCamera(cm);\n> >>> +\tret = openCamera();\n> >>> \tif (!ret) {\n> >>> \t\tret = startCapture();\n> >>> \t}\n> >>> @@ -61,7 +62,7 @@ MainWindow::~MainWindow()\n> >>> \t}\n> >>> }\n> >>>\n> >>> -int MainWindow::createToolbars(CameraManager *cm)\n> >>> +int MainWindow::createToolbars()\n> >>> {\n> >>> \tQAction *action;\n> >>>\n> >>> @@ -70,18 +71,14 @@ int MainWindow::createToolbars(CameraManager *cm)\n> >>> \taction = toolbar_->addAction(QIcon(\":x-circle.svg\"), \"Quit\");\n> >>> \tconnect(action, &QAction::triggered, this, &MainWindow::quit);\n> >>>\n> >>> -\tQAction *cameraAction = new QAction(\"&Cameras\", this);\n> >>> -\ttoolbar_->addAction(cameraAction);\n> >>> +\tQComboBox *cameraCombo = new QComboBox();\n> >>> +\tconnect(cameraCombo, QOverload::of(&QComboBox::activated),\n> >>> +\t\tthis, &MainWindow::setCamera);\n> >>>\n> >>> -\tQToolButton *cameraButton = dynamic_cast(toolbar_->widgetForAction(cameraAction));\n> >>> +\tfor (const std::shared_ptr &cam : cm_->cameras())\n> >>> +\t\tcameraCombo->addItem(QString::fromStdString(cam->name()));\n> >>>\n> >>> -\tcameraButton->setPopupMode(QToolButton::InstantPopup);\n> >>> -\n> >>> -\tfor (const std::shared_ptr &cam : cm->cameras()) {\n> >>> -\t\taction = new QAction(QString::fromStdString(cam->name()));\n> >>> -\t\tcameraButton->addAction(action);\n> >>> -\t\tconnect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });\n> >>> -\t}\n> >>> +\ttoolbar_->addWidget(cameraCombo);\n> >>>\n> >>> \taction = toolbar_->addAction(QIcon(\":play-circle.svg\"), \"start\");\n> >>> \tconnect(action, &QAction::triggered, this, &MainWindow::startCapture);\n> >>> @@ -117,13 +114,19 @@ void MainWindow::updateTitle()\n> >>> \tsetWindowTitle(title_ + \" : \" + QString::number(fps, 'f', 2) + \" fps\");\n> >>> }\n> >>>\n> >>> -int MainWindow::setCamera(const std::shared_ptr &cam)\n> >>> +void MainWindow::setCamera(int index)\n> >>> {\n> >>> +\tconst auto &cameras = cm_->cameras();\n> >>> +\tif (static_cast(index) >= cameras.size())\n> >>> +\t\treturn;\n> >>> +\n> >>> +\tstd::shared_ptr cam = cameras[index];\n> >>> +\n> >>> \tstd::cout << \"Chose \" << cam->name() << std::endl;\n> >>\n> >> I'll remove this debug print and print the camera name if it fails to\n> >> acquire.\n> >>\n> >>> \tif (cam->acquire()) {\n> >>> \t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> >>> -\t\treturn -EBUSY;\n> >>> +\t\treturn;\n> >>> \t}\n> >>>\n> >>> \tstd::cout << \"Switching to camera \" << cam->name() << std::endl;\n> >>> @@ -144,19 +147,17 @@ int MainWindow::setCamera(const std::shared_ptr &cam)\n> >>> \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n> >>>\n> >>> \tstartCapture();\n> >>> -\n> >>> -\treturn 0;\n> >>> }\n> >>>\n> >>> -std::string MainWindow::chooseCamera(CameraManager *cm)\n> >>> +std::string MainWindow::chooseCamera()\n> >>> {\n> >>> \tQStringList cameras;\n> >>> \tbool result;\n> >>>\n> >>> -\tif (cm->cameras().size() == 1)\n> >>> -\t\treturn cm->cameras()[0]->name();\n> >>> +\tif (cm_->cameras().size() == 1)\n> >>> +\t\treturn cm_->cameras()[0]->name();\n> >>>\n> >>> -\tfor (const std::shared_ptr &cam : cm->cameras())\n> >>> +\tfor (const std::shared_ptr &cam : cm_->cameras())\n> >>> \t\tcameras.append(QString::fromStdString(cam->name()));\n> >>>\n> >>> \tQString name = QInputDialog::getItem(this, \"Select Camera\",\n> >>> @@ -168,19 +169,19 @@ std::string MainWindow::chooseCamera(CameraManager *cm)\n> >>> \treturn name.toStdString();\n> >>> }\n> >>>\n> >>> -int MainWindow::openCamera(CameraManager *cm)\n> >>> +int MainWindow::openCamera()\n> >>> {\n> >>> \tstd::string cameraName;\n> >>>\n> >>> \tif (options_.isSet(OptCamera))\n> >>> \t\tcameraName = static_cast(options_[OptCamera]);\n> >>> \telse\n> >>> -\t\tcameraName = chooseCamera(cm);\n> >>> +\t\tcameraName = chooseCamera();\n> >>>\n> >>> \tif (cameraName == \"\")\n> >>> \t\treturn -EINVAL;\n> >>>\n> >>> -\tcamera_ = cm->get(cameraName);\n> >>> +\tcamera_ = cm_->get(cameraName);\n> >>> \tif (!camera_) {\n> >>> \t\tstd::cout << \"Camera \" << cameraName << \" not found\"\n> >>> \t\t\t << std::endl;\n> >>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> >>> index fc85b6a46491..49af0f6ad934 100644\n> >>> --- a/src/qcam/main_window.h\n> >>> +++ b/src/qcam/main_window.h\n> >>> @@ -44,19 +44,21 @@ private Q_SLOTS:\n> >>> \tvoid quit();\n> >>> \tvoid updateTitle();\n> >>>\n> >>> -\tint setCamera(const std::shared_ptr &cam);\n> >>> +\tvoid setCamera(int index);\n> >>>\n> >>> \tint startCapture();\n> >>> \tvoid stopCapture();\n> >>> \tvoid saveImage();\n> >>>\n> >>> private:\n> >>> -\tint createToolbars(CameraManager *cm);\n> >>> -\tstd::string chooseCamera(CameraManager *cm);\n> >>> -\tint openCamera(CameraManager *cm);\n> >>> +\tint createToolbars();\n> >>> +\tstd::string chooseCamera();\n> >>> +\tint openCamera();\n> >>> \tvoid requestComplete(Request *request);\n> >>> \tint display(FrameBuffer *buffer);\n> >>>\n> >>> +\tCameraManager *cm_;\n> >>> +\n> >>> \tQString title_;\n> >>> \tQTimer titleTimer_;\n> >>>\n> >>>> +\n> >>>> +\tfor (const std::shared_ptr &cam : cm->cameras()) {\n> >>>> +\t\taction = new QAction(QString::fromStdString(cam->name()));\n> >>>> +\t\tcameraButton->addAction(action);\n> >>>> +\t\tconnect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });\n> >>>\n> >>> Are you aware that this will create local copies of the\n> >>> std::shared_ptr for each camera ? Probably not a big deal.\n> >>\n> >> Yes, I thought that was needed to make sure they remain in scope. And as\n> >> they are shared_ptr that should be fine right?\n> > \n> > I think it's OK, I just wanted to point it out as lambda capture can\n> > sometimes be confusing.\n> \n> Indeed, but the alternatives felt uglier to code.\n> Essentially I'm only using a lambda to give convenient means of passing\n> parameters through the signal event.\n> \n> If there's a 'neat' way to do so natively in SIGNAL/SLOT I'm open to\n> more suggestions, but when I looked into it, it seemed required to just\n> make an extra function which dealt with it, and at that point - a lambda\n> does that inline.\n\nIt sure does the job :-)\n\n> >> The alternative is to pass the QAction, and get the camera name from\n> >> there, or pass the cam->name() itself and then get the camera by name.\n> >>\n> >> I thought as we already have the 'Camera' this would be better - but I\n> >> can change if it's a problem.\n> >>\n> >> I see in your implementation you move to an index, but I fear this would\n> >> cause problems with hotplug support. But maybe that will be painful\n> >> anyway, and we'll have to reconstruct the camera list to update anytime\n> >> the camera list changes regardless.\n> > \n> > The reason I move to an index is becase the activated signal only gives\n> > an index. There's a way to associate a QVariant to a combo box entry,\n> > maybe we can store the shared_ptr in the QVariant, but I haven't tried\n> > that.\n> \n> If you've got the QAction* you can get the camera name from\n> QAction->text() I think...\n> \n> Oh - but perhaps you don't even have that. Perhaps I'm conflating what\n> was my first approach :-)\n\nAnd the camera name isn't unique :-)\n\n> >>>> +\t}\n> >>>> +\n> >>>> +\treturn 0;\n> >>>> +}\n> >>>> +\n> >>>> void MainWindow::quit()\n> >>>> {\n> >>>> \tQTimer::singleShot(0, QCoreApplication::instance(),\n> >>>> @@ -72,6 +101,37 @@ void MainWindow::updateTitle()\n> >>>> \tsetWindowTitle(title_ + \" : \" + QString::number(fps, 'f', 2) + \" fps\");\n> >>>> }\n> >>>> \n> >>>> +int MainWindow::setCamera(const std::shared_ptr &cam)\n> >>>\n> >>> You can make this a void function, the return value of slots is ignored\n> >>> when connected to signals.\n> >>\n> >> Ah indeed, I started out with this as a void, I must have changed it to\n> >> return int when I pulled over the code with the -EBUSY.\n> >>\n> >> But we can simply ignore that return value indeed, no action will occur.\n> >>\n> >> Later it would be nice if we had a status bar to report specific\n> >> messages through the UI. But that's a 'later' task.\n> >>\n> >> Or maybe a workshop style activity ... ;-)\n> >>\n> >>>> +{\n> >>>> +\tstd::cout << \"Chose \" << cam->name() << std::endl;\n> >>>> +\n> >>>> +\tif (cam->acquire()) {\n> >>>> +\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> >>>> +\t\treturn -EBUSY;\n> >>>> +\t}\n> >>>> +\n> >>>> +\tstd::cout << \"Switching to camera \" << cam->name() << std::endl;\n> >>>> +\n> >>>> +\tstopCapture();\n> >>>> +\tcamera_->release();\n> >>>> +\n> >>>> +\t/*\n> >>>> +\t * If we don't disconnect this signal, it will persist (and get\n> >>>> +\t * re-added and thus duplicated later if we ever switch back to an\n> >>>> +\t * previously streamed camera). This causes all sorts of pain.\n> >>>> +\t *\n> >>>> +\t * Perhaps releasing a camera should disconnect all (public?) connected\n> >>>> +\t * signals forcefully!\n> >>>\n> >>> I'm not sure that would be a good idea, implicit actions are usually\n> >>> confusing.\n> >>>\n> >>>> +\t */\n> >>>> +\tcamera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);\n> >>>> +\tcamera_ = cam;\n> >>>> +\tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n> >>>\n> >>> How about connecting the signal in startCapture() and disconnecting it\n> >>> in stopCapture() ? It will avoid the duplicated connect in openCamera().\n> >>\n> >> Aha - that's much better and clearly obvious :)\n> >>\n> >> I'll update to do so.\n> >>\n> >>>> +\n> >>>> +\tstartCapture();\n> >>>> +\n> >>>> +\treturn 0;\n> >>>> +}\n> >>>> +\n> >>>> std::string MainWindow::chooseCamera(CameraManager *cm)\n> >>>> {\n> >>>> \tQStringList cameras;\n> >>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> >>>> index a11443b30b37..f7c96fdd5c30 100644\n> >>>> --- a/src/qcam/main_window.h\n> >>>> +++ b/src/qcam/main_window.h\n> >>>> @@ -44,7 +44,10 @@ private Q_SLOTS:\n> >>>> \tvoid quit();\n> >>>> \tvoid updateTitle();\n> >>>> \n> >>>> +\tint setCamera(const std::shared_ptr &cam);\n> >>>> +\n> >>>> private:\n> >>>> +\tint createToolbars(CameraManager *cm);\n> >>>> \tstd::string chooseCamera(CameraManager *cm);\n> >>>> \tint openCamera(CameraManager *cm);\n> >>>> \n> >>>> @@ -71,6 +74,7 @@ private:\n> >>>> \tuint32_t previousFrames_;\n> >>>> \tuint32_t framesCaptured_;\n> >>>> \n> >>>> +\tQToolBar *toolbar_;\n> >>>> \tViewFinder *viewfinder_;\n> >>>> \tstd::map> mappedBuffers_;\n> >>>> };","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AFF1E60440\n\tfor ;\n\tFri, 7 Feb 2020 11:39:32 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 23F1652F;\n\tFri, 7 Feb 2020 11:39:32 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581071972;\n\tbh=PfDhIgk47dQF37yAO5nsyJ2myFzH+Pt6pc5gB6oSbvw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IdSwc7vdm/F6ZiKqQNBIRQmqLI6PnQpBy+MjlPFvOl9e9sbgo7MDz5daRus9ye2Y3\n\tGL09ANtDYjFfcutM3iqp5Qyd9Rvpe6JT13fUwV9pdz0jFfnJ+KbNINDLvlM3t2u6PO\n\tjRRU4PnmHwvkuUg/u+NoI+MzsEu0+y2Bexqjbgi0=","Date":"Fri, 7 Feb 2020 12:39:16 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20200207103916.GC4726@pendragon.ideasonboard.com>","References":"<20200206150504.24204-1-kieran.bingham@ideasonboard.com>\n\t<20200206150504.24204-4-kieran.bingham@ideasonboard.com>\n\t<20200206232852.GI7611@pendragon.ideasonboard.com>\n\t<4336eb2e-1301-2fa8-12a2-3615e489ba74@ideasonboard.com>\n\t<20200207101319.GA4726@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and\n\tcamera switching","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","X-List-Received-Date":"Fri, 07 Feb 2020 10:39:32 -0000"}},{"id":3749,"web_url":"https://patchwork.libcamera.org/comment/3749/","msgid":"","date":"2020-02-13T22:34:13","subject":"Re: [libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and\n\tcamera switching","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 07/02/2020 10:39, Laurent Pinchart wrote:\n> On Fri, Feb 07, 2020 at 10:21:48AM +0000, Kieran Bingham wrote:\n>> On 07/02/2020 10:13, Laurent Pinchart wrote:\n>>> On Fri, Feb 07, 2020 at 10:02:23AM +0000, Kieran Bingham wrote:\n>>>> On 06/02/2020 23:28, Laurent Pinchart wrote:\n>>>>> On Thu, Feb 06, 2020 at 03:05:01PM +0000, Kieran Bingham wrote:\n>>>>>> Implement a quit button, and a list of cameras.\n>>>>>>\n>>>>>> Selecting a different camera from the Toolbar will stop the current\n>>>>>> stream, and start streaming the chosen camera device if it can be\n>>>>>> acquired.\n>>>>>>\n>>>>>> Signed-off-by: Kieran Bingham \n>>>>>> ---\n>>>>>> src/qcam/main_window.cpp | 60 ++++++++++++++++++++++++++++++++++++++++\n>>>>>> src/qcam/main_window.h | 4 +++\n>>>>>> 2 files changed, 64 insertions(+)\n>>>>>>\n>>>>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n>>>>>> index b51a16de199d..1c7260f32d0a 100644\n>>>>>> --- a/src/qcam/main_window.cpp\n>>>>>> +++ b/src/qcam/main_window.cpp\n>>>>>> @@ -13,6 +13,8 @@\n>>>>>> #include \n>>>>>> #include \n>>>>>> #include \n>>>>>> +#include \n>>>>>> +#include \n>>>>>> \n>>>>>> #include \n>>>>>> #include \n>>>>>> @@ -27,6 +29,8 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n>>>>>> {\n>>>>>> \tint ret;\n>>>>>> \n>>>>>> +\tcreateToolbars(cm);\n>>>>>> +\n>>>>>> \ttitle_ = \"QCam \" + QString::fromStdString(CameraManager::version());\n>>>>>> \tsetWindowTitle(title_);\n>>>>>> \tconnect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));\n>>>>>> @@ -53,6 +57,31 @@ MainWindow::~MainWindow()\n>>>>>> \t}\n>>>>>> }\n>>>>>> \n>>>>>> +int MainWindow::createToolbars(CameraManager *cm)\n>>>>>> +{\n>>>>>> +\tQAction *action;\n>>>>>> +\n>>>>>> +\ttoolbar_ = addToolBar(\"\");\n>>>>>\n>>>>> You should give a name to the toolbar, otherwise the context menu will\n>>>>> have an empty entry. You can call it \"Main\" or anything similar to start\n>>>>> with.\n>>>>\n>>>> Which context menu?\n>>>>\n>>>> I'm not sure I understand the need. I mean, I don't care, I can add the\n>>>> name - I just can't see /why/ a toolbar needs a name :-)\n>>>>\n>>>> Ugh ... I see right clicking on the toolbar lets you hide it and then\n>>>> you can't get it back again. So /that's/ the context menu ...\n>>>>\n>>>> Should the toolbar be 'permanant'? Or removable - and if removable, how\n>>>> would we expect to get it back. Keyboard shortcut?\n>>>\n>>> If you can make it permanent I think that would be best.\n>>\n>> Agreed.\n\n\nI've figured out how to make this permanent.\n\n>>\n>>>> Perhaps removable is useful to be able to simplify the view - but as\n>>>> this is just a test tool - I don't see the benefit yet.\n>>>>\n>>>> I'll try to look at disabling the context menu or making the main\n>>>> toolbar permanent.\n>>>>\n>>>>>> +\n>>>>>> +\taction = toolbar_->addAction(\"Quit\");\n>>>>>> +\tconnect(action, &QAction::triggered, this, &MainWindow::quit);\n>>>>>> +\n>>>>>> +\tQAction *cameraAction = new QAction(\"&Cameras\", this);\n>>>>>> +\ttoolbar_->addAction(cameraAction);\n>>>>>> +\n>>>>>> +\tQToolButton *cameraButton = dynamic_cast(toolbar_->widgetForAction(cameraAction));\n>>>>>> +\n>>>>>> +\tcameraButton->setPopupMode(QToolButton::InstantPopup);\n>>>>>\n>>>>> Any way we could remove the \"Camera\" entry from the popup menu ? It\n>>>>> seems we may have to insert a manually-created QComboBox. My initial\n>>>>> expriment resulted in the following code. Feel free to fold it in,\n>>>>> modify it, or ignore it if you think it's not a good idea.\n>>>>\n>>>> I would actually like this entry to show the current camera in the toolbar.\n>>>>\n>>>> And yes the duplicated entry is annoying.\n>>>>\n>>>> But I haven't got as far as dealing with such UX issues.\n>>>> I was focussing on getting the core code to work.\n>>>>\n>>>> I'll work through your code and see what how it integrates.\n>>>>\n>>>>\n>>>> Hrm ... one part I don't like about the below is selecting a camera by\n>>>> index. That seems quite fragile once we have hotplug support ?\n>>>\n>>> Agreed, but once we have hotplug support we'll have to remove and add\n>>> entries from the combo box or popup menu, so refactoring will be needed\n>>> anyway. I think hotplug support will also require stable names/IDs, so\n>>> we should then have the tool we need to do the job.\n>>\n>> I think my point is - we already have one at this level. A shared_ptr to\n>> the Camera instance. That will always point to the same camera, and\n>> never change.\n>>\n>> And in the event that the camera is removed, then that Camera will be\n>> marked disconnected, so it will fail to stream - but the 'object' will\n>> be safely preserved until there are no users left?\n> \n> Correct, but I think we'll have to grey it out in the menu to make sure\n> it can't be selected, remove it when appropriate, and handle all other\n> kinds of GUI niceties. As the position in the cameras array won't be a\n> stable key anymore, we'll have to lookup combo box entries through a\n> different mean, requiring some form of stable and unique ID. That will\n> result in quite a bit of refactoring, that's why I'm not concerned about\n> using the index of now.\n\nQComboBox from below looks nice.\nMerged in to the patch.\n\n\n>>>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n>>>>> index 0e994b1e9197..f68b7abccda6 100644\n>>>>> --- a/src/qcam/main_window.cpp\n>>>>> +++ b/src/qcam/main_window.cpp\n>>>>> @@ -10,6 +10,7 @@\n>>>>> #include \n>>>>> #include \n>>>>>\n>>>>> +#include \n>>>>> #include \n>>>>> #include \n>>>>> #include \n>>>>> @@ -29,11 +30,11 @@\n>>>>> using namespace libcamera;\n>>>>>\n>>>>> MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n>>>>> -\t: options_(options), allocator_(nullptr), isCapturing_(false)\n>>>>> +\t: cm_(cm), options_(options), allocator_(nullptr), isCapturing_(false)\n>>>>> {\n>>>>> \tint ret;\n>>>>>\n>>>>> -\tcreateToolbars(cm);\n>>>>> +\tcreateToolbars();\n>>>>>\n>>>>> \ttitle_ = \"QCam \" + QString::fromStdString(CameraManager::version());\n>>>>> \tsetWindowTitle(title_);\n>>>>> @@ -43,7 +44,7 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n>>>>> \tsetCentralWidget(viewfinder_);\n>>>>> \tadjustSize();\n>>>>>\n>>>>> -\tret = openCamera(cm);\n>>>>> +\tret = openCamera();\n>>>>> \tif (!ret) {\n>>>>> \t\tret = startCapture();\n>>>>> \t}\n>>>>> @@ -61,7 +62,7 @@ MainWindow::~MainWindow()\n>>>>> \t}\n>>>>> }\n>>>>>\n>>>>> -int MainWindow::createToolbars(CameraManager *cm)\n>>>>> +int MainWindow::createToolbars()\n>>>>> {\n>>>>> \tQAction *action;\n>>>>>\n>>>>> @@ -70,18 +71,14 @@ int MainWindow::createToolbars(CameraManager *cm)\n>>>>> \taction = toolbar_->addAction(QIcon(\":x-circle.svg\"), \"Quit\");\n>>>>> \tconnect(action, &QAction::triggered, this, &MainWindow::quit);\n>>>>>\n>>>>> -\tQAction *cameraAction = new QAction(\"&Cameras\", this);\n>>>>> -\ttoolbar_->addAction(cameraAction);\n>>>>> +\tQComboBox *cameraCombo = new QComboBox();\n>>>>> +\tconnect(cameraCombo, QOverload::of(&QComboBox::activated),\n>>>>> +\t\tthis, &MainWindow::setCamera);\n>>>>>\n>>>>> -\tQToolButton *cameraButton = dynamic_cast(toolbar_->widgetForAction(cameraAction));\n>>>>> +\tfor (const std::shared_ptr &cam : cm_->cameras())\n>>>>> +\t\tcameraCombo->addItem(QString::fromStdString(cam->name()));\n>>>>>\n>>>>> -\tcameraButton->setPopupMode(QToolButton::InstantPopup);\n>>>>> -\n>>>>> -\tfor (const std::shared_ptr &cam : cm->cameras()) {\n>>>>> -\t\taction = new QAction(QString::fromStdString(cam->name()));\n>>>>> -\t\tcameraButton->addAction(action);\n>>>>> -\t\tconnect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });\n>>>>> -\t}\n>>>>> +\ttoolbar_->addWidget(cameraCombo);\n>>>>>\n>>>>> \taction = toolbar_->addAction(QIcon(\":play-circle.svg\"), \"start\");\n>>>>> \tconnect(action, &QAction::triggered, this, &MainWindow::startCapture);\n>>>>> @@ -117,13 +114,19 @@ void MainWindow::updateTitle()\n>>>>> \tsetWindowTitle(title_ + \" : \" + QString::number(fps, 'f', 2) + \" fps\");\n>>>>> }\n>>>>>\n>>>>> -int MainWindow::setCamera(const std::shared_ptr &cam)\n>>>>> +void MainWindow::setCamera(int index)\n>>>>> {\n>>>>> +\tconst auto &cameras = cm_->cameras();\n>>>>> +\tif (static_cast(index) >= cameras.size())\n>>>>> +\t\treturn;\n>>>>> +\n>>>>> +\tstd::shared_ptr cam = cameras[index];\n>>>>> +\n>>>>> \tstd::cout << \"Chose \" << cam->name() << std::endl;\n>>>>\n>>>> I'll remove this debug print and print the camera name if it fails to\n>>>> acquire.\n>>>>\n>>>>> \tif (cam->acquire()) {\n>>>>> \t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n>>>>> -\t\treturn -EBUSY;\n>>>>> +\t\treturn;\n>>>>> \t}\n>>>>>\n>>>>> \tstd::cout << \"Switching to camera \" << cam->name() << std::endl;\n>>>>> @@ -144,19 +147,17 @@ int MainWindow::setCamera(const std::shared_ptr &cam)\n>>>>> \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n>>>>>\n>>>>> \tstartCapture();\n>>>>> -\n>>>>> -\treturn 0;\n>>>>> }\n>>>>>\n>>>>> -std::string MainWindow::chooseCamera(CameraManager *cm)\n>>>>> +std::string MainWindow::chooseCamera()\n>>>>> {\n>>>>> \tQStringList cameras;\n>>>>> \tbool result;\n>>>>>\n>>>>> -\tif (cm->cameras().size() == 1)\n>>>>> -\t\treturn cm->cameras()[0]->name();\n>>>>> +\tif (cm_->cameras().size() == 1)\n>>>>> +\t\treturn cm_->cameras()[0]->name();\n>>>>>\n>>>>> -\tfor (const std::shared_ptr &cam : cm->cameras())\n>>>>> +\tfor (const std::shared_ptr &cam : cm_->cameras())\n>>>>> \t\tcameras.append(QString::fromStdString(cam->name()));\n>>>>>\n>>>>> \tQString name = QInputDialog::getItem(this, \"Select Camera\",\n>>>>> @@ -168,19 +169,19 @@ std::string MainWindow::chooseCamera(CameraManager *cm)\n>>>>> \treturn name.toStdString();\n>>>>> }\n>>>>>\n>>>>> -int MainWindow::openCamera(CameraManager *cm)\n>>>>> +int MainWindow::openCamera()\n>>>>> {\n>>>>> \tstd::string cameraName;\n>>>>>\n>>>>> \tif (options_.isSet(OptCamera))\n>>>>> \t\tcameraName = static_cast(options_[OptCamera]);\n>>>>> \telse\n>>>>> -\t\tcameraName = chooseCamera(cm);\n>>>>> +\t\tcameraName = chooseCamera();\n>>>>>\n>>>>> \tif (cameraName == \"\")\n>>>>> \t\treturn -EINVAL;\n>>>>>\n>>>>> -\tcamera_ = cm->get(cameraName);\n>>>>> +\tcamera_ = cm_->get(cameraName);\n>>>>> \tif (!camera_) {\n>>>>> \t\tstd::cout << \"Camera \" << cameraName << \" not found\"\n>>>>> \t\t\t << std::endl;\n>>>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n>>>>> index fc85b6a46491..49af0f6ad934 100644\n>>>>> --- a/src/qcam/main_window.h\n>>>>> +++ b/src/qcam/main_window.h\n>>>>> @@ -44,19 +44,21 @@ private Q_SLOTS:\n>>>>> \tvoid quit();\n>>>>> \tvoid updateTitle();\n>>>>>\n>>>>> -\tint setCamera(const std::shared_ptr &cam);\n>>>>> +\tvoid setCamera(int index);\n>>>>>\n>>>>> \tint startCapture();\n>>>>> \tvoid stopCapture();\n>>>>> \tvoid saveImage();\n>>>>>\n>>>>> private:\n>>>>> -\tint createToolbars(CameraManager *cm);\n>>>>> -\tstd::string chooseCamera(CameraManager *cm);\n>>>>> -\tint openCamera(CameraManager *cm);\n>>>>> +\tint createToolbars();\n>>>>> +\tstd::string chooseCamera();\n>>>>> +\tint openCamera();\n>>>>> \tvoid requestComplete(Request *request);\n>>>>> \tint display(FrameBuffer *buffer);\n>>>>>\n>>>>> +\tCameraManager *cm_;\n>>>>> +\n>>>>> \tQString title_;\n>>>>> \tQTimer titleTimer_;\n>>>>>\n>>>>>> +\n>>>>>> +\tfor (const std::shared_ptr &cam : cm->cameras()) {\n>>>>>> +\t\taction = new QAction(QString::fromStdString(cam->name()));\n>>>>>> +\t\tcameraButton->addAction(action);\n>>>>>> +\t\tconnect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });\n>>>>>\n>>>>> Are you aware that this will create local copies of the\n>>>>> std::shared_ptr for each camera ? Probably not a big deal.\n>>>>\n>>>> Yes, I thought that was needed to make sure they remain in scope. And as\n>>>> they are shared_ptr that should be fine right?\n>>>\n>>> I think it's OK, I just wanted to point it out as lambda capture can\n>>> sometimes be confusing.\n>>\n>> Indeed, but the alternatives felt uglier to code.\n>> Essentially I'm only using a lambda to give convenient means of passing\n>> parameters through the signal event.\n>>\n>> If there's a 'neat' way to do so natively in SIGNAL/SLOT I'm open to\n>> more suggestions, but when I looked into it, it seemed required to just\n>> make an extra function which dealt with it, and at that point - a lambda\n>> does that inline.\n> \n> It sure does the job :-)\n> \n>>>> The alternative is to pass the QAction, and get the camera name from\n>>>> there, or pass the cam->name() itself and then get the camera by name.\n>>>>\n>>>> I thought as we already have the 'Camera' this would be better - but I\n>>>> can change if it's a problem.\n>>>>\n>>>> I see in your implementation you move to an index, but I fear this would\n>>>> cause problems with hotplug support. But maybe that will be painful\n>>>> anyway, and we'll have to reconstruct the camera list to update anytime\n>>>> the camera list changes regardless.\n>>>\n>>> The reason I move to an index is becase the activated signal only gives\n>>> an index. There's a way to associate a QVariant to a combo box entry,\n>>> maybe we can store the shared_ptr in the QVariant, but I haven't tried\n>>> that.\n>>\n>> If you've got the QAction* you can get the camera name from\n>> QAction->text() I think...\n>>\n>> Oh - but perhaps you don't even have that. Perhaps I'm conflating what\n>> was my first approach :-)\n> \n> And the camera name isn't unique :-)\n> \n>>>>>> +\t}\n>>>>>> +\n>>>>>> +\treturn 0;\n>>>>>> +}\n>>>>>> +\n>>>>>> void MainWindow::quit()\n>>>>>> {\n>>>>>> \tQTimer::singleShot(0, QCoreApplication::instance(),\n>>>>>> @@ -72,6 +101,37 @@ void MainWindow::updateTitle()\n>>>>>> \tsetWindowTitle(title_ + \" : \" + QString::number(fps, 'f', 2) + \" fps\");\n>>>>>> }\n>>>>>> \n>>>>>> +int MainWindow::setCamera(const std::shared_ptr &cam)\n>>>>>\n>>>>> You can make this a void function, the return value of slots is ignored\n>>>>> when connected to signals.\n>>>>\n>>>> Ah indeed, I started out with this as a void, I must have changed it to\n>>>> return int when I pulled over the code with the -EBUSY.\n>>>>\n>>>> But we can simply ignore that return value indeed, no action will occur.\n>>>>\n>>>> Later it would be nice if we had a status bar to report specific\n>>>> messages through the UI. But that's a 'later' task.\n>>>>\n>>>> Or maybe a workshop style activity ... ;-)\n>>>>\n>>>>>> +{\n>>>>>> +\tstd::cout << \"Chose \" << cam->name() << std::endl;\n>>>>>> +\n>>>>>> +\tif (cam->acquire()) {\n>>>>>> +\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n>>>>>> +\t\treturn -EBUSY;\n>>>>>> +\t}\n>>>>>> +\n>>>>>> +\tstd::cout << \"Switching to camera \" << cam->name() << std::endl;\n>>>>>> +\n>>>>>> +\tstopCapture();\n>>>>>> +\tcamera_->release();\n>>>>>> +\n>>>>>> +\t/*\n>>>>>> +\t * If we don't disconnect this signal, it will persist (and get\n>>>>>> +\t * re-added and thus duplicated later if we ever switch back to an\n>>>>>> +\t * previously streamed camera). This causes all sorts of pain.\n>>>>>> +\t *\n>>>>>> +\t * Perhaps releasing a camera should disconnect all (public?) connected\n>>>>>> +\t * signals forcefully!\n>>>>>\n>>>>> I'm not sure that would be a good idea, implicit actions are usually\n>>>>> confusing.\n>>>>>>>>>>> +\t */\n>>>>>> +\tcamera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);\n>>>>>> +\tcamera_ = cam;\n>>>>>> +\tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n>>>>>\n>>>>> How about connecting the signal in startCapture() and disconnecting it\n>>>>> in stopCapture() ? It will avoid the duplicated connect in openCamera().\n>>>>\n>>>> Aha - that's much better and clearly obvious :)\n>>>>\n>>>> I'll update to do so.\n\nThis does indeed work much better from startCapture and removing in\nstopCapture.\n\n>>>>\n>>>>>> +\n>>>>>> +\tstartCapture();\n>>>>>> +\n>>>>>> +\treturn 0;\n>>>>>> +}\n>>>>>> +\n>>>>>> std::string MainWindow::chooseCamera(CameraManager *cm)\n>>>>>> {\n>>>>>> \tQStringList cameras;\n>>>>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n>>>>>> index a11443b30b37..f7c96fdd5c30 100644\n>>>>>> --- a/src/qcam/main_window.h\n>>>>>> +++ b/src/qcam/main_window.h\n>>>>>> @@ -44,7 +44,10 @@ private Q_SLOTS:\n>>>>>> \tvoid quit();\n>>>>>> \tvoid updateTitle();\n>>>>>> \n>>>>>> +\tint setCamera(const std::shared_ptr &cam);\n>>>>>> +\n>>>>>> private:\n>>>>>> +\tint createToolbars(CameraManager *cm);\n>>>>>> \tstd::string chooseCamera(CameraManager *cm);\n>>>>>> \tint openCamera(CameraManager *cm);\n>>>>>> \n>>>>>> @@ -71,6 +74,7 @@ private:\n>>>>>> \tuint32_t previousFrames_;\n>>>>>> \tuint32_t framesCaptured_;\n>>>>>> \n>>>>>> +\tQToolBar *toolbar_;\n>>>>>> \tViewFinder *viewfinder_;\n>>>>>> \tstd::map> mappedBuffers_;\n>>>>>> };\n>","headers":{"Return-Path":"","Received":["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 78DFF61A10\n\tfor ;\n\tThu, 13 Feb 2020 23:34:16 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D12E5504;\n\tThu, 13 Feb 2020 23:34:15 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581633256;\n\tbh=VDR3uFZ1od9f8C7mCHWim74uTQl/xuE7/DKOkadHT1Y=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=FQpUQ45agNfWGACSKzrdgxYsrrrungbZZvwvgIJwUP8WR3Co0f9IyGrHlo+oE6Fvl\n\t0d3KGzaj1wm3voiabAwXwdRcKXbTUGp2M7yKkQ94kGw2oFyx+j827BfZrto8AhpXXg\n\t8bj5VZWXfowRiefMMrnAVK+LEAscytqfuApnlfNE=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20200206150504.24204-1-kieran.bingham@ideasonboard.com>\n\t<20200206150504.24204-4-kieran.bingham@ideasonboard.com>\n\t<20200206232852.GI7611@pendragon.ideasonboard.com>\n\t<4336eb2e-1301-2fa8-12a2-3615e489ba74@ideasonboard.com>\n\t<20200207101319.GA4726@pendragon.ideasonboard.com>\n\t\n\t<20200207103916.GC4726@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 13 Feb 2020 22:34:13 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200207103916.GC4726@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and\n\tcamera switching","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","X-List-Received-Date":"Thu, 13 Feb 2020 22:34:16 -0000"}}]