pick develop/eagle to master #560
Merged
deepin-bot[bot] merged 8 commits intolinuxdeepin:masterfrom Nov 28, 2025
Merged
Conversation
fix the storage display pick from: linuxdeepin@4cce766 Log: fix the storage display Bug: https://pms.uniontech.com/bug-view-297753.html Change-Id: I0dc129b531e3cb27a0828ff1847d08227646cab9
fix the monitor get info from xrander Log: fix the monitor get info from xrander Bug: https://pms.uniontech.com/bug-view-285297.html Change-Id: Ib43660c92187ec824d5a832194dbc609e3ee0d52
adjust the select usb timeout, change the timeout 0.01s to 1s. Log: adjust the select usb timeout, change the timeout 0.01s to 1s. Task: https://pms.uniontech.com/task-view-371601.html Change-Id: Ie2922388a8b7fc4e23db94123d3d6ff6be364640
fit the UFS for klv Log: fit the UFS for klv Change-Id: I435e4f59b113164fe5a6138c869c85384e1cf5f2
This reverts commit a9acd86.
adjust the select usb timeout, change the timeout 0.01s to 1s. Log: adjust the select usb timeout, change the timeout 0.01s to 1s. Bug: https://pms.uniontech.com/bug-view-301245.html
add the storage overview info pick from: linuxdeepin@05b1de7 Log: add the storage overview info Task: https://pms.uniontech.com/task-view-372725.html Change-Id: I3e7833c548382653026bcf0a6dec75c88ef9e2d9
modify the storage size pick from: linuxdeepin@c6948df Log: modify the storage size Task: https://pms.uniontech.com/task-view-372725.html
Reviewer's GuidePorts a set of hardware-specific behaviors from the develop/eagle branch into master: adds specialComType-based conditionals for disk model/overview formatting and UFS interface labeling, broadens disk size normalization logic, adjusts USB hotplug polling interval, localizes screen size units, and reverses xrandr verbose parsing order to prefer a different monitor entry. Class diagram for updated hardware and display handlingclassDiagram
class DeviceStorage {
- QString m_Name
- QString m_Vendor
- QString m_Interface
- QString m_Size
- quint64 m_SizeBytes
- QString m_MediaType
+ bool setHwinfoInfo(QMap<QString, QString> mapInfo)
+ void checkDiskSize()
+ QString getOverviewInfo()
}
class HWGenerator {
+ void generatorDiskDevice()
}
class MonitorUsb {
+ void monitor()
}
class DeviceMonitor {
- QString m_ScreenSize
+ bool caculateScreenSize(QString edid)
}
class ThreadExecXrandr {
+ void getMonitorInfoFromXrandrVerbose()
}
class Common {
<<static>>
+ int specialComType
+ QString specialVendorType()
+ QString specialHString()
}
DeviceStorage .. Common : uses
HWGenerator .. Common : uses
MonitorUsb .. Common : uses
DeviceMonitor .. Common : uses
ThreadExecXrandr .. Common : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review我来对这段代码的修改进行审查分析：
tv.tv_sec = 1;
tv.tv_usec = 0;优点：
建议：
m_ScreenSize = QString("%1 %2(%3mm X %4mm)").arg(QString::number(inch, '0', 1)).arg(translateStr("inch")).arg(width).arg(height);优点：
建议：
if (Common::specialComType <= 0) {
setAttribute(mapInfo, "Model", m_Name);
}优点：
b) checkDiskSize函数： // if (m_Interface.contains("UFS", Qt::CaseInsensitive)) { // TODO Ignore ufs disk优点：
建议：
c) getOverviewInfo函数： if (Common::specialComType == 5){
// ...
}优点：
建议：
if (Common::specialComType == 2) {
tempMap["Interface"] = "UFS 3.1";
}优点：
建议：
std::reverse(lstMap.begin(), lstMap.end());优点：
建议：
总体建议：
|
![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:
- There are multiple hard-coded checks against specific
Common::specialComTypevalues (e.g.,<= 0,== 2,== 5); consider introducing a strongly-typed enum or named constants to make the intent clearer and reduce the risk of misuse when adding future variants. - The commented-out
m_Interface.contains("UFS")condition incheckDiskSize()leaves dead code and an unclear TODO; either remove it or replace it with a clearer configuration-based condition (e.g., gated byspecialComType) so the UFS-specific behavior is explicitly controlled. - The change of the
selecttimeout inMonitorUsb::monitor()from 10ms to 1s significantly alters event responsiveness; consider making this interval configurable or documenting why a 1s delay is acceptable to avoid unexpected latency regressions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are multiple hard-coded checks against specific `Common::specialComType` values (e.g., `<= 0`, `== 2`, `== 5`); consider introducing a strongly-typed enum or named constants to make the intent clearer and reduce the risk of misuse when adding future variants.
- The commented-out `m_Interface.contains("UFS")` condition in `checkDiskSize()` leaves dead code and an unclear TODO; either remove it or replace it with a clearer configuration-based condition (e.g., gated by `specialComType`) so the UFS-specific behavior is explicitly controlled.
- The change of the `select` timeout in `MonitorUsb::monitor()` from 10ms to 1s significantly alters event responsiveness; consider making this interval configurable or documenting why a 1s delay is acceptable to avoid unexpected latency regressions.
## Individual Comments
### Comment 1
<location> `deepin-devicemanager/src/DeviceManager/DeviceStorage.cpp:530-539` </location>
<code_context>
- }
- }
+
+// if (m_Interface.contains("UFS", Qt::CaseInsensitive)) { // TODO Ignore ufs disk
+ if (m_SizeBytes > 255*gbyte && m_SizeBytes < 257*gbyte) {
+ m_Size = "256 GB";
+ } else if (m_SizeBytes > 511*gbyte && m_SizeBytes < 513*gbyte) {
+ m_Size = "512 GB";
+ } else if (m_SizeBytes > 999*gbyte && m_SizeBytes < 1025*gbyte) {
+ m_Size = "1 TB";
+ } else if (m_SizeBytes > 1999*gbyte && m_SizeBytes < 2049*gbyte) {
+ m_Size = "2 TB";
+ }
+// }
if (m_Interface.contains("USB", Qt::CaseInsensitive)) {
if (m_SizeBytes > 15*gbyte && m_SizeBytes < 17*gbyte) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Uncommented UFS guard changes behavior from UFS-only to all disks, which doesn’t match the TODO comment.
With the `if (m_Interface.contains("UFS"...))` guard commented out, the rounding now applies to every disk, even though the TODO says `Ignore ufs disk`, which suggests the opposite intent. If the goal is to skip UFS, make that explicit:
```cpp
if (m_Interface.contains("UFS", Qt::CaseInsensitive)) {
return; // Ignore UFS disk
}
// apply size normalization for the rest
```
Right now this broadens the behavior and may unintentionally affect non‑UFS devices.
</issue_to_address>
### Comment 2
<location> `deepin-devicemanager/src/DeviceManager/DeviceStorage.cpp:625-634` </location>
<code_context>
+
+ // 见内网gerrit项目 os-config 中机型的 specialComType , 示例配置文件位置如下：
+ // os-config/hardware/机型/etc/dsg/configs/overrides/org.deepin.devicemanager/org.deepin.devicemanager/4000-org.deepin.devicemanager.override.json
+ if (Common::specialComType == 5){
+ if (m_Interface.contains("UFS", Qt::CaseInsensitive)) {
+ overViewInfo = QString("%1 %2").arg(m_Size).arg("UFS");
+ } else if (m_Interface.contains("USB", Qt::CaseInsensitive)) {
+ overViewInfo = QString("%1 %2").arg(m_Size).arg("USB");
+ } else {
+ overViewInfo = QString("%1 %2").arg(m_Size).arg(m_MediaType);
+ }
+ }
+
+ return overViewInfo;
}
</code_context>
<issue_to_address>
**suggestion:** Consider using i18n for media/interface tokens in the overview string or aligning them with existing localization.
In the `specialComType == 5` branch, `"UFS"` and `"USB"` are hard-coded while `m_MediaType` might already be localized, so `getOverviewInfo()` can mix localized and English text. If this string is user-facing, consider wrapping these tokens in translation helpers (as with `translateStr("inch")` in `DeviceMonitor`) or reusing existing localized labels for USB/UFS/media types to keep localization consistent.
Suggested implementation:
```cpp
if (Common::specialComType == 5) {
// Use translated labels for interface types to avoid mixing localized and English text
const QString ufsLabel = tr("UFS");
const QString usbLabel = tr("USB");
if (m_Interface.contains("UFS", Qt::CaseInsensitive)) {
overViewInfo = QString("%1 %2").arg(m_Size, ufsLabel);
} else if (m_Interface.contains("USB", Qt::CaseInsensitive)) {
overViewInfo = QString("%1 %2").arg(m_Size, usbLabel);
} else {
overViewInfo = QString("%1 %2").arg(m_Size, m_MediaType);
}
}
return overViewInfo;
```
- Ensure `DeviceStorage` is a `QObject`-derived class with the `Q_OBJECT` macro so that `tr()` is available. This is likely already true in this codebase.
- If your localization setup uses a custom helper (e.g. `translateStr()` as in `DeviceMonitor`), you can replace `tr("UFS")` / `tr("USB")` with that helper to align with existing conventions.
</issue_to_address>
### Comment 3
<location> `deepin-devicemanager-server/deepin-deviceinfo/src/hotplug/monitorusb.cpp:58-59` </location>
<code_context>
FD_SET(fd, &fds);
- tv.tv_sec = 0;
- tv.tv_usec = 10000;
+ tv.tv_sec = 1;
+ tv.tv_usec = 0;
int ret = select(fd + 1, &fds, nullptr, nullptr, &tv);
</code_context>
<issue_to_address>
**question (performance):** Increasing the select timeout to 1s may introduce noticeable delay for USB hotplug handling.
The timeout has changed from 10ms to 1s, which reduces wakeups and CPU usage but can delay USB event handling by up to 1s. To balance responsiveness and resource usage, consider a smaller value (e.g., 100–200ms) or making the timeout configurable.
</issue_to_address>
### Comment 4
<location> `deepin-devicemanager/src/Tool/ThreadExecXrandr.cpp:279` </location>
<code_context>
QList<QMap<QString, QString>> lstMap;
loadXrandrVerboseInfo(lstMap, "xrandr --verbose");
-
+ std::reverse(lstMap.begin(), lstMap.end());
QList<QMap<QString, QString> >::const_iterator it = lstMap.begin();
for (; it != lstMap.end(); ++it) {
</code_context>
<issue_to_address>
**question (bug_risk):** Reversing `lstMap` changes monitor processing order and may impact which monitor is treated as primary or first.
This change makes the first monitor processed in `getMonitorInfoFromXrandrVerbose()` correspond to the last entry from `xrandr --verbose`. If any downstream code relies on the first processed entry representing the primary/preferred monitor or stops at the first suitable match, behavior may change. Consider limiting the reversal to the specific logic that requires it, or document this ordering change explicitly.
</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
+625
to
+634
| if (Common::specialComType == 5){ | ||
| if (m_Interface.contains("UFS", Qt::CaseInsensitive)) { | ||
| overViewInfo = QString("%1 %2").arg(m_Size).arg("UFS"); | ||
| } else if (m_Interface.contains("USB", Qt::CaseInsensitive)) { | ||
| overViewInfo = QString("%1 %2").arg(m_Size).arg("USB"); | ||
| } else { | ||
| overViewInfo = QString("%1 %2").arg(m_Size).arg(m_MediaType); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
suggestion: Consider using i18n for media/interface tokens in the overview string or aligning them with existing localization.
In the specialComType == 5 branch, "UFS" and "USB" are hard-coded while m_MediaType might already be localized, so getOverviewInfo() can mix localized and English text. If this string is user-facing, consider wrapping these tokens in translation helpers (as with translateStr("inch") in DeviceMonitor) or reusing existing localized labels for USB/UFS/media types to keep localization consistent.
Suggested implementation:
if (Common::specialComType == 5) {
// Use translated labels for interface types to avoid mixing localized and English text
const QString ufsLabel = tr("UFS");
const QString usbLabel = tr("USB");
if (m_Interface.contains("UFS", Qt::CaseInsensitive)) {
overViewInfo = QString("%1 %2").arg(m_Size, ufsLabel);
} else if (m_Interface.contains("USB", Qt::CaseInsensitive)) {
overViewInfo = QString("%1 %2").arg(m_Size, usbLabel);
} else {
overViewInfo = QString("%1 %2").arg(m_Size, m_MediaType);
}
}
return overViewInfo;
- Ensure
DeviceStorageis aQObject-derived class with theQ_OBJECTmacro so thattr()is available. This is likely already true in this codebase. - If your localization setup uses a custom helper (e.g.
translateStr()as inDeviceMonitor), you can replacetr("UFS")/tr("USB")with that helper to align with existing conventions.
Comment on lines
+58
to
+59
| tv.tv_sec = 1; | ||
| tv.tv_usec = 0; |
There was a problem hiding this comment.
question (performance): Increasing the select timeout to 1s may introduce noticeable delay for USB hotplug handling.
The timeout has changed from 10ms to 1s, which reduces wakeups and CPU usage but can delay USB event handling by up to 1s. To balance responsiveness and resource usage, consider a smaller value (e.g., 100–200ms) or making the timeout configurable.
| QList<QMap<QString, QString>> lstMap; | ||
| loadXrandrVerboseInfo(lstMap, "xrandr --verbose"); | ||
|
|
||
| std::reverse(lstMap.begin(), lstMap.end()); |
There was a problem hiding this comment.
question (bug_risk): Reversing lstMap changes monitor processing order and may impact which monitor is treated as primary or first.
This change makes the first monitor processed in getMonitorInfoFromXrandrVerbose() correspond to the last entry from xrandr --verbose. If any downstream code relies on the first processed entry representing the primary/preferred monitor or stops at the first suitable match, behavior may change. Consider limiting the reversal to the specific logic that requires it, or document this ordering change explicitly.
lzwind
approved these changes
Nov 28, 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 |
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.
pick develop/eagle to master
Summary by Sourcery
Adjust storage, display, and USB monitoring behavior for specific OEM configurations and improve device information presentation.
New Features:
Bug Fixes:
Enhancements: