chore: extend toml #554
Merged
chore: extend toml #554
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
extend toml and uniform some model arm code Log: extend toml Bug: https://pms.uniontech.com/task-view-368603.html
deepin pr auto review我来对这次代码变更进行审查。这是一个较大的变更,涉及多个文件的修改,主要集中在设备管理和配置方面。我将从代码质量、性能和安全性几个方面进行分析。
建议改进的地方:
总的来说,这次变更提高了代码的可维护性和灵活性,但还需要在错误处理和测试方面进一步完善。建议在后续版本中持续改进这些方面。 |
Reviewer's GuideRefactors TOML-driven device customization and ARM board special handling across the device manager: introduces a generic tomlmatchkey-based matching mechanism, reworks Wi-Fi/Bluetooth/network/disk/monitor generation for special vendors, centralizes translation via translateStr, and tightens storage / network enumeration and sizing logic while adding configuration-driven TOML file selection. Sequence diagram for TOML file discovery and applicationsequenceDiagram
autonumber
participant UserSession
participant MainWindow
participant Common
participant CmdTool
participant DeviceManager
UserSession->>MainWindow: startApplication()
MainWindow->>MainWindow: initWindowTitle()
MainWindow->>Common: tomlFilesNameSet(tomlNameFromDConfig)
UserSession->>CmdTool: loadDmidecodeInfo(key="dmidecode1")
CmdTool->>CmdTool: loadOemTomlFileName(mapInfo)
CmdTool->>CmdTool: parseOemTomlInfo(defaultTomlName)
alt customTomlName configured
CmdTool->>Common: tomlFilesNameGet()
CmdTool->>CmdTool: parseOemTomlInfo(customTomlName)
end
loop forEach DeviceType
CmdTool->>DeviceManager: tomlDeviceSet(deviceType)
DeviceManager->>DeviceManager: load tomlMapLst for deviceType
loop forEach tomlEntry in tomlMapLst
DeviceManager->>DeviceManager: tomlSetBytomlmatchkey(deviceType, device, tomlmatchkey, tomlconfigdemanding)
alt tomlconfigdemanding == "adjust"
alt tomlmatchkey matches device
DeviceManager->>DeviceManager: tomlDeviceMapSet(deviceType, device, tomlEntry)
end
else tomlconfigdemanding == "delete"
alt tomlmatchkey matches device
DeviceManager->>DeviceManager: tomlDeviceMapSet(deviceType, device, tomlEntry)
DeviceManager->>DeviceManager: tomlDeviceDel(deviceType, device)
end
else tomlconfigdemanding == "add"
alt no device matched
DeviceManager->>DeviceManager: createDevice(deviceType)
DeviceManager->>DeviceManager: tomlDeviceMapSet(deviceType, newDevice, tomlEntry)
DeviceManager->>DeviceManager: tomlDeviceAdd(deviceType, newDevice)
end
else default matching
alt not matched by tomlmatchkey
DeviceManager->>DeviceManager: findByModalias(deviceType, device, modalias)
DeviceManager->>DeviceManager: findByVIDPID(deviceType, device, vid, pid)
DeviceManager->>DeviceManager: findByVendorName(deviceType, device, vendor, name)
end
alt matched and not yet fixed
DeviceManager->>DeviceManager: tomlDeviceMapSet(deviceType, device, tomlEntry)
alt tomlDeviceMapSet returns TOML_Del
DeviceManager->>DeviceManager: tomlDeviceDel(deviceType, device)
end
end
end
end
end
Sequence diagram for updated network device generationsequenceDiagram
autonumber
participant HWGenerator
participant DeviceGenerator
participant DeviceManager
participant CmdTool as CmdInfoProvider
participant DeviceNetwork
HWGenerator->>DeviceGenerator: generatorNetworkDevice()
activate DeviceGenerator
Note over DeviceGenerator: New logic builds list solely from lshw/hwinfo
DeviceGenerator->>DeviceManager: cmdInfo("lshw_network")
DeviceManager-->>DeviceGenerator: QList~QMap~ for lshw
loop forEach lshwEntry
alt has serial and logical name
DeviceGenerator->>DeviceGenerator: validate logicalName and macAddress
alt logicalName contains wlan and hasWlan is true
DeviceGenerator->>DeviceGenerator: skip entry (only 1 wlan)
else invalid MAC format or all zeros/ffs
DeviceGenerator->>DeviceGenerator: skip entry
else sysfs path exists in /sys/class/net
DeviceGenerator->>DeviceNetwork: new DeviceNetwork()
DeviceGenerator->>DeviceNetwork: setInfoFromLshw(lshwEntry)
DeviceGenerator->>DeviceGenerator: append to lstDevice
alt logicalName contains wlan
DeviceGenerator->>DeviceGenerator: set hasWlan = true
end
end
else
DeviceGenerator->>DeviceGenerator: skip entry
end
end
DeviceGenerator->>DeviceManager: cmdInfo("hwinfo_network")
DeviceManager-->>DeviceGenerator: QList~QMap~ for hwinfo
loop forEach hwinfoEntry
alt contains HW Address and Device File
DeviceGenerator->>DeviceGenerator: find matching DeviceNetwork in lstDevice
alt hwinfoEntry has unique_id
DeviceGenerator->>DeviceNetwork: setEnableValue(false)
else match by HW Address or Device File
DeviceGenerator->>DeviceNetwork: setInfoFromHwinfo(hwinfoEntry)
else no match
DeviceGenerator->>DeviceNetwork: setCanUninstall(false)
DeviceGenerator->>DeviceNetwork: setForcedDisplay(true)
end
else
DeviceGenerator->>DeviceGenerator: skip entry
end
end
loop forEach device in lstDevice
DeviceGenerator->>DeviceManager: addNetworkDevice(device)
end
deactivate DeviceGenerator
Class diagram for updated TOML matching and device generation logicclassDiagram
direction TB
class DeviceManager {
+tomlDeviceSet(deviceType)
+tomlDeviceReadKeyValue(deviceType, device, key) QString
+tomlDeviceMapSet(deviceType, device, mapInfo) TomlFixMethod
+tomlDeviceDel(deviceType, device)
+tomlDeviceAdd(deviceType, device)
+tomlSetBytomlmatchkey(deviceType, device, tomltomlmatchkey, tomltomlconfigdemanding) bool
+findByModalias(deviceType, device, modalias) bool
+findByVIDPID(deviceType, device, vid, pid) bool
+findByVendorName(deviceType, device, vendor, name) bool
-m_ListDeviceBluetooth
-m_ListDeviceNetwork
}
class DeviceGenerator {
+generatorNetworkDevice()
+generatorAudioDevice()
+getDiskInfoFromSmartCtl()
}
class HWGenerator {
+generatorCpuDevice()
+generatorAudioDevice()
+generatorBluetoothDevice()
+generatorNetworkDevice()
+generatorDiskDevice()
+generatorMonitorDevice()
+generatorPowerDevice()
+generatorKeyboardDevice()
+generatorMemoryDevice()
+generatorCameraDevice()
}
class DeviceStorage {
-m_SizeBytes : quint64
-m_Size : QString
-m_Interface : QString
-m_MediaType : QString
+unitConvertByDecimal()
+checkDiskSize()
+loadBaseDeviceInfo()
+loadOtherDeviceInfo()
+loadTableData()
+getInfoFromsmartctl(mapInfo)
}
class DeviceNetwork {
+setInfoFromLshw(mapInfo) void
+setInfoFromHwinfo(mapInfo) bool
+setIsWireless(sysfs) void
+loadTableData() void
-m_Name : QString
-m_Model : QString
-m_Vendor : QString
-m_IsWireless : bool
-m_TableData : QStringList
}
class DeviceBluetooth {
+setInfoFromTomlOneByOne(mapInfo) TomlFixMethod
+loadTableData() void
-m_Name : QString
-m_Model : QString
-m_Vendor : QString
-m_TableData : QStringList
}
class DeviceBaseInfo {
+getTableData() const QStringList&
+readDeviceInfoKeyValue(key) const QString
+isValueValid(value) bool
+getDriverVersion() const QString
+getOtherAttribs() const QList~QPair~QString,QString~~
#loadTableData() void
#addBaseDeviceInfo(key, value) void
#addOtherDeviceInfo(key, value) void
}
class DeviceMonitor {
+parseMonitorSize(sizeDescription, inch) QString
+caculateScreenSize() void
-m_Width : int
-m_Height : int
-m_ScreenSize : QString
}
class CmdTool {
+parseOemTomlInfo(filename) bool
+loadDmidecodeInfo(key, debugfile) void
}
class Common {
+boardVendorType() QString
+specialVendorType() QString
+specialHString() QString
+tomlFilesNameGet() QString
+tomlFilesNameSet(name) void
+executeClientCmd(cmd, args, workPath, msecsWaiting, useEnv) QByteArray
}
class MainWindow {
+initWindowTitle() void
}
DeviceGenerator <|-- HWGenerator
DeviceBaseInfo <|-- DeviceStorage
DeviceBaseInfo <|-- DeviceNetwork
DeviceBaseInfo <|-- DeviceBluetooth
DeviceBaseInfo <|-- DeviceMonitor
DeviceManager o--> DeviceBaseInfo
DeviceManager o--> DeviceNetwork
DeviceManager o--> DeviceBluetooth
DeviceManager o--> DeviceStorage
CmdTool ..> DeviceManager : uses tomlDeviceSet
CmdTool ..> Common : uses tomlFilesNameGet
MainWindow ..> Common : uses tomlFilesNameSet
HWGenerator ..> DeviceManager : uses tomlDeviceMapSet
DeviceGenerator ..> DeviceManager : uses cmdInfo, setStorageInfoFromSmartctl
DeviceStorage ..> Common : uses specialVendorType
DeviceNetwork ..> Common : uses translateStr, boardVendorType
DeviceBluetooth ..> Common : uses translateStr
DeviceMonitor ..> Common : uses translateStr
DeviceManager ..> Common : uses specialHString
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 and found some issues that need to be addressed.
- In DeviceGenerator::generatorNetworkDevice, the initial construction of lstDevice from /sys/class/net was removed but the new lshw-based creation of DeviceNetwork objects is still nested inside a loop over lstDevice, which starts empty, so no devices will ever be created from lshw; consider moving the
new DeviceNetwork()creation logic outside thefor (itDevice = lstDevice.begin(); ... )loop or restructuring to iterate lshw entries directly when building the list. - DeviceBaseInfo::getTableData now sets
trKey = iteminstead of calling translateStr(item), which effectively disables translations of table entries; if this was not intentional, restore the translateStr call or otherwise ensure localized strings are still returned. - DeviceManager::tomlSetBytomlmatchkey takes a tomltomlconfigdemanding parameter that is never used and always returns true/false solely based on matching key/value pairs; either remove the unused parameter or make the function responsible only for matching and keep the policy logic (add/adjust/delete) clearly separated in tomlDeviceSet to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In DeviceGenerator::generatorNetworkDevice, the initial construction of lstDevice from /sys/class/net was removed but the new lshw-based creation of DeviceNetwork objects is still nested inside a loop over lstDevice, which starts empty, so no devices will ever be created from lshw; consider moving the `new DeviceNetwork()` creation logic outside the `for (itDevice = lstDevice.begin(); ... )` loop or restructuring to iterate lshw entries directly when building the list.
- DeviceBaseInfo::getTableData now sets `trKey = item` instead of calling translateStr(item), which effectively disables translations of table entries; if this was not intentional, restore the translateStr call or otherwise ensure localized strings are still returned.
- DeviceManager::tomlSetBytomlmatchkey takes a tomltomlconfigdemanding parameter that is never used and always returns true/false solely based on matching key/value pairs; either remove the unused parameter or make the function responsible only for matching and keep the policy logic (add/adjust/delete) clearly separated in tomlDeviceSet to avoid confusion.
## Individual Comments
### Comment 1
<location> `deepin-devicemanager/src/GenerateDevice/DeviceGenerator.cpp:303-309` </location>
<code_context>
- }
-
// 设置从lshw中获取的信息
const QList<QMap<QString, QString>> &lstLshw = DeviceManager::instance()->cmdInfo("lshw_network");
for (QList<QMap<QString, QString> >::const_iterator it = lstLshw.begin(); it != lstLshw.end(); ++it) {
- if ((*it).find("serial") == (*it).end() && (*it).find("logical name") == (*it).end())
+ if ((*it).find("serial") == (*it).end() || (*it).find("logical name") == (*it).end())
continue;
- const QString &serialNumberLshw = (*it)["serial"];
const QString &logicalNameLshw = (*it)["logical name"];
for (QList<DeviceNetwork *>::iterator itDevice = lstDevice.begin(); itDevice != lstDevice.end(); ++itDevice) {
- const QString &serialNumberLst = (*itDevice)->hwAddress();
- const QString &logicalNameLst = (*itDevice)->logicalName();
</code_context>
<issue_to_address>
**issue (bug_risk):** Network devices are never created because the lshw loop iterates an always-empty lstDevice.
Previously `lstDevice` was pre-populated (e.g. from `/sys/class/net`) and lshw/hwinfo only enriched existing entries. In the new code that enumeration is gone, and device creation happens only inside the `for (itDevice = lstDevice.begin(); ...)` loop. Because `lstDevice` is empty, the loop body (including `new DeviceNetwork()` and `lstDevice.append(device)`) never runs, so no network devices are added to `DeviceManager`. Please either restore an initial population of `lstDevice` or create `DeviceNetwork` objects directly while processing each valid lshw entry, without wrapping that logic in the `for (itDevice ...)` loop.
</issue_to_address>
### Comment 2
<location> `deepin-devicemanager/src/DeviceManager/DeviceInfo.cpp:138` </location>
<code_context>
loadTableData();
m_TableDataTr.clear();
for (const auto &item : m_TableData) {
- QString trKey = translateStr(item);
- if(trKey.isEmpty())
+ QString trKey = item;
+ if (trKey.isEmpty())
m_TableDataTr.append(item);
</code_context>
<issue_to_address>
**issue (bug_risk):** Table data is no longer passed through translateStr, effectively disabling translations in getTableData.
With `trKey` now initialized to `item`, `trKey.isEmpty()` is almost never true, so `translateStr` is no longer called and translatable entries stay unlocalized. If you still intend to use the centralized translation mechanism, `trKey` should be assigned from `translateStr(item)` (or similar logic that calls `translateStr` before falling back to `item`).
</issue_to_address>
### Comment 3
<location> `deepin-devicemanager/src/GenerateDevice/HWGenerator.cpp:152-161` </location>
<code_context>
getBluetoothInfoFromLshw();
- getBluetoothInfoFromCatWifiInfo();
+
+ QList<QMap<QString, QString> > lstWifiInfo = readFileSysWifi();
+ if (lstWifiInfo.size() == 0) {
+ return;
+ }
+ QList<QMap<QString, QString> >::const_iterator it = lstWifiInfo.begin();
+ QMap<QString, QString> tempMap;
+ for (; it != lstWifiInfo.end(); ++it) {
+ if ((*it).size() < 3) {
+ continue;
+ }
+
+ foreach (const QString &key, (*it).keys()) {
+ tempMap.insert(key, (*it)[key]);
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** tempMap is reused across wifi-info iterations in generatorBluetoothDevice, which can unintentionally mix entries.
Because `tempMap` is declared outside the `for (; it != lstWifiInfo.end(); ++it)` loop and never cleared, each wifi entry writes into the same map. Keys from earlier entries can remain and pollute later ones, leading to incorrect data being associated with devices. To avoid this, either declare `QMap<QString, QString> tempMap;` inside the loop or call `tempMap.clear()` at the start of each iteration.
Suggested implementation:
```cpp
QList<QMap<QString, QString> > lstWifiInfo = readFileSysWifi();
if (lstWifiInfo.size() == 0) {
return;
}
QList<QMap<QString, QString> >::const_iterator it = lstWifiInfo.begin();
for (; it != lstWifiInfo.end(); ++it) {
if ((*it).size() < 3) {
continue;
}
QMap<QString, QString> tempMap;
foreach (const QString &key, (*it).keys()) {
tempMap.insert(key, (*it)[key]);
}
```
If `tempMap` is referenced outside the `for` loop (after its closing brace), its scope will need to be adjusted. In that case, ensure that whatever data structure receives `tempMap` per wifi entry (e.g. a list or device constructor) is updated *inside* the loop before `tempMap` goes out of scope, instead of relying on a single reused map after the loop.
</issue_to_address>
### Comment 4
<location> `deepin-devicemanager/src/DeviceManager/DeviceManager.cpp:601` </location>
<code_context>
return QString("");
}
+bool DeviceManager::tomlSetBytomlmatchkey(DeviceType deviceType, DeviceBaseInfo *device, const QString &tomltomlmatchkey, const QString &tomltomlconfigdemanding)
+{
+ Q_UNUSED(deviceType)
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new TOML matching and mode logic by splitting parsing/matching into small helpers and centralizing mode handling to simplify control flow and remove unused parameters.
You can reduce the new complexity without changing behavior by separating parsing/matching and isolating the mode handling. The current implementation mixes concerns and carries unused parameters.
### 1. Split `tomlSetBytomlmatchkey` into parser + matcher, and remove unused params
Right now `tomlSetBytomlmatchkey`:
- Takes `deviceType` and `tomltomlconfigdemanding` but never uses them.
- Parses the string and matches in the same function.
Extract the parsing logic and make `tomlSetBytomlmatchkey` a thin wrapper, or call the matcher directly from `tomlDeviceSet`:
```cpp
// keep as small helpers
static QMap<QString, QString> parseTomlMatchKey(const QString &tomlmatchkey)
{
QMap<QString, QString> itemMap;
const QStringList keyValues = tomlmatchkey.split(",");
for (const QString &keyValue : keyValues) {
const QStringList wordlst = keyValue.split("=");
if (wordlst.size() != 2)
continue;
QString key = wordlst[0].remove('"').trimmed();
QString valuetmp = wordlst[1];
QString value;
if (valuetmp.contains("#")) {
value = valuetmp.split("#").first().remove('"').trimmed();
} else {
value = valuetmp.remove('"').trimmed();
}
if (!key.isEmpty() && !value.isEmpty())
itemMap.insert(key, value);
}
return itemMap;
}
static bool matchesTomlMatchKey(DeviceBaseInfo *device,
const QMap<QString, QString> &itemMap)
{
if (itemMap.isEmpty())
return false;
for (auto it = itemMap.constBegin(); it != itemMap.constEnd(); ++it) {
const QString deviceVal = device->readDeviceInfoKeyValue(it.key());
if (deviceVal.compare(it.value(), Qt::CaseInsensitive) != 0)
return false;
}
return true;
}
```
Then simplify `tomlSetBytomlmatchkey` (or remove it entirely and use the helpers directly):
```cpp
bool DeviceManager::tomlSetBytomlmatchkey(DeviceBaseInfo *device,
const QString &tomltomlmatchkey)
{
const auto conditions = parseTomlMatchKey(tomltomlmatchkey);
return matchesTomlMatchKey(device, conditions);
}
```
And update the call site:
```cpp
bool isSameOne = false;
if (!tomltomlmatchkey.isEmpty())
isSameOne = tomlSetBytomlmatchkey(device, tomltomlmatchkey);
```
This removes unused parameters and makes both parsing and matching testable in isolation.
### 2. Make mode handling explicit and separate from matching
The nested logic inside `tomlDeviceSet` mixes:
- matching (`tomlSetBytomlmatchkey + findBy*`)
- mode behavior (`adjust/delete/add/default`)
- bookkeeping (`fixSameOne`)
You can push most of the mode-specific behavior into a small helper, so the main loop reads more linearly and the flags are used in fewer places:
```cpp
enum class TomlConfigMode { Default, Adjust, Delete, Add };
static TomlConfigMode toTomlConfigMode(const QString &modeStr)
{
if (modeStr == "adjust") return TomlConfigMode::Adjust;
if (modeStr == "delete") return TomlConfigMode::Delete;
if (modeStr == "add") return TomlConfigMode::Add;
return TomlConfigMode::Default;
}
bool DeviceManager::applyTomlRule(DeviceType deviceType,
DeviceBaseInfo *device,
const QMap<QString, QString> &tomlMap,
TomlConfigMode mode,
bool &fixSameOne)
{
// assumes caller has already determined "isSameOne"
switch (mode) {
case TomlConfigMode::Adjust:
fixSameOne = true;
tomlDeviceMapSet(deviceType, device, tomlMap);
return true;
case TomlConfigMode::Delete:
fixSameOne = true;
tomlDeviceDel(deviceType, device);
delete device;
return true;
case TomlConfigMode::Default:
fixSameOne = true;
if (TOML_Del == tomlDeviceMapSet(deviceType, device, tomlMap)) {
tomlDeviceDel(deviceType, device);
delete device;
}
return true;
case TomlConfigMode::Add:
// handled after loop; nothing to do here per device
return false;
}
return false;
}
```
Then inside `tomlDeviceSet`, the per-device loop becomes easier to follow:
```cpp
TomlConfigMode mode = toTomlConfigMode(tomltomlconfigdemanding);
auto matchConditions = parseTomlMatchKey(tomltomlmatchkey);
for (int i = 0; i < lst.size(); ++i) {
DeviceBaseInfo *device = lst[i];
bool isSameOne = matchesTomlMatchKey(device, matchConditions);
if (!isSameOne && mode == TomlConfigMode::Default) {
// fallback matching only in default mode
if (!isSameOne) isSameOne = findByModalias(deviceType, device, modalias);
if (!isSameOne) isSameOne = findByVIDPID(deviceType, device, vid, pid);
if (!isSameOne) isSameOne = findByVendorName(deviceType, device, vendor, name);
}
if (!isSameOne || fixSameOne)
continue;
applyTomlRule(deviceType, device, tomlMapLst[j], mode, fixSameOne);
}
```
This:
- Removes string-driven branching from the middle of the loop.
- Avoids reusing `fixSameOne` with subtly different meanings.
- Cleanly separates the “how do we match?” (`matchesTomlMatchKey` + `findBy*`) from “what do we do once matched?” (`applyTomlRule`).
These changes keep all existing behavior but make the flow much more readable and safer to extend.
</issue_to_address>
### Comment 5
<location> `deepin-devicemanager/src/GenerateDevice/DeviceGenerator.cpp:297` </location>
<code_context>
void DeviceGenerator::generatorNetworkDevice()
{
+ bool hasWlan =false;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying `generatorNetworkDevice` by removing the unused inner loop over `lstDevice` and extracting MAC/logical-name validation into helper functions to make the control flow clearer.
You can simplify `generatorNetworkDevice` quite a bit without changing behavior.
### 1. lshw loop: remove unused inner iteration over `lstDevice`
Right now you iterate over `lstDevice` but never use `*itDevice` inside the body; you only append new devices. On the first iteration `lstDevice` is empty, so the device‑creation code never runs.
You can keep all existing filters and the `hasWlan` behavior while dropping the inner loop entirely:
```cpp
void DeviceGenerator::generatorNetworkDevice()
{
bool hasWlan = false;
QList<DeviceNetwork *> lstDevice;
const QList<QMap<QString, QString>> &lstLshw =
DeviceManager::instance()->cmdInfo("lshw_network");
for (auto it = lstLshw.begin(); it != lstLshw.end(); ++it) {
if (it->find("serial") == it->end() || it->find("logical name") == it->end())
continue;
const QString &logicalNameLshw = (*it)["logical name"];
const QString &macAddressLshw = (*it)["serial"];
if (logicalNameLshw.contains("p2p", Qt::CaseInsensitive))
continue;
if (logicalNameLshw.contains("wlan", Qt::CaseInsensitive) && hasWlan)
continue;
if (macAddressLshw.contains("00:00:00:00:00:00", Qt::CaseInsensitive) ||
macAddressLshw.contains("ff:ff:ff:ff:ff:ff", Qt::CaseInsensitive) ||
macAddressLshw.isEmpty())
continue;
QRegularExpression macRegex("([0-9A-Fa-f]{2}[:-]){5}([0-9A-Fa-f]{2})");
if (!macRegex.match(macAddressLshw).hasMatch())
continue;
QDir dir("/sys/class/net/" + logicalNameLshw);
if (!dir.exists())
continue;
auto *device = new DeviceNetwork();
device->setInfoFromLshw(*it);
lstDevice.append(device);
if (logicalNameLshw.contains("wlan", Qt::CaseInsensitive))
hasWlan = true;
}
// ... existing hwinfo code ...
}
```
This keeps the same filtering behavior and use of `hasWlan`, but makes the control flow obvious: “for each lshw entry that passes filters, maybe create a device”.
### 2. Extract MAC/name filtering into helpers
The inline filters and regex make the main function hard to scan. You can pull them into local statics to reduce nesting and improve readability, without touching semantics:
```cpp
static bool isValidMac(const QString &mac)
{
if (mac.isEmpty())
return false;
if (mac.contains("00:00:00:00:00:00", Qt::CaseInsensitive) ||
mac.contains("ff:ff:ff:ff:ff:ff", Qt::CaseInsensitive))
return false;
static const QRegularExpression macRegex(
QStringLiteral("([0-9A-Fa-f]{2}[:-]){5}([0-9A-Fa-f]{2})"));
return macRegex.match(mac).hasMatch();
}
static bool isInterestingLogicalName(const QString &name, bool hasWlan)
{
if (name.contains("p2p", Qt::CaseInsensitive))
return false;
if (name.contains("wlan", Qt::CaseInsensitive) && hasWlan)
return false;
return true;
}
```
Then the lshw loop simplifies further:
```cpp
if (!isInterestingLogicalName(logicalNameLshw, hasWlan))
continue;
if (!isValidMac(macAddressLshw))
continue;
```
This keeps behavior identical but makes `generatorNetworkDevice` shorter and easier to reason about, and the validation logic becomes independently testable.
</issue_to_address>
### Comment 6
<location> `deepin-devicemanager/src/GenerateDevice/HWGenerator.cpp:118` </location>
<code_context>
qCDebug(appLog) << "HWGenerator::generatorAudioDevice end";
}
+QList<QMap<QString, QString> > readFileSysWifi()
+{
+ QList<QMap<QString, QString> > lstWifiInfo;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new wifi-based Bluetooth/Network handling into small helpers and avoiding shared mutable maps and early returns to simplify control flow and make behavior clearer.
The `readFileSysWifi` extraction is good, but the new Bluetooth/Network paths now have subtle state and duplication that are easy to simplify without changing behavior.
### 1. `generatorBluetoothDevice`: shared `tempMap` across iterations
`tempMap` is declared outside the loop and reused:
```cpp
QList<QMap<QString, QString> >::const_iterator it = lstWifiInfo.begin();
QMap<QString, QString> tempMap;
for (; it != lstWifiInfo.end(); ++it) {
...
foreach (const QString &key, (*it).keys()) {
tempMap.insert(key, (*it)[key]);
}
...
tempMap["Vendor"] = Common::specialHString();
tempMap["Type"] = "Bluetooth Device";
tempMap["Name"] = tempMap["Chip Type"];
}
...
for (int i = 0; i < lst.size(); i++) {
...
if (bus.contains("UART",Qt::CaseInsensitive)) {
DeviceManager::instance()->tomlDeviceMapSet(DT_Bluetoorh, device,tempMap);
}
}
```
This makes `tempMap` accumulate keys across all wifi entries and only the final state is applied to all UART Bluetooth devices. To make the intent explicit and avoid state leaks, build a single normalized map from the wifi info outside the loop, or rebuild a fresh map per entry and decide which one to use.
If only one wifi entry is expected, a simpler (and more obviously correct) version is:
```cpp
static QMap<QString, QString> buildBluetoothWifiMap(const QMap<QString, QString> &src)
{
QMap<QString, QString> m = src;
if (m["Chip Type"].contains(Common::specialHString(), Qt::CaseInsensitive)) {
m["Chip Type"] = m["Chip Type"].remove(Common::specialHString()).trimmed();
}
m["Vendor"] = Common::specialHString();
m["Type"] = "Bluetooth Device";
m["Name"] = m["Chip Type"];
return m;
}
void HWGenerator::generatorBluetoothDevice()
{
qCDebug(appLog) << "HWGenerator::generatorBluetoothDevice start";
getBluetoothInfoFromHciconfig();
getBlueToothInfoFromHwinfo();
getBluetoothInfoFromLshw();
const auto lstWifiInfo = readFileSysWifi();
QMap<QString, QString> wifiBtMap;
if (!lstWifiInfo.isEmpty() && lstWifiInfo.first().size() >= 3) {
wifiBtMap = buildBluetoothWifiMap(lstWifiInfo.first());
}
QList<DeviceBaseInfo *> lst = DeviceManager::instance()->convertDeviceList(DT_Bluetoorh);
for (int i = 0; i < lst.size(); ++i) {
DeviceBaseInfo *device = lst[i];
const QString bus = DeviceManager::instance()->tomlDeviceReadKeyValue(DT_Bluetoorh, device, "Bus");
if (bus.contains("UART", Qt::CaseInsensitive) && !wifiBtMap.isEmpty()) {
DeviceManager::instance()->tomlDeviceMapSet(DT_Bluetoorh, device, wifiBtMap);
}
}
qCDebug(appLog) << "HWGenerator::generatorBluetoothDevice end";
}
```
Key points:
- No reuse of a mutable `tempMap` across wifi entries.
- All wifi-based string massaging for Bluetooth lives in `buildBluetoothWifiMap`.
If you must support multiple wifi entries later, you can extend this to pick a specific one (first, last, or matching some criterion) without changing the call site pattern.
### 2. `generatorNetworkDevice`: duplicate wifi normalization
`generatorNetworkDevice` manually does almost the same normalization as the old `getNetworkInfoFromCatWifiInfo`:
```cpp
QList<QMap<QString, QString> > lstWifiInfo = readFileSysWifi();
if (lstWifiInfo.size() == 0) {
return;
}
QList<QMap<QString, QString> >::const_iterator it = lstWifiInfo.begin();
for (; it != lstWifiInfo.end(); ++it) {
if ((*it).size() < 3) {
continue;
}
QMap<QString, QString> tempMap;
foreach (const QString &key, (*it).keys()) {
tempMap.insert(key, (*it)[key]);
}
tempMap["Name"] = tempMap["Chip Type"];
if (tempMap["Chip Type"].contains(Common::specialHString(), Qt::CaseInsensitive)) {
tempMap["Name"] = tempMap["Chip Type"].remove(Common::specialHString()).trimmed();
}
if (tempMap["NIC Type"].contains("WLAN", Qt::CaseInsensitive)) {
}
tempMap["Vendor"] = "HISILICON";
tempMap["Type"] = "Wireless network";
QList<DeviceBaseInfo *> lst = DeviceManager::instance()->convertDeviceList(DT_Network);
...
}
```
This logic is almost identical to Bluetooth’s and to the removed `getNetworkInfoFromCatWifiInfo`. You can keep behavior identical while consolidating the normalization and avoiding repeated list conversion inside the loop:
```cpp
static QMap<QString, QString> buildNetworkWifiMap(const QMap<QString, QString> &src)
{
QMap<QString, QString> m = src;
m["Name"] = m["Chip Type"];
if (m["Chip Type"].contains(Common::specialHString(), Qt::CaseInsensitive)) {
m["Name"] = m["Chip Type"].remove(Common::specialHString()).trimmed();
}
if (m["NIC Type"].contains("WLAN", Qt::CaseInsensitive)) {
// keep your existing handling if needed
}
m["Vendor"] = "HISILICON";
m["Type"] = "Wireless network";
return m;
}
void HWGenerator::generatorNetworkDevice()
{
qCDebug(appLog) << "HWGenerator::generatorNetworkDevice start";
DeviceGenerator::generatorNetworkDevice();
const auto lstWifiInfo = readFileSysWifi();
if (lstWifiInfo.isEmpty()) {
qCDebug(appLog) << "HWGenerator::generatorNetworkDevice no wifi info";
qCDebug(appLog) << "HWGenerator::generatorNetworkDevice end";
return; // or skip wifi augmentation only, see next section
}
QList<DeviceBaseInfo *> lst = DeviceManager::instance()->convertDeviceList(DT_Network);
for (const auto &wifiInfo : lstWifiInfo) {
if (wifiInfo.size() < 3)
continue;
const QMap<QString, QString> tempMap = buildNetworkWifiMap(wifiInfo);
for (int i = 0; i < lst.size(); ++i) {
DeviceBaseInfo *device = lst[i];
const QString logicalName =
DeviceManager::instance()->tomlDeviceReadKeyValue(DT_Network, device, "Logical Name");
if (logicalName.contains("wlan", Qt::CaseInsensitive)) {
DeviceManager::instance()->tomlDeviceMapSet(DT_Network, device, tempMap);
}
}
}
qCDebug(appLog) << "HWGenerator::generatorNetworkDevice end";
}
```
This keeps all wifi-derived massaging in one helper and removes the repeated `convertDeviceList` call inside the loop.
### 3. Early return vs guarding wifi-specific logic
In both generators you now `return` early if `lstWifiInfo.size() == 0`, which means any future non‑wifi logic added after that will silently not execute.
If you want to keep that behavior for now but make it safer for future changes, you can narrow the scope of the early exit to the wifi augmentation only. For example, in `generatorNetworkDevice`:
```cpp
void HWGenerator::generatorNetworkDevice()
{
qCDebug(appLog) << "HWGenerator::generatorNetworkDevice start";
DeviceGenerator::generatorNetworkDevice();
const auto lstWifiInfo = readFileSysWifi();
if (!lstWifiInfo.isEmpty()) {
// wifi-augmentation block using buildNetworkWifiMap(...)
...
}
qCDebug(appLog) << "HWGenerator::generatorNetworkDevice end";
}
```
Same pattern can be applied to `generatorBluetoothDevice`: only skip the wifi-derived fields when wifi info is missing, but still run other generator logic.
---
These small helpers and scoping changes:
- Remove the subtle mutable state in `generatorBluetoothDevice`.
- Deduplicate wifi normalization between Bluetooth and Network (and the old helpers).
- Make it harder for future modifications to accidentally change behavior when `/sys/hisys/wal/wifi_devices_info` is missing.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| loadTableData(); | ||
| m_TableDataTr.clear(); | ||
| for (const auto &item : m_TableData) { | ||
| QString trKey = translateStr(item); |
There was a problem hiding this comment.
issue (bug_risk): Table data is no longer passed through translateStr, effectively disabling translations in getTableData.
With trKey now initialized to item, trKey.isEmpty() is almost never true, so translateStr is no longer called and translatable entries stay unlocalized. If you still intend to use the centralized translation mechanism, trKey should be assigned from translateStr(item) (or similar logic that calls translateStr before falling back to item).
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 |
Contributor
|
This pr cannot be merged! (status: unstable) |
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.
extend toml and uniform some model arm code
Log: extend toml
Bug: https://pms.uniontech.com/task-view-368603.html
Summary by Sourcery
Extend and generalize TOML-based hardware configuration handling while unifying several ARM / vendor-specific code paths and improving device parsing and presentation.
New Features:
Bug Fixes:
Enhancements:
Documentation: