-
Notifications
You must be signed in to change notification settings - Fork 40
refactor(enum): rename special computer type enums to consistent nami… #599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…ng convention - Rename kCustomType to kSpecialType8 - Add new kSpecialType9 enum value - Update all references throughout codebase to use new enum names - Add support for kSpecialType9 in CPU frequency and monitor display logic pick from: linuxdeepin@6db8731 log: rename special computer type enums to consistent naming convention task: https://pms.uniontech.com/task-view-385813.html Change-Id: Id3da3d2b9ca530992ecddff69beb0da7a8fffcb6
Reviewer's GuideRefactors the SpecialComputerType enum to a consistent kSpecialType* naming scheme, introduces kSpecialType8 and kSpecialType9, and updates CPU, monitor, disk, and startup logic to handle the new names and the new special type behavior. Class diagram for Common::SpecialComputerType refactor and usagesclassDiagram
class Common {
+static SpecialComputerType specialComType
+static QString getArch()
+static QString checkBoardVendorFlag()
+static bool isHwPlatform()
}
class SpecialComputerType {
<<enumeration>>
Unknow = -1
NormalCom
kSpecialType1
kSpecialType2
kSpecialType3
kSpecialType4
kSpecialType5
kSpecialType6
kSpecialType7
kSpecialType8
kSpecialType9
}
class DeviceMonitor {
+void setInfoFromHwinfo(mapInfo)
+TomlFixMethod setInfoFromTomlOneByOne(mapInfo)
+QString getOverviewInfo()
+void loadBaseDeviceInfo()
+void loadOtherDeviceInfo()
+bool setMainInfoFromXrandr(info, rate)
-QString m_Name
-QString m_Vendor
-QString m_Model
-QString m_DisplayInput
-QString m_Interface
-QString m_ScreenSize
-QString m_CurrentResolution
-QString m_AspectRatio
-QString m_RefreshRate
}
class DeviceCpu {
+void setCpuInfo(mapLscpu, mapCpuinfo, mapProduct)
-QString m_Frequency
-QString m_MaxFrequency
}
class HWGenerator {
+void generatorDiskDevice()
}
class MainApplication {
+int main(argc, argv)
}
Common o-- SpecialComputerType : defines
DeviceMonitor ..> Common : uses_specialComType
DeviceCpu ..> Common : uses_specialComType
HWGenerator ..> Common : uses_specialComType
MainApplication ..> Common : uses_specialComType
Flow diagram for specialComType-dependent behavior across componentsflowchart TD
A["Application start"] --> B["Read Common::specialComType"]
B --> C{"specialComType in {Unknow, NormalCom, kSpecialType8}?"}
C -- "yes" --> D["Common::isHwPlatform returns false"]
C -- "no" --> E["Common::isHwPlatform returns true"]
B --> F{"specialComType == kSpecialType2?"}
F -- "yes" --> G["DeviceMonitor::setInfoFromTomlOneByOne sets m_IsTomlSet"]
F -- "no" --> H["Skip TOML flag for monitor"]
B --> I{"specialComType in {kSpecialType6, kSpecialType7, kSpecialType9}?"}
I -- "yes" --> J["DeviceMonitor::getOverviewInfo shows size only"]
I -- "no" --> K["DeviceMonitor::getOverviewInfo shows name and size"]
B --> L{"specialComType in {kSpecialType5, kSpecialType6, kSpecialType7, kSpecialType9}?"}
L -- "yes" --> M["DeviceMonitor::loadBaseDeviceInfo hides basic fields"]
L -- "no" --> N["DeviceMonitor::loadBaseDeviceInfo shows name, vendor, type, input"]
B --> O{"specialComType == kSpecialType4?"}
O -- "yes" --> P["DeviceMonitor::loadOtherDeviceInfo parses refresh rate from CurrentResolution"]
O -- "no" --> Q["Standard refresh rate handling"]
B --> R{"specialComType in {kSpecialType1, kSpecialType5, kSpecialType6, kSpecialType7, kSpecialType9}?"}
R -- "yes" --> S["DeviceMonitor::setMainInfoFromXrandr sets m_RefreshRate and custom m_CurrentResolution"]
R -- "no" --> T["DeviceMonitor::setMainInfoFromXrandr sets m_CurrentResolution with rate suffix"]
B --> U{"specialComType in {kSpecialType5, kSpecialType6, kSpecialType7, kSpecialType9}?"}
U -- "yes" --> V["DeviceCpu::setCpuInfo adjusts m_Frequency and m_MaxFrequency"]
U -- "no" --> W["DeviceCpu::setCpuInfo keeps default frequencies"]
B --> X{"specialComType == kSpecialType2?"}
X -- "yes" --> Y["HWGenerator::generatorDiskDevice sets disk Interface to UFS"]
X -- "no" --> Z["HWGenerator uses detected disk Interface"]
B --> AA{"specialComType == kSpecialType8?"}
AA -- "yes" --> AB["main.cpp pre-caches GPU info via CommonTools::preGenerateGpuInfo"]
AA -- "no" --> AC["No GPU pre-generation on startup"]
D --> AD["End platform-specific behavior"]
E --> AD
S --> AD
T --> AD
V --> AD
W --> AD
Y --> AD
Z --> AD
AB --> AD
AC --> AD
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- There are several places (e.g., DeviceMonitor::loadBaseDeviceInfo/loadOtherDeviceInfo) where string literals were wrapped in extra parentheses ("Name" → ("Name")); these add no benefit and reduce consistency with surrounding code, so consider reverting to the simpler form.
- Multiple conditions now repeat long chains of
specialComTypecomparisons (e.g., in DeviceMonitor::getOverviewInfo, loadBaseDeviceInfo, setMainInfoFromXrandr, and DeviceCpu::setCpuInfo); consider introducing small helper predicates (e.g.,isNoNameMonitorType(),isAdjustedRefreshRateType()) to centralize and simplify this logic. - Since the enum values were renamed to generic
kSpecialTypeNidentifiers while still encoding specific vendor/model semantics (PGUW, KLVV, etc.), it may be helpful to document or centralize the mapping between eachkSpecialTypeNand its underlying meaning to keep the codebase understandable over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are several places (e.g., DeviceMonitor::loadBaseDeviceInfo/loadOtherDeviceInfo) where string literals were wrapped in extra parentheses ("Name" → ("Name")); these add no benefit and reduce consistency with surrounding code, so consider reverting to the simpler form.
- Multiple conditions now repeat long chains of `specialComType` comparisons (e.g., in DeviceMonitor::getOverviewInfo, loadBaseDeviceInfo, setMainInfoFromXrandr, and DeviceCpu::setCpuInfo); consider introducing small helper predicates (e.g., `isNoNameMonitorType()`, `isAdjustedRefreshRateType()`) to centralize and simplify this logic.
- Since the enum values were renamed to generic `kSpecialTypeN` identifiers while still encoding specific vendor/model semantics (PGUW, KLVV, etc.), it may be helpful to document or centralize the mapping between each `kSpecialTypeN` and its underlying meaning to keep the codebase understandable over time.
## Individual Comments
### Comment 1
<location> `deepin-devicemanager/src/commonfunction.cpp:172` </location>
<code_context>
QString Common::checkBoardVendorFlag()
</code_context>
<issue_to_address>
**suggestion:** The mapping from `SpecialComputerType` to `boardVendorKey` doesn’t cover all enum values; verify that the default case is correct for the new types.
In `checkBoardVendorFlag`, `NormalCom`, `kSpecialType1..5`, and `kSpecialType8` are handled explicitly, but `kSpecialType6`, `kSpecialType7`, and `kSpecialType9` fall through to the default (empty key). Since `kSpecialType9` is used in device-specific branches elsewhere, consider adding explicit cases (or clearly documenting that these share the default behavior) to avoid unintended configuration differences.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| boardVendorKey = "PGUX"; | ||
| break; | ||
| case kCustomType: | ||
| case kSpecialType8: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: The mapping from SpecialComputerType to boardVendorKey doesn’t cover all enum values; verify that the default case is correct for the new types.
In checkBoardVendorFlag, NormalCom, kSpecialType1..5, and kSpecialType8 are handled explicitly, but kSpecialType6, kSpecialType7, and kSpecialType9 fall through to the default (empty key). Since kSpecialType9 is used in device-specific branches elsewhere, consider adding explicit cases (or clearly documenting that these share the default behavior) to avoid unintended configuration differences.
deepin pr auto review这段代码主要进行了一次重构,将硬编码的数字类型标识符(如 1. 语法与逻辑审查
2. 代码质量审查
3. 代码性能审查
4. 代码安全审查
总结与改进建议代码示例建议修改 enum SpecialComputerType {
Unknown = -1, // 修正拼写
NormalCom, // 普通机型
kSpecialType1, // 对应原 PGUW (厂商A)
kSpecialType2, // 对应原 KLVV (厂商B - UFS接口)
kSpecialType3, // 对应原 KLVU
kSpecialType4, // 对应原 PGUV
kSpecialType5, // 对应原 PGUX
kSpecialType6,
kSpecialType7,
kSpecialType8, // 对应原 kCustomType
kSpecialType9 // 新增机型
};建议修改 // 修改前
addBaseDeviceInfo(("Name"), m_Name);
// 修改后
addBaseDeviceInfo("Name", m_Name);关于 // 修正特定机型(KLVV/PGUW等)BIOS报告频率偏差的问题: 2.189GHz 实际应为 2.188GHz
if (Common::specialComType == Common::kSpecialType2 || ...) {
m_Frequency = m_Frequency.replace("2.189", "2.188");
// ...
}总体而言,这次提交提高了代码的可读性和可维护性,但在命名语义化和硬编码处理方面还有进一步优化的空间。 |
|
[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 |
…ng convention
pick from: 6db8731
log: rename special computer type enums to consistent naming convention
task: https://pms.uniontech.com/task-view-385813.html
Change-Id: Id3da3d2b9ca530992ecddff69beb0da7a8fffcb6
Summary by Sourcery
Unify special computer type enums under a consistent naming scheme and extend special-type handling across device monitoring and CPU logic.
New Features:
Enhancements: