feat: enable the configuration for device blacklist.#503
Merged
Conversation
Use the DConfig mechanism to enable the configuration for device blacklist. 使用DConfig机制，开发设备黑名单功能，处在黑名单中的设备不会被应用显示。如果现有设备全部在黑名单中，则会有一个默认设备用于预览。 代码来自xiwo分支。 pick from:183e017c67513974b76786469eb03df8f2e2b6e8 Bug: https://pms.uniontech.com/bug-view-349401.html
Reviewer's GuideImplements a DConfig-driven camera device blacklist and integrates it into device selection, grouping, and UI behavior, including maintaining a thread-safe list of valid devices and reacting to device list changes. Sequence diagram for DConfig-driven device blacklist and valid device list updatessequenceDiagram
actor User
participant main
participant DConfig
participant DataManager
participant DevNumMonitor
participant CMainWindow
participant videowidget
User->>main: start application
main->>DConfig: value(deviceBlacklist)
DConfig-->>main: QStringList deviceBlacklist
main->>DataManager: setDeviceBlacklist(deviceBlacklist)
DataManager-->>main: (blacklist stored)
DevNumMonitor->>DevNumMonitor: timeOutSlot()
DevNumMonitor->>DevNumMonitor: check_device_list_events(get_v4l2_device_handler())
DevNumMonitor-->>DevNumMonitor: [device list changed] true
DevNumMonitor-->>CMainWindow: deviceListChanged
CMainWindow->>videowidget: updateValidDevices()
videowidget->>videowidget: updateValidDevices()
videowidget->>videowidget: get_device_list()
videowidget->>DataManager: isDeviceValid(vid,pid,name)
DataManager-->>videowidget: bool
videowidget->>videowidget: m_validDevices.push_back(ValidDevice)
videowidget->>videowidget: delayInit()
videowidget->>videowidget: updateValidDevices()
videowidget->>videowidget: getFirstValidDevice()
videowidget->>videowidget: isDeviceValidByDevice(device)
alt [device invalid]
videowidget->>videowidget: switchCamera(validDevice,"")
else [device valid]
videowidget->>videowidget: switchCamera(device,"")
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review★ 总体评分：95分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 void videowidget::onChangeDev()
{
// ... 前置代码 ...
} else if (groupNum == 1) {
// 加锁获取当前设备信息和有效设备列表快照
QString currentDevice = str;
QVector<ValidDevice> validDevicesSnapshot;
{
QReadLocker locker(&m_mutexValidDevices);
validDevicesSnapshot = m_validDevices;
}
if (validDevicesSnapshot.empty()) {
switchCamera("", ""); // 无有效设备，但是有设备，走默认逻辑
} else {
int idx = -1;
for (int i = 0; i < validDevicesSnapshot.size(); ++i) {
if (validDevicesSnapshot[i].getDevice() == currentDevice) {
idx = i;
break;
}
}
if (idx == -1 || idx >= validDevicesSnapshot.size() - 1) {
// 获取第一个有效设备，直接切换到该设备
const ValidDevice &dev = validDevicesSnapshot[0];
switchCamera(dev.getDevice().toStdString().c_str(), dev.getName().toStdString().c_str());
} else {
// 切换到当前设备的下一个有效设备
const ValidDevice &dev = validDevicesSnapshot[idx + 1];
switchCamera(dev.getDevice().toStdString().c_str(), dev.getName().toStdString().c_str());
}
}
} else {
// ... 后续代码 ...
} |
lzwind
approved these changes
Jul 21, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: add-uos, 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In DevNumMonitor::timeOutSlot, get_device_list()->num_devices is used without checking that get_device_list() and its list_devices are non-null, which can lead to crashes if the handler or device list is unavailable; consider adding null checks before accessing num_devices.
- updateValidDevices() returns early when devList or its list_devices is null but does not appear to release any resources associated with the device list; if get_device_list() requires explicit cleanup, ensure the list is properly freed in all exit paths.
- The blacklist matching in DataManager::setDeviceBlacklist and isDeviceValid relies on exact vid,pid,name strings; if device names can vary (e.g., localization or minor formatting changes), consider normalizing or relaxing name matching to avoid unintentionally treating valid devices as unlisted.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In DevNumMonitor::timeOutSlot, get_device_list()->num_devices is used without checking that get_device_list() and its list_devices are non-null, which can lead to crashes if the handler or device list is unavailable; consider adding null checks before accessing num_devices.
- updateValidDevices() returns early when devList or its list_devices is null but does not appear to release any resources associated with the device list; if get_device_list() requires explicit cleanup, ensure the list is properly freed in all exit paths.
- The blacklist matching in DataManager::setDeviceBlacklist and isDeviceValid relies on exact vid,pid,name strings; if device names can vary (e.g., localization or minor formatting changes), consider normalizing or relaxing name matching to avoid unintentionally treating valid devices as unlisted.
## Individual Comments
### Comment 1
<location path="src/src/videowidget.cpp" line_range="1388-1391" />
<code_context>
// 收到建议使用 QMap<QString, QVector<v4l2_dev_sys_data_t *>> 来存储分组数据，但我们担心影响现有代码逻辑，
// 所以暂时保留 QVector<QPair<QString, QVector<v4l2_dev_sys_data_t *>>> 来存储分组数据。
- if (devlist == nullptr) {
+ if (devlist == nullptr || devlist->list_devices == nullptr || devlist->num_devices == 0) {
qWarning() << __func__ << "devlist is NULL!";
return 0;
}
</code_context>
<issue_to_address>
**suggestion:** Warning message no longer matches the broader null/empty checks.
The condition now includes `devlist == nullptr`, `devlist->list_devices == nullptr`, and `devlist->num_devices == 0`, but the warning still only reports "devlist is NULL". Please update the message to reflect all these cases (e.g. "devlist is NULL or empty") so logs accurately describe the failure condition.
```suggestion
if (devlist == nullptr || devlist->list_devices == nullptr || devlist->num_devices == 0) {
qWarning() << __func__ << "devlist is NULL or empty (list_devices is NULL or num_devices == 0)!";
return 0;
}
```
</issue_to_address>
### Comment 2
<location path="src/src/videowidget.h" line_range="83" />
<code_context>
+/**
+* @brief ValidDevice 有效相机设备，与黑名单机制配套使用
+*/
+class ValidDevice
+{
+public:
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the ValidDevice type, its lookup helpers, and lock usage to match current needs and reduce boilerplate and cognitive overhead.
The `ValidDevice` + locking + helper set looks more complex than it needs to be for the current usage.
You can keep all functionality while reducing boilerplate and cognitive load:
### 1. Simplify `ValidDevice` to a POD-like struct
If you don’t need mutation semantics or encapsulation, you can drop the setters/getters and use public members. Equality can be a free function or operator on the struct:
```cpp
struct ValidDevice
{
QString vid; // 相机VID
QString pid; // 相机PID
QString name; // 相机名称
QString device; // 相机设备节点路径
};
inline bool operator==(const ValidDevice &a, const ValidDevice &b)
{
return a.vid == b.vid
&& a.pid == b.pid
&& a.name == b.name
&& a.device == b.device;
}
```
Usage in `videowidget` stays the same but without getter/setter overhead:
```cpp
ValidDevice dev{vid, pid, name, device};
// access: dev.device, dev.vid, etc.
```
### 2. Collapse multiple lookup helpers into one
`isDeviceValidByDevice` and `getValidDeviceIndexByDevice` can be represented by a single `findValidDeviceByDevice` helper returning a pointer, which callers can use for “exists?” and “index” logic as needed:
```cpp
// in videowidget:
const ValidDevice *findValidDeviceByDevice(const QString &device) const
{
for (const auto &d : m_validDevices) {
if (d.device == device)
return &d;
}
return nullptr;
}
// example usage:
bool isDeviceValidByDevice(const QString &device)
{
return findValidDeviceByDevice(device) != nullptr;
}
int getValidDeviceIndexByDevice(const QString &device)
{
for (int i = 0; i < m_validDevices.size(); ++i) {
if (m_validDevices[i].device == device)
return i;
}
return -1;
}
```
Or you can even keep only `findValidDeviceByDevice` public and make the others private/inline wrappers if external code doesn’t need them.
### 3. Reconsider `QReadWriteLock` usage
If `m_validDevices` is only accessed on the UI thread (common for widgets), you can remove `QReadWriteLock` entirely and keep the data structure unguarded:
```cpp
QVector<ValidDevice> m_validDevices; // 有效相机设备列表
// QReadWriteLock m_mutexValidDevices; // remove if single-threaded
```
If you *do* need synchronization, a simpler pattern is to hide the lock inside the helpers so callers don’t have to manage it:
```cpp
QVector<ValidDevice> m_validDevices;
QReadWriteLock m_mutexValidDevices;
// helper with internal locking
const ValidDevice *videowidget::findValidDeviceByDevice(const QString &device) const
{
QReadLocker locker(&m_mutexValidDevices);
for (const auto &d : m_validDevices) {
if (d.device == device)
return &d;
}
return nullptr;
}
```
This keeps the same behavior but reduces the number of places that have to reason about lock lifetime and type.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1388
to
1391
| if (devlist == nullptr || devlist->list_devices == nullptr || devlist->num_devices == 0) { | ||
| qWarning() << __func__ << "devlist is NULL!"; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
suggestion: Warning message no longer matches the broader null/empty checks.
The condition now includes devlist == nullptr, devlist->list_devices == nullptr, and devlist->num_devices == 0, but the warning still only reports "devlist is NULL". Please update the message to reflect all these cases (e.g. "devlist is NULL or empty") so logs accurately describe the failure condition.
Suggested change
| if (devlist == nullptr || devlist->list_devices == nullptr || devlist->num_devices == 0) { | |
| qWarning() << __func__ << "devlist is NULL!"; | |
| return 0; | |
| } | |
| if (devlist == nullptr || devlist->list_devices == nullptr || devlist->num_devices == 0) { | |
| qWarning() << __func__ << "devlist is NULL or empty (list_devices is NULL or num_devices == 0)!"; | |
| return 0; | |
| } |
| /** | ||
| * @brief ValidDevice 有效相机设备，与黑名单机制配套使用 | ||
| */ | ||
| class ValidDevice |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the ValidDevice type, its lookup helpers, and lock usage to match current needs and reduce boilerplate and cognitive overhead.
The ValidDevice + locking + helper set looks more complex than it needs to be for the current usage.
You can keep all functionality while reducing boilerplate and cognitive load:
1. Simplify ValidDevice to a POD-like struct
If you don’t need mutation semantics or encapsulation, you can drop the setters/getters and use public members. Equality can be a free function or operator on the struct:
struct ValidDevice
{
QString vid; // 相机VID
QString pid; // 相机PID
QString name; // 相机名称
QString device; // 相机设备节点路径
};
inline bool operator==(const ValidDevice &a, const ValidDevice &b)
{
return a.vid == b.vid
&& a.pid == b.pid
&& a.name == b.name
&& a.device == b.device;
}Usage in videowidget stays the same but without getter/setter overhead:
ValidDevice dev{vid, pid, name, device};
// access: dev.device, dev.vid, etc.2. Collapse multiple lookup helpers into one
isDeviceValidByDevice and getValidDeviceIndexByDevice can be represented by a single findValidDeviceByDevice helper returning a pointer, which callers can use for “exists?” and “index” logic as needed:
// in videowidget:
const ValidDevice *findValidDeviceByDevice(const QString &device) const
{
for (const auto &d : m_validDevices) {
if (d.device == device)
return &d;
}
return nullptr;
}
// example usage:
bool isDeviceValidByDevice(const QString &device)
{
return findValidDeviceByDevice(device) != nullptr;
}
int getValidDeviceIndexByDevice(const QString &device)
{
for (int i = 0; i < m_validDevices.size(); ++i) {
if (m_validDevices[i].device == device)
return i;
}
return -1;
}Or you can even keep only findValidDeviceByDevice public and make the others private/inline wrappers if external code doesn’t need them.
3. Reconsider QReadWriteLock usage
If m_validDevices is only accessed on the UI thread (common for widgets), you can remove QReadWriteLock entirely and keep the data structure unguarded:
QVector<ValidDevice> m_validDevices; // 有效相机设备列表
// QReadWriteLock m_mutexValidDevices; // remove if single-threadedIf you do need synchronization, a simpler pattern is to hide the lock inside the helpers so callers don’t have to manage it:
QVector<ValidDevice> m_validDevices;
QReadWriteLock m_mutexValidDevices;
// helper with internal locking
const ValidDevice *videowidget::findValidDeviceByDevice(const QString &device) const
{
QReadLocker locker(&m_mutexValidDevices);
for (const auto &d : m_validDevices) {
if (d.device == device)
return &d;
}
return nullptr;
}This keeps the same behavior but reduces the number of places that have to reason about lock lifetime and type.
Contributor
Author
|
/merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use the DConfig mechanism to enable the configuration for device blacklist. 使用DConfig机制，开发设备黑名单功能，处在黑名单中的设备不会被应用显示。如果现有设备全部在黑名单中，则会有一个默认设备用于预览。 代码来自xiwo分支。
pick from:183e017c67513974b76786469eb03df8f2e2b6e8
Bug: https://pms.uniontech.com/bug-view-349401.html
Summary by Sourcery
Introduce a configurable camera device blacklist and update camera selection logic to respect it while maintaining sensible fallbacks.
New Features:
Enhancements:
Chores: