[libcamera-devel] [RFC PATCH] Add CameraName to the MainWindow

[Umang Jain]

Hi Utkarsh

Thank you for the patch.

On 3/19/22 19:55, Utkarsh Tiwari via libcamera-devel wrote:
> Showing Camera Id on the main window is quite non-intuitive for the user
>
> Implementation: A duplicate function exists in cam/main.cpp . There must be a better way to do this instead of duplication of code
> Signed-off-by: Utkarsh Tiwari <utkarsh02t at gmail.com>
> ---
>   src/qcam/main_window.cpp | 61 +++++++++++++++++++++++++++++++++-------
>   src/qcam/main_window.h   |  2 +-
>   2 files changed, 52 insertions(+), 11 deletions(-)
>
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index dd0e51f5..35e737c7 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -12,8 +12,10 @@
>   #include <string>
>   
>   #include <libcamera/camera_manager.h>
> +#include <libcamera/property_ids.h>
>   #include <libcamera/version.h>
>   
> +
spurious new line
>   #include <QComboBox>
>   #include <QCoreApplication>
>   #include <QFileDialog>
> @@ -197,7 +199,7 @@ int MainWindow::createToolbars()
>   		this, &MainWindow::switchCamera);
>   
>   	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> -		cameraCombo_->addItem(QString::fromStdString(cam->id()));
> +		cameraCombo_->addItem(QString::fromStdString(cameraName(cam.get())));
>   
>   	toolbar_->addWidget(cameraCombo_);
>   
> @@ -287,6 +289,45 @@ void MainWindow::switchCamera(int index)
>   	startStopAction_->setChecked(true);
>   }
>   
> +std::string MainWindow::cameraName(const libcamera::Camera *camera)
> +{
> +	const ControlList &props = camera->properties();
> +	bool addModel = true;
> +	std::string name;
> +
> +	/*
> +	 * Construct the name from the camera location, model and ID. The model
> +	 * is only used if the location isn't present or is set to External.
> +	 */
> +	if (props.contains(properties::Location)) {
> +		switch (props.get(properties::Location)) {
> +		case properties::CameraLocationFront:
> +			addModel = false;
> +			name = "Internal front camera ";
> +			break;
> +		case properties::CameraLocationBack:
> +			addModel = false;
> +			name = "Internal back camera ";
> +			break;
> +		case properties::CameraLocationExternal:
> +			name = "External camera ";
> +			break;
> +		}
> +	}
> +
> +	if (addModel && props.contains(properties::Model)) {
> +		/*
> +		 * If the camera location is not availble use the camera model
> +		 * to build the camera name.
> +		 */
> +		name = "'" + props.get(properties::Model) + "' ";
> +	}
> +
> +	name += "(" + camera->id() + ")";
> +
> +	return name;
> +}
> +


I get it that this is copied over from cam/main.cpp but since we are 
trying to keep the camera names construction in-sync across applications 
(cam and qcam), have you thought or discussed the possibility that this 
helper can go in libcamera::Camera class itself ? For e.g. as

Camera::name()

OR

Camera::displayName()

as a helper provided by libcamera. Applications not happy with this 
helper (given out by libcamera) can still construct/append their own 
quirks based on id(), location-property and so on.. as it is done right 
now anyway.

I don't know, this might require some discussion but would simplify 
things a bit.

>   std::string MainWindow::chooseCamera()
>   {
>   	QStringList cameras;
> @@ -311,24 +352,24 @@ std::string MainWindow::chooseCamera()
>   
>   int MainWindow::openCamera()
>   {
> -	std::string cameraName;
> +	std::string cameraId;
>   
>   	/*
>   	 * Use the camera specified on the command line, if any, or display the
>   	 * camera selection dialog box otherwise.
>   	 */
>   	if (options_.isSet(OptCamera))
> -		cameraName = static_cast<std::string>(options_[OptCamera]);
> +		cameraId = static_cast<std::string>(options_[OptCamera]);
>   	else
> -		cameraName = chooseCamera();
> +		cameraId = chooseCamera();
>   
> -	if (cameraName == "")
> +	if (cameraId == "")
>   		return -EINVAL;
>   
>   	/* Get and acquire the camera. */
> -	camera_ = cm_->get(cameraName);
> +	camera_ = cm_->get(cameraId);
>   	if (!camera_) {
> -		qInfo() << "Camera" << cameraName.c_str() << "not found";
> +		qInfo() << "Camera" << cameraId.c_str() << "not found";
>   		return -ENODEV;
>   	}


This hunk is a variable rename so should be separated out in a different 
patch I think.

>   
> @@ -339,7 +380,7 @@ int MainWindow::openCamera()
>   	}
>   
>   	/* Set the combo-box entry with the currently selected Camera. */
> -	cameraCombo_->setCurrentText(QString::fromStdString(cameraName));
> +	cameraCombo_->setCurrentText(QString::fromStdString(cameraName(camera_.get())));
>   
>   	return 0;
>   }
> @@ -604,7 +645,7 @@ void MainWindow::processHotplug(HotplugEvent *e)
>   	HotplugEvent::PlugEvent event = e->hotplugEvent();
>   
>   	if (event == HotplugEvent::HotPlug) {
> -		cameraCombo_->addItem(QString::fromStdString(camera->id()));
> +		cameraCombo_->addItem(QString::fromStdString(cameraName(camera)));
>   	} else if (event == HotplugEvent::HotUnplug) {
>   		/* Check if the currently-streaming camera is removed. */
>   		if (camera == camera_.get()) {
> @@ -614,7 +655,7 @@ void MainWindow::processHotplug(HotplugEvent *e)
>   			cameraCombo_->setCurrentIndex(0);
>   		}
>   
> -		int camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));
> +		int camIndex = cameraCombo_->findText(QString::fromStdString(cameraName(camera)));
>   		cameraCombo_->removeItem(camIndex);
>   	}
>   }
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 3fbe872c..b31c584f 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -70,7 +70,7 @@ private Q_SLOTS:
>   
>   private:
>   	int createToolbars();
> -
Spurious deletion of newline. It should be retained
> +	std::string cameraName(const libcamera::Camera *camera);
>   	std::string chooseCamera();
>   	int openCamera();
>