<div dir="auto"><div>Hi Kieran thanks for the review <br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 10 Aug, 2022, 03:14 Kieran Bingham, <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank" rel="noreferrer">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Quoting Utkarsh Tiwari (2022-08-09 21:50:38)<br>
> The camera selection dialog currently only displays the camera Id.<br>
> Display the camera location and camera model if available.<br>
> <br>
> Signed-off-by: Utkarsh Tiwari <<a href="mailto:utkarsh02t@gmail.com" rel="noreferrer noreferrer" target="_blank">utkarsh02t@gmail.com</a>><br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" rel="noreferrer noreferrer" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> ---<br>
> Difference :<br>
> 1. Now use value_or for optional<br>
> 2. Not use lambda and make a private member function<br>
> src/qcam/cam_select_dialog.cpp | 52 ++++++++++++++++++++++++++++++++++<br>
> src/qcam/cam_select_dialog.h | 10 +++++++<br>
> 2 files changed, 62 insertions(+)<br>
> <br>
> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp<br>
> index d8982800..f97ad6eb 100644<br>
> --- a/src/qcam/cam_select_dialog.cpp<br>
> +++ b/src/qcam/cam_select_dialog.cpp<br>
> @@ -7,6 +7,7 @@<br>
> <br>
> #include "cam_select_dialog.h"<br>
> <br>
> +#include <memory><br>
> #include <string><br>
> <br>
> #include <libcamera/camera.h><br>
> @@ -30,6 +31,14 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag<br>
> for (const auto &cam : cm_->cameras())<br>
> cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));<br>
> <br>
> + /* Set camera information labels. */<br>
> + cameraLocation_ = new QLabel;<br>
> + cameraModel_ = new QLabel;<br>
> +<br>
> + handleCameraChange();<br>
> + connect(cameraIdComboBox_, &QComboBox::currentTextChanged,<br>
> + this, &CameraSelectorDialog::handleCameraChange);<br>
> +<br>
> /* Setup the QDialogButton Box */<br>
> QDialogButtonBox *buttonBox =<br>
> new QDialogButtonBox(QDialogButtonBox::Ok |<br>
> @@ -41,7 +50,10 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag<br>
> this, &QDialog::reject);<br>
> <br>
> /* Set the layout. */<br>
> +<br>
> layout->addRow("Camera:", cameraIdComboBox_);<br>
> + layout->addRow("Location:", cameraLocation_);<br>
> + layout->addRow("Model:", cameraModel_);<br>
> layout->addWidget(buttonBox);<br>
> }<br>
> <br>
> @@ -63,3 +75,43 @@ void CameraSelectorDialog::cameraRemoved(libcamera::Camera *camera)<br>
> <br>
> cameraIdComboBox_->removeItem(cameraIndex);<br>
> }<br>
> +<br>
> +/* Camera Information */<br>
> +void CameraSelectorDialog::handleCameraChange()<br>
> +{<br>
> + updateCamInfo(cm_->get(getCameraId()));<br>
> +}<br>
> +<br>
> +void CameraSelectorDialog::updateCamInfo(const std::shared_ptr<libcamera::Camera> &camera)<br>
> +{<br>
> + if (!camera)<br>
> + return;<br>
> +<br>
> + const libcamera::ControlList &cameraProperties = camera->properties();<br>
> +<br>
> + const auto &location =<br>
> + cameraProperties.get(libcamera::properties::Location);<br>
> + if (location) {<br>
> + switch (*location) {<br>
> + case libcamera::properties::CameraLocationFront:<br>
> + cameraLocation_->setText("Internal front camera");<br>
> + break;<br>
> + case libcamera::properties::CameraLocationBack:<br>
> + cameraLocation_->setText("Internal back camera");<br>
> + break;<br>
> + case libcamera::properties::CameraLocationExternal:<br>
> + cameraLocation_->setText("External camera");<br>
> + break;<br>
> + default:<br>
> + cameraLocation_->setText("Unknown");<br>
> + }<br>
> + } else {<br>
> + cameraLocation_->setText("Unknown");<br>
> + }<br>
> +<br>
> + const auto &model = cameraProperties<br>
> + .get(libcamera::properties::Model)<br>
> + .value_or("Unknown");<br>
> +<br>
> + cameraModel_->setText(QString::fromStdString(model));<br>
> +}<br>
> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h<br>
> index 567083ae..359df811 100644<br>
> --- a/src/qcam/cam_select_dialog.h<br>
> +++ b/src/qcam/cam_select_dialog.h<br>
> @@ -11,11 +11,14 @@<br>
> <br>
> #include <libcamera/camera.h><br>
> #include <libcamera/camera_manager.h><br>
> +#include <libcamera/controls.h><br>
> +#include <libcamera/property_ids.h><br>
> <br>
> #include <QComboBox><br>
> #include <QDialog><br>
> #include <QDialogButtonBox><br>
> #include <QFormLayout><br>
> +#include <QLabel><br>
> #include <QString><br>
> <br>
> class CameraSelectorDialog : public QDialog<br>
> @@ -33,9 +36,16 @@ public:<br>
> void cameraAdded(libcamera::Camera *camera);<br>
> <br>
> void cameraRemoved(libcamera::Camera *camera);<br>
> +<br>
> + /* Camera Information */<br>
> + void updateCamInfo(const std::shared_ptr<libcamera::Camera> &camera);<br>
> + void handleCameraChange();<br>
> +<br>
> private:<br>
> libcamera::CameraManager *cm_;<br>
> <br>
> /* UI elements. */<br>
> QComboBox *cameraIdComboBox_;<br>
> + QLabel *cameraLocation_;<br>
> + QLabel *cameraModel_;<br>
<br>
Do these actually need to be pointers? What happens if they are<br>
allcoated as part of the CameraSelectorDialog ? - then we wouldn't need<br>
to call 'new' and they wouldn't be leaked (well, the lifetime would be<br>
directly associated with the CameraSelectorDialog at least)<br></blockquote></div></div><div dir="auto">Actually yes, the layout needs a pointer to the widget. Also the widget</div><div dir="auto">to which the layout belongs takes the ownership of the widget. And when</div><div dir="auto">it gets destroyed these are too. </div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> };<br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div></div>