feat: normalize disk size only for specific devices.#179
Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:release/1071from Oct 23, 2025
Merged
feat: normalize disk size only for specific devices.#179deepin-bot[bot] merged 1 commit intolinuxdeepin:release/1071from
deepin-bot[bot] merged 1 commit intolinuxdeepin:release/1071from
Conversation
Reviewer's GuideThis PR refactors the hardware device update flow to drop unused parameters, implements raw disk size parsing and normalization to standard presets, and integrates a centralized size formatting method for consistent output. Class diagram for updated DeviceStorage classclassDiagram
class DeviceStorage {
- QString m_model
- QString m_vendor
- QString m_size
+ void getDiskInfoInterface(QString devicePath, QString &interface, QString &type)
+ void updateForHWDevice()
+ void getMapInfoFromSmartctl(QMap<QString, QString> &mapInfo, QString info, QString ch)
}
DeviceStorage : updateForHWDevice() now normalizes m_size for specific devices
DeviceStorage : updateForHWDevice() signature changed (devicePath parameter removed)
Flow diagram for disk size normalization in updateForHWDeviceflowchart TD
A["updateForHWDevice() called"] --> B["Check if product_name is empty"]
B -- Yes --> C["Return"]
B -- No --> D["Clear m_model and m_vendor"]
D --> E["Check if m_size is not empty"]
E -- Yes --> F["Parse m_size to bytes"]
F --> G["Convert bytes to GB"]
G --> H["Round GB to standard sizes"]
H --> I["Set m_size to 256 GB, 512 GB, 1 TB, or 2 TB"]
E -- No --> J["End"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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:
- Extract the magic thresholds (300, 600, 1200, 2200) into named constants or a configuration to improve readability and ease future adjustments.
- The size parsing and normalization logic is split between updateForHWDevice and formatDeviceSize, which can lead to inconsistencies; consider centralizing all size conversion and formatting in a single utility method.
- Disks larger than 2 TB (or values above 2200 GB) aren’t covered by the current logic—add a fallback or extend the threshold rules to handle higher capacities.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the magic thresholds (300, 600, 1200, 2200) into named constants or a configuration to improve readability and ease future adjustments.
- The size parsing and normalization logic is split between updateForHWDevice and formatDeviceSize, which can lead to inconsistencies; consider centralizing all size conversion and formatting in a single utility method.
- Disks larger than 2 TB (or values above 2200 GB) aren’t covered by the current logic—add a fallback or extend the threshold rules to handle higher capacities.
## Individual Comments
### Comment 1
<location> `service/diskoperation/DeviceStorage.cpp:632-634` </location>
<code_context>
+ qint64 bytes = 0;
+ QString sizeStr = m_size.toLower();
+
+ if (sizeStr.contains("bytes")) {
+ sizeStr = sizeStr.split("bytes").first().trimmed();
+ bytes = sizeStr.toLongLong();
+ } else {
+ // Handle GB/TB directly
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential parsing issue for non-numeric byte strings.
Add validation to handle cases where non-numeric characters precede 'bytes', as toLongLong() may not parse these correctly.
</issue_to_address>
### Comment 2
<location> `service/diskoperation/DeviceStorage.cpp:637` </location>
<code_context>
+ bytes = sizeStr.toLongLong();
+ } else {
+ // Handle GB/TB directly
+ double value = sizeStr.split(" ").first().toDouble();
+ if (sizeStr.contains("gb")) {
+ bytes = value * 1000 * 1000 * 1000;
</code_context>
<issue_to_address>
**suggestion:** No fallback for unrecognized units or malformed size strings.
Consider adding error handling or logging for unrecognized or malformed units to prevent silent failures.
</issue_to_address>
### Comment 3
<location> `service/diskoperation/DeviceStorage.cpp:664` </location>
<code_context>
+ }
+}
+
+void DeviceStorage::formatDeviceSize()
+{
+ static QRegularExpression numRegex("(\\d+)(?:\\.\\d+)?\\s*(TB|GB|MB)");
</code_context>
<issue_to_address>
**suggestion:** Regular expression does not handle lowercase units or extra whitespace.
Make the regex case-insensitive and allow for extra whitespace to ensure all valid unit formats are matched.
```suggestion
static QRegularExpression numRegex(R"((\d+)(?:\.\d+)?\s*(TB|GB|MB)\s*)", QRegularExpression::CaseInsensitiveOption);
```
</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
+632
to
+634
| if (sizeStr.contains("bytes")) { | ||
| sizeStr = sizeStr.split("bytes").first().trimmed(); | ||
| bytes = sizeStr.toLongLong(); |
There was a problem hiding this comment.
issue (bug_risk): Potential parsing issue for non-numeric byte strings.
Add validation to handle cases where non-numeric characters precede 'bytes', as toLongLong() may not parse these correctly.
| bytes = sizeStr.toLongLong(); | ||
| } else { | ||
| // Handle GB/TB directly | ||
| double value = sizeStr.split(" ").first().toDouble(); |
There was a problem hiding this comment.
suggestion: No fallback for unrecognized units or malformed size strings.
Consider adding error handling or logging for unrecognized or malformed units to prevent silent failures.
max-lvs
previously approved these changes
Oct 21, 2025
only for specific devices. Log: as above. Bug: https://pms.uniontech.com/bug-view-329231.html Task: https://pms.uniontech.com/task-view-368603.html
deepin pr auto review我来对这个diff进行代码审查：
改进建议： // 建议添加这些常量
static const int MIN_DISK_SIZE_GB = 200;
static const int SIZE_256GB_THRESHOLD = 300;
static const int SIZE_512GB_THRESHOLD = 600;
static const int SIZE_1TB_THRESHOLD = 1200;
static const int SIZE_2TB_THRESHOLD = 2200;
void DeviceStorage::updateForHWDevice(const QString &/*devicePath*/)
{
if (Utils::readContent("/proc/bootdevice/product_name").trimmed().isEmpty())
return;
// ... 其他代码保持不变 ...
// Normalize disk size
if (!m_size.isEmpty()) {
qint64 bytes = 0;
bool ok = false;
QString sizeStr = m_size.toLower().trimmed();
try {
if (sizeStr.contains("bytes")) {
bytes = sizeStr.split("bytes").first().trimmed().toLongLong(&ok);
} else {
// Handle GB/TB directly
double value = sizeStr.split(" ").first().toDouble(&ok);
if (ok) {
if (sizeStr.contains("gb")) {
bytes = static_cast<qint64>(value * 1000 * 1000 * 1000);
} else if (sizeStr.contains("tb")) {
bytes = static_cast<qint64>(value * 1000 * 1000 * 1000 * 1000);
}
}
}
// Convert to standardized format
if (ok && bytes > 0) {
// Convert to GB and round to standard sizes
int gb = qRound(bytes / (1000.0 * 1000 * 1000));
if (gb < MIN_DISK_SIZE_GB) {
return;
} else if (gb <= SIZE_256GB_THRESHOLD) {
m_size = "256 GB";
} else if (gb <= SIZE_512GB_THRESHOLD) {
m_size = "512 GB";
} else if (gb <= SIZE_1TB_THRESHOLD) {
m_size = "1 TB";
} else if (gb <= SIZE_2TB_THRESHOLD) {
m_size = "2 TB";
}
}
} catch (...) {
// 处理可能的转换异常
return;
}
}
}主要改进点：
这些改进将使代码更加健壮和安全，同时保持原有的功能不变。 |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `service/diskoperation/DeviceStorage.cpp:632-634` </location>
<code_context>
+ qint64 bytes = 0;
+ QString sizeStr = m_size.toLower();
+
+ if (sizeStr.contains("bytes")) {
+ sizeStr = sizeStr.split("bytes").first().trimmed();
+ bytes = sizeStr.toLongLong();
+ } else {
+ // Handle GB/TB directly
</code_context>
<issue_to_address>
**issue:** No error handling for non-numeric size strings.
If sizeStr is not a valid number, toLongLong() returns 0, which could cause incorrect results. Please add validation for the conversion.
</issue_to_address>
### Comment 2
<location> `service/diskoperation/DeviceStorage.cpp:636-642` </location>
<code_context>
+ bytes = sizeStr.toLongLong();
+ } else {
+ // Handle GB/TB directly
+ double value = sizeStr.split(" ").first().toDouble();
+ if (sizeStr.contains("gb")) {
+ bytes = value * 1000 * 1000 * 1000;
</code_context>
<issue_to_address>
**suggestion:** Potential locale issue with toDouble conversion.
If size strings may use different decimal separators, explicitly set the locale in toDouble() or validate the input format to avoid conversion errors.
```suggestion
// Handle GB/TB directly
QLocale locale(QLocale::C); // Use C locale for consistent decimal separator
double value = locale.toDouble(sizeStr.split(" ").first());
if (sizeStr.contains("gb")) {
bytes = value * 1000 * 1000 * 1000;
} else if (sizeStr.contains("tb")) {
bytes = value * 1000 * 1000 * 1000 * 1000;
}
```
</issue_to_address>
### Comment 3
<location> `service/diskoperation/DeviceStorage.cpp:635-642` </location>
<code_context>
+ } else {
+ // Handle GB/TB directly
+ double value = sizeStr.split(" ").first().toDouble();
+ if (sizeStr.contains("gb")) {
+ bytes = value * 1000 * 1000 * 1000;
+ } else if (sizeStr.contains("tb")) {
+ bytes = value * 1000 * 1000 * 1000 * 1000;
+ }
</code_context>
<issue_to_address>
**suggestion:** No handling for MB or other units.
Consider updating the logic to support MB and other units if they may be encountered.
```suggestion
} else {
// Handle KB/MB/GB/TB directly
QStringList parts = sizeStr.split(" ");
if (parts.size() == 2) {
double value = parts[0].toDouble();
QString unit = parts[1].trimmed();
if (unit == "kb") {
bytes = value * 1000;
} else if (unit == "mb") {
bytes = value * 1000 * 1000;
} else if (unit == "gb") {
bytes = value * 1000 * 1000 * 1000;
} else if (unit == "tb") {
bytes = value * 1000 * 1000 * 1000 * 1000;
} else {
// Unknown unit, fallback to value as bytes
bytes = value;
}
}
```
</issue_to_address>
### Comment 4
<location> `service/diskoperation/DeviceStorage.cpp:646-659` </location>
<code_context>
+ if (bytes > 0) {
+ // Convert to GB and round to standard sizes
+ int gb = qRound(bytes / (1000.0 * 1000 * 1000));
+ if (gb < 200) {
+ return;
+ } else if (gb <= 300) {
+ m_size = "256 GB";
+ } else if (gb <= 600) {
+ m_size = "512 GB";
+ } else if (gb <= 1200){
+ m_size = "1 TB";
+ } else if (gb <= 2200) {
+ m_size = "2 TB";
+ }
</code_context>
<issue_to_address>
**suggestion:** No fallback for sizes above 2 TB.
For disk sizes over 2 TB, m_size remains unset. Please add a fallback for these cases or clearly document this behavior.
```suggestion
if (bytes > 0) {
// Convert to GB and round to standard sizes
int gb = qRound(bytes / (1000.0 * 1000 * 1000));
if (gb < 200) {
return;
} else if (gb <= 300) {
m_size = "256 GB";
} else if (gb <= 600) {
m_size = "512 GB";
} else if (gb <= 1200){
m_size = "1 TB";
} else if (gb <= 2200) {
m_size = "2 TB";
} else {
m_size = "Over 2 TB";
}
```
</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
+632
to
+634
| if (sizeStr.contains("bytes")) { | ||
| sizeStr = sizeStr.split("bytes").first().trimmed(); | ||
| bytes = sizeStr.toLongLong(); |
There was a problem hiding this comment.
issue: No error handling for non-numeric size strings.
If sizeStr is not a valid number, toLongLong() returns 0, which could cause incorrect results. Please add validation for the conversion.
Comment on lines
+636
to
+642
| // Handle GB/TB directly | ||
| double value = sizeStr.split(" ").first().toDouble(); | ||
| if (sizeStr.contains("gb")) { | ||
| bytes = value * 1000 * 1000 * 1000; | ||
| } else if (sizeStr.contains("tb")) { | ||
| bytes = value * 1000 * 1000 * 1000 * 1000; | ||
| } |
There was a problem hiding this comment.
suggestion: Potential locale issue with toDouble conversion.
If size strings may use different decimal separators, explicitly set the locale in toDouble() or validate the input format to avoid conversion errors.
Suggested change
| // Handle GB/TB directly | |
| double value = sizeStr.split(" ").first().toDouble(); | |
| if (sizeStr.contains("gb")) { | |
| bytes = value * 1000 * 1000 * 1000; | |
| } else if (sizeStr.contains("tb")) { | |
| bytes = value * 1000 * 1000 * 1000 * 1000; | |
| } | |
| // Handle GB/TB directly | |
| QLocale locale(QLocale::C); // Use C locale for consistent decimal separator | |
| double value = locale.toDouble(sizeStr.split(" ").first()); | |
| if (sizeStr.contains("gb")) { | |
| bytes = value * 1000 * 1000 * 1000; | |
| } else if (sizeStr.contains("tb")) { | |
| bytes = value * 1000 * 1000 * 1000 * 1000; | |
| } |
Comment on lines
+635
to
+642
| } else { | ||
| // Handle GB/TB directly | ||
| double value = sizeStr.split(" ").first().toDouble(); | ||
| if (sizeStr.contains("gb")) { | ||
| bytes = value * 1000 * 1000 * 1000; | ||
| } else if (sizeStr.contains("tb")) { | ||
| bytes = value * 1000 * 1000 * 1000 * 1000; | ||
| } |
There was a problem hiding this comment.
suggestion: No handling for MB or other units.
Consider updating the logic to support MB and other units if they may be encountered.
Suggested change
| } else { | |
| // Handle GB/TB directly | |
| double value = sizeStr.split(" ").first().toDouble(); | |
| if (sizeStr.contains("gb")) { | |
| bytes = value * 1000 * 1000 * 1000; | |
| } else if (sizeStr.contains("tb")) { | |
| bytes = value * 1000 * 1000 * 1000 * 1000; | |
| } | |
| } else { | |
| // Handle KB/MB/GB/TB directly | |
| QStringList parts = sizeStr.split(" "); | |
| if (parts.size() == 2) { | |
| double value = parts[0].toDouble(); | |
| QString unit = parts[1].trimmed(); | |
| if (unit == "kb") { | |
| bytes = value * 1000; | |
| } else if (unit == "mb") { | |
| bytes = value * 1000 * 1000; | |
| } else if (unit == "gb") { | |
| bytes = value * 1000 * 1000 * 1000; | |
| } else if (unit == "tb") { | |
| bytes = value * 1000 * 1000 * 1000 * 1000; | |
| } else { | |
| // Unknown unit, fallback to value as bytes | |
| bytes = value; | |
| } | |
| } |
Comment on lines
+646
to
+659
| if (bytes > 0) { | ||
| // Convert to GB and round to standard sizes | ||
| int gb = qRound(bytes / (1000.0 * 1000 * 1000)); | ||
| if (gb < 200) { | ||
| return; | ||
| } else if (gb <= 300) { | ||
| m_size = "256 GB"; | ||
| } else if (gb <= 600) { | ||
| m_size = "512 GB"; | ||
| } else if (gb <= 1200){ | ||
| m_size = "1 TB"; | ||
| } else if (gb <= 2200) { | ||
| m_size = "2 TB"; | ||
| } |
There was a problem hiding this comment.
suggestion: No fallback for sizes above 2 TB.
For disk sizes over 2 TB, m_size remains unset. Please add a fallback for these cases or clearly document this behavior.
Suggested change
| if (bytes > 0) { | |
| // Convert to GB and round to standard sizes | |
| int gb = qRound(bytes / (1000.0 * 1000 * 1000)); | |
| if (gb < 200) { | |
| return; | |
| } else if (gb <= 300) { | |
| m_size = "256 GB"; | |
| } else if (gb <= 600) { | |
| m_size = "512 GB"; | |
| } else if (gb <= 1200){ | |
| m_size = "1 TB"; | |
| } else if (gb <= 2200) { | |
| m_size = "2 TB"; | |
| } | |
| if (bytes > 0) { | |
| // Convert to GB and round to standard sizes | |
| int gb = qRound(bytes / (1000.0 * 1000 * 1000)); | |
| if (gb < 200) { | |
| return; | |
| } else if (gb <= 300) { | |
| m_size = "256 GB"; | |
| } else if (gb <= 600) { | |
| m_size = "512 GB"; | |
| } else if (gb <= 1200){ | |
| m_size = "1 TB"; | |
| } else if (gb <= 2200) { | |
| m_size = "2 TB"; | |
| } else { | |
| m_size = "Over 2 TB"; | |
| } |
max-lvs
approved these changes
Oct 23, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: itsXuSt, max-lvs 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
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.
Log: as above.
Bug: https://pms.uniontech.com/bug-view-329231.html
Task: https://pms.uniontech.com/task-view-368603.html
Summary by Sourcery
Add disk size normalization for hardware devices by parsing raw size strings and mapping them to standard capacity tiers, and restrict the updateForHWDevice logic to devices with a non-empty boot product name.
Enhancements: