Perf: Improve program performance

[GongHeng2017]

-- Function parameter should be passed by const reference.
-- Local varible item shadows outer variable
-- Class has a constructor with 1 argument that is not explicit.

Summary by Sourcery

Improve application performance and code maintainability across device management components by refactoring common routines, optimizing data handling, and consolidating translation logic.

Bug Fixes:

• Fix local variable shadowing issues and enforce explicit single‐argument constructors to avoid unintended conversions
• Corrected numeric parsing in storage and monitor components to handle new size formats and edge cases
• Adjusted DM DeviceMonitor parsing to support custom xrandr formats and maintain correct current resolution display

Enhancements:

• Convert numerous QString parameters and return values to use const references to reduce copying overhead
• Rework DeviceBaseInfo to cache and batch translate attribute keys, headers, and table data for faster lookups
• Deduplicate audio devices using a hash‐based priority scheme and retry logic to stabilize enable operations
• Implement custom sorting for storage devices that prioritizes UFS, then SSD/HDD, and finally USB media
• Modularize hardware generators by extracting shared parsing functions (EDID, WiFi, GPU commands) and handling vendor‐specific quirks
• Improve DeviceInput by mapping bus IDs to interface types via /proc/bus/input and parsing input device descriptors
• Introduce a translation string generator to ensure lupdate captures all user‐facing text keys

[deepin-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: GongHeng2017

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• debian/deepin/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sourcery-ai]

Reviewer's Guide

This PR refactors translation and attribute handling across device classes, consolidates and caches translated strings, optimizes parameter passing, unifies EDID parsing logic, enhances device list deduplication and sorting, and streamlines TOML configuration mapping in the DeviceManager.

Class diagram for DeviceBaseInfo and translation/attribute handling changes

classDiagram
    class DeviceBaseInfo {
        +translateStr(const QString &inStr) : const QString
        +nameTr() : const QString
        +getOtherTranslationAttribs() : const QList<QPair<QString, QString>> &
        +getBaseTranslationAttribs() : const QList<QPair<QString, QString>> &
        +readDeviceInfoKeyValue(const QString &key) : const QString
        +setDeviceInfoKeyValue(const QString &key, const QString &value) : bool
        +generatorTranslate()
        +setSysPath(const QString &newSysPath)
        +setUniqueID(const QString &UniqueID)
        -m_LstOtherInfoTr : QList<QPair<QString, QString>>
        -m_LstBaseInfoTr : QList<QPair<QString, QString>>
        -m_TableHeaderTr : QStringList
        -m_TableDataTr : QStringList
    }

Loading

Class diagram for DeviceStorage: media type, interface, and attribute changes

classDiagram
    class DeviceStorage {
        +mediaType() : const QString &
        +interface() : const QString &
        +setMediaType(const QString &name, const QString &value) : bool
        +checkDiskSize()
        +getOverviewInfo() : const QString
        -m_MediaType : QString
        -m_Interface : QString
        -m_Size : QString
        -m_SizeBytes : quint64
    }

Loading

Class diagram for DeviceCpu: translation and attribute changes

classDiagram
    class DeviceCpu {
        +loadBaseDeviceInfo()
        +loadOtherDeviceInfo()
        +loadTableHeader()
        +setInfoFromDmidecode(const QMap<QString, QString> &mapInfo)
        +setInfoFromTomlOneByOne(const QMap<QString, QString> &mapInfo)
        +getTrNumber()
        -m_trNumber : QMap<int, QString>
    }

Loading

Class diagram for DeviceMonitor: translation, EDID, and attribute changes

classDiagram
    class DeviceMonitor {
        +setInfoFromEdid(const QMap<QString, QString> &mapInfo)
        +setInfoFromEdidForCustom(const QMap<QString, QString> &mapInfo)
        +setInfoFromHwinfo(const QMap<QString, QString> &mapInfo)
        +setInfoFromXradr(const QString &main, const QString &edid, const QString &rate, const QString &xrandr) : bool
        +getMonitorResolutionMap(QString rawText, QString key, bool round) : QMap<QString, QStringList>
        +loadBaseDeviceInfo()
        +loadOtherDeviceInfo()
        -m_RefreshRate : QString
        -m_RawInterface : QString
    }

Loading

Class diagram for DeviceManager: TOML mapping and device handling changes

classDiagram
    class DeviceManager {
        +tomlDeviceMapSet(DeviceType deviceType, DeviceBaseInfo *device, const QMap<QString, QString> &mapInfo) : TomlFixMethod
        +tomlDeviceReadKeyValue(DeviceType deviceType, DeviceBaseInfo *device, const QString &key) : QString
        +tomlSetBytomlmatchkey(DeviceType deviceType, DeviceBaseInfo *device, const QString &tomltomlmatchkey, const QString &tomltomlconfigdemanding) : bool
        +orderDiskByType()
        +setAudioDeviceEnable(DeviceAudio * const devivce, bool enable)
    }

Loading

Class diagram for HWGenerator: refactored device generator methods

classDiagram
    class HWGenerator {
        +generatorMonitorDevice() : void
        +generatorPowerDevice() : void
        +generatorKeyboardDevice() : void
        +generatorMemoryDevice() : void
        +generatorCameraDevice() : void
        +getAudioInfoFromCatAudio() : void
        +getBluetoothInfoFromHciconfig() : void
        +getBluetoothInfoFromHwinfo() : void
        +getBluetoothInfoFromLshw() : void
        +getMemoryInfoFromLshw() : void
    }

Loading

File-Level Changes

Change	Details	Files
Centralized and cached translation lookups	Introduced translateStr(), nameTr(), and translation-caching methods for base/other attributes and table headers/data Replaced inline tr()/QObject::tr() calls with direct strings and deferred translation via translateStr Added generatorTranslate() placeholder to drive lupdate coverage	DeviceInfo.cpp
Refactored DeviceManager TOML mapping	Renamed tomlDeviceSet to tomlDeviceMapSet and added tomlDeviceReadKeyValue Implemented tomlSetBytomlmatchkey() to match and apply configs by composite keys Streamlined add, delete, and delete-by-match flows in tomlDeviceSet	DeviceManager.cpp
Improved audio device deduplication and enable logic	Replaced nested removals with hash-based dedup, preserving highest-priority instances Added setAudioDeviceEnable() to retry enable operations up to three times	DeviceManager.cpp DeviceAudio.cpp
Custom sorting and sizing for storage devices	Added orderDiskByType() to sort storage by UFS, mediaType, then USB Optimized size display for special board types and removed fragile regex handling	DeviceStorage.cpp DeviceManager.cpp
Unified EDID parsing and monitor handling	Moved EDID hexdump logic into CommonTools::parseEDID Enhanced DeviceMonitor to parse resolutions via getMonitorResolutionMap() Handled special board types for xrandr parsing and display size formatting	commonfunction.cpp commontools.cpp DeviceMonitor.cpp HWGenerator.cpp
Enhanced input device bus parsing	Added getDetailBusInfo() mapping Bus IDs to interface descriptions Parsed /proc/bus/input/devices in getMouseInfoFromBusDevice() to set interface	DeviceInput.cpp

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey @GongHeng2017 - I've reviewed your changes - here's some feedback:

• Avoid spawning QProcess in tight loops (e.g. parsing EDID or bus data); instead read and parse sysfs files or batch external calls to reduce blocking I/O and improve responsiveness.
• Calling tr() on dynamic QStrings (e.g. translating table data at runtime) won’t be picked up by the lupdate tool—use explicit translation keys or static tr() calls for all user-facing strings.
• The deduplication logic in deleteDisableDuplicate_AudioDevice is overly complex—consider grouping devices by unique ID and selecting the preferred candidate in a single pass for better clarity and performance.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- Avoid spawning QProcess in tight loops (e.g. parsing EDID or bus data); instead read and parse sysfs files or batch external calls to reduce blocking I/O and improve responsiveness.
- Calling tr() on dynamic QStrings (e.g. translating table data at runtime) won’t be picked up by the lupdate tool—use explicit translation keys or static tr() calls for all user-facing strings.
- The deduplication logic in deleteDisableDuplicate_AudioDevice is overly complex—consider grouping devices by unique ID and selecting the preferred candidate in a single pass for better clarity and performance.

## Individual Comments

### Comment 1
<location> `deepin-devicemanager/src/DeviceManager/DeviceInfo.cpp:758` </location>
<code_context>
+    return QString("");
+}
+
+bool DeviceBaseInfo::setDeviceInfoKeyValue(const QString &key, const QString &value)
+{
+    if (key.isEmpty() ||value.isEmpty())
+        return false;
+
+    for (QPair<QString, QString> pair : getBaseAttribs()) {
+        if (key == pair.first) {
+            pair.second = value;
+            return true;
+        }
+    }
+
+    for (QPair<QString, QString> pair : getOtherAttribs()) {
+        if (key == pair.first) {
+            pair.second = value;
</code_context>

<issue_to_address>
Modifying copies of QPair does not update the original list.

Assignments to pair.second only modify the local copy, not the original list. Iterate by reference or use indices to update the stored pairs directly.
</issue_to_address>

### Comment 2
<location> `deepin-devicemanager/src/DeviceManager/DeviceMonitor.cpp:167` </location>
<code_context>
 }

-void DeviceMonitor::setInfoFromEdid(const QMap<QString, QString> &mapInfo)
+void DeviceMonitor::setInfoFromEdidForCustom(const QMap<QString, QString> &mapInfo)
 {
-    m_Name = "Monitor " + mapInfo["Vendor"];
     setAttribute(mapInfo, "Size", m_ScreenSize);
     setAttribute(mapInfo, "Vendor", m_Vendor);
     setAttribute(mapInfo, "Date", m_ProductionWeek);
     setAttribute(mapInfo, "Display Input", m_DisplayInput);
     setAttribute(mapInfo, "Model", m_Model);
+    m_Name = m_Model;
     getOtherMapInfo(mapInfo);
 }
</code_context>

<issue_to_address>
Potential confusion between setInfoFromEdid and setInfoFromEdidForCustom.

If both methods are necessary, please add documentation clarifying when each should be used.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
void DeviceMonitor::setInfoFromEdidForCustom(const QMap<QString, QString> &mapInfo)
{
    setAttribute(mapInfo, "Size", m_ScreenSize);
    setAttribute(mapInfo, "Vendor", m_Vendor);
    setAttribute(mapInfo, "Date", m_ProductionWeek);
    setAttribute(mapInfo, "Display Input", m_DisplayInput);
    setAttribute(mapInfo, "Model", m_Model);
    m_Name = m_Model;
    getOtherMapInfo(mapInfo);
}
=======
/**
 * @brief Sets device information from EDID data.
 *
 * This method should be used for standard EDID parsing and populates device attributes
 * such as name, size, vendor, production week, display input, and model.
 * Use this method when you want to set device info based on the default EDID data.
 */
void DeviceMonitor::setInfoFromEdid(const QMap<QString, QString> &mapInfo)
{
    m_Name = "Monitor " + mapInfo["Vendor"];
    setAttribute(mapInfo, "Size", m_ScreenSize);
    setAttribute(mapInfo, "Vendor", m_Vendor);
    setAttribute(mapInfo, "Date", m_ProductionWeek);
    setAttribute(mapInfo, "Display Input", m_DisplayInput);
    setAttribute(mapInfo, "Model", m_Model);
    getOtherMapInfo(mapInfo);
}

/**
 * @brief Sets device information from custom EDID data.
 *
 * This method should be used when custom EDID parsing is required, for example,
 * when the model name should be used as the device name instead of the vendor.
 * Use this method for custom or non-standard EDID data sources.
 */
void DeviceMonitor::setInfoFromEdidForCustom(const QMap<QString, QString> &mapInfo)
{
    setAttribute(mapInfo, "Size", m_ScreenSize);
    setAttribute(mapInfo, "Vendor", m_Vendor);
    setAttribute(mapInfo, "Date", m_ProductionWeek);
    setAttribute(mapInfo, "Display Input", m_DisplayInput);
    setAttribute(mapInfo, "Model", m_Model);
    m_Name = m_Model;
    getOtherMapInfo(mapInfo);
}
>>>>>>> REPLACE

</suggested_fix>

### Comment 3
<location> `deepin-devicemanager/src/DeviceManager/DeviceGpu.cpp:323` </location>
<code_context>
-    addOtherDeviceInfo(tr("Memory Address"), m_MemAddress);
-    addOtherDeviceInfo(tr("IO Port"), m_IOPort);
-    addOtherDeviceInfo(tr("Bus Info"), m_BusInfo);
-    if (type != "KLVV" && type != "KLVU" && type != "PGUV" && type != "PGUW") {
-        addOtherDeviceInfo(tr("Maximum Resolution"), m_MaximumResolution);
-        addOtherDeviceInfo(tr("Minimum Resolution"), m_MinimumResolution);
+    addOtherDeviceInfo(("Module Alias"), m_Modalias);
+    addOtherDeviceInfo(("Physical ID"), m_PhysID);
+    addOtherDeviceInfo(("Memory Address"), m_MemAddress);
+    addOtherDeviceInfo(("IO Port"), m_IOPort);
+    addOtherDeviceInfo(("Bus Info"), m_BusInfo);
+    if (type != Common::specialHString()) {
+        addOtherDeviceInfo(("Maximum Resolution"), m_MaximumResolution);
+        addOtherDeviceInfo(("Minimum Resolution"), m_MinimumResolution);
</code_context>

<issue_to_address>
The check for special board types has been changed to a single function call.

Please verify that Common::specialHString() correctly identifies all relevant board types, as this change consolidates multiple checks into one and may affect logic if not equivalent.
</issue_to_address>

### Comment 4
<location> `deepin-devicemanager/src/DeviceManager/DeviceInput.cpp:354` </location>
<code_context>
+        }
+    }
+
+    if (nameToBusMap.contains(m_Name)) {
+        QString busID = nameToBusMap.value(m_Name);
+        m_Interface = getDetailBusInfo(busID).interfaceType;
+    }
+}
</code_context>

<issue_to_address>
Device name is used as the key for bus mapping, which may not be unique.

Using device names as keys can cause mapping errors if names are duplicated. Use a unique identifier for each device if possible.
</issue_to_address>

### Comment 5
<location> `deepin-devicemanager/src/DeviceManager/DeviceInput.cpp:413` </location>
<code_context>
     }
     QString info = file.readAll();

-    if (m_Name.contains("PS/2")) {
+    if (m_Name.contains("PS/2") || m_Interface.contains("PS/2")) {
 //        QStringList lines = info.split("\n");
 //        for (QString line : lines) {
</code_context>

<issue_to_address>
Wakeup logic now checks both m_Name and m_Interface for 'PS/2'.

Please confirm that expanding the check to both fields won't cause unintended matches.
</issue_to_address>

### Comment 6
<location> `deepin-devicemanager/src/GenerateDevice/DeviceGenerator.cpp:252` </location>
<code_context>
     getMonitorInfoFromHwinfo();
 }

+bool isValidLogicalName(const QString& logicalName)
+{
+    if (logicalName.contains("p2p", Qt::CaseInsensitive) || logicalName.isEmpty())
+        return false;
+
+    QString addressFilePath = "/sys/class/net/" + logicalName;
</code_context>

<issue_to_address>
isValidLogicalName uses QDir to check for existence in /sys/class/net.

QDir may not reliably detect network interfaces if the logical name isn't a directory. Consider QFileInfo or a more direct existence check.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
    QString addressFilePath = "/sys/class/net/" + logicalName;
    QDir dir(addressFilePath);
    if (dir.exists())
        return true;

    qCInfo(appLog) << dir << "not exist in /sys/class/net/";
    return false;
=======
    QString addressFilePath = "/sys/class/net/" + logicalName;
    QFileInfo info(addressFilePath);
    if (info.exists())
        return true;

    qCInfo(appLog) << addressFilePath << "not exist in /sys/class/net/";
    return false;
>>>>>>> REPLACE

</suggested_fix>

### Comment 7
<location> `deepin-devicemanager/src/GenerateDevice/DeviceGenerator.cpp:271` </location>
<code_context>
+    if (macAddress.contains("00:00:00:00:00:00", Qt::CaseInsensitive) || macAddress.contains("ff:ff:ff:ff:ff:ff", Qt::CaseInsensitive) || macAddress.isEmpty())
+        return false;
+
+    QRegularExpression macRegex("([0-9A-Fa-f]{2}[:-]){5}([0-9A-Fa-f]{2})");
+    if (!macRegex.match(macAddress).hasMatch())
+        return false;
</code_context>

<issue_to_address>
MAC address validation regex may match substrings.

Add ^ and $ to the regex to ensure it matches only complete MAC addresses, not substrings within longer strings.
</issue_to_address>

### Comment 8
<location> `deepin-devicemanager/src/GenerateDevice/DeviceFactory.cpp:43` </location>
<code_context>
             else if (type == "KLVU")
-                generator = new KLUGenerator();
+                generator = new HWGenerator();
             else if (type == "PGUX")
-                generator = new PanguXGenerator();
+                generator = new HWGenerator();
+            else if (type == "CustomType")
</code_context>

<issue_to_address>
Removed PanguXGenerator in favor of HWGenerator for 'PGUX' type.

Please verify that HWGenerator fully replicates any specialized behavior previously handled by PanguXGenerator for 'PGUX' devices.
</issue_to_address>

### Comment 9
<location> `deepin-devicemanager/src/Tool/EDIDParser.cpp:214` </location>
<code_context>
+        }
+    }
+
+    if (Common::specialComType == 7){ // sepcial task:378963
+        m_Width = 296;
+        m_Height = 197;
+    }
     double inch = sqrt((m_Width / 2.54) * (m_Width / 2.54) + (m_Height / 2.54) * (m_Height / 2.54))/10;
</code_context>

<issue_to_address>
Hardcoded screen size for specialComType == 7.

Refactor this hardcoded value or add a clear comment to explain its purpose and scope.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
    if (Common::specialComType == 7){ // sepcial task:378963
        m_Width = 296;
        m_Height = 197;
    }
=======
    // Special handling for device type 7 (see task:378963)
    // These values represent the physical width and height in millimeters for the specific device.
    constexpr int SPECIAL_TYPE_7_WIDTH_MM = 296;
    constexpr int SPECIAL_TYPE_7_HEIGHT_MM = 197;
    if (Common::specialComType == 7) {
        m_Width = SPECIAL_TYPE_7_WIDTH_MM;
        m_Height = SPECIAL_TYPE_7_HEIGHT_MM;
    }
>>>>>>> REPLACE

</suggested_fix>

### Comment 10
<location> `deepin-devicemanager/src/Tool/EDIDParser.cpp:278` </location>
<code_context>
+    }
+
+    // 如果没有找到有效的监视器名称，设置默认值
+    if (m_MonitorName.isEmpty())
+        m_MonitorName = "Unknown Monitor";
 }

</code_context>

<issue_to_address>
Default monitor name is set to 'Unknown Monitor' if not found.

If the UI supports multiple languages, localize this string or allow it to be configured.

Suggested implementation:

```cpp
    // 如果没有找到有效的监视器名称，设置默认值（支持本地化）
    if (m_MonitorName.isEmpty())
        m_MonitorName = QObject::tr("Unknown Monitor");

```

If `EDIDParser` is not a subclass of `QObject`, you may need to pass a `QObject*` context or use a global translation context. Alternatively, you could provide a configuration mechanism for the default monitor name if required by your application's architecture.
</issue_to_address>

### Comment 11
<location> `deepin-devicemanager-server/deepin-devicecontrol/src/controlinterface.cpp:143` </location>
<code_context>

     // 先从数据库中查找路径，防止设备更换usb接口
     QString sPath = EnableSqlManager::getInstance()->authorizedPath(value);
-    if (sPath.isEmpty()) {
-        sPath = path;
-    }
+    sPath = path;

</code_context>

<issue_to_address>
Always overwriting sPath with path may break fallback logic.

This change removes the fallback to the database value, which may cause issues if the fallback is required for some hardware.
</issue_to_address>

### Comment 12
<location> `deepin-devicemanager/src/GenerateDevice/HWGenerator.cpp:102` </location>
<code_context>
     getAudioInfoFromCatAudio();
 }

+QList<QMap<QString, QString> > readFileSysWifi()
+{
+    QList<QMap<QString, QString> >  lstWifiInfo;
</code_context>

<issue_to_address>
Consider extracting the repeated logic for reading and normalizing wifi device info into shared helper functions, and only handling device-specific adjustments in each generator method.

Consider factoring out the “read & normalize `/sys/hisys/wal/wifi_devices_info`” logic into two very small helpers and then only doing the per-device adjustments in each generator. That collapses ~20 duplicated lines in each of generatorBluetoothDevice and generatorNetworkDevice into a few calls:

```cpp
// commonfunction.h
namespace Common {
  // 1) read & split the file into a list of key→value maps
  static QList<QMap<QString,QString>> parseSysWifiInfo() {
    QList<QMap<QString,QString>> out;
    QFile f("/sys/hisys/wal/wifi_devices_info");
    if (!f.open(QIODevice::ReadOnly)) return out;
    auto lines = QString(f.readAll()).split('\n');
    f.close();
    QMap<QString,QString> m;
    for (auto &ln : lines) {
      if (ln.isEmpty()) continue;
      auto parts = ln.split(':', QString::SkipEmptyParts);
      if (parts.size() == 2)
        m[parts[0].trimmed()] = parts[1].trimmed();
    }
    if (!m.isEmpty()) out.append(m);
    return out;
  }

  // 2) common cleanup of "Chip Type"
  static QMap<QString,QString> normalizeWifiEntry(QMap<QString,QString> raw) {
    raw["Chip Type"] =
      raw["Chip Type"]
        .remove(Common::specialHString(), Qt::CaseInsensitive)
        .trimmed();
    return raw;
  }
}
```

Then in HWGenerator:

```cpp
void HWGenerator::generatorBluetoothDevice() {
  // reuse existing steps...
  auto list = Common::parseSysWifiInfo();
  for (auto raw : list) {
    if (raw.size() < 3) continue;
    auto m = Common::normalizeWifiEntry(raw);
    // per-BT tweaks:
    m["Vendor"] = Common::specialHString();
    m["Type"]   = "Bluetooth Device";
    m["Name"]   = m["Chip Type"];
    // apply only to UART-bus devices:
    for (auto *dev : DeviceManager::instance()->convertDeviceList(DT_Bluetoorh)) {
      if (DeviceManager::instance()
            ->tomlDeviceReadKeyValue(DT_Bluetoorh, dev, "Bus")
            .contains("UART", Qt::CaseInsensitive))
      {
        DeviceManager::instance()
          ->tomlDeviceMapSet(DT_Bluetoorh, dev, m);
      }
    }
  }
}
```

```cpp
void HWGenerator::generatorNetworkDevice() {
  DeviceGenerator::generatorNetworkDevice();
  auto list = Common::parseSysWifiInfo();
  for (auto raw : list) {
    if (raw.size() < 3) continue;
    auto m = Common::normalizeWifiEntry(raw);
    // per-NW tweaks:
    m["Vendor"] = "HISILICON";
    m["Type"]   = "Wireless network";
    m["Name"]   = m["Chip Type"];
    for (auto *dev : DeviceManager::instance()->convertDeviceList(DT_Network)) {
      if (DeviceManager::instance()
            ->tomlDeviceReadKeyValue(DT_Network, dev, "Logical Name")
            .contains("wlan", Qt::CaseInsensitive))
      {
        DeviceManager::instance()
          ->tomlDeviceMapSet(DT_Network, dev, m);
      }
    }
  }
}
```

This removes the duplicated file-reading & map-building loops, clarifies the per-device differences, and keeps all current behavior.
</issue_to_address>

### Comment 13
<location> `deepin-devicemanager/src/DeviceManager/DeviceCpu.cpp:308` </location>
<code_context>
 void DeviceCpu::getTrNumber()
 {
     // 将数字转换为英文翻译
</code_context>

<issue_to_address>
Consider refactoring the repeated number-to-string mapping in getTrNumber() into a static utility function to avoid code duplication and repeated initialization.

Here’s one way to collapse that gigantic `getTrNumber()` into a tiny helper and never rebuild it on every call:

1. Create a small utility in, say, `cpu_utils.h`:

```cpp
// cpu_utils.h
#pragma once
#include <QMap>
#include <QString>

inline const QMap<int, QString> &numberNames()
{
    static const QMap<int, QString> names = {
        { 1, QObject::tr("One") },
        { 2, QObject::tr("Two") },
        { 4, QObject::tr("Four") },
        { 6, QObject::tr("Six") },
        // … up to 256 …
        {256, QObject::tr("Two hundred and fifty-six")}
    };
    return names;
}

inline QString spellOut(int n)
{
    const auto &m = numberNames();
    return m.contains(n) ? m[n] : QString::number(n);
}
```

2. In your `.cpp`, drop `DeviceCpu::getTrNumber()` entirely and in `getOverviewInfo()` just:

```cpp
#include "cpu_utils.h"

const QString DeviceCpu::getOverviewInfo()
{
    const QString cores = spellOut(m_CPUCoreNum);
    const QString procs = spellOut(m_LogicalCPUNum);

    return QString("%1 (%2 %3 / %4 %5)")
          .arg(m_Name)
          .arg(cores)
          .arg(tr("Core(s)"))
          .arg(procs)
          .arg(tr("Processor"));
}
```

– You only instantiate that map once, keep all functionality, and cut hundreds of lines down to a handful.
</issue_to_address>

### Comment 14
<location> `deepin-devicemanager/src/Tool/commontools.cpp:170` </location>
<code_context>
     return "/usr/share/deepin-devicemanager/";
 }
+
+void CommonTools::parseEDID(const QStringList &allEDIDS, const QString &input, bool isHW)
+{
+    for (auto edid:allEDIDS) {
</code_context>

<issue_to_address>
Consider refactoring all EDID-related logic into a dedicated helper or class to separate concerns and simplify parseEDID.

Consider extracting all EDID‐related logic (including the hexdump invocation, line parsing, and DeviceMonitor creation) into its own helper/class. For example:

1. Create an `EdidCollector` that runs hexdump and returns raw hex‐lines:
```cpp
// EdidCollector.h
class EdidCollector {
public:
    // `hw` selects hexdump vs. hexdump -C
    static QStringList collect(const QString &path, bool hw) {
        QProcess proc;
        proc.start(QString("hexdump %1 %2")
                   .arg(hw ? "" : "-C")
                   .arg(path));
        proc.waitForFinished(-1);
        QString out = proc.readAllStandardOutput();
        return out.split('\n', Qt::SkipEmptyParts);
    }
};
```

2. Parse raw lines into a single EDID string:
```cpp
// EdidCollector.cpp (continued)
QString buildEdidString(const QStringList &lines, bool hw) {
    QString edid;
    for (auto &l : lines) {
        if (hw) {
            auto words = l.trimmed().split(' ', Qt::SkipEmptyParts);
            if (words.size() != 9) continue;
            words.removeFirst();
            edid += words.join("") + "\n";
        } else {
            int a = l.indexOf(' '), b = l.indexOf('|');
            if (a < 0 || b < 0) continue;
            auto chunk = l.mid(a+2, b-a-3).trimmed();
            for (auto &h : chunk.split(QRegExp("\\s+"), Qt::SkipEmptyParts))
                if (h.size()==2) edid += h;
            if (edid.size() % 32 == 0) edid += '\n';
        }
    }
    return edid;
}
```

3. Then in `CommonTools::parseEDID`, simply:
```cpp
void CommonTools::parseEDID(const QStringList &all, const QString &input, bool isHW) {
    for (auto &path : all) {
        auto lines    = EdidCollector::collect(path, isHW);
        auto edidStr  = buildEdidString(lines, isHW);
        if (edidStr.split('\n').size() < 4) continue;

        EDIDParser p;
        QString err;
        p.setEdid(edidStr, err, "\n", !isHW);

        QMap<QString, QString> m {
            { "Vendor",       p.vendor() },
            { "Model",        isHW ? p.model() : p.monitorName() },
            { "Size",         p.screenSize() },
            { "Display Input", input }
        };

        auto dev = new DeviceMonitor();
        isHW ? dev->setInfoFromEdid(m)
             : dev->setInfoFromEdidForCustom(m);
        DeviceManager::instance()->addMonitor(dev);
    }
}
```

This keeps all functionality, but splits responsibilities:
- `EdidCollector` for running/parsing hexdump
- small `buildEdidString` for normalizing lines
- “driver” code in `CommonTools` just wires them together.
</issue_to_address>

### Comment 15
<location> `deepin-devicemanager/src/Tool/EDIDParser.cpp:222` </location>
<code_context>
+    m_ScreenSize = QString("%1 %2(%3mm×%4mm)").arg(QString::number(inch, '0', Common::specialComType == 7 ? 0 : 1)).arg(QObject::tr("inch")).arg(m_Width).arg(m_Height);
+}
+
+void EDIDParser::parseMonitorName()
+{
+    // EDID中从字节54开始有4个18字节的Descriptor Block
</code_context>

<issue_to_address>
Consider refactoring large methods by extracting byte block reading and name extraction into helper functions for clarity and testability.

```markdown
You’ve added a lot of low-level byte/index logic into two huge methods.  To keep each step small and testable, pull the “read 18-byte blocks” and “extract name” steps into helpers, then rewrite `parseMonitorName()` in just a few lines.

For example:

```cpp
// helper to return all four 18-byte descriptor blocks
QList<QByteArray> EDIDParser::descriptorBlocks() const {
    QList<QByteArray> blocks;
    const int starts[4] = {54, 72, 90, 108};
    for (int s : starts) {
        QByteArray block;
        for (int offset = 0; offset < 18; ++offset) {
            int byteIndex = s + offset;
            int line      = byteIndex / 16;
            int byteInLn  = byteIndex % 16;
            bool ok;
            auto hex     = getBytes(line, byteInLn);
            auto value   = hexToDec(hex).toUInt(&ok);
            if (!ok) { block.clear(); break; }
            block.append(char(value));
        }
        if (block.size() == 18)
            blocks.append(block);
    }
    return blocks;
}

// helper to pull out the product‐name string, if any
QString EDIDParser::extractMonitorName(const QByteArray &blk) const {
    // blk[0,1]==0x00, blk[3]==0xFC|0xFE → bytes 5–17 are ASCII name
    if (blk.size() < 18 || blk[0] || blk[1]) return {};
    if (blk[3] != char(0xFC) && blk[3] != char(0xFE)) return {};
    return QString::fromLatin1(blk.mid(5,13)).trimmed();
}

// finally, the cleaned‐up parseMonitorName()
void EDIDParser::parseMonitorName() {
    for (auto &blk : descriptorBlocks()) {
        auto name = extractMonitorName(blk);
        if (!name.isEmpty()) {
            m_MonitorName = name;
            return;
        }
    }
    m_MonitorName = QStringLiteral("Unknown Monitor");
}
```

Do the same for `parseScreenSize()` by splitting it into:
- `parseDetailedTiming()`  
- `parseMaxSize()`  
- and a coordinating `parseScreenSize()` that picks/merges their results.

This keeps each function < 30 lines, removes magic indices, and makes unit-testing each piece trivial.
</issue_to_address>

### Comment 16
<location> `deepin-devicemanager/src/commonfunction.cpp:35` </location>
<code_context>
 static QString boardVendorKey = "";
 int Common::specialComType = -1;
-
+static QString tomlFilesName = "tomlFilesName";
 QString Common::getArch()
 {
</code_context>

<issue_to_address>
Consider replacing the ASCII loop and mutable global with direct string literals and a single accessor or constant.

Here are a couple of ways to collapse the new indirections (and remove mutable globals) while keeping all the behavior:

1) Replace `specialHString()` + ASCII‐loop with a literal  

```cpp
// in Common.h
static constexpr auto SPECIAL_H_STRING = QStringLiteral("HUAWEI");

// in Common.cpp
QString Common::specialVendorType() {
    auto type = boardVendorType();
    // only these types return the HUAWEI string
    if (type == "KLVV" || type == "KLVU" || type == "PGUV" || type == "PGUW")
        return SPECIAL_H_STRING;
    return QStringLiteral("normalmagical");
}
```

2) Collapse tomlFilesName getter/setter into one function (or, if it never really needs to change, turn it into a true constant)  

Option A – truly constant:  
```cpp
// in Common.h or anonymous namespace in .cpp
static const QString TOML_FILES_NAME = QStringLiteral("tomlFilesName");

// anywhere you used tomlFilesNameGet():
//   TOML_FILES_NAME
```

Option B – single accessor that can optionally override:  
```cpp
// in Common.cpp
QString Common::tomlFilesName(const QString &overrideName) {
    static QString name = QStringLiteral("tomlFilesName");
    if (!overrideName.isEmpty()) name = overrideName;
    return name;
}

// remove tomlFilesNameSet and tomlFilesNameGet
```

Both approaches remove the extra ASCII loop and the separate mutable global plus its two accessors, while leaving all existing call sites and behavior intact.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (bug_risk): Modifying copies of QPair does not update the original list.

Assignments to pair.second only modify the local copy, not the original list. Iterate by reference or use indices to update the stored pairs directly.

[sourcery-ai]

suggestion: Potential confusion between setInfoFromEdid and setInfoFromEdidForCustom.

If both methods are necessary, please add documentation clarifying when each should be used.

Suggested change

	void DeviceMonitor::setInfoFromEdidForCustom(const QMap<QString, QString> &mapInfo)
	{
	m_Name = "Monitor " + mapInfo["Vendor"];
	setAttribute(mapInfo, "Size", m_ScreenSize);
	setAttribute(mapInfo, "Vendor", m_Vendor);
	setAttribute(mapInfo, "Date", m_ProductionWeek);
	setAttribute(mapInfo, "Display Input", m_DisplayInput);
	setAttribute(mapInfo, "Model", m_Model);
	m_Name = m_Model;
	getOtherMapInfo(mapInfo);
	}
	/**
	* @brief Sets device information from EDID data.
	*
	* This method should be used for standard EDID parsing and populates device attributes
	* such as name, size, vendor, production week, display input, and model.
	* Use this method when you want to set device info based on the default EDID data.
	*/
	void DeviceMonitor::setInfoFromEdid(const QMap<QString, QString> &mapInfo)
	{
	m_Name = "Monitor " + mapInfo["Vendor"];
	setAttribute(mapInfo, "Size", m_ScreenSize);
	setAttribute(mapInfo, "Vendor", m_Vendor);
	setAttribute(mapInfo, "Date", m_ProductionWeek);
	setAttribute(mapInfo, "Display Input", m_DisplayInput);
	setAttribute(mapInfo, "Model", m_Model);
	getOtherMapInfo(mapInfo);
	}
	/**
	* @brief Sets device information from custom EDID data.
	*
	* This method should be used when custom EDID parsing is required, for example,
	* when the model name should be used as the device name instead of the vendor.
	* Use this method for custom or non-standard EDID data sources.
	*/
	void DeviceMonitor::setInfoFromEdidForCustom(const QMap<QString, QString> &mapInfo)
	{
	setAttribute(mapInfo, "Size", m_ScreenSize);
	setAttribute(mapInfo, "Vendor", m_Vendor);
	setAttribute(mapInfo, "Date", m_ProductionWeek);
	setAttribute(mapInfo, "Display Input", m_DisplayInput);
	setAttribute(mapInfo, "Model", m_Model);
	m_Name = m_Model;
	getOtherMapInfo(mapInfo);
	}

[sourcery-ai]

question (bug_risk): The check for special board types has been changed to a single function call.

Please verify that Common::specialHString() correctly identifies all relevant board types, as this change consolidates multiple checks into one and may affect logic if not equivalent.

[sourcery-ai]

issue (bug_risk): Device name is used as the key for bus mapping, which may not be unique.

Using device names as keys can cause mapping errors if names are duplicated. Use a unique identifier for each device if possible.

[sourcery-ai]

question (bug_risk): Wakeup logic now checks both m_Name and m_Interface for 'PS/2'.

Please confirm that expanding the check to both fields won't cause unintended matches.

[sourcery-ai]

issue (complexity): Consider extracting the repeated logic for reading and normalizing wifi device info into shared helper functions, and only handling device-specific adjustments in each generator method.

Consider factoring out the “read & normalize /sys/hisys/wal/wifi_devices_info” logic into two very small helpers and then only doing the per-device adjustments in each generator. That collapses ~20 duplicated lines in each of generatorBluetoothDevice and generatorNetworkDevice into a few calls:

// commonfunction.h
namespace Common {
  // 1) read & split the file into a list of key→value maps
  static QList<QMap<QString,QString>> parseSysWifiInfo() {
    QList<QMap<QString,QString>> out;
    QFile f("/sys/hisys/wal/wifi_devices_info");
    if (!f.open(QIODevice::ReadOnly)) return out;
    auto lines = QString(f.readAll()).split('\n');
    f.close();
    QMap<QString,QString> m;
    for (auto &ln : lines) {
      if (ln.isEmpty()) continue;
      auto parts = ln.split(':', QString::SkipEmptyParts);
      if (parts.size() == 2)
        m[parts[0].trimmed()] = parts[1].trimmed();
    }
    if (!m.isEmpty()) out.append(m);
    return out;
  }

  // 2) common cleanup of "Chip Type"
  static QMap<QString,QString> normalizeWifiEntry(QMap<QString,QString> raw) {
    raw["Chip Type"] =
      raw["Chip Type"]
        .remove(Common::specialHString(), Qt::CaseInsensitive)
        .trimmed();
    return raw;
  }
}

Then in HWGenerator:

void HWGenerator::generatorBluetoothDevice() {
  // reuse existing steps...
  auto list = Common::parseSysWifiInfo();
  for (auto raw : list) {
    if (raw.size() < 3) continue;
    auto m = Common::normalizeWifiEntry(raw);
    // per-BT tweaks:
    m["Vendor"] = Common::specialHString();
    m["Type"]   = "Bluetooth Device";
    m["Name"]   = m["Chip Type"];
    // apply only to UART-bus devices:
    for (auto *dev : DeviceManager::instance()->convertDeviceList(DT_Bluetoorh)) {
      if (DeviceManager::instance()
            ->tomlDeviceReadKeyValue(DT_Bluetoorh, dev, "Bus")
            .contains("UART", Qt::CaseInsensitive))
      {
        DeviceManager::instance()
          ->tomlDeviceMapSet(DT_Bluetoorh, dev, m);
      }
    }
  }
}

void HWGenerator::generatorNetworkDevice() {
  DeviceGenerator::generatorNetworkDevice();
  auto list = Common::parseSysWifiInfo();
  for (auto raw : list) {
    if (raw.size() < 3) continue;
    auto m = Common::normalizeWifiEntry(raw);
    // per-NW tweaks:
    m["Vendor"] = "HISILICON";
    m["Type"]   = "Wireless network";
    m["Name"]   = m["Chip Type"];
    for (auto *dev : DeviceManager::instance()->convertDeviceList(DT_Network)) {
      if (DeviceManager::instance()
            ->tomlDeviceReadKeyValue(DT_Network, dev, "Logical Name")
            .contains("wlan", Qt::CaseInsensitive))
      {
        DeviceManager::instance()
          ->tomlDeviceMapSet(DT_Network, dev, m);
      }
    }
  }
}

This removes the duplicated file-reading & map-building loops, clarifies the per-device differences, and keeps all current behavior.

[sourcery-ai]

issue (complexity): Consider refactoring the repeated number-to-string mapping in getTrNumber() into a static utility function to avoid code duplication and repeated initialization.

Here’s one way to collapse that gigantic getTrNumber() into a tiny helper and never rebuild it on every call:

1. Create a small utility in, say, cpu_utils.h:

// cpu_utils.h
#pragma once
#include <QMap>
#include <QString>

inline const QMap<int, QString> &numberNames()
{
    static const QMap<int, QString> names = {
        { 1, QObject::tr("One") },
        { 2, QObject::tr("Two") },
        { 4, QObject::tr("Four") },
        { 6, QObject::tr("Six") },
        // … up to 256 …
        {256, QObject::tr("Two hundred and fifty-six")}
    };
    return names;
}

inline QString spellOut(int n)
{
    const auto &m = numberNames();
    return m.contains(n) ? m[n] : QString::number(n);
}

2. In your .cpp, drop DeviceCpu::getTrNumber() entirely and in getOverviewInfo() just:

#include "cpu_utils.h"

const QString DeviceCpu::getOverviewInfo()
{
    const QString cores = spellOut(m_CPUCoreNum);
    const QString procs = spellOut(m_LogicalCPUNum);

    return QString("%1 (%2 %3 / %4 %5)")
          .arg(m_Name)
          .arg(cores)
          .arg(tr("Core(s)"))
          .arg(procs)
          .arg(tr("Processor"));
}

– You only instantiate that map once, keep all functionality, and cut hundreds of lines down to a handful.

[sourcery-ai]

issue (complexity): Consider refactoring all EDID-related logic into a dedicated helper or class to separate concerns and simplify parseEDID.

Consider extracting all EDID‐related logic (including the hexdump invocation, line parsing, and DeviceMonitor creation) into its own helper/class. For example:

1. Create an EdidCollector that runs hexdump and returns raw hex‐lines:

// EdidCollector.h
class EdidCollector {
public:
    // `hw` selects hexdump vs. hexdump -C
    static QStringList collect(const QString &path, bool hw) {
        QProcess proc;
        proc.start(QString("hexdump %1 %2")
                   .arg(hw ? "" : "-C")
                   .arg(path));
        proc.waitForFinished(-1);
        QString out = proc.readAllStandardOutput();
        return out.split('\n', Qt::SkipEmptyParts);
    }
};

2. Parse raw lines into a single EDID string:

// EdidCollector.cpp (continued)
QString buildEdidString(const QStringList &lines, bool hw) {
    QString edid;
    for (auto &l : lines) {
        if (hw) {
            auto words = l.trimmed().split(' ', Qt::SkipEmptyParts);
            if (words.size() != 9) continue;
            words.removeFirst();
            edid += words.join("") + "\n";
        } else {
            int a = l.indexOf(' '), b = l.indexOf('|');
            if (a < 0 || b < 0) continue;
            auto chunk = l.mid(a+2, b-a-3).trimmed();
            for (auto &h : chunk.split(QRegExp("\\s+"), Qt::SkipEmptyParts))
                if (h.size()==2) edid += h;
            if (edid.size() % 32 == 0) edid += '\n';
        }
    }
    return edid;
}

3. Then in CommonTools::parseEDID, simply:

void CommonTools::parseEDID(const QStringList &all, const QString &input, bool isHW) {
    for (auto &path : all) {
        auto lines    = EdidCollector::collect(path, isHW);
        auto edidStr  = buildEdidString(lines, isHW);
        if (edidStr.split('\n').size() < 4) continue;

        EDIDParser p;
        QString err;
        p.setEdid(edidStr, err, "\n", !isHW);

        QMap<QString, QString> m {
            { "Vendor",       p.vendor() },
            { "Model",        isHW ? p.model() : p.monitorName() },
            { "Size",         p.screenSize() },
            { "Display Input", input }
        };

        auto dev = new DeviceMonitor();
        isHW ? dev->setInfoFromEdid(m)
             : dev->setInfoFromEdidForCustom(m);
        DeviceManager::instance()->addMonitor(dev);
    }
}

This keeps all functionality, but splits responsibilities:

• EdidCollector for running/parsing hexdump
• small buildEdidString for normalizing lines
• “driver” code in CommonTools just wires them together.

[sourcery-ai]

issue (complexity): Consider refactoring large methods by extracting byte block reading and name extraction into helper functions for clarity and testability.

You’ve added a lot of low-level byte/index logic into two huge methods.  To keep each step small and testable, pull the “read 18-byte blocks” and “extract name” steps into helpers, then rewrite `parseMonitorName()` in just a few lines.

For example:

```cpp
// helper to return all four 18-byte descriptor blocks
QList<QByteArray> EDIDParser::descriptorBlocks() const {
    QList<QByteArray> blocks;
    const int starts[4] = {54, 72, 90, 108};
    for (int s : starts) {
        QByteArray block;
        for (int offset = 0; offset < 18; ++offset) {
            int byteIndex = s + offset;
            int line      = byteIndex / 16;
            int byteInLn  = byteIndex % 16;
            bool ok;
            auto hex     = getBytes(line, byteInLn);
            auto value   = hexToDec(hex).toUInt(&ok);
            if (!ok) { block.clear(); break; }
            block.append(char(value));
        }
        if (block.size() == 18)
            blocks.append(block);
    }
    return blocks;
}

// helper to pull out the product‐name string, if any
QString EDIDParser::extractMonitorName(const QByteArray &blk) const {
    // blk[0,1]==0x00, blk[3]==0xFC|0xFE → bytes 5–17 are ASCII name
    if (blk.size() < 18 || blk[0] || blk[1]) return {};
    if (blk[3] != char(0xFC) && blk[3] != char(0xFE)) return {};
    return QString::fromLatin1(blk.mid(5,13)).trimmed();
}

// finally, the cleaned‐up parseMonitorName()
void EDIDParser::parseMonitorName() {
    for (auto &blk : descriptorBlocks()) {
        auto name = extractMonitorName(blk);
        if (!name.isEmpty()) {
            m_MonitorName = name;
            return;
        }
    }
    m_MonitorName = QStringLiteral("Unknown Monitor");
}

Do the same for parseScreenSize() by splitting it into:

• parseDetailedTiming()
• parseMaxSize()
• and a coordinating parseScreenSize() that picks/merges their results.

This keeps each function < 30 lines, removes magic indices, and makes unit-testing each piece trivial.

[sourcery-ai]

issue (complexity): Consider replacing the ASCII loop and mutable global with direct string literals and a single accessor or constant.

Here are a couple of ways to collapse the new indirections (and remove mutable globals) while keeping all the behavior:

1. Replace specialHString() + ASCII‐loop with a literal

// in Common.h
static constexpr auto SPECIAL_H_STRING = QStringLiteral("HUAWEI");

// in Common.cpp
QString Common::specialVendorType() {
    auto type = boardVendorType();
    // only these types return the HUAWEI string
    if (type == "KLVV" || type == "KLVU" || type == "PGUV" || type == "PGUW")
        return SPECIAL_H_STRING;
    return QStringLiteral("normalmagical");
}

2. Collapse tomlFilesName getter/setter into one function (or, if it never really needs to change, turn it into a true constant)

Option A – truly constant:

// in Common.h or anonymous namespace in .cpp
static const QString TOML_FILES_NAME = QStringLiteral("tomlFilesName");

// anywhere you used tomlFilesNameGet():
//   TOML_FILES_NAME

Option B – single accessor that can optionally override:

// in Common.cpp
QString Common::tomlFilesName(const QString &overrideName) {
    static QString name = QStringLiteral("tomlFilesName");
    if (!overrideName.isEmpty()) name = overrideName;
    return name;
}

// remove tomlFilesNameSet and tomlFilesNameGet

Both approaches remove the extra ASCII loop and the separate mutable global plus its two accessors, while leaving all existing call sites and behavior intact.