pick develop/eagle to master #559
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
fix the w515 device of monitor display pick from: linuxdeepin@d6a9c9f Log: fix the w515 device of monitor display Task: https://pms.uniontech.com/task-view-368603.html
fix the w525 device of monitor display pick form: linuxdeepin@02ee8d0 Log: fix the w525 device of monitor display Task: https://pms.uniontech.com/task-view-368603.html
fix the UFS display Log: fix the UFS display Bug: https://pms.uniontech.com/bug-view-276131.html Change-Id: I41512e4466ff5b0dc4196b4c444ad5488c44a2b2
fix the camera info show incorrect Log: fix the camera info show incorrect Bug: https://pms.uniontech.com/bug-view-290539.html Change-Id: Ibea8fb2ff9150674c7257fdc6ab9ca1159d77ff2
fix the bluetooth of false identification pick from: linuxdeepin@366a729 Log: fix the bluetooth of false identification Task: https://pms.uniontech.com/task-view-360593.html Change-Id: I56de4352eed27414aba935daa97cd72ef76dd309
fix the device of monitor display pick from: linuxdeepin@02f7a69 Log: fix the device of monitor display Bug: https://pms.uniontech.com/bug-view-296425.html https: //pms.uniontech.com/bug-view-296689.html Change-Id: Ief79bbd158fef4f7a2b9332ec87e82246cd55a87
Reviewer's GuideAdds monitor refresh-rate handling and special OEM-type behaviors, extends Bluetooth device detection, adjusts UFS disk metadata generation, and ensures device images are forcibly displayable. Class diagram for updated device model classesclassDiagram
class DeviceMonitor {
QString m_Name
QString m_ScreenSize
QString m_CurrentResolution
QString m_RefreshRate
QString getOverviewInfo()
void loadBaseDeviceInfo()
void loadOtherDeviceInfo()
bool setMainInfoFromXrandr(QString info, QString rate)
}
class DeviceGenerator {
void getBlueToothInfoFromHwinfo()
}
class HWGenerator {
void generatorDiskDevice()
}
class DeviceImage {
bool m_CanEnable
bool m_CanUninstall
bool m_forcedDisplay
DeviceImage()
}
Flow diagram for monitor resolution and refresh-rate handlingflowchart TD
A[setMainInfoFromXrandr called with info, rate] --> B[Parse current rate with regular expression]
B --> C{Rate match found and boardVendorType not empty?}
C -- No --> H[Set m_CurrentResolution without rate]
C -- Yes --> D{Common.specialComType == 1?}
D -- Yes --> E[Round rate integer part and append .00 plus suffix]
D -- No --> F[Round rate integer part and append suffix]
E --> G[Set curRate]
F --> G[Set curRate]
G --> I{Common.specialComType is 1 or 6?}
I -- Yes --> J[Set m_RefreshRate from curRate]
I -- No --> K[Do not change m_RefreshRate]
J --> L
K --> L
L{Common.specialComType is 5 or 6?} -- Yes --> M[Set m_CurrentResolution to screen size only]
L -- No --> N[Set m_CurrentResolution to screen size and curRate with space before @]
H --> O[Return from function]
M --> O[Return from function]
N --> O[Return from function]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review我来对这段代码进行审查：
问题与建议：
总体建议：
|
![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:
- In
DeviceMonitor::getOverviewInfo,ovis assigned twice with effectively the same pattern; consider computing it once with a single conditional expression to avoid redundant formatting and make the intent clearer. - The parsing of
m_CurrentResolutionto extractm_RefreshRateinloadOtherDeviceInfoadds a second place where resolution strings are interpreted (in addition tosetMainInfoFromXrandr); consider centralizing this parsing logic to a helper to keep the format assumptions consistent. - The Bluetooth device detection condition now hardcodes a full device name string (
"SCM2625 BT 5.2 network adapter"); if this is expected to vary slightly across systems, consider using a case-insensitivecontainscheck or a more general pattern to make the match more robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `DeviceMonitor::getOverviewInfo`, `ov` is assigned twice with effectively the same pattern; consider computing it once with a single conditional expression to avoid redundant formatting and make the intent clearer.
- The parsing of `m_CurrentResolution` to extract `m_RefreshRate` in `loadOtherDeviceInfo` adds a second place where resolution strings are interpreted (in addition to `setMainInfoFromXrandr`); consider centralizing this parsing logic to a helper to keep the format assumptions consistent.
- The Bluetooth device detection condition now hardcodes a full device name string (`"SCM2625 BT 5.2 network adapter"`); if this is expected to vary slightly across systems, consider using a case-insensitive `contains` check or a more general pattern to make the match more robust.
## Individual Comments
### Comment 1
<location> `deepin-devicemanager/src/DeviceManager/DeviceMonitor.cpp:450-451` </location>
<code_context>
+ curRate = QString::number(ceil(curRate.left(pos).toDouble())) + curRate.right(curRate.size() - pos);
+ }
+ }
+ if (Common::specialComType == 1 || Common::specialComType == 6) {
+ m_RefreshRate = QString("%1").arg(curRate);
}
- if (Common::specialComType == 5) {
</code_context>
<issue_to_address>
**issue (bug_risk):** m_RefreshRate may retain stale values when no rate is detected
`m_RefreshRate` is only updated when `reRate.indexIn(rate)` matches and the `specialComType` condition holds. When no rate is found (or the condition fails), it retains the previous value, which may be wrong for the current monitor. Please clear or reset `m_RefreshRate` when no valid rate is detected so consumers can distinguish “no rate” from a stale value.
</issue_to_address>
### Comment 2
<location> `deepin-devicemanager/src/DeviceManager/DeviceMonitor.cpp:342` </location>
<code_context>
QString ov;
ov = QString("%1(%2)").arg(m_Name).arg(m_ScreenSize);
+ if (Common::specialComType == 6) {
+ ov = QString("(%1)").arg(m_ScreenSize);
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new monitor-handling logic to remove redundant assignments, replace magic specialComType numbers with named enums, and centralize resolution/refresh-rate formatting into small helpers.
You can keep all the new behavior but significantly reduce complexity and duplication with a few small refactors.
### 1. Simplify `getOverviewInfo`
The current logic reassigns `ov` unnecessarily and repeats the same format string:
```cpp
QString ov;
ov = QString("%1(%2)").arg(m_Name).arg(m_ScreenSize);
if (Common::specialComType == 6) {
ov = QString("(%1)").arg(m_ScreenSize);
} else {
ov = QString("%1(%2)").arg(m_Name).arg(m_ScreenSize);
}
```
You can remove the redundant assignment and use a conditional expression (or a tiny helper) instead:
```cpp
const QString DeviceMonitor::getOverviewInfo()
{
qCDebug(appLog) << "Getting monitor overview information";
const QString ov = (Common::specialComType == 6)
? QString("(%1)").arg(m_ScreenSize)
: QString("%1(%2)").arg(m_Name).arg(m_ScreenSize);
qCDebug(appLog) << "Monitor overview:" << ov;
return ov;
}
```
### 2. Replace magic `specialComType` numbers with named values
You already have several magic numbers (1, 4, 5, 6). You can centralize their meaning without changing behavior by introducing an enum (or `enum class`) and using it where you branch:
```cpp
namespace Common {
enum SpecialComType {
Normal = 0,
RateWithDecimals = 1, // was 1
ParseResolutionRate = 4, // was 4
NoRateInResolution = 5, // was 5
NameLessOverview = 6 // was 6
};
}
```
Then update conditions locally for clarity:
```cpp
if (Common::specialComType == Common::NameLessOverview) { ... }
if (Common::specialComType == Common::ParseResolutionRate) { ... }
// etc.
```
This keeps all branching exactly as-is but makes the intent much clearer and reduces cognitive load.
### 3. Localize resolution/refresh-rate formatting
Right now, resolution and refresh rate formatting is scattered:
- `setMainInfoFromXrandr`:
- `specialComType == 1`: tweak `curRate` string with a `".00"` inserted.
- `specialComType == 1 || 6`: set `m_RefreshRate`.
- `specialComType == 5 || 6`: set `m_CurrentResolution` without `@`.
- Else: `"Size @Rate"` format with a space.
- `loadOtherDeviceInfo`:
- `specialComType == 4`: parse `m_CurrentResolution` to extract refresh rate.
You can centralize this into compact helpers that preserve behavior but remove duplication and condition scattering.
#### 3.1 Extract a `formatCurRate` helper
This removes the duplicated `curRate` expression with/without `".00"`:
```cpp
static QString formatCurRate(const QString &rawRate, int specialComType)
{
QRegularExpression rateStart("[a-zA-Z]");
QRegularExpressionMatch rateMatch = rateStart.match(rawRate);
int pos = rateMatch.capturedStart();
if (pos <= 0 || rawRate.size() <= pos || Common::boardVendorType().isEmpty())
return rawRate;
const QString base = QString::number(ceil(rawRate.left(pos).toDouble()));
const QString suffix = rawRate.right(rawRate.size() - pos);
if (specialComType == Common::RateWithDecimals) { // was 1
return base + ".00" + suffix;
}
return base + suffix;
}
```
Use it in `setMainInfoFromXrandr`:
```cpp
QString curRate = formatCurRate(rate, Common::specialComType);
if (Common::specialComType == Common::RateWithDecimals ||
Common::specialComType == Common::NameLessOverview) {
m_RefreshRate = curRate;
}
if (Common::specialComType == Common::NoRateInResolution ||
Common::specialComType == Common::NameLessOverview) {
m_CurrentResolution = QT_REGEXP_CAPTURE(reScreenSize, 1, info);
} else {
m_CurrentResolution = QString("%1 @%2")
.arg(QT_REGEXP_CAPTURE(reScreenSize, 1, info))
.arg(curRate);
}
```
Behavior is unchanged; logic is easier to follow and the `curRate` formatting is in one place.
#### 3.2 Extract refresh-rate parsing from `m_CurrentResolution`
The parsing for `specialComType == 4` can also be centralized to avoid repeating rules elsewhere:
```cpp
static QString extractRefreshRateFromResolution(const QString &resolution)
{
if (!resolution.contains('@'))
return {};
const QStringList parts = resolution.split('@', QT_SKIP_EMPTY_PARTS);
if (parts.size() != 2)
return {};
return parts.at(1).trimmed();
}
```
Then `loadOtherDeviceInfo` becomes:
```cpp
if (m_CurrentResolution != "@Hz") {
addOtherDeviceInfo("Current Resolution", m_CurrentResolution);
addOtherDeviceInfo("Display Ratio", m_AspectRatio);
if (Common::specialComType == Common::ParseResolutionRate) {
m_RefreshRate = extractRefreshRateFromResolution(m_CurrentResolution);
}
}
```
This centralizes the `@` parsing logic; if other `specialComType` values later need similar parsing, it’s now reusable.
---
All of the above keeps your feature behavior intact, but:
- eliminates redundant assignments and duplicate string formatting,
- replaces magic numbers with named constants,
- and localizes resolution/refresh-rate formatting and parsing into small, focused helpers.
</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
+450
to
+451
| m_RefreshRate = QString("%1").arg(curRate); | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): m_RefreshRate may retain stale values when no rate is detected
m_RefreshRate is only updated when reRate.indexIn(rate) matches and the specialComType condition holds. When no rate is found (or the condition fails), it retains the previous value, which may be wrong for the current monitor. Please clear or reset m_RefreshRate when no valid rate is detected so consumers can distinguish “no rate” from a stale value.
| QString ov; | ||
|
|
||
| ov = QString("%1(%2)").arg(m_Name).arg(m_ScreenSize); | ||
| if (Common::specialComType == 6) { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the new monitor-handling logic to remove redundant assignments, replace magic specialComType numbers with named enums, and centralize resolution/refresh-rate formatting into small helpers.
You can keep all the new behavior but significantly reduce complexity and duplication with a few small refactors.
1. Simplify getOverviewInfo
The current logic reassigns ov unnecessarily and repeats the same format string:
QString ov;
ov = QString("%1(%2)").arg(m_Name).arg(m_ScreenSize);
if (Common::specialComType == 6) {
ov = QString("(%1)").arg(m_ScreenSize);
} else {
ov = QString("%1(%2)").arg(m_Name).arg(m_ScreenSize);
}You can remove the redundant assignment and use a conditional expression (or a tiny helper) instead:
const QString DeviceMonitor::getOverviewInfo()
{
qCDebug(appLog) << "Getting monitor overview information";
const QString ov = (Common::specialComType == 6)
? QString("(%1)").arg(m_ScreenSize)
: QString("%1(%2)").arg(m_Name).arg(m_ScreenSize);
qCDebug(appLog) << "Monitor overview:" << ov;
return ov;
}2. Replace magic specialComType numbers with named values
You already have several magic numbers (1, 4, 5, 6). You can centralize their meaning without changing behavior by introducing an enum (or enum class) and using it where you branch:
namespace Common {
enum SpecialComType {
Normal = 0,
RateWithDecimals = 1, // was 1
ParseResolutionRate = 4, // was 4
NoRateInResolution = 5, // was 5
NameLessOverview = 6 // was 6
};
}Then update conditions locally for clarity:
if (Common::specialComType == Common::NameLessOverview) { ... }
if (Common::specialComType == Common::ParseResolutionRate) { ... }
// etc.This keeps all branching exactly as-is but makes the intent much clearer and reduces cognitive load.
3. Localize resolution/refresh-rate formatting
Right now, resolution and refresh rate formatting is scattered:
setMainInfoFromXrandr:specialComType == 1: tweakcurRatestring with a".00"inserted.specialComType == 1 || 6: setm_RefreshRate.specialComType == 5 || 6: setm_CurrentResolutionwithout@.- Else:
"Size @Rate"format with a space.
loadOtherDeviceInfo:specialComType == 4: parsem_CurrentResolutionto extract refresh rate.
You can centralize this into compact helpers that preserve behavior but remove duplication and condition scattering.
3.1 Extract a formatCurRate helper
This removes the duplicated curRate expression with/without ".00":
static QString formatCurRate(const QString &rawRate, int specialComType)
{
QRegularExpression rateStart("[a-zA-Z]");
QRegularExpressionMatch rateMatch = rateStart.match(rawRate);
int pos = rateMatch.capturedStart();
if (pos <= 0 || rawRate.size() <= pos || Common::boardVendorType().isEmpty())
return rawRate;
const QString base = QString::number(ceil(rawRate.left(pos).toDouble()));
const QString suffix = rawRate.right(rawRate.size() - pos);
if (specialComType == Common::RateWithDecimals) { // was 1
return base + ".00" + suffix;
}
return base + suffix;
}Use it in setMainInfoFromXrandr:
QString curRate = formatCurRate(rate, Common::specialComType);
if (Common::specialComType == Common::RateWithDecimals ||
Common::specialComType == Common::NameLessOverview) {
m_RefreshRate = curRate;
}
if (Common::specialComType == Common::NoRateInResolution ||
Common::specialComType == Common::NameLessOverview) {
m_CurrentResolution = QT_REGEXP_CAPTURE(reScreenSize, 1, info);
} else {
m_CurrentResolution = QString("%1 @%2")
.arg(QT_REGEXP_CAPTURE(reScreenSize, 1, info))
.arg(curRate);
}Behavior is unchanged; logic is easier to follow and the curRate formatting is in one place.
3.2 Extract refresh-rate parsing from m_CurrentResolution
The parsing for specialComType == 4 can also be centralized to avoid repeating rules elsewhere:
static QString extractRefreshRateFromResolution(const QString &resolution)
{
if (!resolution.contains('@'))
return {};
const QStringList parts = resolution.split('@', QT_SKIP_EMPTY_PARTS);
if (parts.size() != 2)
return {};
return parts.at(1).trimmed();
}Then loadOtherDeviceInfo becomes:
if (m_CurrentResolution != "@Hz") {
addOtherDeviceInfo("Current Resolution", m_CurrentResolution);
addOtherDeviceInfo("Display Ratio", m_AspectRatio);
if (Common::specialComType == Common::ParseResolutionRate) {
m_RefreshRate = extractRefreshRateFromResolution(m_CurrentResolution);
}
}This centralizes the @ parsing logic; if other specialComType values later need similar parsing, it’s now reusable.
All of the above keeps your feature behavior intact, but:
- eliminates redundant assignments and duplicate string formatting,
- replaces magic numbers with named constants,
- and localizes resolution/refresh-rate formatting and parsing into small, focused helpers.
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 |
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 monitor, Bluetooth, storage, and device image handling for specific hardware configurations and vendors.
New Features:
Enhancements: