feat: The prompt images for the FLM(support camera switch) have been …#457
Conversation
Reviewer's GuideImplements enhanced camera error prompts for devices with a hardware camera switch by adding a secondary guidance text, switching to new SVG icons when camera-switch support is enabled, and wiring this behavior to a configurable DConfig flag via DataManager. Sequence diagram for startup config and camera switch prompt selectionsequenceDiagram
participant OS
participant main
participant DConfig
participant DataManager
participant videowidget
OS->>main: start application
main->>DConfig: load config file
DConfig-->>main: supportCameraSwitch value
main->>DataManager: instance()
main->>DataManager: setIsSupportCameraSwitch(supportCameraSwitch)
OS->>videowidget: construct videowidget
videowidget->>DataManager: instance()
videowidget->>DataManager: isSupportCameraSwitch()
alt supportCameraSwitch is true
videowidget->>videowidget: set camera_switch_not_on svg
videowidget->>videowidget: set m_pCamErrItemSub text
else supportCameraSwitch is false
videowidget->>videowidget: set Not_connected svg
videowidget->>videowidget: leave m_pCamErrItemSub empty
end
Class diagram for enhanced camera switch promptsclassDiagram
class DataManager {
-static DataManager* m_dataManager
-EncodeEnv m_encodeEnv
-DeviceStatus m_devStatus
-bool m_H264EncoderExists
-bool m_isSupportCameraSwitch
+static DataManager* instance()
+EncodeEnv encodeEnv()
+void setEncodeEnv(EncodeEnv env)
+void setDevStatus(DeviceStatus status)
+DeviceStatus getdevStatus()
+void setEncExists(bool status)
+bool encExists()
+void setIsSupportCameraSwitch(bool isTrue)
+bool isSupportCameraSwitch()
}
class videowidget {
-QGraphicsTextItem* m_pCamErrItem
-QGraphicsTextItem* m_pCamErrItemSub
-QGraphicsSvgItem* m_pSvgItem
-QGraphicsScene* m_pNormalScene
-QGraphicsView* m_pNormalView
-QLabel* m_flashLabel
+videowidget(DWidget* parent)
+void showNocam()
+void showCamUsed()
+void ReceiveOpenGLstatus(bool result)
+void ReceiveMajorImage(QImage* image, int result)
+void showCountDownLabel(PREVIEW_ENUM_STATE state)
+void resizeEvent(QResizeEvent* size)
+void showCountdown()
+void onEndBtnClicked()
+void onTakePic(bool bTrue)
+void onTakeVideo()
+void itemPosChange()
+void stopEverything()
+void onThemeTypeChanged(DGuiApplicationHelper_ColorType themeType)
}
videowidget ..> DataManager : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Theme- and camera-switch–dependent SVG/text setup in
showNocam()andonThemeTypeChanged()is largely duplicated; consider extracting a helper method that takes the theme/device state and configuresm_pSvgItem,m_pCamErrItem, andm_pCamErrItemSubin one place to reduce repetition and potential inconsistencies. - The secondary hint string for camera-switch devices is constructed multiple times from the same literal; define it once (e.g., as a
constexpror a static function returning the translated string) to avoid divergence if the text or translation is updated later. - Visibility management for
m_pCamErrItemandm_pCamErrItemSubis now spread across many branches with near-identical hide/show logic; adding small helpers (e.g.,showCameraError(bool showMain, bool showSub)) would make the state transitions easier to follow and less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Theme- and camera-switch–dependent SVG/text setup in `showNocam()` and `onThemeTypeChanged()` is largely duplicated; consider extracting a helper method that takes the theme/device state and configures `m_pSvgItem`, `m_pCamErrItem`, and `m_pCamErrItemSub` in one place to reduce repetition and potential inconsistencies.
- The secondary hint string for camera-switch devices is constructed multiple times from the same literal; define it once (e.g., as a `constexpr` or a static function returning the translated string) to avoid divergence if the text or translation is updated later.
- Visibility management for `m_pCamErrItem` and `m_pCamErrItemSub` is now spread across many branches with near-identical hide/show logic; adding small helpers (e.g., `showCameraError(bool showMain, bool showSub)`) would make the state transitions easier to follow and less error-prone.
## Individual Comments
### Comment 1
<location path="src/src/videowidget.cpp" line_range="330" />
<code_context>
- if (!m_pCamErrItem->isVisible())
+ if (!m_pCamErrItem->isVisible()) {
m_pCamErrItem->show();
+ m_pCamErrItemSub->show();
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Showing the secondary camera error text during countdown VIDEO state is likely unintended.
In `showCountDownLabel(PREVIEW_ENUM_STATE state)`, for the `VIDEO` case you now show `m_pCamErrItemSub` while hiding `m_pCamErrItem`. Since the secondary label is used for camera error / no-camera states, this will display error-like text during a normal countdown. Please avoid forcing `m_pCamErrItemSub->show()` here (e.g., hide it or leave it controlled solely by the error logic) to prevent conflicting UI signals.
</issue_to_address>
### Comment 2
<location path="src/src/videowidget.cpp" line_range="440-444" />
<code_context>
if (result && m_openglwidget) {
qDebug() << "OpenGL status received";
//Success
- if (m_pCamErrItem->isVisible())
+ if (m_pCamErrItem->isVisible()) {
m_pCamErrItem->hide();
+ m_pCamErrItemSub->hide();
</code_context>
<issue_to_address>
**issue (bug_risk):** Hiding the secondary error item only when the primary error item is visible can leave stale text on screen.
In `ReceiveOpenGLstatus`, `m_pCamErrItemSub->hide()` is currently guarded by `if (m_pCamErrItem->isVisible())`, but there are paths (e.g. `showCountDownLabel(VIDEO)`) where `m_pCamErrItem` is hidden while `m_pCamErrItemSub` remains visible. In those cases, a successful OpenGL init won’t clear the secondary text. Please hide `m_pCamErrItemSub` unconditionally when clearing error/placeholder overlays, or at least guard it with its own `isVisible()` check rather than tying it to `m_pCamErrItem`’s state.
</issue_to_address>
### Comment 3
<location path="src/src/videowidget.cpp" line_range="148" />
<code_context>
// 画布错误提示图元
m_pCamErrItem = new QGraphicsTextItem;
m_pCamErrItem->setZValue(2);
+ m_pCamErrItemSub = new QGraphicsTextItem;
+ m_pCamErrItemSub->setTextWidth(370);
+ m_pCamErrItemSub->document()->setDefaultTextOption(QTextOption(Qt::AlignCenter)); // 设置文本居中
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared helpers for error visibility, theming/text, and layout to avoid duplicating `m_pCamErrItem` logic for `m_pCamErrItemSub` across the widget.
You’ve effectively duplicated most of the `m_pCamErrItem` logic for `m_pCamErrItemSub`, which does increase incidental complexity and makes future changes harder (theme updates, visibility rules, layout, fonts).
You can keep all behavior and reduce duplication by centralizing:
1. **Show/hide logic** for both error items.
2. **Theme-dependent styling and text** (main + sub).
3. **Layout** for both items.
### 1. Centralize error visibility
Instead of touching both items in many places, introduce a small helper:
```cpp
// in videowidget.h
private:
void setErrorVisible(bool visible);
// in videowidget.cpp
void videowidget::setErrorVisible(bool visible)
{
m_pCamErrItem->setVisible(visible);
m_pCamErrItemSub->setVisible(visible);
m_pSvgItem->setVisible(visible);
}
```
Then replace scattered blocks like:
```cpp
if (m_pCamErrItem->isVisible()) {
m_pCamErrItem->hide();
m_pCamErrItemSub->hide();
}
if (m_pSvgItem->isVisible())
m_pSvgItem->hide();
```
with:
```cpp
setErrorVisible(false);
```
and:
```cpp
m_pCamErrItem->show();
m_pCamErrItemSub->show();
m_pSvgItem->show();
```
with:
```cpp
setErrorVisible(true);
```
This affects places like `showNocam`, `showCamUsed`, `ReceiveOpenGLstatus`, `ReceiveMajorImage`, `showCountDownLabel`, `resizeEvent`, `onEndBtnClicked`, `onTakePic`, `onTakeVideo`, `stopEverything`.
### 2. Centralize theme + text selection
The same theme logic and subtext is repeated in `showNocam` and `onThemeTypeChanged`. You can move it into one function that updates *both* items:
```cpp
// in videowidget.h
private:
void updateErrorAssets(DGuiApplicationHelper::ColorType themeType,
DevStatus devStatus); // or exact type of getdevStatus()
// in videowidget.cpp
void videowidget::updateErrorAssets(DGuiApplicationHelper::ColorType themeType,
DevStatus devStatus)
{
QString mainText;
QString subText;
const bool isNoCam = (devStatus == NOCAM);
const bool supportSwitch = DataManager::instance()->isSupportCameraSwitch();
const bool isLight = (themeType == DGuiApplicationHelper::LightType);
if (isNoCam) {
mainText = tr("No webcam found");
if (supportSwitch) {
subText = tr("Your camera might not be turned on. If you need to use it, "
"please check whether the hardware switch of the camera or "
"the camera hotkey on the keyboard is enabled");
}
const char *icon = nullptr;
if (isLight) {
icon = supportSwitch
? ":/images/icons/light/camera_switch_not_on.svg"
: ":/images/icons/light/Not connected.svg";
} else {
icon = supportSwitch
? ":/images/icons/dark/camera_switch_not_on_dark.svg"
: ":/images/icons/dark/Not connected_dark.svg";
}
m_pSvgItem->setSharedRenderer(new QSvgRenderer(icon, this));
} else { // CAM_CANNOT_USE etc.
mainText = tr("The webcam is in use");
const char *icon = isLight
? ":/images/icons/light/Take up.svg"
: ":/images/icons/dark/Take up_dark.svg";
m_pSvgItem->setSharedRenderer(new QSvgRenderer(icon, this));
subText.clear();
}
QColor clr(Qt::white);
clr.setAlphaF(isLight ? 0.8 : 0.2);
m_pCamErrItem->setDefaultTextColor(clr);
m_pCamErrItemSub->setDefaultTextColor(clr);
QFont mainFont("SourceHanSansSC");
mainFont.setWeight(QFont::DemiBold);
mainFont.setPixelSize(17);
m_pCamErrItem->setFont(mainFont);
m_pCamErrItem->setPlainText(mainText);
QFont subFont("SourceHanSansSC");
subFont.setPixelSize(12);
m_pCamErrItemSub->setFont(subFont);
m_pCamErrItemSub->setPlainText(subText);
}
```
Then:
- `showNocam()` becomes mostly “state wiring”:
```cpp
void videowidget::showNocam()
{
if (!m_pNormalView->isVisible())
m_pNormalView->show();
m_pNormalItem->hide();
if (m_openglwidget)
m_openglwidget->hide();
if (m_GridType != Grid_None) {
if (!m_pGridLineItem->isVisible())
m_pGridLineItem->show();
if (m_gridlinewidget->isVisible())
m_gridlinewidget->hide();
}
emit sigDeviceChange();
auto theme = DGuiApplicationHelper::instance()->themeType();
updateErrorAssets(theme, DataManager::instance()->getdevStatus());
m_pNormalScene->setSceneRect(m_pSvgItem->boundingRect());
m_flashLabel->hide();
itemPosChange();
setErrorVisible(true);
emit noCam();
if (getCapStatus())
onEndBtnClicked();
}
```
- `onThemeTypeChanged()` no longer re‑implements the same branches:
```cpp
void videowidget::onThemeTypeChanged(DGuiApplicationHelper::ColorType themeType)
{
if (!m_pSvgItem->isVisible())
return;
updateErrorAssets(themeType, DataManager::instance()->getdevStatus());
m_pNormalScene->setSceneRect(m_pSvgItem->boundingRect());
itemPosChange();
}
```
This keeps the feature (main + sub text, proper icon selection) but removes duplicated conditionals and font/color blocks.
### 3. Layout helper (optional small clean‑up)
`itemPosChange()` already encapsulates positioning; it can be the single place that knows about text layout:
```cpp
void videowidget::itemPosChange()
{
const auto svgRect = m_pSvgItem->boundingRect();
const auto mainRect = m_pCamErrItem->boundingRect();
const auto subRect = m_pCamErrItemSub->boundingRect();
m_pCamErrItem->setPos((svgRect.width() - mainRect.width()) / 2, svgRect.height());
m_pCamErrItemSub->setPos((svgRect.width() - subRect.width()) / 2,
svgRect.height() + mainRect.height());
m_pSvgItem->update();
m_pCamErrItem->update();
m_pCamErrItemSub->update();
}
```
Callers should only call `itemPosChange()` after changing text/fonts; they don’t need to know coordinates.
---
Applying these small helpers will keep your new secondary text line behavior while significantly reducing duplication and the chance of future inconsistencies.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
0f0fed5 to
6a6e3ca
Compare
…optimized. The prompt images for the FLM(support camera switch) have been optimized, and secondary prompts have also been added. FLM设备(带有摄像头开关)的提示图片进行了优化,同时增加了二级提示。 Task: https://pms.uniontech.com/task-view-386441.html v20 BUG 分支合一到v25主线 Task: https://pms.uniontech.com/task-view-383481.html
deepin pr auto review代码审查报告总体概述本次代码变更主要是为相机应用添加了对物理摄像头开关的支持功能,包括:
详细审查意见1. 语法逻辑1.1 SVG资源文件
1.2 配置文件
1.3 主程序逻辑
// 建议修改
if (dconfig && dconfig->isValid() && dconfig->keyList().contains("supportCameraSwitch")) {
bool supportSwitch = dconfig->value("supportCameraSwitch").toBool();
DataManager::instance()->setIsSupportCameraSwitch(supportSwitch);
} else {
// 添加默认值处理
DataManager::instance()->setIsSupportCameraSwitch(false);
}1.4 DataManager类
// 建议添加
Q_PROPERTY(bool isSupportCameraSwitch READ isSupportCameraSwitch WRITE setIsSupportCameraSwitch)1.5 VideoWidget类
// 建议添加独立方法
void videowidget::updateErrorMessages(const QString &mainMsg, const QString &subMsg)
{
m_pCamErrItem->setPlainText(mainMsg);
m_pCamErrItemSub->setPlainText(subMsg);
// 更新位置
itemPosChange();
}2. 代码质量2.1 命名规范
2.2 代码重复
// 建议添加
void videowidget::updateCameraErrorDisplay()
{
QString mainMsg, subMsg;
if (DataManager::instance()->getdevStatus() == NOCAM) {
mainMsg = tr("No webcam found");
if (DataManager::instance()->isSupportCameraSwitch()) {
subMsg = tr("Your camera might not be turned on...");
}
} else {
mainMsg = tr("The webcam is in use");
}
updateErrorMessages(mainMsg, subMsg);
}2.3 注释
3. 代码性能3.1 SVG渲染
// 建议添加成员变量
QSvgRenderer *m_cameraSwitchRenderer;
QSvgRenderer *m_noCameraRenderer;
// 在构造函数中初始化
m_cameraSwitchRenderer = new QSvgRenderer(QString(":/images/icons/light/camera_switch_not_on.svg"), this);
m_noCameraRenderer = new QSvgRenderer(QString(":/images/icons/light/Not connected.svg"), this);3.2 文本更新
// 建议添加检查
if (m_pCamErrItem->toPlainText() != newMessage) {
m_pCamErrItem->setPlainText(newMessage);
}4. 代码安全4.1 配置读取
// 建议修改
QVariant switchValue = dconfig->value("supportCameraSwitch");
if (switchValue.isValid() && switchValue.canConvert<bool>()) {
DataManager::instance()->setIsSupportCameraSwitch(switchValue.toBool());
} else {
DataManager::instance()->setIsSupportCameraSwitch(false);
}4.2 翻译字符串
5. 其他建议
总结本次代码变更整体实现了摄像头开关支持功能,逻辑基本正确,但存在一些代码重复和性能优化空间。建议按照上述意见进行优化,以提高代码质量和可维护性。 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lichaofan2008, lzwind The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/forcemerge |
|
This pr force merged! (status: unstable) |
…optimized.
The prompt images for the FLM(support camera switch) have been optimized, and secondary prompts have also been added.
FLM设备(带有摄像头开关)的提示图片进行了优化,同时增加了二级提示。
Task: https://pms.uniontech.com/task-view-386441.html
v20 BUG 分支合一到v25主线
Task: https://pms.uniontech.com/task-view-383481.html
Summary by Sourcery
Update camera error UI to support optimized prompt images and a secondary hint when devices have a hardware camera switch.
New Features:
Enhancements: