pick develop/eagle to master #572
Merged
Conversation
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
- Change waitForDone(-1) to waitForDone(60000) in MainJob::updateAllDevice() - Add 60-second timeout to prevent device manager from hanging during device info loading - Improve application responsiveness and user experience pick from: linuxdeepin@00f82e6 Log: Set timeout for device info loading to prevent infinite waiting Bug: https://pms.uniontech.com/bug-view-331983.html Change-Id: Iab0310719df4cfa7e13176975ee0db59572d88ce
Modified CPU frequency display for kSpecialType6 devices include current frequency and max frequency to correct hardware reporting inaccuracies. Log: Fix CPU frequency display for special device type Task: https://pms.uniontech.com/task-view-381765.html Change-Id: I3a715b263868c6825ee42941b64a7c5de6a53035
Enhanced readability of the architecture mapping table by reformatting the static QMap initialization with proper indentation and line breaks. also add comments for variables. Log: Refactor architecture mapping code style Change-Id: Iea19f9e13e7b8f59f66d34877e555994832a0221
Modified CPU frequency display for kSpecialType5 devices include current frequency and max frequency to correct hardware reporting inaccuracies. Log: Fix CPU frequency display for special device type Task: https://pms.uniontech.com/task-view-381921.html Change-Id: Ia80c7e9edf7618188874e0648cb599920f49eada
Modified CPU frequency display for kSpecialType7 devices include current frequency and max frequency to correct hardware reporting inaccuracies. Log: Fix CPU frequency display for special device type Task: https://pms.uniontech.com/task-view-381765.html Change-Id: Id37a4b03a83ccb6428a1bd3edf601d905edbd2c8
Added condition to reset the driver information when its value is "usbfs" (case-insensitive comparison) to handle invalid driver identifiers. Log: Filter out usbfs driver values Bug: https://pms.uniontech.com/bug-view-333969.html Change-Id: I4f2632c9ccef6f9df668460adda9b9078a65b932
Reviewer's GuideAligns master with develop/eagle by adding special handling for specific board types (kSpecialType5–7), cleaning up USBFS driver reporting, tightening thread pool wait timeouts, and performing minor style/comment tweaks across device manager components. Sequence diagram for MainJob::updateAllDevice with bounded wait timeoutsequenceDiagram
participant MainJob
participant ThreadPool
participant DeviceInfoWorker
MainJob->>ThreadPool: updateDeviceInfo()
activate ThreadPool
ThreadPool->>DeviceInfoWorker: start run()
activate DeviceInfoWorker
Note over DeviceInfoWorker: Collect device information
DeviceInfoWorker-->>ThreadPool: run() completed
deactivate DeviceInfoWorker
MainJob->>ThreadPool: waitForDone(60000)
ThreadPool-->>MainJob: return after workers done or timeout
deactivate ThreadPool
Class diagram for Common board type handling in CPU infoclassDiagram
class Common {
<<utility>>
+BoardType specialComType
+enum BoardType
}
class BoardType {
<<enumeration>>
+KLVV
+KLVU
+PGUV
+kSpecialType5
+kSpecialType6
+kSpecialType7
+kCustomType
}
class DeviceCpu {
+QString m_Name
+QString m_Frequency
+QString m_MaxFrequency
+void setCpuInfo(QMap_QString_QString mapLscpu, QMap_QString_QString mapHwinfo, int logicalNum, int coreNum)
}
Common --> BoardType : defines
DeviceCpu --> Common : uses specialComType
class QMap_QString_QString {
<<typedef>>
}
Class diagram for DeviceOthers USBFS driver handlingclassDiagram
class DeviceOthers {
+QString m_Driver
+QString m_Avail
+QString m_MaximumPower
+QString m_Speed
+QString m_LogicalName
+QString m_Modalias
+QString m_VID_PID
+QString m_PhysID
+void setInfoFromLshw(QMap_QString_QString mapInfo)
+void setInfoFromHwinfo(QMap_QString_QString mapInfo)
+void setForcedDisplay(bool forced)
}
class QMap_QString_QString {
<<typedef>>
}
DeviceOthers ..> QMap_QString_QString : reads attributes
class LshwOutput {
+QString vendor
+QString driver
+QString avail
+QString maxpower
+QString speed
+QString logical_name
}
class HwinfoOutput {
+QString Hardware_Class
+QString Driver
+QString Module_Alias
+QString VID_PID
}
DeviceOthers ..> LshwOutput : parsed from
DeviceOthers ..> HwinfoOutput : parsed from
Class diagram for MainJob updateAllDevice and thread pool wait timeoutclassDiagram
class MainJob {
+bool m_firstUpdate
+ThreadPool* m_pool
+void updateAllDevice()
}
class ThreadPool {
+void updateDeviceInfo()
+void waitForDone(int timeoutMs)
}
MainJob --> ThreadPool : uses
class DeviceInfoWorker {
+void run()
}
ThreadPool o-- DeviceInfoWorker : manages workers
Class diagram for EDIDParser and ThreadExecXrandr comment-aligned fieldsclassDiagram
class EDIDParser {
+QString m_Vendor
+QString m_Model
+QString m_ReleaseDate
+QString m_ScreenSize
+QString m_MonitorName
+QString m_EDIDHexString
+int m_Width
+int m_Height
+QStringList m_ListEdid
+QMap_QString_QString m_MapCh
}
class ThreadExecXrandr {
+bool m_Gpu
+bool m_isDXcbPlatform
+QStringList m_monitorLst
}
class QMap_QString_QString {
<<typedef>>
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
lzwind
approved these changes
Dec 1, 2025
|
[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 |
deepin pr auto review我来对这段代码的修改进行审查：
改进建议：
// 建议使用配置文件或常量定义来管理这些特殊值
const QMap<QString, QString> FREQUENCY_CORRECTIONS = {
{"2.189", "2.188"},
{"2189", "2188"}
};
if (Common::specialComType >= Common::kSpecialType5 &&
Common::specialComType <= Common::kSpecialType7) {
for (auto it = FREQUENCY_CORRECTIONS.begin(); it != FREQUENCY_CORRECTIONS.end(); ++it) {
m_Frequency = m_Frequency.replace(it.key(), it.value());
m_MaxFrequency = m_MaxFrequency.replace(it.key(), it.value());
}
}
if (!m_pool->waitForDone(60000)) {
qCWarning(appLog) << "Timeout while waiting for device update to complete";
}
static const QString USBFS_DRIVER = "usbfs";
if (m_Driver.toLower() == USBFS_DRIVER) {
m_Driver.clear();
}总体来说，这些修改主要关注于代码格式化和特定问题的修复，提高了代码的可读性和健壮性。建议在后续开发中继续保持良好的代码风格，并考虑使用更灵活的方式来处理特殊情况。 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The CPU frequency adjustment for specialComType 5–7 relies on hard-coded string replacements ("2.189" → "2.188"), which is brittle; consider parsing and adjusting numeric values or basing this on detected hardware characteristics instead of literal substrings.
- The
waitForDone(60000)timeout changes behavior from infinite wait to 60 seconds without visible handling of a timeout case; it may be safer to either make this timeout configurable or explicitly handle/log the scenario where the pool does not complete within the timeout. - The
if (m_Driver.toLower() == "usbfs") m_Driver.clear();logic is duplicated in bothsetInfoFromLshwandsetInfoFromHwinfo; consider extracting this into a small helper to keep the normalization logic in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The CPU frequency adjustment for specialComType 5–7 relies on hard-coded string replacements ("2.189" → "2.188"), which is brittle; consider parsing and adjusting numeric values or basing this on detected hardware characteristics instead of literal substrings.
- The `waitForDone(60000)` timeout changes behavior from infinite wait to 60 seconds without visible handling of a timeout case; it may be safer to either make this timeout configurable or explicitly handle/log the scenario where the pool does not complete within the timeout.
- The `if (m_Driver.toLower() == "usbfs") m_Driver.clear();` logic is duplicated in both `setInfoFromLshw` and `setInfoFromHwinfo`; consider extracting this into a small helper to keep the normalization logic in one place.
## Individual Comments
### Comment 1
<location> `deepin-devicemanager/src/DeviceManager/DeviceCpu.cpp:56-61` </location>
<code_context>
m_Name.replace(QRegularExpression("x [0-9]*$"), "");
qCDebug(appLog) << "CPU name after regex replacement: " << m_Name;
+ if (Common::specialComType == Common::kSpecialType5 ||
+ Common::specialComType == Common::kSpecialType6 ||
+ Common::specialComType == Common::kSpecialType7) {
+ m_Frequency = m_Frequency.replace("2.189", "2.188");
+ m_MaxFrequency = m_MaxFrequency.replace("2189", "2188");
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider making the frequency replacement more robust and avoiding the redundant assignment.
In this block:
1) `QString::replace` mutates in place and returns a reference, so `m_Frequency = m_Frequency.replace(...)` / `m_MaxFrequency = ...` is redundant. A simple `m_Frequency.replace("2.189", "2.188");` is enough.
2) The replacement for `"2189"` is very broad and will also hit occurrences inside longer values (e.g. `12189`, `21890`) or in other fields if the format changes. If you only intend to adjust a specific frequency value, consider tightening the match (e.g. more constrained pattern/regex or validating the whole field before replacing) to avoid unintended substitutions.
```suggestion
if (Common::specialComType == Common::kSpecialType5 ||
Common::specialComType == Common::kSpecialType6 ||
Common::specialComType == Common::kSpecialType7) {
// Avoid broad substring replacements: only adjust when the full value matches.
if (m_Frequency == "2.189") {
m_Frequency = "2.188";
}
if (m_MaxFrequency == "2189") {
m_MaxFrequency = "2188";
}
}
```
</issue_to_address>
### Comment 2
<location> `deepin-devicemanager-server/deepin-deviceinfo/src/mainjob.cpp:179` </location>
<code_context>
qCDebug(appLog) << "Updating existing device info";
m_pool->updateDeviceInfo();
}
- m_pool->waitForDone(-1);
+ m_pool->waitForDone(60000);
PERF_PRINT_END("POINT-01");
m_firstUpdate = false;
</code_context>
<issue_to_address>
**issue:** Introducing a 60s wait timeout may leave device updates incomplete without any handling.
Changing `waitForDone` from infinite to 60s changes behavior: if the pool is still busy after 60s, we return and still set `m_firstUpdate` to `false`, treating the update as complete. For long-running updates, this can leave device info partially updated with no signal to the caller. Consider either making the timeout configurable, or explicitly handling a timeout (if the pool API supports it), e.g. by logging it, not clearing `m_firstUpdate`, retrying, or otherwise surfacing an error state.
</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
+56
to
+61
| if (Common::specialComType == Common::kSpecialType5 || | ||
| Common::specialComType == Common::kSpecialType6 || | ||
| Common::specialComType == Common::kSpecialType7) { | ||
| m_Frequency = m_Frequency.replace("2.189", "2.188"); | ||
| m_MaxFrequency = m_MaxFrequency.replace("2189", "2188"); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Consider making the frequency replacement more robust and avoiding the redundant assignment.
In this block:
-
QString::replacemutates in place and returns a reference, som_Frequency = m_Frequency.replace(...)/m_MaxFrequency = ...is redundant. A simplem_Frequency.replace("2.189", "2.188");is enough. -
The replacement for
"2189"is very broad and will also hit occurrences inside longer values (e.g.12189,21890) or in other fields if the format changes. If you only intend to adjust a specific frequency value, consider tightening the match (e.g. more constrained pattern/regex or validating the whole field before replacing) to avoid unintended substitutions.
Suggested change
| if (Common::specialComType == Common::kSpecialType5 || | |
| Common::specialComType == Common::kSpecialType6 || | |
| Common::specialComType == Common::kSpecialType7) { | |
| m_Frequency = m_Frequency.replace("2.189", "2.188"); | |
| m_MaxFrequency = m_MaxFrequency.replace("2189", "2188"); | |
| } | |
| if (Common::specialComType == Common::kSpecialType5 || | |
| Common::specialComType == Common::kSpecialType6 || | |
| Common::specialComType == Common::kSpecialType7) { | |
| // Avoid broad substring replacements: only adjust when the full value matches. | |
| if (m_Frequency == "2.189") { | |
| m_Frequency = "2.188"; | |
| } | |
| if (m_MaxFrequency == "2189") { | |
| m_MaxFrequency = "2188"; | |
| } | |
| } |
| qCDebug(appLog) << "Updating existing device info"; | ||
| m_pool->updateDeviceInfo(); | ||
| } | ||
| m_pool->waitForDone(-1); |
There was a problem hiding this comment.
issue: Introducing a 60s wait timeout may leave device updates incomplete without any handling.
Changing waitForDone from infinite to 60s changes behavior: if the pool is still busy after 60s, we return and still set m_firstUpdate to false, treating the update as complete. For long-running updates, this can leave device info partially updated with no signal to the caller. Consider either making the timeout configurable, or explicitly handling a timeout (if the pool API supports it), e.g. by logging it, not clearing m_firstUpdate, retrying, or otherwise surfacing an error state.
Contributor
Author
|
/forcemerge |
Contributor
|
This pr force merged! (status: unstable) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
pick develop/eagle to master
Summary by Sourcery
Adjust device manager behavior for specific hardware types and improve robustness and code clarity across CPU, device, and threading handling.
New Features:
Bug Fixes:
Enhancements: